adapter: let the group committer write at a caller-chosen timestamp - #37921
adapter: let the group committer write at a caller-chosen timestamp#37921aljoscha wants to merge 1 commit into
Conversation
5f840cd to
73d116f
Compare
73d116f to
1018d45
Compare
64f8e8f to
6604aa0
Compare
6604aa0 to
e725d26
Compare
6e84c8e to
b92c253
Compare
b92c253 to
2b7f53b
Compare
2b7f53b to
8f93d7b
Compare
8f93d7b to
b3630c1
Compare
57395d7 to
41ef569
Compare
The group committer picks a timestamp from the oracle, which is right for a write whose diffs are valid whenever they land. A write computed against a snapshot is not like that: its diffs are only correct at the frontier they were read at, so the timestamp has to come from the caller. `TableWriteCmd::TimestampedWrite` carries that timestamp and gets exactly one attempt. An `InvalidUppers` means another writer holds the upper at or past it, so the snapshot the diffs were computed from is stale and the answer is `TimestampPassed`, never a retry at a fresh timestamp. Retrying stale diffs later is precisely the lost update this exists to prevent. The write also names the `GlobalId` it was computed against, and the committer refuses it if the table's latest generation has moved on, which is how a concurrent `ALTER TABLE` is caught rather than applied to rows of the wrong arity. Both txns-shard write paths now share `attempt_write_to_txns`, so the catalog upper advance, the metric, the `InvalidUppers` classification and the oracle apply exist once. The two differ only in what they do with a conflict, which is the difference worth seeing. The result travels back through a responder whose `Drop` reports `Indeterminate`. Without that, a dropped sender on shutdown would panic the waiting session task instead of failing its statement. Callers arrive later in the stack. Until they do, a `dead_code` allowance on the unreached items keeps the crate warning-free, and it goes away with the commit that adds them.
41ef569 to
a07c57b
Compare
ggevay
left a comment
ggevay
left a comment
There was a problem hiding this comment.
LGTM, very clean scaffolding. One description question, plus some small inline comments (mostly optional doc/comment nits).
Description accuracy: "The request also names the GlobalId it was computed against, and the committer refuses it if the table's latest generation has moved on." As far as I can tell, this refusal exists only for the blind path (WriteTarget on UserWriteResponder::Internal) and happens during commit staging on the coordinator, not in the committer. TimestampedWriteRequest carries pre-resolved GlobalIds and relies on its (part 6) submitter's catalog check plus committer-channel FIFO instead. If that reading is right, could you tweak the paragraph? Precision here matters for reviewing parts 6 and 7.
| } | ||
|
|
||
| /// An OCC write whose diffs are valid only at `target_timestamp`. | ||
| pub(crate) struct TimestampedWriteRequest { |
There was a problem hiding this comment.
Could the doc also state the submission contract? These GlobalIds reach the table-write worker unvalidated, and a non-empty append for an id whose handle a Forget already dropped fails an assert in persist_handles.rs and takes the process down. The safety argument seems to be: validate the target against the current catalog on the coordinator loop and send with no await in between, so channel FIFO keeps the append ahead of any Forget. Part 6's handle_attempt_write does exactly that, but a sentence here would keep a future submitter honest.
| appends | ||
| .entry(target.item_id) | ||
| .or_default() | ||
| .extend(writes.into_values().flatten()); |
There was a problem hiding this comment.
nit, optional: this relies on the single-key invariant that only part 6's handle_attempt_write enforces, and a violation would silently append rows to the wrong table. A one-line assert that every key of writes equals target.item_id would make this self-defending. Fine to fold into part 6 instead.
| /// Attempts an OCC write exactly once at its requested timestamp. | ||
| /// | ||
| /// Unlike blind writes, an `InvalidUppers` conflict must return to the | ||
| /// subscribe loop. Retrying the same diffs at a fresh timestamp would apply |
There was a problem hiding this comment.
nit: this doc reaches into its caller's internals ("the subscribe loop", "the OCC semaphore on the frontend"), coupling the committer's doc to one caller's implementation. Could it stay at the level of this function's contract? E.g. "a conflict is reported as TimestampPassed and is the caller's to resolve with a new snapshot", and the throttle/permit bullets as preconditions on the caller: the requested timestamp must not run the write timeline ahead of the clock, and the caller bounds how many requests are in flight. (Still fine to mention what callers do as an example of how to use this function, the ask is just that the contract not be stated in terms of it.)
| writes, | ||
| write_locks: None, | ||
| responder: UserWriteResponder::Session(pending_txn), | ||
| responder, |
There was a problem hiding this comment.
nit, optional: with the old header comment gone (rightly, "otherwise defer the write" no longer holds for the internal path), a corrected one-liner would help skimming this now ~115-line arm. E.g.: "Without handed-off locks, acquire just-in-time. On a miss, session writes defer, internal writes re-queue."
| ControlFlow::Continue(()) | ||
| } | ||
|
|
||
| /// Writes at a fresh oracle timestamp, retrying an upper conflict at a new |
There was a problem hiding this comment.
nit, optional: the old doc also said "and applies a successful write to the oracle". That still holds (via attempt_write_to_txns, whose Applied implies the oracle apply), and commit relies on it, so consider keeping the clause here so the contract stays complete at this boundary too.
2 participants
Motivation
Part 4 of 7 in a stack that moves
DELETE,UPDATEandINSERT ... SELECToff the coordinator onto the session task, using optimistic concurrency
control. Design doc:
20260210_incremental_occ_read_then_write.md(lands in part 6).
The group committer picks a write timestamp from the oracle, which is right
for a write whose diffs are valid whenever they land. A write computed
against a snapshot is not like that. Its diffs are only correct at the
frontier they were read at, so the timestamp has to come from the caller and
the committer has to be able to refuse it.
Closes SQL-590
Description
TableWriteCmd::TimestampedWritecarries a caller-chosen timestamp and getsexactly one attempt. An
InvalidUppersmeans another writer holds the upperat or past that timestamp, so the snapshot the diffs were computed from is
stale, and the answer is
TimestampPassed. Never a retry at a freshtimestamp: retrying stale diffs later is precisely the lost update this
exists to prevent. Deciding what to do about a conflict is the caller's job,
and it needs a new snapshot to do it.
The request also names the
GlobalIdit was computed against, and thecommitter refuses it if the table's latest generation has moved on. That is
how a concurrent
ALTER TABLEis caught rather than applied to rows of thewrong arity.
Both txns-shard write paths now share
attempt_write_to_txns, so the catalogupper advance, the metric, the
InvalidUppersclassification and the oracleapply exist once. The two differ only in what they do with a conflict, which
is the difference worth seeing.
The result travels back through a responder whose
DropreportsIndeterminate. Without that, a dropped sender on shutdown would panic thewaiting session task instead of failing its statement.
Temporary scaffolding. Nothing calls this until part 6, so a
dead_codeallowance on the unreached items keeps the crate warning-free.Part 6 removes it in the same commit that adds the callers.
Verification
No caller yet, so this part is carried by the tests in parts 6 and 7, and by
existing coverage showing the shared
attempt_write_to_txnsdid not changeordinary group commit behavior.