adapter: use occ for read-then write, with incremental retries#35192
Draft
aljoscha wants to merge 15 commits into
Draft
adapter: use occ for read-then write, with incremental retries#35192aljoscha wants to merge 15 commits into
aljoscha wants to merge 15 commits into
Conversation
Contributor
|
Thanks for opening this PR! Here are a few tips to help make the review process smooth for everyone. PR title guidelines
Pre-merge checklist
|
aljoscha
force-pushed
the
adapter-read-then-write-occ-incremental
branch
4 times, most recently
from
February 27, 2026 13:59
1675464 to
2cb8e6d
Compare
aljoscha
force-pushed
the
adapter-read-then-write-occ-incremental
branch
from
March 16, 2026 15:30
865091f to
22a54a2
Compare
aljoscha
force-pushed
the
adapter-read-then-write-occ-incremental
branch
11 times, most recently
from
April 23, 2026 10:33
179deb6 to
8b8d81c
Compare
aljoscha
force-pushed
the
adapter-read-then-write-occ-incremental
branch
10 times, most recently
from
April 28, 2026 14:39
aff3f2e to
f444d6d
Compare
aljoscha
force-pushed
the
adapter-read-then-write-occ-incremental
branch
2 times, most recently
from
May 4, 2026 10:48
9ad320f to
fc3f78c
Compare
aljoscha
force-pushed
the
adapter-read-then-write-occ-incremental
branch
4 times, most recently
from
May 7, 2026 16:09
5d95752 to
4cce320
Compare
aljoscha
force-pushed
the
adapter-read-then-write-occ-incremental
branch
2 times, most recently
from
May 21, 2026 13:07
4a12808 to
4d539be
Compare
aljoscha
force-pushed
the
adapter-read-then-write-occ-incremental
branch
6 times, most recently
from
June 9, 2026 17:27
c47d196 to
f4cdd4c
Compare
aljoscha
force-pushed
the
adapter-read-then-write-occ-incremental
branch
from
July 22, 2026 16:29
f4cdd4c to
08eada6
Compare
Behavior-preserving changes peeled off the frontend OCC read-then-write commit so each can be reviewed independently: - Refactor `unroll_sql_execute` to use the `StatementLoggingGuard` abstraction (`begin_statement_logging` + a new `StatementLoggingGuard::retire`) instead of hand-rolled `begin_statement_execution` / `log_began_execution` / `log_ended_execution` calls. This mirrors how `frontend_peek` already drives the guard; the begin path is still keyed on `outer_ctx_extra.is_none()`, the error path retires with `Errored`, and the success path defuses and reuses the id, so behavior is unchanged. - Document that Materialize's `UPDATE t SET x = x` reports 0 affected rows (the +1/-1 diffs consolidate away), diverging from PostgreSQL. Comment only. - Deflake `test_subscribe_outlive_cluster` by retrying the post-cancel `ROLLBACK`, which can race with the asynchronous query cancel. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
We now use SUBSCRIBE instead of PEEK to maintain the desired set of updates that need to be written. We also don't acquire locks on tables but instead optimistically try and write our updates at the timestamp right at our current subscribe frontier. Additionally, we take the opportunity this provides and move the sequencing code from the coordinator main loop to the frontend, similar to how we have done that for peeks in frontend_peek.rs. Work towards MaterializeInc/database-issues#6686
Startup-only dyncfgs such as the frontend OCC read-then-write flag are read once during coordinator construction from the dyncfg `ConfigSet` mirror (`system_config().dyncfgs()`), before coordinator bootstrap runs its own sync. During catalog open that mirror is only synced from the `SystemVars` value map when a durable `SystemConfiguration` update is applied (`apply_updates` -> `dyncfg_updates`, gated on `update_system_config`). Values supplied purely via `system_parameter_default` -- e.g. test `with_system_parameter_default`, or LaunchDarkly defaults with no persisted catalog value -- set the var defaults but never trigger that sync, so the startup read observed the compile-time default instead of the configured one. Mirror the effective values into the `ConfigSet` right after the `pre_item_updates` apply, where both the defaults and any durable `SystemConfiguration` values are present. Reading effective values there keeps explicit (LaunchDarkly / catalog) values taking precedence over defaults. This also closes the same latent gap for the `ENABLE_EXPRESSION_CACHE` startup read in deployments with no durable system configuration. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The Performance section claimed low concurrency is "comparable or better than main". Measuring the full implementation shows that holds only for large writes (the subscribe streams diffs directly, while the old path peeks every matched row and recomputes diffs). Small writes regress ~1.5-2x at low/no concurrency due to per-operation subscribe install/snapshot/teardown that the legacy fast-path peek avoids (nightly ManySmallUpdates, scalability UpdateWorkload). This is an accepted cost: high write throughput is a non-goal. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds an adversarial QA suite for the frontend OCC read-then-write path (`enable_adapter_frontend_occ_read_then_write`) in src/environmentd/tests/server.rs, plus a findings writeup next to the design doc. Passing correctness/edge-case tests (active): - concurrent DELETE multiset over-delete safety - multiset multiplicity in DELETE/UPDATE/INSERT...SELECT counts - NOT NULL enforcement (no partial commit) for UPDATE and INSERT...SELECT - INSERT RETURNING values (constant and read-dependent) - row-moving UPDATE correctness through the Let/Negate/map MIR transform - INSERT...SELECT from a materialized view - concurrent mixed DML produces no internal errors / coordinator panics Bug-repro tests (#[ignore]d so CI stays green; run with --run-ignored): - ensure_read_linearized ignores statement_timeout for a far-future as_of (e.g. reading a REFRESH AT far-future MV): the session hangs indefinitely, contradicting the code comment that claims it "hits statement_timeout". - a single hung far-future RTW holds an OCC semaphore permit for its entire lifetime, starving the global permit pool and wedging all read-then-writes; victims also ignore statement_timeout because they block on permit acquisition before the OCC loop where the timeout is enforced. - legacy-path companion showing the indefinite hang itself is pre-existing (the OCC-specific harm is the permit-pool amplification + the false comment). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rally `ensure_read_linearized` parks on a `tokio::time::sleep` loop until the oracle reaches `as_of`, with no statement-timeout check. For a far-future `as_of` (e.g. reading a `REFRESH AT <far future>` MV) the session hung indefinitely, and — worse — held an OCC semaphore permit the whole time, so a few such ops starved the permit pool and wedged all read-then-writes (victims block on permit acquisition, before the OCC loop where the timeout was enforced). Enforce statement_timeout in one central place: the existing tokio::select! in `try_frontend_read_then_write_with_cancel`, which already owns the whole operation's lifetime and handles cancellation. A new arm sleeps for statement_timeout (0 => off, via a pending future), and on fire returns StatementTimeout and forwards a privileged cancel to the coordinator (mirroring the cancel arm). Dropping the operation future releases the OCC permit, read holds, and subscribe handle. This bounds every phase — planning, permit acquisition, timestamp determination, linearization, and the OCC loop. The in-loop run_occ_loop timeout is kept as defense-in-depth. Adds QA tests (src/environmentd/tests/server.rs, prefix qa_occ_/qa_legacy_) covering this fix and OCC correctness (multiset over-delete safety, multiplicity counts, NOT NULL, INSERT RETURNING, row-moving UPDATE, INSERT...SELECT from an MV, concurrent mixed DML). The legacy-path far-future hang is documented but left unchanged (out of scope; #[ignore]d test). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Apply formatting after the semantic rebase and let periodic group commit retry frontend blind writes that cannot immediately acquire their table locks. This avoids a coordinator-loop retry spin while preserving lock synchronization with the legacy path.
A timestamp conflict has a definitive non-durable outcome. Clear the submitted state after every write response so a later cancellation or statement timeout can stop while the subscribe waits for fresh progress.
Restore the original handling of explicit transactions in the frontend DML path: constant INSERTs are blind writes that compose with explicit transactions via the session's write ops, so they must be handled before the transaction-mode check. Non-constant DML in explicit transactions is rejected with OperationProhibitsTransaction. The previous arrangement bailed out to the coordinator early, which broke constant INSERTs in explicit transactions and made non-constant ones trip the coordinator's read-then-write failsafe at COMMIT. Also avoid a silent as conversion when encoding the cancellation reason and regenerate the metrics catalog for the new OCC retry histogram.
The disallowed-methods lint on postgres::Client::batch_execute landed with per-function allows on existing tests. Apply the same convention to the frontend OCC read-then-write tests.
Gate frontend DML on the transaction state exactly like the coordinator's handle_execute: in a multi-statement transaction (implicit batch or explicit block) only an AST-constant INSERT without RETURNING may proceed, everything else is prohibited in a transaction block, with the coordinator's redaction rules. Previously the gate keyed on is_implicit(), which admitted multi-statement implicit batches into the OCC loop. An OCC write commits durably mid-batch and cannot be rolled back when a later statement in the batch fails, silently breaking the batch's atomicity. The gate runs before planning so error precedence matches the coordinator, and a backstop before the OCC loop preserves the dedicated mz_now() error for AST-constant INSERTs whose planned expression turns out non-constant. Also from review: * Only clear write_submitted on TimestampPassed, the one outcome where the write definitively did not land and the attempt keeps running. Terminal outcomes leave it set so a concurrent cancellation can never fabricate an error for a write that may have committed. Document the FrontendWriteAttemptState contract. * Retire EXECUTE-level statement logging as errored when the frontend read-then-write path fails before taking logging ownership, instead of letting the guard drop record the statement as aborted. * Destructure the single-table invariant of internal writes in group commit staging and soft-panic on violation instead of misreporting it as TargetChanged. * Document the internal write lock retry behavior, the cancel watch operation-id scoping, and add a regression test for DML in multi-statement implicit batches.
postgres::Error's Display is just the error kind, the server message has to be read from as_db_error(), like the neighboring tests do. The multi-statement batch test asserted on Display and always failed.
…rtup-only vars Two behavior changes from review of the frontend OCC read-then-write path. An INSERT whose values read no persisted state is a blind write. Its diffs do not depend on any read frontier, so they can wait for the session's transaction to commit. The OCC loop already runs such a selection as a dataflow and already recognizes it, the subscribe finishes and it takes the blind-write branch. Inside a transaction we now hand those diffs back and buffer them as session write ops, so the transaction's commit submits them to group commit like any other buffered write. This restores parity with the coordinator for values that are constant in the AST but too large to fold into a literal, for example INSERT INTO t SELECT generate_series(1, 20000), which previously errored with the flag on. A read-dependent write can never be deferred, and is already rejected before it reaches the loop, so the timestamped write path soft-panics if it ever sees a deferred write. Startup-only system parameters no longer reject ALTER SYSTEM SET, RESET, and RESET ALL. The routing decision is sampled once at boot and every session inherits it, so a runtime change cannot make two write paths live at once, it only fails to take effect until the next restart. We now let the change through and warn that it needs a restart, instead of blocking RESET ALL entirely for any environment where the value was set by the system parameter sync.
aljoscha
force-pushed
the
adapter-read-then-write-occ-incremental
branch
from
July 25, 2026 12:22
087b4ab to
77e21c1
Compare
Follow-up from review of the frontend OCC read-then-write path. The frontend used the marker add_transaction_ops call as its writability check, which the coordinator does not. sequence_insert checks allows_writes() first and reports ReadOnlyTransaction, while the marker op only rejects a subset of the non-writable states and reports its own errors for them. A write after ALTER TABLE RENAME, after a subscribe FETCH, or after a DDL statement therefore surfaced a different error and SQLSTATE depending on which path sequenced it. The frontend now runs the same pre-check before it does any work. The deferral invariant, that a write reaching the loop inside a transaction reads no persisted state, held only by coincidence of three distant properties. The AST gate matches on the raw statement and lets AST-constant INSERTs through, and an AST-constant statement can plan to a selection with Get nodes, because SQL-implemented builtins read system relations. Only the unrelated system-relation rejection in validate_read_then_write_dependencies kept such a statement out of the loop. The invariant is now local: after planning, where the statement is still in hand for the error message, a write with read dependencies inside a transaction gets the same prohibited-in-transaction error the gate produces, before we execute a dataflow for it. The soft-panic guard in the loop stays as defense in depth. The startup-only warning fired before catalog_transact, so a rejected ALTER SYSTEM SET warned about a change that never happened. Both notices move after the successful transaction. SET and RESET stay name-based, the operator named the parameter so echoing the restart requirement is always appropriate, while RESET ALL stays value-based because it names every parameter. The notice now describes the requirement rather than asserting that a change occurred, which is true for both policies. With the rejection of ALTER SYSTEM SET gone, max_concurrent_occ_writes became settable to 0, which sizes the OCC semaphore to zero permits and blocks every read-then-write until its statement timeout after the next restart. The parameter now carries a domain constraint requiring at least 1. Finally, a note records that buffered session writes carry no target-generation guard, unlike the immediate path: a WriteOp only names the CatalogItemId and commit staging resolves whatever global id is current, so a concurrent ALTER TABLE ADD COLUMN is unsafe for any buffered write. That is pre-existing and not fixed here.
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
1 participant
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.
We now use SUBSCRIBE instead of PEEK to maintain the desired set of
updates that need to be written. We also don't acquire locks on tables
but instead optimistically try and write our updates at the timestamp
right at our current subscribe frontier.
Additionally, we take the opportunity this provides and move the
sequencing code from the coordinator main loop to the frontend, similar
to how we have done that for peeks in frontend_peek.rs.
Work towards https://github.com/MaterializeInc/database-issues/issues/6686
Implementation of https://github.com/MaterializeInc/materialize/blob/63645b72e83ee26d2cfa99d25d773a06e6accb74/doc/developer/design/20260210_incremental_occ_read_then_write.md