test: stabilize prepared current timestamp assertion - #27694
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep-reviewed exact head 88b75ce9917acb6f56a3c9fc6a16d7029d8db747 against base ff4270c844c4b630cf1d921da813ba119d5b5e89, issue #27633, the full two-file diff, and exact-head CI.
The old BVT oracle was invalid: separate EXECUTE calls are separate statements and may legitimately cross a timestamp boundary, so expecting one distinct value was timing-dependent. The replacement asserts the correct deterministic contract: one prepared-statement execution inserts two rows, and both CURRENT_TIMESTAMP(6) occurrences observe the same statement timestamp. count(*) = 2 prevents a vacuous distinct-count pass, and explicit DEALLOCATE PREPARE closes the statement lifecycle before table teardown.
This does not lose the per-execution contract: TestBuiltInCurrentTimestampReadsStatementTimePerExecution deterministically reuses the same cached overload while injecting two different statement times and verifies each execution reads its own value. The BVT remains the public prepared-SQL/statement-consistency oracle without sleeps or wall-clock assumptions.
git diff --check is clean; exact-head build/UT/SCA are green, and the changed prepare.test was selected by the passing PESSIMISTIC BVT. No correctness, test-quality, performance, cleanup, or unhappy-path blocker found.
Merge Queue Status
This pull request spent 1 hour 25 minutes 38 seconds in the queue, with no time running CI. Waiting for
All conditions
ReasonThe merge conditions cannot be satisfied due to failing checks Failing checks:
HintYou may have to fix your CI before adding the pull request to the queue again. Requeued — the merge queue status continues in this comment ↓. |
aptend
left a comment
aptend
left a comment
There was a problem hiding this comment.
Deep-reviewed exact head 38bee17 against merge-base b0ae66c, issue #27633, the complete two-file diff, all review/comment history, and the statement-time semantics. The old assertion compared two separate EXECUTE statements, so crossing a microsecond boundary legitimately produced two distinct timestamps. The replacement tests the deterministic contract: one prepared INSERT evaluates two CURRENT_TIMESTAMP(6) expressions under one statement time; count(*) = 2 prevents a vacuous distinct-count result, count(distinct a) = 1 proves statement consistency, and DEALLOCATE PREPARE closes the lifecycle. The existing package test independently verifies that reusing the same cached overload across separate executions reads each execution’s new statement time. No production or hot-path behavior changes. Validation passed on the exact head: diff check, package selection/dependency classification, build and vet for pkg/sql/plan/function, and 20 focused CGo-controlled repetitions of TestBuiltInCurrentTimestampReadsStatementTimePerExecution. No blocking correctness, flakiness, cleanup, or coverage issue found.
Merge Queue Status
This pull request spent 41 minutes 6 seconds in the queue, with no time running CI. ReasonThe pull request can't be updated
HintYou should update or rebase your pull request manually. If you do, this pull request will automatically be requeued once the queue conditions match again. Tick the box to put this pull request back in the merge queue (same as
|
4 participants
What type of PR is this?
Which issue(s) this PR fixes:
Fixes #27633
What this PR does / why we need it:
prepare.testassumed that two separate executions of a prepared statement would return the sameCURRENT_TIMESTAMPvalue. Each execution has its own statement time, so that assertion was timing-dependent and flaky.This change:
CURRENT_TIMESTAMP(6)calls within one prepared-statement execution;Validation:
TestBuiltInCurrentTimestampReadsStatementTimePerExecutionpassed with exact test selection.prepare.testpassed twice on the same clean local instance: 531/531 statements, 100% success on both runs.