adapter: let the oracle choose the read-then-write write timestamp - #38322
Conversation
407f1bf to
1006eaa
Compare
| //! are frontier-independent, so the caller of the loop submits them right after | ||
| //! it. | ||
| //! | ||
| //! ## The frontier certifies, the oracle chooses |
There was a problem hiding this comment.
this explanation needs to be quite a bit more concise, and focus on our local invariants and stuff
There was a problem hiding this comment.
Cut from 50 lines to 25, and reorganised around the local invariants: the three the loop holds (F >= T before submitting, payload t < T strictly, T > as_of) as a bullet list, then one paragraph on why F bounds neither T nor the table upper, then two sentences on the far-ahead input.
Dropped from it: the worked example of the append-then-apply_write window, the DELETE ... IN (SELECT ... FROM mv) shape, and the retry-convergence argument. They live in run_occ_loop and in the design doc instead.
| // by `statement_timeout`. | ||
| self.ensure_read_linearized(&timeline, as_of).await?; | ||
|
|
||
| // The OCC loop derives its write target from this oracle, and reaching |
There was a problem hiding this comment.
more concise please
There was a problem hiding this comment.
13 lines to 7. It now says only what the ordering buys, that it leaves the oracle at or above as_of so the target clears as_of and the payload holds the snapshot, plus one sentence that a far-future as_of parks here because the data does not exist yet.
| // write might later have to treat as concurrent. | ||
| let fold_target = write_target.unwrap_or(min_target); | ||
|
|
||
| // Whether this iteration attempts a write. |
There was a problem hiding this comment.
please make shorter and more concise
There was a problem hiding this comment.
The three blocks around the loop are down from 24 lines to 13 in total. The retry invariant is 6 lines instead of 16, the fold-target note is 3 instead of 4, and the bypass explanation keeps only the two things a reader needs: waiting for a message can wait forever when an input has settled, and the arm cannot spin because every attempt costs an AttemptWrite round trip and a retry_count.
1006eaa to
d914c9c
Compare
Follow-up to MaterializeInc#38322, so currently based on top of it
Follow-up to MaterializeInc#38322, so currently based on top of it
| /// step above its write timestamp, so hitting the bound means the oracle has | ||
| /// run away from the wall clock rather than that the statement asked for | ||
| /// anything unusual. Committing there would advance the oracle further, and | ||
| /// the oracle is monotone and durable, so every later write and |
There was a problem hiding this comment.
feels like we're duplicating this description in a few too many places, make a sweep, make sure we adhere to our commenting guidelines, please
There was a problem hiding this comment.
Swept. The explanation now lives in one place, write_ts_upper_bound, and the other three sites state their local fact and point at it:
AdapterError::ReadThenWriteTimestampTooFarAhead: what it means plus "nothing was appended", then the pointer. The one thing it adds is that this commit changes the producer, the oracle rather than an input frontier.commit_timestamped: "Committing here would apply the target to the oracle below, which is what makes it stick. Seewrite_ts_upper_bound."WriteResult::TimestampTooFarAhead: unchanged one-liner.
That is 21 lines down to 7 across the three.
| //! retries converge because both refusals name a timestamp above the one they | ||
| //! rejected. | ||
| //! | ||
| //! Taking `T` from the oracle is also what makes a far-ahead input ordinary: a |
There was a problem hiding this comment.
probably don't need this last paragraph, and can tighten up the rest as well
There was a problem hiding this comment.
Paragraph gone. I kept one sentence of it in the opener, because without it the section says what the rule is and never says why the frontier is the wrong source: "Taking T from F instead would let a settled input, whose frontier is legitimately days out, carry the timeline with it." Say the word and that goes too, the commit message carries the same argument.
| /// | ||
| /// `None` means there is nothing to linearize against. Either the | ||
| /// subscribe ran to completion, so the emptiness rests on no frontier at | ||
| /// all, or the answer speaks about `as_of`, which the caller linearized |
There was a problem hiding this comment.
don't know what this "speaks about as_of" means here,
There was a problem hiding this comment.
That language is gone everywhere. It was doing two jobs badly, and the field name was the root of it.
observed_ts is now empty_as_of, and it says what it is: the timestamp the emptiness holds at. The caller brings the oracle up to it. Nothing "speaks about" anything.
| /// Diffs from a selection that reads no persisted state. The subscribe ran | ||
| /// to completion, so they are frontier-independent and the caller chooses | ||
| /// whether to submit them now or buffer them into the transaction. | ||
| /// Diffs no frontier can change, from a subscribe that ran to completion. |
There was a problem hiding this comment.
speculating about callers and other places in the code, so should stick to our facts and invariants
There was a problem hiding this comment.
Cut. The variant doc states the contract and the reason it exists, and the caller-side detail is gone.
| // data does not exist yet, so no choice of write timestamp avoids that. | ||
| self.ensure_read_linearized(&timeline, as_of).await?; | ||
|
|
||
| // The loop takes its write target from this oracle, and reaching one takes |
There was a problem hiding this comment.
pleaes make more concise and to the point
There was a problem hiding this comment.
13 lines to 4:
// Ordering is load-bearing: this leaves the oracle at or above `as_of`,
// which is what makes the loop target clear `as_of` and so include the
// snapshot. A far-future `as_of` parks here until the clock arrives,
// bounded by `statement_timeout`.
| } | ||
| None => Ok(response), | ||
| None => { | ||
| // The subscribe closed cleanly, so the emptiness |
There was a problem hiding this comment.
we sure this is true?
There was a problem hiding this comment.
No, and thank you for pulling on it. Two things wrong with it.
ensure_read_linearized reads read_ts before it nudges, and returns straight away when the oracle is already past the timestamp. The common zero-row case reports as_of, which the pre-read linearization already covered, so it does not park at all. And when it does have to wait, it asks the group committer for a commit rather than waiting for a keepalive, so the wait is a commit round trip, not up to a default_timestamp_interval.
Now:
// The wait is a no-op where the oracle is already past `empty_as_of`,
// which is the common `WHERE <no match>`, and otherwise costs the group
// commit that `ensure_read_linearized` asks for. Either way the
// subscribe handle is gone, so the permit guards nothing and holding it
// would throttle unrelated writes.
The design doc had inherited the same wrong cost model in its benchmark section, so I corrected that too.
| /// Semantically this is a SELECT at `target - 1` followed by an INSERT at | ||
| /// `target`, where `target` comes from `write_oracle` and the subscribe's | ||
| /// frontier certifies that the payload is the complete set of diffs below | ||
| /// it. Because we hold no write lock, a concurrent writer may bump the |
There was a problem hiding this comment.
no need to talk about locks here, please just describe the facts and invariants, if any
There was a problem hiding this comment.
Locks gone, and the rest is now the contract rather than a narration: where target comes from, what the frontier certifies, what a conflict returns, and the two obligations on the caller. 24 lines to 16.
|
|
||
| // Retry invariant: `state` keeps every row the subscribe ever sent, and | ||
| // folding by target means the payload is what the query returns as of | ||
| // `target - 1`, retractions cancelling the snapshot rows they replace. |
There was a problem hiding this comment.
talking about retractions and the snapshot is too specific here, just say we consolidate at that timestamp or something
There was a problem hiding this comment.
Now: "the payload is the selection consolidated at target - 1, and the diffs at or above target are concurrent with the write, so a retry folds them in only once it raises the target." 6 lines to 3.
| // write would have to treat as concurrent. | ||
| let fold_target = write_target.unwrap_or(min_target); | ||
|
|
||
| // A conflict can leave a target the frontier we already observed |
There was a problem hiding this comment.
this comment can be crisped up, pleass
There was a problem hiding this comment.
all we're really doing is: are we already ready -> proceed to write, if not, wait, right? and then we need some argument for why we don't livelock or deadlock?
There was a problem hiding this comment.
Yes, exactly that. Rewritten to your framing:
// Already certified for the target we hold? Write. Otherwise wait for
// the next subscribe message. Waiting first would hang after a
// conflict, since an input settled until its next refresh sends
// nothing further and does not close the channel either.
//
// Termination: the write arm awaits a round trip and only a conflict
// returns to this one, raising `retry_count` towards
// `max_occ_retries`, so neither arm spins.
| // tracks the wall clock rather than any input's frontier. | ||
| let peek_write_ts = oracle.peek_write_ts().await; | ||
| let Some(chosen) = peek_write_ts.try_step_forward() else { | ||
| // There is no timestamp above `Timestamp::MAX` to |
There was a problem hiding this comment.
a bit heavy on the comments here
There was a problem hiding this comment.
Trimmed to the two facts worth keeping, that one step above the oracle write timestamp is the smallest value commit_timestamped accepts, and that a timeline at Timestamp::MAX is a broken environment rather than anything the statement did. 14 lines to 6.
| ); | ||
| continue; | ||
| } | ||
| // The payload has to contain the snapshot the subscribe |
There was a problem hiding this comment.
is our comment duplicating the logic and the assert message?
There was a problem hiding this comment.
It was. The comment is now three lines that say only what the assert message does not: that the branch is unreachable while the caller holds up its end, and that we clamp so the payload rule survives if it ever is not.
|
|
||
| if state.payload.is_empty() { | ||
| // Everything below `target` canceled out. Nothing to write, | ||
| // and the answer speaks about `target` exactly as a write |
There was a problem hiding this comment.
not sure I understand this "speaks about" language, and this comment in general. We need to have our logic explained at the root place where it matters, not all these match arms and ifs
There was a problem hiding this comment.
This one turned into the real fix of the pass, so it is worth reading the diff rather than the reply.
You were right that the logic was scattered, and the reason was that the rule was not stated anywhere, so each site invented its own phrasing for it. The rule is: an answer holds as of the timestamp our view is complete through, and the caller brings the oracle up to that. Stated once, on OccOutcome::NoRowsMatched.empty_as_of, with one helper:
fn empty_as_of(complete_below: Timestamp) -> TimestampUnder that reading the three producers stop being three rules. The channel-close case reports None, emptiness holding at every timestamp. The loop reports empty_as_of(target). And process_message reports empty_as_of(min(ts, fold_target)), which subsumes the two cases you flagged at line 1871 below, since for a first pass fold_target is as_of + 1 and that expression is exactly as_of. The special case for it is deleted.
| /// rising targets each diff moves exactly once, so incremental folding | ||
| /// costs no more than one fold at the end and keeps memory at the | ||
| /// consolidated size in between. | ||
| fn fold_below(&mut self, target: Timestamp) { |
There was a problem hiding this comment.
is it a bit brittle that we have a field current_upper but also a fold that takes a target, and in our doc for OccState we refer to target for stuff. Better to make our contract more tight/self contained, I'd say
There was a problem hiding this comment.
Agreed, and fixed by making the type own the split.
OccState no longer refers to a target it does not have. It has split, payload is the consolidated net strictly below it, pending is the rest, and fold_below raises it. Lowering it was a documented obligation on two call sites that derived the target independently, which is what you are calling brittle. It is now enforced in fold_below: a lower split soft panics and clamps to the previous one, so the payload contract holds either way.
current_upper stays, and its doc now says what it is on its own terms, the last progress timestamp, which certifies no diff will arrive below it.
|
|
||
| /// Moves the diffs the predicate rejects into `payload`, consolidates both | ||
| /// halves, and recomputes `byte_size`. | ||
| fn fold(&mut self, stays_pending: impl Fn(Timestamp) -> bool) { |
There was a problem hiding this comment.
my intuition would have been that the predicate says what get's folded, not the other way rounnd
There was a problem hiding this comment.
Mine too, in hindsight. The closure is gone. fold takes Option<Timestamp>, the split point, None meaning no split, and fold_below / fold_all are the two callers. No predicate to read in either direction.
| return ProcessResult::NoRowsMatched { observed_ts: ts }; | ||
| if ts > as_of && state.payload.is_empty() { | ||
| if as_of.try_step_forward() == Some(fold_target) { | ||
| // The payload is then exactly the net of the times at |
There was a problem hiding this comment.
why do we need these two cases and the lengthy description? It being this long points at maybe a smell here
There was a problem hiding this comment.
It was a smell, and the two cases were one case wearing a disguise. See my reply at line 1576.
Both branches asked "at what timestamp does this emptiness hold". The first pass answers as_of, a raised target answers one below the target, and both are empty_as_of(min(ts, fold_target)). One branch now, guarded by state.is_empty() rather than payload.is_empty().
That guard also removes a behavior change I had had to flag in the PR description. Diffs pending above the target no longer short-circuit to "0 rows": the statement goes on to a write attempt and writes the ones that land below its target. Only a diff at or after the target stays out, which is just concurrency. 30 lines to 15, and one fewer thing to explain.
| The `group_commit_before_apply_write` failpoint holds the winning writer | ||
| inside that window, the same one a second `environmentd` process opens on | ||
| its own with no ordering against local Persist visibility. | ||
|
|
There was a problem hiding this comment.
we're adding a lot of prose without changing much, is that needed throughout this file?
There was a problem hiding this comment.
Trimmed. The docstring keeps the premise it has to pin, that the parked winner puts the frontier past the oracle target so the first attempt loses and the answer comes from the conflict retry, and drops the restatement of the rules themselves. 17 lines to 7, and the two inline blocks 16 to 10.
Nothing else in the file grew, the rest of the diff there is the guard change and an assertion message.
ggevay
left a comment
There was a problem hiding this comment.
LGTM, one wording nit inline.
A Nightly is running on the branch: https://buildkite.com/materialize/nightly/builds/18137 — worth a look before merge. The feature benchmarks are the main interest: the common zero-row statement no longer pays a commit, so ManySmallUpdates should visibly improve.
| /// [`OccOutcome::NoRowsMatched`] must be linearized against its | ||
| /// `empty_as_of` before the response goes out. | ||
| /// | ||
| /// `write_oracle` is `None` only for a selection that pins no timeline. Such |
There was a problem hiding this comment.
nit: "write_oracle is None only for a selection that pins no timeline" (here and at the write_oracle binding above) reads against governing_timeline's own doc, which maps a pins-no-timeline read side (TimestampDependent) to EpochMilliseconds and reserves None for timestamp-independent selections. "Timestamp-independent" in both spots would remove the contradiction.
There was a problem hiding this comment.
Right on both spots, and the contradiction is with a doc in the same file, which makes it worse. get_timeline maps TimestampDependent to Some(EpochMilliseconds) and only TimestampIndependent to None (timestamp_selection.rs:213-219), and governing_timeline's own doc uses "pins no timeline" as the label for the former. So my two spots called None the case that is actually Some. Both now say "timestamp-independent selection". Pushed.
1ff7c9b to
0c1723a
Compare
|
I looked at the Nightly, and the feature benchmark does not show the improvement. It is worth writing down why, because the expectation was mine to begin with, from a paragraph in the design doc.
0.2% is nothing, and the run bounds how much it could have been. Individual measurements of a single build came in at 0.650, 0.657, 0.663, 0.672 and 0.679, a spread of about 4%, so anything under roughly 30ms is invisible here. Simulating the scenario's arithmetic, 100 statements over 10 rows with all matched rows collapsing to one value, gives 67 to 82 zero-row statements depending on seed. So the commit this removes is worth under about half a millisecond per statement in that setup, and the 3.5x that scenario carries is dominated by the rest of the per-statement work, above all a subscribe dataflow installed and torn down for every statement. Things I checked before concluding that, because a null result is usually a broken measurement:
So the mechanism is real and the effect is below this benchmark's noise floor. I have corrected the design doc, which implied the residue largely disappears for this workload and invited exactly this expectation, and the PR description now says the same. If we want the scenario faster, the target is the per-statement subscribe, not this. On the rest of that Nightly: 4 real failures, — AJ, aljoscha's agent |
0c1723a to
7e2f679
Compare
|
Retracting my previous comment: it was wrong. The improvement is real, and it is large. I had drawn a conclusion from the Nightly alone, and the Nightly's baseline number is the thing that does not reproduce. Measured locally on the two CI-built images, with the path forced on for both sides and the flag verified inside each running container:
No measurement on one side overlaps the other in any run, and the control pins both machine noise and "these two images are otherwise equivalent". The part that convinces me it is the intended mechanism and not a machine artifact is the group-commit counter, Both sides ran the OCC path in all of this: What I cannot explain is the Nightly, which reported 0.648 against 0.649. I checked the things that would make it vacuous and they all came back the other way: no The design doc and the PR description now carry the measured numbers, including that the scenario is still ~2x the lock path, so the rest of its gap is the per-statement subscribe dataflow rather than anything this change touches. Apologies for the noise of a wrong conclusion in between. — AJ, aljoscha's agent |
7e2f679 to
bac6265
Compare
QA LLM Review1. MEDIUM -- zero-row linearization test now passes vacuously
Dropping the DetailsLet The UPDATE therefore picks So by the time the loop adopts This is not the old behaviour: at the merge base the loop concluded emptiness at the The docstring's claim that the retry lands "still at a timestamp the oracle has not To restore a witness the emptiness has to be concluded without a round trip through the |
bac6265 to
dfe4065
Compare
|
Confirmed and fixed. The finding is right, and I checked the mechanism rather than the narrative: Reworked along the lines you suggested. The winner is now a blind UPDATE t SET v = v + 1 WHERE k = <key> AND NOT EXISTS (SELECT 1 FROM guard WHERE g = <key>)So The witness is now the guard row rather than the target row. Two things I added on top of the suggestion:
Also dropped the — AJ, aljoscha's agent |
…8320) Part of: [SQL-641](https://linear.app/materializeinc/issue/SQL-641) Safeguard for the write timestamp the frontend OCC read-then-write path chooses. Independent of #38260. #38322 stacks on this and makes the far-future case work rather than merely be refused. ## The hole `GroupCommitter::commit_timestamped` documented an obligation it never discharged: ``` /// * The wall-clock throttle. `target_timestamp` is the caller's to choose, /// and it must not run the write timeline ahead of the clock. ``` Nothing checked it. Every other write allocates from the oracle, `GREATEST(write_ts + 1, now())`, so it cannot jump. A caller-chosen target can, and the oracle is monotone and durable: * Under the default **strict serializable**, every later write and read on the timeline blocks until the wall clock catches up. * Under **serializable** it is worse. Those reads never consult the oracle (`needs_linearized_read_ts` is false for them), so they pick a timestamp near the clock and an acknowledged write stays invisible. * It **survives restart** through the catalog shard's upper, which boot re-applies to the oracle. The startup sleep that once waited for the clock is gone. The frontend path produces such a target today, no bug involved: it writes at the frontier its subscribe observed, and a selection over a `REFRESH` materialized view settles until its next refresh. With `REFRESH EVERY '15 seconds'` that is a ~6s oracle jump. With a coarser interval it is a stalled environment. ## What this does **Enforces the bound where it is stated.** A target above `write_ts_upper_bound(now)` is refused before the append, and the session turns `WriteResult::TimestampTooFarAhead` into a statement error. The ceiling is the same `now + 2 * 5000ms` that `check_runaway_write_ts` already measured against, now named. A backstop, not the fix: the worst outcome becomes one failed statement instead of a stalled timeline, for any caller of the timestamped-write API. **Leaves the detector logging.** `check_runaway_write_ts` stays an `error!`. With the refusal above in place, every way left to reach it is a runaway this process inherited, a durable one re-applied at boot or a backwards clock step, and the group committer's throttle is written to wait those out. Failing there would turn a stalled timeline into a crash loop, which is the opposite of what the boot check below decides. **Reports the restart channel.** Boot logs an error when the catalog upper is already past the bound. It does not refuse to start: the timeline is stalled either way, and a process that will not boot turns that into an outage plus a crash loop. ## Tests `test_far_future_write_timestamp_is_refused` builds a genuinely far-future target from a materialized view with two refreshes, one seconds out so the view becomes readable, one in the year 3000 so the target is deterministic rather than racing a refresh interval. It asserts the refusal, that the oracle did not move (checked before reading the target back, since a read cannot be served once the oracle is out there), and that the timeline still takes a write and a read. The error is `FEATURE_NOT_SUPPORTED` here because retrying does not help while the frontier stays out there. #38322 changes the producer set to "the oracle itself ran away" and moves it to `INTERNAL_ERROR` with a hint that matches.
dfe4065 to
85e32a0
Compare
| read timestamp to at least the write timestamp, so at write time it holds that | ||
| `write_ts <= oracle_read_ts`. The linearization invariant is maintained. | ||
|
|
||
| A statement that matches no rows has no write to ride on, and it still made a |
There was a problem hiding this comment.
one last thing, we need to make this and the next paragraph more concise, just describe what happens and why and why it's correct please, none of that "rides on" messaging
There was a problem hiding this comment.
Cut to two paragraphs, and the "rides on" framing is gone from here and from the module doc, which had the same phrasing.
What is left is what happens (no write, so nothing advances the oracle for it, it reports the timestamp its view is complete through and waits for the oracle to reach that), why the common case is free (empty at as_of means complete through as_of, already linearized), and why it is correct (the answer describes the timestamp it reports, later reads land at or above it, and the hazard is only the other direction). 19 lines to 15.
— AJ, aljoscha's agent
| statement. The residue is the price of the linearization guarantee rather than a | ||
| defect: correctness requires the oracle to advance, and only a commit advances | ||
| it. | ||
| roughly 90% of its 100 updates end up matching no rows. A zero-row answer has to |
There was a problem hiding this comment.
please cut most of this, but maybe add a short "Note: " section that explains the updated experiment results
There was a problem hiding this comment.
Cut. The original paragraph is back, with one correction so it is not stating something false: a selection already empty at its as_of needs no commit at all, and the residue is the selection that empties later. 30 lines of mine down to 11.
The numbers moved into a short Note: after it: 1.6x to 2.3x on ManySmallUpdates, the control, group commits per statement falling from ~1.09 to 0.13-0.37, and the remaining ~2x against the lock path being the per-statement subscribe dataflow. The note says the figures above it were taken before these two changes, since that is the only reason the two sets of numbers can sit next to each other without contradicting.
— AJ, aljoscha's agent
85e32a0 to
a52a661
Compare
| //! the table moved past `T`, so the compare-and-append refuses and the loop | ||
| //! retries higher. Persist arbitrates the timestamp, not the frontier. | ||
| //! | ||
| //! ## A zero-row answer |
There was a problem hiding this comment.
this should closely match the design doc, or be quite a bit shorter and reference the design doc please
There was a problem hiding this comment.
Shortened both sections and made the design doc the place the argument lives, since it did not actually hold the rationale I would have been pointing at. ### The OCC loop gains it: the target comes from the oracle rather than the frontier because a frontier certifies what the loop has a complete view of, which is a different question from which timestamp to write at, an input can legitimately sit far in the future (a REFRESH materialized view), the three invariants, and the note that frontier >= T does not make them equal so a refusal is the design.
The module doc keeps only what someone reading this file needs and points there:
- "The frontier certifies, the oracle chooses": 19 lines to 16, and what is left is the three invariants, which are the off-by-ones the code turns on, plus the NOTE that a refusal is expected rather than a bug. The motivation moved out.
- "A zero-row answer": 19 lines to 9, one paragraph with the rule and the reason the common case is free, pointing at
### Linearizationfor why the lower timestamp is not weaker.
The design doc is 20 lines longer, the module doc 22 shorter.
— AJ, aljoscha's agent
a52a661 to
08a0b7b
Compare
The OCC loop wrote at the frontier its subscribe reported, which conflates two jobs. A frontier certifies what the loop has a complete view of. Choosing the timestamp to write at is a separate decision, and the two coincide only because the target table is usually what pins the frontier. They come apart when another input does. A materialized view with a `REFRESH` option settles until its next refresh, so a selection over one reports a frontier hours or days ahead of the clock while the target table's upper is still near it. Writing there ratchets the timeline's oracle into the future, and the oracle is monotone and durable, so every later write and strict-serializable read blocks until the clock catches up. Under `serializable` the write is simply invisible. So the oracle chooses and the frontier certifies. The target `T` is one step above the oracle's write timestamp, the smallest value `commit_timestamped` accepts, and a conflict hands back the next eligible one. The loop writes at `T` once the frontier reaches `T`, with every diff from strictly below `T` as the payload and everything at or after it held back for a later target. Three boundaries carry the correctness. A progress message at `F` certifies completeness below `F`, so readiness is `F >= T`. The payload is `t < T` strictly, because a diff at `T` is concurrent with the write. And `T > as_of`, because the snapshot arrives at `as_of` and has to be in the payload, which the pre-read linearization guarantees by leaving the oracle at or above `as_of`. Readiness gives `T <= F`, not equality, so the payload can exclude diffs the subscribe already delivered in `[T, F)`. For a selection that reads the target table those say the table moved past `T`, the compare-and-append refuses, and the loop retries at the timestamp the refusal names. Persist arbitrates, not the frontier. This path produces that window itself: the committer appends at the target and applies it to the oracle only afterwards, so while one write sits in between, a second statement's target is a step behind the table's upper. Retries converge because both refusals name a strictly higher timestamp and the loop waits for the frontier to certify each one. A zero-row answer no longer waits on a frontier either. It reports the timestamp the emptiness holds as of, one below whichever of the frontier and the target certifies less. For the common `UPDATE ... WHERE <no match>` that is the statement's own `as_of`, already linearized before the subscribe started, so the answer costs no group commit, and the two cases the loop used to separate turn out to be the same rule. Tests: the far-future refresh-MV write commits near the clock instead of being refused, and a `serializable` session reads back its own such write, which pins the anomaly this closes. Unit tests cover the fold's boundaries, where the off-by-ones live. The zero-row linearization workflow now parks its winner with a blind write rather than a timestamped one, so the answer is reached without a round trip through the group committer and the wait for the oracle is observable at all.
08a0b7b to
f1ade3e
Compare
The benchmark story, settledThird and final word on The nightly on this branch reports 0.761s against 0.755s, no change, which reproduces the earlier nightly row rather than contradicting it. My local A/B reported a large win. Both are correct measurements, and I finally know why they differ. What I checked and ruled out.
What it actually is. The change removes the commit, and that part is environment independent: group commits per statement fall from about 1.09 to 0.13-0.37, roughly 75 fewer over the scenario's 100 statements. What the removal is worth in wallclock is whatever those commits cost. Locally 75 commits account for 0.6s, about 8ms each, a metadata store round trip on that disk. On the benchmark agents the same 75 commits vanish into run-to-run noise. The part worth keeping. The feature benchmark cannot see this improvement, which also means it would not flag a future change that put the commit back. That is now recorded in the design doc's performance section along with both sets of numbers, rather than the single-environment "1.6x to 2.3x" claim I had put there, which was overclaiming a local result as a general one. My process error, stated plainly: I measured wallclock first and the mechanism last. The group-commit counter is the environment-independent quantity and should have been the headline from the start. — AJ, aljoscha's agent |
Follow-up to MaterializeInc#38322, so currently based on top of it
Closes: SQL-641
Stacked on #38320. Makes the far-future write timestamp correct rather than merely refused. Rebased onto main now that #37924 has landed.
The conflation
The OCC loop wrote at the frontier its subscribe reported. A frontier certifies what the loop has a complete view of. Choosing which timestamp to write at is a separate decision, and the two coincide only because the target table is usually what pins the frontier. Nothing in the code stated that dependency, and the comment on the conflict arm was the same constraint seen from the other side, satisfied by pulling the target down to the frontier.
They come apart when another input pins it. A materialized view with a
REFRESHoption settles until its next refresh, so a selection over one reports a frontier hours or days out while the target table's upper is still near the clock.The scheme
T = peek_write_ts + 1, the smallest valuecommit_timestampedaccepts. A conflict hands backnext_eligible_timestamp, adopted verbatim.F >= T. A progress message atFcertifies completeness strictly belowF, which is the snapshot atT - 1.t < T, strictly. A diff atTis concurrent with the write and waits for a later target.T > as_of, because the snapshot arrives atas_ofand has to be in the payload.ensure_read_linearized(as_of)guarantees it by leaving the oracle at or aboveas_of.Readiness gives
T <= F, not equality. WhereFis aboveTthe payload excludes diffs in[T, F), and for a selection reading the target table those say the table moved pastT, so the compare-and-append refuses and the loop retries higher. Persist arbitrates, not the frontier. This path produces that window itself: the committer appends at the target and applies it to the oracle only afterwards, so while one write sits in between, a second statement's target is a step behind the table's upper. Retries converge because both refusals name a strictly higher timestamp and the loop waits for the frontier to certify each one.Zero rows stop waiting on a frontier
NoRowsMatchedlinearized against the frontier the emptiness was concluded at, which is what made a settled-frontier statement park for days. The rule is now stated once, onOccOutcome::NoRowsMatched.empty_as_of: the answer holds as of the timestamp our view is complete through, and the caller brings the oracle up to that. It isempty_as_of(min(frontier, target)), whichever certifies less. For the commonUPDATE ... WHERE <no match>that is the statement's ownas_of, linearized before the subscribe started, so the answer costs no group commit at all.That removes the commit a zero-row statement used to need: group commits per statement fall from about 1.09 to 0.13-0.37, some 75 fewer over
ManySmallUpdates' 100 statements. What that is worth in wallclock depends on what a commit costs. Comparing the same two images, the scenario runs 0.73s against 1.34s locally and 0.761s against 0.755s on the CI benchmark agents, so the feature benchmark cannot see this improvement. See the comment below for the full measurement.Also
OccStatesplits into a consolidatedpayload(below the target) and timestampedpending(at or after it), which subsumesmax_data_tsand the guard that refused to write when a diff at or after the target had been accumulated.byte_sizespans both halves, somax_result_sizestill measures everything accumulated.Net effect: no timestamp this path writes at, and no wait it performs, is governed by an input's frontier. Every one is governed by the oracle, which is clamped to the clock. That is also what makes the path immune to a frontier that is wrong rather than merely far out, which is how #38260 presented.
Tests
test_refresh_mv_write_commits_near_wall_clock: the far-future case from adapter: refuse a write timestamp past the write timeline's bound #38320 succeeds, commits 3 rows, leaves the oracle near the clock, and the timeline still takes writes.test_serializable_read_sees_own_refresh_mv_write: aserializablesession reads back its own refresh-MV-sourced write. This is the anomaly the old behavior produced, invisible to strict-serializable tests because those block instead.workflow_test_occ_zero_row_write_linearizationis reworked so that it still witnesses what it claims. Its winner is now a blindINSERTinto a table the selection is gated on, rather than an OCCDELETEon the target. A timestamped winner leaves the oracle untouched while parked, so the UPDATE would pick the winner's own timestamp, submit, and queue behind it on the group committer, and its answer would only come back after the winner applied. A blind winner allocates before parking, so the UPDATE's target clears it, the answer is reached with no submission, and the linearization wait is observable again.Reviewer notes
One behavior change: a statement whose frontier lags the oracle ends in
StatementTimeoutrather thanReadThenWriteContention, since it waits for the frontier instead of burning retries, and holds its OCC permit while it waits. The design doc describes this as the intended behavior now.A diff that arrives at or after the chosen target is not in the statement's answer, which is ordinary concurrency rather than a change of behavior. Diffs below the target are written as before.
Two things left undone on purpose:
UpperConflictreportsnext_eligible = target + 1whenInvalidUppersalready carries the real upper. Plumbing it through would converge in one retry instead ofk.kis 1 or 2 in practice.