adapter: don't store txn read holds for timestamp-less determinations#37641
Open
ggevay wants to merge 1 commit into
Open
adapter: don't store txn read holds for timestamp-less determinations#37641ggevay wants to merge 1 commit into
ggevay wants to merge 1 commit into
Conversation
Fixes SQL-520, an instance of the "insufficient read holds" family (DB-156): the frontend peek sequencing stored the read holds it acquired for a timestamp-independent query (e.g. a query over a constant-folded view) as the transaction's read holds. `txn_read_holds` play a double role for multi-statement read transactions: they hold back the sinces of the transaction's timedomain, and their id set is the allowed timedomain that later statements are validated against. Both only work if the holds are established by the same statement that determines the transaction timestamp, over the same id bundle. A timestamp-independent query gets a `NoTimestamp` determination, which doesn't pin the transaction timestamp, so a later query determines the transaction timestamp over its own timedomain. The stored holds of the timestamp-independent query then wrongly widened the allowed timedomain: a subsequent statement could pass the timedomain validation and read a collection whose since is not covered by the transaction timestamp. Concretely, in the Parallel Workload (rename + naughty identifiers) failure of SQL-520: 1. The session created a temp table (whose since starts at its exact creation timestamp) and a temp view whose optimized expression is constant (a join on a constant-false condition). 2. The first query of a read-only transaction selected from the constant temp view: TimestampIndependent, `NoTimestamp` determination, but its timedomain (the whole mz_temp schema, including the fresh temp table) was stored as the transaction's read holds. 3. The second query determined the transaction timestamp over its own, disjoint timedomain, choosing a timestamp behind the temp table's since (its inputs included a source whose upper lagged behind the temp table's creation time). 4. The third query selected from the temp table: the poisoned hold set made the timedomain validation pass instead of returning `RelationOutsideTimeDomain`, and the peek's as_of was behind the temp table's since, tripping the read hold assert (and, without soft asserts, `SinceViolation`). The fix mirrors the old peek sequencing (`sequence_peek_timestamp`), which only stores read holds into `txn_read_holds` when the determination has a timestamp. This also fixes a spurious `RelationOutsideTimeDomain` error when a statement sequenced by the old path (e.g. EXECUTE) followed a timestamp-independent statement sequenced by the frontend path in the same transaction, because the old path validates against the stored hold set whenever one exists. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
6f90982 to
fe7b392
Compare
Contributor
Author
|
@def-, after this is merged, all the |
Contributor
|
Yay! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Finally fixes SQL-520, and probably fixes DB-156, aka "the mysterious read hold issue"!
The root cause was that the frontend peek sequencing stored the read holds it acquired for a timestamp-independent query (e.g. a query over a constant-folded view) as the transaction's read holds.
txn_read_holdsplay a double role for multi-statement read transactions: they hold back the sinces of the transaction's timedomain, and their id set is the allowed timedomain that later statements are validated against. Both only work if the holds are established by the same statement that determines the transaction timestamp, over the same id bundle.A timestamp-independent query gets a
NoTimestampdetermination, which doesn't pin the transaction timestamp, so a later query determines the transaction timestamp over its own timedomain. The stored holds of the timestamp-independent query then wrongly widened the allowed timedomain: a subsequent statement could pass the timedomain validation and read a collection whose since is not covered by the transaction timestamp.Concretely, in the "Parallel Workload (rename + naughty identifiers)" failure of SQL-520:
NoTimestampdetermination, but its timedomain (the whole mz_temp schema, including the fresh temp table) was stored as the transaction's read holds.RelationOutsideTimeDomain, and the peek's as_of was behind the temp table's since, tripping the read hold assert (and, without soft asserts,SinceViolation).The fix mirrors the old peek sequencing (
sequence_peek_timestamp), which only stores read holds intotxn_read_holdswhen the determination has a timestamp. This also fixes a spuriousRelationOutsideTimeDomainerror when a statement sequenced by the old path (e.g. EXECUTE) followed a timestamp-independent statement sequenced by the frontend path in the same transaction, because the old path validates against the stored hold set whenever one exists.Parallel Workload runs: