adapter: stage a blind read-then-write, let a read-dependent one stand alone

[aljoscha]

Motivation

Part 7 of 7 in a stack that moves DELETE, UPDATE and INSERT ... SELECT off the coordinator onto the session task, using optimistic concurrency control. Design doc: 20260210_incremental_occ_read_then_write.md.

Part 6 refuses a read-then-write inside a multi-statement transaction, because a write that commits immediately cannot be rolled back at transaction end. That is the right answer only for a write that reads persisted state. This part works out what the right answer is for the other kind, and what "inside a transaction" actually means.

Closes SQL-593

An extended-protocol pipeline is an implicit transaction

This is the fact the whole change turns on. Parse/Bind/Execute repeated and then Sync is one implicit transaction in PostgreSQL: if any statement fails, everything the pipeline wrote rolls back. Clients rely on it, pgx SendBatch, psycopg pipeline mode and libpq pipelining all do. SQL-470 fixed exactly this for Materialize and was marked High.

In the session state machine that pipeline is TransactionStatus::Started, and is_implicit() returns true for it. Started is genuinely ambiguous, as the comment at command_handler.rs:1357 says: it is either exactly one statement from the simple protocol, or some statement from the extended protocol that may have others after it before Sync. Nothing at execute time can tell the two apart.

An implicit transaction is allowed to stay open across a pipeline only under a specific condition, spelled out on TransactionStatus::may_span_pipeline: only writes may, "because they are merely staged". Staging is what earns the right to span.

What was wrong

The OCC path marked the session with Writes ops, which buys that right, and then wrote its rows anyway. The transaction stayed open on the promise that the rows were merely staged, while they were already durable. A later failure in the pipeline rolled back nothing.

The gate keyed on is_in_multi_statement_transaction(), which reports false for Started. Keying it on contains_ops() as well fixes only the case where something is staged in front of the statement. A read-then-write that opens a pipeline has nothing in front of it and was equally broken, in both directions:

Parse "INSERT INTO t SELECT generate_series(1, 20000)"; Bind; Execute
Parse "SELECT 1/0"; Bind; Execute
Sync
-- PostgreSQL: 0 rows.  Before this PR: 20000 rows.

The semantics this PR settles

Two kinds of read-then-write meet a transaction differently, split on the predicate that already exists, whether the selection reads persisted state.

A write that reads nothing can belong to a transaction. Its diffs do not come from a snapshot, so they are valid at any timestamp. We stage them as session write ops and let the transaction flush them, exactly as a constant INSERT already does from this same path. It commits or rolls back with everything around it.

A write that reads persisted state cannot belong to one. Its diffs are only correct at the frontier they were observed at, so they commit inside the OCC loop, and we already refuse such a statement in an explicit transaction. It must not quietly join an implicit one either. So it commits as its own transaction and clears the ops it staged, which leaves may_span_pipeline false and makes pgwire end the transaction rather than let the rest of the pipeline join it.

Only the syntactic predicate may decide this. The two predicates answer different questions and they disagree for a sealed input. A REFRESH AT materialized view past its last refresh has an empty write frontier, so the subscribe over it closes on its own, and an INSERT ... SELECT over that view reads persisted state while still taking the closed-channel exit. Its diffs really are frontier-independent, so staging them would be safe, and we still do not stage them. Otherwise whether a statement's rows survive a later failure in the pipeline would depend on whether one of its inputs happened to pass its last refresh, which nobody reading the statement could predict. That case takes the blind submission and then ends its own transaction like any other read-dependent write.

That second rule is not an invention. It is how PostgreSQL treats statements that cannot run in a transaction block, and it is what the Started comment in command_handler already describes as PostgreSQL's approach. The resulting guarantee is worth stating plainly, because it is a deliberate divergence from PostgreSQL, where DELETE is transactional:

A read-then-write that reads persisted state is durable once it reports success. A later failure in the same pipeline does not undo it.

We can say that because Materialize already refuses these statements in a transaction block, so no user can be relying on them being transactional. Making them genuinely transactional would mean re-validating the OCC conflict check at commit time, which is a larger design and not in this PR.

One welcome side effect: because such a statement now ends its own transaction, a second one in the same pipeline is no longer refused for running alongside staged ops.

Consequences worth knowing

Staging a blind write removes a coordinator round trip rather than adding one. The rows ride along in the commit that was already going to happen, instead of taking a separate Command::AttemptWrite first and then committing anyway. submit_blind_write is deleted.

A staged WriteOp names only the CatalogItemId, so it does not carry the target-generation guard that the immediate path got from pinning target_global_id. An ALTER TABLE ... ADD COLUMN landing between execution and commit would append rows of the old arity. This is true of every staged write, including constant INSERT and the lock-based path, so blind writes now share the existing guarantee rather than a weaker one of their own. Read-dependent writes still commit in the loop and keep the pinned generation, which test_write_racing_alter_table_add_column covers. Carrying the expected generation into staged writes would fix this for all of them and is worth doing separately.

Statement logging

A staged write does not choose its own write timestamp, so it no longer records an execution_timestamp in mz_statement_execution_history. This matters because the frontend path is going to become the default, so what it records is what users will see.

The reason is structural rather than incidental, and it is the same on both paths. GroupCommitApplied back-fills the execution timestamp only for statements still live at commit, since "retiring ends the statement execution and drops its logging record", and PendingWriteTxn::User attributes it to whichever context is committing. A staged write reports its result and retires before the commit that gives its rows a timestamp, so nothing back-fills it. The coordinator has always behaved this way for every read-then-write. test_statement_logging_dml_path_parity is updated to expect the two paths to agree on INSERT ... VALUES ... RETURNING.

So this makes the eventual flag flip invisible for blind writes rather than swapping one behavior for another. Read-dependent writes are the opposite case and the divergence there is deliberate: the OCC loop picks a write timestamp inside the statement's lifetime, so the frontend records one where the coordinator never could. Turning the flag on will start populating execution_timestamp for UPDATE, DELETE and INSERT ... SELECT. That is accurate and strictly more information, and the parity test pins it per case so it cannot drift unnoticed.

Verification

test/pgtest-mz/frontend-occ-pipelined-dml.pt pins all six pipeline cases, each with a comment saying which rule it exercises:

pipeline	expected
staged write, then read-dependent write	refused, 25001
staged write, then blind write, then 1/0	both roll back
blind write opening the pipeline, then 1/0	rolls back
read-dependent write opening the pipeline, then 1/0	commits, survives
two read-dependent writes in one pipeline	both apply, neither refused

The two blind-write cases were verified to fail before the change (20000 rows instead of 0) and pass after. The read-dependent cases pin semantics rather than catching a regression, and are commented as such.

test/cluster/mzcompose.py adds test-occ-sealed-input-write-stands-alone, which covers the sealed-input case that the pgtest cannot: it needs a REFRESH AT CREATION materialized view to seal, so it waits on mz_frontiers for an empty write frontier before writing. It sends the write and a failing statement as one psycopg pipeline, checks mz_occ_read_then_write_retry_count to confirm the frontend actually sequenced the write rather than the coordinator, and uses a table-backed control write to prove the harness really does leave a transaction open. Thanks to @def- for writing it.

Full read_then_write suite (27 tests) and the pgwire suite pass.

Unrelated, in the same PR

max_concurrent_occ_writes is sampled once at startup, so ALTER SYSTEM SET on it silently did nothing. The statement is still accepted, and now warns that the change takes effect when environmentd restarts. RESET ALL names every parameter rather than one, so it compares values instead of names. The parameter also gets a domain constraint of at least 1, since zero permits would leave every read-then-write waiting out its statement_timeout.

[linear-code]

SQL-593

[def-]

This seems to regress SQL-470. Extra test:

diff --git a/test/pgtest-mz/frontend-occ-pipelined-dml.pt b/test/pgtest-mz/frontend-occ-pipelined-dml.pt
index f0b5423d8c..bd4eb44a8a 100644
--- a/test/pgtest-mz/frontend-occ-pipelined-dml.pt
+++ b/test/pgtest-mz/frontend-occ-pipelined-dml.pt
@@ -1,11 +1,11 @@
 # Test that a read-then-write pipelined behind a write in the same
-# extended-protocol implicit transaction is rejected.
+# extended-protocol implicit transaction never commits on its own.
 #
 # An extended-protocol pipeline keeps the transaction in the `Started` state
-# until `Sync`, accumulating the earlier statement's write ops. A DELETE cannot
-# see those ops, so running it would commit durably against a snapshot that
-# lacks them. That reorders the DELETE before the INSERT, and the DELETE would
-# survive the rollback that the failing pipeline performs on the INSERT.
+# until `Sync`, accumulating the earlier statement's write ops. A statement that
+# reads persisted state cannot see those ops, so it is rejected. One that reads
+# nothing is admitted and has to join them, not commit immediately. Either way
+# the pipeline's writes stand or fall together.

 send
 Query {"query": "DROP TABLE IF EXISTS t"}
@@ -21,6 +21,7 @@ ReadyForQuery {"status":"I"}
 CommandComplete {"tag":"CREATE TABLE"}
 ReadyForQuery {"status":"I"}

+# A DELETE reads its target, so it is refused outright.
 send
 Parse {"query": "INSERT INTO t VALUES (1)"}
 Bind
@@ -54,3 +55,50 @@ RowDescription {"fields":[{"name":"count"}]}
 DataRow {"fields":["0"]}
 CommandComplete {"tag":"SELECT 1"}
 ReadyForQuery {"status":"I"}
+
+# A blind write reads nothing, so it is admitted and must buffer its rows into
+# the pipeline's write ops. 20000 rows is above the constant-folding limit, so
+# the values stay a dataflow and take the read-then-write path instead of being
+# folded and buffered by the constant-insert path. Committing them immediately
+# would order them ahead of the earlier `VALUES (1)` op and leave them behind
+# when the failing pipeline rolls that op back.
+send
+Parse {"query": "INSERT INTO t VALUES (1)"}
+Bind
+Execute
+Parse {"query": "INSERT INTO t SELECT generate_series(1, 20000)"}
+Bind
+Execute
+Parse {"query": "SELECT 1/0"}
+Bind
+Execute
+Sync
+----
+
+until err_field_typs=SC
+ReadyForQuery
+----
+ParseComplete
+BindComplete
+CommandComplete {"tag":"INSERT 0 1"}
+ParseComplete
+BindComplete
+CommandComplete {"tag":"INSERT 0 20000"}
+ParseComplete
+BindComplete
+ErrorResponse {"fields":[{"typ":"S","value":"ERROR"},{"typ":"C","value":"22012"}]}
+ReadyForQuery {"status":"I"}
+
+# Neither INSERT may survive: the blind write joined the transaction that the
+# division by zero rolled back.
+send
+Query {"query": "SELECT count(*) FROM t"}
+----
+
+until
+ReadyForQuery
+----
+RowDescription {"fields":[{"name":"count"}]}
+DataRow {"fields":["0"]}
+CommandComplete {"tag":"SELECT 1"}
+ReadyForQuery {"status":"I"}

Running bin/cargo-test -p mz-environmentd test_pgtest_mz_frontend_occ_pipelined_dml fails:

    failure:
    ../../test/pgtest-mz/frontend-occ-pipelined-dml.pt:98:
    ReadyForQuery

    expected:
    RowDescription {"fields":[{"name":"count"}]}
    DataRow {"fields":["0"]}
    CommandComplete {"tag":"SELECT 1"}
    ReadyForQuery {"status":"I"}

    actual:
    RowDescription {"fields":[{"name":"count"}]}
    DataRow {"fields":["20000"]}
    CommandComplete {"tag":"SELECT 1"}
    ReadyForQuery {"status":"I"}

[aljoscha]

(This is aj, Aljoscha's agent.)

Thanks, this was a real bug and it went deeper than the case you tested. Fixed in a158e257, and I retitled the PR because the semantics changed rather than just the predicate.

Your case reproduces exactly as you reported it, 20000 rows instead of 0. Trace at the decision point:

rtw-entry contains_ops=true is_multi=false
defer_write=false
outcome=Blind COMMITTED-IMMEDIATELY

defer_write keyed on is_in_multi_statement_transaction(), which is false for Started.

The obvious fix would have been wrong. Aligning that predicate with the gate's is_in_multi_statement_transaction() || contains_ops() fixes your case and leaves a hole, because a read-then-write that opens a pipeline has no ops in front of it:

Parse "INSERT INTO t SELECT generate_series(1, 20000)"; Bind; Execute
Parse "SELECT 1/0"; Bind; Execute
Sync
-- 20000 rows, both predicates false

And the same hole exists for read-dependent writes, on main, independent of this PR. A DELETE opening a pipeline commits and survives the rollback, 2 rows where PostgreSQL gives 3. Control: with enable_adapter_frontend_occ_read_then_write off the same test passes, so it is the OCC path. Existing coverage missed it because test/pgtest/transactions.pt uses constant INSERTs, which stage normally.

The root cause is may_span_pipeline, which lets an implicit transaction stay open only for writes, and says why: "because they are merely staged". The OCC path marked Writes ops, buying that right, then wrote the rows anyway.

So the fix splits on whether the selection reads persisted state. A write that reads nothing stages its rows and commits or rolls back with the pipeline, like a constant INSERT. A write that reads persisted state cannot be in a transaction at all, so it commits as its own transaction and clears its ops, which is how PostgreSQL treats statements that cannot run in a transaction block. That last one is a deliberate divergence and it is spelled out in the PR description.

Two things you may want to check specifically. Staging turns out to remove a coordinator round trip rather than add one, since the rows now ride along in a commit that was already happening, so submit_blind_write is gone. And a staged WriteOp loses the target-generation guard, which I have documented as shared with every other staged write rather than papered over.

Your test is in frontend-occ-pipelined-dml.pt along with five others, one per rule. The two blind-write cases were confirmed failing before the change and passing after.

[def-]

New test:

diff --git a/test/cluster/mzcompose.py b/test/cluster/mzcompose.py
index f1ba82da05..819b0a1595 100644
--- a/test/cluster/mzcompose.py
+++ b/test/cluster/mzcompose.py
@@ -31,6 +31,7 @@ import websocket
 from psycopg import Cursor, sql
 from psycopg.errors import (
     DatabaseError,
+    DivisionByZero,
     InternalError_,
     OperationalError,
     QueryCanceled,
@@ -4888,6 +4889,113 @@ def workflow_test_occ_zero_row_write_linearization(c: Composition) -> None:
                 )


+def workflow_test_occ_sealed_input_write_stands_alone(c: Composition) -> None:
+    """A read-then-write whose selection reads persisted state must commit as
+    its own transaction, including when its subscribe ends on its own.
+
+    The OCC path stages a mutation's diffs only when the selection reads
+    nothing, because only then are they frontier-independent. A selection that
+    reads persisted state commits inside the OCC loop and ends the implicit
+    transaction it opened instead of spanning the rest of an extended-protocol
+    pipeline, so a later failure there cannot undo it.
+
+    Which of the two happens is decided twice, and the answers differ. Before
+    the dataflow runs it is `depends_on()` on the selection. Once it runs it is
+    whether the subscribe closed on its own, which it also does for a sealed
+    persisted input: a `REFRESH AT` materialized view past its last refresh has
+    an empty write frontier, so the sink's output frontier reaches the empty
+    antichain. An `INSERT ... SELECT` over such a view reads persisted state and
+    still takes the closed-channel exit, so nothing in the statement decides
+    whether its rows survive. An input passing its last refresh does.
+    """
+
+    def rows_surviving(write: str) -> int:
+        """Rows left in `dst` once `write` has succeeded and the pipeline behind
+        it has failed. Leaves `dst` empty again.
+
+        `write` is sent as one extended-protocol pipeline with a failing
+        statement behind it and no `Sync` between them. A pipeline is an
+        implicit transaction, so rows going missing here is exactly `write`
+        having joined that transaction instead of committing on its own.
+        """
+        with c.sql_connection() as conn:
+            try:
+                # psycopg pipelines the extended protocol only, and syncs when
+                # the block ends. Autocommit keeps a `BEGIN` off the front,
+                # which the OCC path would refuse outright.
+                with conn.cursor() as cur, conn.pipeline():
+                    cur.execute(write.encode())
+                    cur.execute(b"SELECT 1 / 0")
+            except DivisionByZero:
+                pass
+            else:
+                raise AssertionError(f"the statement behind {write!r} succeeded")
+        rows = c.sql_query("SELECT count(*) FROM dst")[0][0]
+        c.sql("DELETE FROM dst")
+        return rows
+
+    with c.override(
+        Materialized(
+            # Sampled once at startup, so this cannot be an `ALTER SYSTEM SET`.
+            additional_system_parameter_defaults={
+                "enable_adapter_frontend_occ_read_then_write": "true"
+            },
+            # The closed-channel arm soft-asserts that it only ever stages a
+            # dependency-free write, which a sealed input violates. That abort
+            # would pre-empt the outcome asserted below. Once the arm routes a
+            # sealed input to its own transaction the assert is unreachable, so
+            # nothing here turns on how this is configured.
+            soft_assertions=False,
+        )
+    ):
+        c.up("materialized")
+        c.sql(dedent("""
+                CREATE TABLE src (a int);
+                INSERT INTO src VALUES (1), (2), (3);
+                CREATE TABLE dst (a int);
+                CREATE MATERIALIZED VIEW sealed WITH (REFRESH AT CREATION) AS
+                    SELECT a FROM src;
+                """))
+
+        # An empty write frontier is what closes the subscribe, and it surfaces
+        # as a NULL `write_frontier`. Hydration alone is not enough, so wait for
+        # the seal rather than for the first successful read.
+        sealed = """SELECT f.write_frontier IS NULL
+                    FROM mz_internal.mz_frontiers f
+                    JOIN mz_materialized_views m ON (m.id = f.object_id)
+                    WHERE m.name = 'sealed'"""
+        deadline = time.time() + 120
+        while c.sql_query(sealed) != [(True,)]:
+            assert (
+                time.time() < deadline
+            ), "the REFRESH AT CREATION materialized view never sealed"
+            time.sleep(0.1)
+
+        # Control: the same pipeline over `src`, whose subscribe never ends on
+        # its own. It pins the harness, since rows lost here would mean the
+        # pipeline was never an open transaction to begin with.
+        write = "INSERT INTO dst SELECT a FROM src"
+        rows = rows_surviving(write)
+        assert rows == 3, f"{write} succeeded, then lost {3 - rows} of its 3 rows"
+
+        # Ask the process rather than the catalog which path that took: the
+        # write only reaches the histogram if the frontend sequenced it, and on
+        # the coordinator's lock path none of this is about read-then-write
+        # transaction handling at all.
+        metrics = c.exec(
+            "materialized", "curl", "localhost:6878/metrics", capture=True
+        ).stdout
+        assert any(
+            line.startswith("mz_occ_read_then_write_retry_count_count ")
+            and float(line.split()[1]) > 0
+            for line in metrics.splitlines()
+        ), "no read-then-write went through the OCC path"
+
+        write = "INSERT INTO dst SELECT a FROM sealed"
+        rows = rows_surviving(write)
+        assert rows == 3, f"{write} succeeded, then lost {3 - rows} of its 3 rows"
+
+
 def workflow_test_refresh_mv_warmup(
     c: Composition, parser: WorkflowArgumentParser
 ) -> None:

Running it fails: bin/mzcompose --find cluster run test-occ-sealed-input-write-stands-alone

AssertionError: INSERT INTO dst SELECT a FROM sealed succeeded, then lost 3 of its 3 rows

[aljoscha]

(This is aj, Aljoscha's agent.)

Good catch, and thanks for writing the test. You found a real hole in my fix, and it is the case my own module doc had described without my code honoring it.

What was wrong. I keyed the destination on the outcome rather than on the decision. The Blind arm staged unconditionally, but Blind does not imply "reads nothing": a sealed input closes the subscribe cleanly too, so an INSERT ... SELECT over a REFRESH AT view past its last refresh took the staging arm while reading persisted state. It then joined the pipeline and the 1 / 0 rolled it back, which is your 3 lost rows. The soft_assert you had to switch off was mine, and it was simply wrong: disagreement in that direction is expected, not a bug.

The fix splits the Blind arm on stages_rows, the syntactic predicate. A statement that reads nothing stages. A statement that reads persisted state takes the blind submission (submit_blind_write, which I had deleted and have restored) and then ends its own transaction like any other read-dependent write. The bogus assert is gone.

On why we do not just stage it, since the diffs genuinely are frontier-independent and staging would be safe: your docstring made the argument for me. If we staged, whether a statement's rows survive a failure later in the pipeline would depend on whether one of its inputs happened to pass its last refresh. Nothing in the statement would tell a user which they get. So the syntactic answer decides transaction membership, and the dynamic one only decides how the write is submitted. I have written that separation into the module docs and the PR description, since it is the part that is easy to get wrong again.

I took your test as-is, with two changes. I dropped soft_assertions=False, because with the assert removed the run should be clean and I would rather the test guard that. And I reworded the last paragraph of the docstring to say that only the syntactic answer may decide membership, which is the invariant rather than the symptom.

One caveat on verification: our VM crashed and I cannot run tests locally at the moment, so this is riding on CI. If test-occ-sealed-input-write-stands-alone still fails I would rather hear it from you than from me guessing.

[aljoscha]

looks like this part cuts into another rustdoc/comment? what happend there?

[aljoscha]

(This is aj, Aljoscha's agent.)

You're right, and it was worse than one spot. I damaged two rustdoc blocks, both from the same sloppy move.

When I restored submit_blind_write and added end_own_transaction, I placed each by searching backwards for the nearest /// and inserting there. That anchor landed inside an existing doc comment rather than before it, so in both cases I wedged a function into the middle of someone else's rustdoc:

• validate_read_then_write lost the end of its doc. It read "...the missing-entry branches below are", then my end_own_transaction doc and body, then an orphaned "/// failsafes rather than a live concurrent-DDL path." attached to nothing useful.
• ensure_read_linearized lost its doc entirely. Its "Block until the oracle for this query's timeline has advanced to as_of..." ran straight into submit_blind_write's doc, so that whole explanation ended up describing the wrong function and ensure_read_linearized was left undocumented.

Both compiled and both passed rustdoc, because two adjacent doc comments are perfectly valid syntax. Only reading it catches this, which is what you did.

Fixed in the push just now. validate_read_then_write's sentence is whole again, ensure_read_linearized has its doc back, and the two functions I moved sit as their own items with their own docs. end_own_transaction now goes above the validate_read_then_write doc block, and submit_blind_write after ensure_read_linearized's body.

Worth me noting the lesson rather than just the fix: inserting code by regex-anchoring on a nearby comment is how this happened, and a compile plus a rustdoc run does not catch it. I'll place moved items relative to item boundaries.

[aljoscha]

@def- this is ready for another look, please 🙏

[def-]

Nothing else from QA side

[ggevay]

LGTM, just some minor comments inline.

[ggevay]

This NOTE's conclusion doesn't hold on main. Since #38051, group commit rejects arity-stale staged rows rather than appending them: stage_group_commit runs stale_write_target over each session write's TableData::Rows and retires the transaction with ConcurrentDependencyMutation (40001) when the first row's arity no longer matches the latest RelationDesc. An ALTER TABLE ... ADD COLUMN that lands between here and the commit therefore fails the transaction instead of appending old-arity rows under the new schema.

The first half is still right, a staged WriteOp carries no generation pin the way the immediate path's target_global_id does. Suggest rewording to what actually happens: no pin, but commit staging rejects stale rows, so the staged write degrades to the same retryable outcome the immediate path reports as TargetChanged.

[aljoscha]

(This is aj, Aljoscha's agent.)

You're right and the NOTE was simply wrong. stage_group_commit runs stale_write_target over each session write and retires with ConcurrentDependencyMutation, so an ALTER TABLE ... ADD COLUMN landing before the commit fails the transaction rather than appending old-arity rows. Rewritten along the lines you suggested.

Two details I added while checking it, because the reframing can overclaim in the other direction. The comparison reads rows.first() per staged write rather than every row, which is fine since one statement's diffs share an arity, but the comment should not say "each row". And it does not hold for every staged write: TableData::Batches is skipped outright, which is how COPY FROM arrives, and those are safe for a different reason. So the "no generation pin" half is universal and the arity check is not, and the comment now separates the two.

I kept a hedge that the check compares arity, so it stands in for the descriptor rather than pinning it. AlterTableAddColumn is the only descriptor-mutating ALTER on a table today and it always changes arity, so the check is sound now, but it is sound by coincidence rather than by construction.

[ggevay]

Two small things about this gate.

First, error precedence: the base's version of this check ran contains_mz_now before refusing, to match the coordinator, and this version refuses deps-first. A statement carrying both mz_now and a dependency-carrying selection now reports 25001 where the lock path reports the dedicated mz_now error. The corner is reachable, though admittedly contrived: ConstantVisitor treats only Table and Show references as non-constant, so a VALUES mixing mz_now() with a SQL-implemented builtin passes the entry carve-out on both paths (the coordinator's handle_execute has the identical carve-out), and for example pg_get_viewdef expands inline to (SELECT definition FROM mz_catalog.mz_views WHERE ...), which makes the planned selection dependency-carrying. On the lock path, sequence_insert has no transaction refusal past its allows_writes check, so sequence_read_then_write reports the dedicated mz_now error (inner.rs:2834). Restoring the precedence looks like three lines: re-run contains_mz_now(&rtw_plan) inside this refusal branch before returning the transaction error, the helper is already imported here.

Second, the refusal branch itself is untested. The population it still refuses is exactly the AST-constant-but-dependency-carrying class, and no test exercises one inside a transaction (every "cannot be run inside a transaction block" assertion in the tree hits the entry gate: RETURNING, table references visible in the AST, or DELETE/UPDATE). The base refused everything at this point, so nothing pins the narrowing. One case next to test_nonconstant_insert_in_transaction could pin both halves at once: an AST-constant dependency-carrying INSERT in a transaction, with and without mz_now.

[aljoscha]

(This is aj, Aljoscha's agent.)

Both halves done, and the second half turned out to matter more than the first.

The precedence fix is in: the refusal branch now reports mz_now before the transaction state. One correction, contains_mz_now was not already imported in client.rs, so it needed a use. I also fixed my own first attempt at explaining why, which claimed the coordinator rejects mz_now before looking at the transaction. It does not. It looks at the transaction first and admits this class of statement past that gate, then reports mz_now while sequencing.

The test is where it got interesting. I wrote one asserting the lock path runs INSERT INTO t VALUES (pg_get_viewdef('v')) inside a transaction, and that is wrong: mz_views is a BuiltinTable, so id.is_user() is false and validate_read_then_write_dependencies rejects it as a system table. My test would have panicked on its first iteration, before reaching the branch I had changed. It was a regression test for nothing.

Chasing that down produced a second finding of the same shape as yours. The whole population reaching this gate reads a system relation, since that is what the AST carve-out admits, so the statement is invalid whatever the transaction state. The gate was reporting 25001 for all of it while the lock path reported the invalid selection. Reporting the transaction is the worse answer for the same reason it was for mz_now: it names a condition the caller could remove, when the statement cannot work in any transaction state. So the unconditional questions now both come first, with the resolve-and-validate step extracted so the gate and validate_read_then_write ask it the same way. The two paths agree again, and the test pins that plus the autocommit case.

The transaction refusal stays as the answer for a statement whose dependencies are all valid, which nothing reaches today. It is the guard that keeps a read-dependent write out of a transaction if that ever changes.

[ggevay]

The disagreement tripwire covers this arm but not its zero-row sibling. If the predicates disagree and the selection happens to match no rows, the NoRowsMatched arm receives observed_ts: Some(..) for a statement whose syntactic predicate said it reads nothing, and it silently parks in ensure_read_linearized instead of making the disagreement loud. Nothing is durable on that path, so it is strictly less severe, but it is the same invariant violated, and stages_rows && observed_ts.is_some() is exactly the signature. A matching soft_assert_or_log there would close the blind spot.

[aljoscha]

(This is aj, Aljoscha's agent.)

Added, as soft_assert_or_log!(!(stages_rows && observed_ts.is_some()), ...).

I went looking for a false-positive path first, since a tripwire that can fire on a legitimate execution is worse than no tripwire, and I convinced myself it cannot. stages_rows means depends_on() is empty, and depends_on is computed on the HIR selection whose visitor descends into subqueries, so an import-free dataflow is the only way to get there. observed_ts is only ever Some from a progress row with ts > as_of, and an import-free dataflow has no capability beyond T::minimum(), then jumps to the empty frontier, whose progress message send_progress_message suppresses. So the only reachable outcomes for a staging statement are Blind or NoRowsMatched { observed_ts: None }, which is consistent with the existing Committed assert never having fired.

I also updated the module docs, which described a single tripwire in the Committed arm and explained it by "the write is durable, so all that arm can do is make the disagreement loud". That reasoning does not carry to the new one, where nothing is durable and the point is that a correct answer was reached through a state we believe impossible.

[ggevay]

This disjunction now exists twice, here and in the gate in try_frontend_read_then_write, with near-identical comments explaining the Started-with-ops trap. The two arm-shaped gates encode the same concept a third and fourth time, as Started(_) if !contains_ops, in the entry gate and in the coordinator's handle_execute. Given that this exact predicate diverging is how both pipeline bugs happened, it seems worth a name on TransactionStatus, next to is_in_multi_statement_transaction. Something like is_effectively_multi_statement(), returning this disjunction, whose doc carries the two things a caller must know: why Started with ops counts (a pipeline stays Started while it accumulates ops), and that it must be evaluated before the current statement stages anything of its own, because afterwards contains_ops reports the statement's own marker. The arm-shaped gates reduce to it cleanly, since Default, Failed and an ops-free Started all report false.

Worth doing in this PR: the method plus the four gate sites are a small mechanical change, and this PR is where the fourth site appears. (DISCARD ALL's discardable in sequencer.rs is a fifth, negated variant, but folding that one needs an argument that Default and Failed cannot reach it, so it can stay as is.)

[aljoscha]

(This is aj, Aljoscha's agent.)

Done, with one correction to the plan and a rename.

The method is there, but your justification for folding the arm-shaped gates does not hold: contains_ops reads through inner(), which includes Failed(txn), so the disjunction reports true for a failed transaction holding ops while those gates deliberately let Failed pass. Folding them would change behavior rather than name it. The coordinator's also has Default => unreachable!() and handles DECLARE inside the ops-free arm. So I converted only the two sites that spelled the disjunction out, and the method's doc records where it stands relative to the gates, including that nothing can reach them in that state since pgwire admits only COMMIT and ROLLBACK once a transaction has failed. Your DISCARD ALL observation was right and it is a third such gate, so the message says three.

On the name: is_effectively_multi_statement sat next to is_in_multi_statement_transaction, whose summary is "whether the transaction may contain multiple statements", which is close enough to what the new method computes that the two are hard to tell apart. It is now may_share_transaction_with_other_statements, and the summary states the consequence the callers care about rather than restating the neighbour.

[ggevay]

The list here is already incomplete: enable_expression_cache is a third startup-only parameter. catalog/open.rs's comment above sync_dyncfgs() names it together with FRONTEND_READ_THEN_WRITE as the two startup-only dyncfg reads, and it is in the same ALTER SYSTEM-reachable registry, so it warns for neither SET nor RESET ALL while the two entries here do. Worth adding it, plus a restart sentence to FRONTEND_READ_THEN_WRITE's description the way MAX_CONCURRENT_OCC_WRITES already has one. Longer term the drift-proof shape is a startup_only attribute on the definition itself with this list derived from it, but that can wait.

[aljoscha]

(This is aj, Aljoscha's agent.)

Added enable_expression_cache, and put the restart sentence in both descriptions rather than only the frontend flag's, since the cache flag had no signal either.

I retitled the commit, because "every startup-only parameter" is not what it achieves. The zero-downtime deployment parameters are boot-sampled too, in environmentd's startup rather than in this path, and they are ALTER SYSTEM-reachable and mirrored to durable config for exactly that reason. Warning for those is a question for that layer and I did not want to answer it here, so the commit says what it covers. Your startup_only attribute is the real fix and I noted it as such.

[ggevay]

With part 7 wrapping up the stack, here is a recap of the agreed follow-ups in one place, plus some new candidates.

Already agreed in the #37923 review threads:

• The client.rs/PeekClient restructure package: lift the Plan::Insert arm and share the conversion tail with sequence_insert (thread), context structs for the long signatures (thread), move the read-then-write surface into the module and rename PeekClient (thread).
• OCC-loop readability, deliberately deferred until after merge: the ProcessResult split and ?-able arms (thread), write_diffs/no_rows helpers (thread).
• Shared cluster-resolution helper with frontend_peek (thread) and a generic InRange<T, R> (thread).
• Statement-logging test consolidation: promote the parity harness's fields to value assertions and drop the two standalone tests (thread, thread), the retry-histogram assertion for the ALTER-race test (thread), harness batching (thread).
• Runtime-changeable max_concurrent_occ_writes vs. keeping the warning (thread), and matching Postgres on no-op UPDATE counts once the lock path is gone (thread).
• Already filed: SQL-628 (lagging-dependency notice + retry outcome label), SQL-630 (restart-based coverage of both paths).

New candidates, from a duplication sweep over the code the stack touched. Filtered to what still applies after the lock-based path is deleted (a few more findings dissolve with it and aren't worth doing before). Each was verified by reading. The (~N) markers are our confidence, in percent, that the fix is worth it, i.e. that it comes out net-simpler rather than just moving code.

1. (confidence ~95) From<&Result<ExecuteResponse, ..>> for StatementEndedExecutionReason panics for the terminates-elsewhere responses, and peek_client.rs's terminates_elsewhere hand-mirrors that panic set behind a wildcard arm, so a new response variant can silently land in the panic group and take down a connection. Replacing the impl with an Option-returning function deletes both the panics and the mirror. Three call sites total, unrelated to the lock path.
2. (~92) Statement counting and emit_timestamp_notice are hand-mirrored between the coordinator and the frontends (count_statement's own doc says matching is the point). Two small helpers, net roughly -27 lines.
3. (~93 that some form is warranted, ~90 for the preferred form) The connection_cancel_watches map has seven touch points carrying three distinct policies: two installs with opposite stale-cancel handling (inherit vs. discard), three removes (including a nested-FETCH guard), a signal, and a watch-subscribe, with no cross-references between them. Our preferred form is a small ConnectionCancelWatches newtype with documented methods: the per-site policy comments relocate into the method docs (so the line count stays roughly neutral), and the private map makes the compiler force future call sites through a named policy instead of a coin flip, which the existing or_insert_with hazard comment shows is a real trap. The lighter alternative is two named install methods or cross-comments.
4. A batch of small shared helpers, each collapsing the copies that outlive the deletion: the PeekResponseUnary to AdapterError classification (~93, the frontend and http copies remain), the real-time-recency eligibility predicate (~95, three copies, all peek/OCC machinery), write_target_changed (~95, two verbatim copies, both new OCC code), row-constraint validation (~93, insert_constant, copy_from and the OCC loop remain), the timeout-zero-means-off convention (~93, three sites remain, in two idioms), and the mutation response tag mapping plus RETURNING finishing (~90, shared between insert_constant's send_diffs and the OCC response builder, while the row counting should stay separate).
5. (~90, an enhancement rather than a dedup) message_linearize_reads has the same idle-tick wait that the zero-row group-commit nudge just fixed: the oracle only advances via group commit, so on a quiet timeline any strict-serializable read whose timestamp lands ahead of the oracle parks for up to a default_timestamp_interval, and every real-time-recency read does. The frontend defers all its strict-serializable peek linearization into this same queue, so the beneficiary set is not just RTR. The re-check plumbing already exists and the code runs on the coordinator, so the fix is a guarded group_commit_tx.notify(), smaller than the write-side nudge it mirrors. The one subtlety: nudge only on a waiter's first park and only when the target timestamp is at or below wall clock, so a far-future read (clock skew) cannot drive an empty-commit loop.
6. (~95) Parallel-workload oracle hygiene: move the increment SQL onto ReadThenWriteCounter, assert its autocommit dependency, run validate and the cluster pin on the wedged-run path too, and fix the stale weight comment. After the lock path is gone the oracle can also tighten: on pure OCC a timeout error implies not-committed, so the timeout string can move to DEFINITELY_NOT_COMMITTED.

Plus some one-line polish: cross-comments for the two cancel-watchdog test timings, a comment at both ends of the who-records-the-write-timestamp contract between appends.rs and the frontend, and a name for the shared 1s linearization poll cap.

One interim-only note, which dies with the lock path: txn-wal-fencing's NOT_APPLIED classifies statement timeouts as definitely-not-applied, which is wrong on the lock-path instance the mixed-mode test always runs (a timeout there can race a commit), so its counter band can misjudge until the deletion. A one-line reclassification would de-risk the interim.

Happy to expand on any of these, or to file them as issues.

[aljoscha]

don't refer to the other paths which will get deleted, just make this good and stand on it's own please

[aljoscha]

(This is aj, Aljoscha's agent.)

Fixed, and thanks for catching it. Pushed as a commit on top.

The comment now states the rule without comparing anything: a reason that holds wherever the statement runs is reported before one that holds only inside a transaction. Naming the transaction for a statement that can never work tells the caller to retry outside a transaction, where they get the same refusal again. That is the whole argument, and it survives the other path being deleted.

I swept the rest of what I added and found one more of the same kind, so I fixed it too. The new predicate's doc pointed at two gates by name to say they classify a failed transaction the other way, which is a comparison rather than a contract. It now states the rule: the method reports true for a failed transaction holding ops, and a caller who cares must check for itself.

The one reference I left is in the test module docs, where the paragraph says which tests cover which path. That file's job right now is comparing the two, so the reference is the content rather than a crutch, and it goes away with the tests when the flag does. Say the word if you want that gone as well.

Worth noting the commit messages still explain these changes by reference to the other path, since that is genuinely why the changes were made. I read your comment as being about the code, which has to stand on its own after the deletion. Happy to scrub the messages too if you would rather they read that way.