adapter: correct initial timestamp determination in transactions #20267

maddyblue · 2023-06-30T10:31:39Z

In #19026 various timestamp functions were refactored to allow EXPLAIN TIMESTAMP to reflect (and effect) the current transaction. This is tricky code and the refactor incorrectly changed the behavior of transactions. In a transaction our intention is to find a timestamp during the first statement that will be valid for any other object a user might query later in the transaction, then aquire read holds on all those referenced objects. In that change, the first statement only used the referenced IDs when determining a timestamp, not all nearby (objects in the same schema) IDs. Read holds were still correctly aquired on all nearby objects.

This could cause the transaction's timestamp to not be in advance of the since of other nearby objects. For example, take two objects in the table. At the first query, object A has a since of 10, object B has a since of 20. A first query in the transaction SELECT * FROM A would choose 10 as its timestamp, then aquire read holds on A and B at 10. A later query in the transaction SELECT * FROM B would choose 10 as its timestamp (which it fetched from the in-progress transaction state), and panic because B's since is in advance of 10.

One of the reasons this happened is the transaction metadata only held on to the TimestampContext. EXPLAIN TIMESTAMP needs a larger struct, a TimestampDetermination. Teach the session metadata to track this outer struct so EXPLAIN can use it. This allows us to now only call determine_timestamp once during transaction start, instead of during each query in a transaction. Refactor some of the other read handling code to look more like it did pre-19026.

After this code, the test repro from 19743 no longer causes crashes.

Fixes #19743
Fixes #19897
Fixes #19398

Motivation

This PR fixes a recognized bug.

Tips for reviewer

Looking at the code right before 19026 is useful:

materialize/src/adapter/src/coord/sequencer/inner.rs

Line 2242 in 0e5efd4

let mut read_holds = None;

Checklist

This PR has adequate test coverage / QA involvement has been duly considered.
This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
If this PR will require changes to cloud orchestration, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
This PR includes the following user-facing behavior changes:
- Fix timestamp generation for transactions with multiple statements that could lead to crashes.

maddyblue · 2023-06-30T10:33:27Z

There's no tests for this yet because there weren't before and writing a test scenario for exactly this is not currently within the realm of what our test code can do deterministically. We should add tests for this (both randomized and deterministic), but those may take some days to write and I think it may be better to get this into the release before those because we're already in a known broken state. The by-hand tests I did worked, so some tests have been done, just none automated.

def-

Based on Philip's local repro the issue seems fixed. A deterministic test would be even better of course.

jkosh44 · 2023-06-30T13:37:07Z

I understand how the problem described leads to the errors in #19743, but I'm having trouble figuring out how they lead to the identifier missing errors in the other issues. Would you be able to include a description of that in your explanation?

src/adapter/src/coord/sequencer/inner.rs

jkosh44 · 2023-06-30T14:17:57Z

src/adapter/src/coord/sequencer/inner.rs

+        source_bundle: &CollectionIdBundle,
        source_ids: &BTreeSet<GlobalId>,


This is an existing issue, but I'm finding it very difficult to tell the difference between source_ids and source_bundle. source_ids seems to be IDs directly referenced in the query, while source_bundle seems to be the result of a call to sufficient_collections on an index_oracle.

Not at all important for this PR, I just wanted to call it out.

src/adapter/src/coord/sequencer/inner.rs

benesch · 2023-07-02T20:40:57Z

You had marked this as closing #20197 but I'm not seeing how that issue is connected to this PR, so I removed that from the PR description. Let me know if I've erred.

In MaterializeInc#19026 various timestamp functions were refactored to allow `EXPLAIN TIMESTAMP` to reflect (and effect) the current transaction. This is tricky code and the refactor incorrectly changed the behavior of transactions. In a transaction our intention is to find a timestamp during the first statement that will be valid for any other object a user might query later in the transaction, then aquire read holds on all those referenced objects. In that change, the first statement only used the referenced IDs when determining a timestamp, not all nearby (objects in the same schema) IDs. Read holds were still correctly aquired on all nearby objects. This could cause the transaction's timestamp to not be in advance of the since of other nearby objects. For example, take two objects in the table. At the first query, object A has a since of 10, object B has a since of 20. A first query in the transaction `SELECT * FROM A` would choose 10 as its timestamp, then aquire read holds on A and B at 10. A later query in the transaction `SELECT * FROM B` would choose 10 as its timestamp (which it fetched from the in-progress transaction state), and panic because B's since is in advance of 10. One of the reasons this happened is the transaction metadata only held on to the TimestampContext. `EXPLAIN TIMESTAMP` needs a larger struct, a TimestampDetermination. Teach the session metadata to track this outer struct so EXPLAIN can use it. This allows us to now only call determine_timestamp once during transaction start, instead of during each query in a transaction. Refactor some of the other read handling code to look more like it did pre-19026.

jkosh44 · 2023-07-05T22:11:51Z

I understand how the problem described leads to the errors in #19743, but I'm having trouble figuring out how they lead to the identifier missing errors in the other issues. Would you be able to include a description of that in your explanation?

Just wanted to bump this comment. I'm still a bit confused about this.

jkosh44

LGTM

maddyblue · 2023-07-05T22:33:03Z

I understand how the problem described leads to the errors in #19743, but I'm having trouble figuring out how they lead to the identifier missing errors in the other issues. Would you be able to include a description of that in your explanation?

I don't have an explanation about why this error happened or why this PR no longer is producing that error. My best guess is that those objects were dropped in other connections, and because the read hold was made for a time in the future, the controller thought it was safe to remove them. Then later when a query comes along for them, they don't exist.

maddyblue requested a review from a team as a code owner June 30, 2023 10:31

def- approved these changes Jun 30, 2023

View reviewed changes

jkosh44 reviewed Jun 30, 2023

View reviewed changes

src/adapter/src/coord/sequencer/inner.rs Outdated Show resolved Hide resolved

def- mentioned this pull request Jul 4, 2023

Randomized parallel testing with DDLs and transactions #20274

Closed

maddyblue force-pushed the fix-sequence-timestamp branch from 44a6fbd to bf093bf Compare July 5, 2023 21:35

maddyblue force-pushed the fix-sequence-timestamp branch from bf093bf to 0b176dd Compare July 5, 2023 21:38

jkosh44 approved these changes Jul 5, 2023

View reviewed changes

maddyblue merged commit e628185 into MaterializeInc:main Jul 5, 2023
62 checks passed

maddyblue deleted the fix-sequence-timestamp branch July 5, 2023 22:33

def- mentioned this pull request Jul 5, 2023

parallel-workload test #20354

Merged

5 tasks

materialize-bot mentioned this pull request Jul 7, 2023

release: v0.60.0 required reviews #20416

Closed

17 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adapter: correct initial timestamp determination in transactions #20267

adapter: correct initial timestamp determination in transactions #20267

maddyblue commented Jun 30, 2023 •

edited by benesch

maddyblue commented Jun 30, 2023

def- left a comment

jkosh44 commented Jun 30, 2023

jkosh44 Jun 30, 2023

jkosh44 Jun 30, 2023

benesch commented Jul 2, 2023

jkosh44 commented Jul 5, 2023

jkosh44 left a comment

maddyblue commented Jul 5, 2023

		source_bundle: &CollectionIdBundle,
		source_ids: &BTreeSet<GlobalId>,

adapter: correct initial timestamp determination in transactions #20267

adapter: correct initial timestamp determination in transactions #20267

Conversation

maddyblue commented Jun 30, 2023 • edited by benesch

Motivation

Tips for reviewer

Checklist

maddyblue commented Jun 30, 2023

def- left a comment

Choose a reason for hiding this comment

jkosh44 commented Jun 30, 2023

jkosh44 Jun 30, 2023

Choose a reason for hiding this comment

jkosh44 Jun 30, 2023

Choose a reason for hiding this comment

benesch commented Jul 2, 2023

jkosh44 commented Jul 5, 2023

jkosh44 left a comment

Choose a reason for hiding this comment

maddyblue commented Jul 5, 2023

maddyblue commented Jun 30, 2023 •

edited by benesch