Repository navigation
[pull] main from MaterializeInc:main - #1262
Merged
Merged
Conversation
`parse_option_map` committed on the `MAP` keyword alone and then
demanded
`[`, so a bare `map` in an option-value position was a hard parse error
rather than falling through to the item name it is.
`map` is a legal bare identifier everywhere else. `SELECT "map"` prints
as
`SELECT map` and reparses fine, so `AstDisplay` prints an option value
that
is the item name `map` unquoted, and that output failed to reparse:
CREATE SINK s FROM t INTO KAFKA CONNECTION c (TOPIC = "map")
printed `TOPIC = map` and then failed with "Expected left square
bracket,
found right parenthesis". Every generic option value was affected, a
materialized view's `PARTITION BY` among them.
Look ahead for the bracket instead. The alternative, quoting `map` in
`can_be_printed_bare`, would change the printed form of every `map`
identifier in every position, and would change what `quote_ident('map')`
returns, to work around one non-backtracking call site. The sibling
branches of `parse_option_value` all backtrack already.
Expression position never needed that quoting, but the comment in
`can_be_printed_bare` gave the wrong reason: the map-literal dispatch
keys
off the `[`, not the `=>` the comment cited, so `map[1]` is not
"unambiguously a subscript" on its own. What keeps a bare `map` safe
there
is `write_subscript_receiver`, which parenthesizes a context-sensitive
keyword receiver into `(map)[1]`. Corrected in place, no behavior
change.
Found by the sql_roundtrip cargo-fuzz target. The datadriven case pins
the
printed form and AST. The added parser test covers the sink, source and
materialized-view option positions, a genuine `MAP['a' => 'b']` option
value that must still take the map branch, and the expression forms the
corrected comment relies on.
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
The Int16 counts that precede the repeated groups of Parse and Bind (parameter types, format codes, parameter values) were decoded as signed, but PostgreSQL decodes them as unsigned, so a client may send up to 65535 parameters. A count above 32767 read as negative, so the group was skipped and the cursor was left misaligned for the rest of the message. Closes: [SQL-491](https://linear.app/materializeinc/issue/SQL-491) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
### Motivation Part 2 of 7 in a stack that moves `DELETE`, `UPDATE` and `INSERT ... SELECT` off the coordinator onto the session task, using optimistic concurrency control. Two independent cleanups that the later parts need, both of which stand on their own. The first is a live bug. Closes [SQL-588](https://linear.app/materializeinc/issue/SQL-588) ### Description **`ConfigSet` sync at catalog open.** `apply_updates` only mirrors `SystemVars` into the dyncfg `ConfigSet` when a durable `SystemConfiguration` update is present. A deployment that configures a parameter purely through `system_parameter_default` has no such update, so nothing syncs, and anything reading that dyncfg during bootstrap sees the compile-time default instead of the configured value. `ENABLE_EXPRESSION_CACHE` is read a few lines below and is affected today. Syncing the effective values once, after both the defaults and the durable configuration are in the map, fixes every such read. **One subscribe optimization pipeline.** Subscribe optimization had two entry points that differed only in whether the source was a query or an existing id, and each rebuilt the same pipeline around that one decision. Both rebuilt the same `SinkDesc` verbatim, down to the same three comments, and differed only in whether the source needed HIR lowering first. Taking the source as a `SubscribeSource` and the output shape as a parameter leaves the pipeline in one place. The view optimizer's expression-preparation variants collapse the same way, into one constructor plus a builder method for the constant folding limit. The dyncfg commit also splits `dyncfg_updates`, which computed the updates and applied them to its own `ConfigSet` in one method. The only two callers of the applying behavior are the ones that discard the result, so the call sites read like a getter whose value is thrown away, and a later change making the getter pure would compile cleanly while silently reintroducing the bug. `sync_dyncfgs` names that intent. The six callers that forward the updates to metrics or the controllers keep the pure `dyncfg_updates`. ### Verification Existing subscribe and `COPY FROM` coverage. No behavior change is intended for the optimizer part. The dyncfg change is observable as `ENABLE_EXPRESSION_CACHE` now taking effect when set only via `system_parameter_default`.
…37920) **NOTE: This looks big, but a large chunk is moving tests around, in a separate commit.** ### Motivation Part 3 of 7 in a stack that moves `DELETE`, `UPDATE` and `INSERT ... SELECT` off the coordinator onto the session task, using optimistic concurrency control. This part is a restructuring, not a bugfix. A statement sequenced from the session task has its end-of-execution logging obligation handed between several places, and each of them can be the last one to hold it. Whether a log row appears, and what reason it carries, depends on which path the statement takes out of the session task. That is survivable while the only such statement is a peek. It stops being survivable in part 6. Read-then-write is a long-running operation, so part 6 bounds it with cancellation and `statement_timeout` in a `tokio::select!` that drops the operation's future when either fires. The obligation must not live inside that future. If it does, dropping the future runs the guard's `Drop`, which reports `aborted` with no message, and a user who just saw "canceling statement due to statement timeout" finds a log row recording neither the error nor its text. Hoisting the obligation into the frame beside the `select!` is what makes that drop safe, which is why this lands before the path that needs it rather than with it. Closes [SQL-589](https://linear.app/materializeinc/issue/SQL-589) ### Description `ExecutionLogging` is a slot that owns the obligation for one `SessionClient::execute` call, with a single retirement site that maps the call's outcome to an end reason. Handing the statement back to the coordinator passes the obligation as data in `Command::Execute`, and `handle_execute` arms it again on receipt, which is safe because the command travels a channel only the coordinator loop drains. Three defects follow from the old structure and go away with it: - an `EXECUTE` that unrolled to its inner statement got a guard whose retire channel was already dropped, so if that guard ever ran its `Drop` the end event went nowhere and the entry stayed open forever - a statement whose future is dropped mid-flight was recorded as `aborted` rather than carrying its error - a plan that passes the AST allowlist but then fails the plan-kind check bailed out after logging had begun, closing the frontend's entry as errored while the coordinator started its own from scratch, so one statement produced two log rows and counted `query_total` twice None of the three is reachable from SQL on the peek path, which is why none of them gets a test here. The first needs a drop in the few microseconds between installing the guard and the takeover, or a shutdown. The second needs an inner future for something to drop, and the peek path has none. The third sits behind `soft_panic_or_log`. They are latent, which is the point: the caller that reaches the second one arrives in part 6, which carries its tests. Frontend-sequenced subscribes also start counting towards `mz_subscribe_outputs`. The only site bumping it was `handle_execute`, which a statement the frontend took over never reaches, and `SessionMetrics` had no accessor for it, so those subscribes went uncounted. That is a behavior change beyond the restructuring. End events also move from `Client::send`, which panics when the coordinator is gone, to a `try_send` that drops them. A guard can outlive the coordinator during shutdown, and losing one end event is cheaper than panicking in `Drop` and taking the connection with it. Every other logging event keeps the asserting send. `end_statement_execution` takes the end timestamp from its caller. The frontend already recorded when execution finished and the handler discarded it, so `finished_at` and the `ExecutionFinished` lifecycle event charged the statement for however long its message waited in the coordinator's queue. The client never waited for that queue, because the response leaves the session task while the end event is a fire-and-forget send on an unbounded channel. `began_at` already came from the frontend's clock, so the two ends of a statement's duration were being read from different clocks. ### Verification `test_statement_logging_finished_at_excludes_coordinator_queue` covers the `finished_at` change, which is the one thing here observable from SQL today. It stalls the coordinator inside `Catalog::transact` and then runs a constant `SELECT` from a session whose catalog snapshot is already warm, so the statement needs nothing from the coordinator. The logged `finished_at` has to fall before the client saw the statement return. On the old code it lands about three seconds later. All existing `test_statement_logging_*` tests are unchanged and pass. The remaining tests for these paths are DML-specific and arrive with part 6, which adds a harness asserting the frontend and coordinator paths produce the same `mz_statement_execution_history` rows for the same statement, plus the cancellation and statement-timeout tests that reach the latent defects above. The second commit moves the existing tests out of `server.rs` into `statement_logging.rs`. It is a pure move: every moved item is byte-identical to its previous form, and the only other edits are the imports each file now needs. `server.rs` was 7585 lines, of which the statement logging tests and their harness were a contiguous 994-line block.
Substantially broadens what the parallel-workload stress test exercises, from a doc and surface-area audit, so nightly runs cover far more of the SQL and DDL surface, at larger scale, and find more product bugs. Every bug this coverage surfaced is worked around (the triggering action is disabled or narrowed, each with a TODO naming the tracked issue) so the nightly stays green until the bug is fixed. ## New coverage Adds ~34 actions and query-generation breadth: - **SELECT surface**: GROUP BY, DISTINCT / DISTINCT ON, outer and lateral joins, subqueries, CTEs, WITH MUTUALLY RECURSIVE, table functions, temporal filters, and the aggregate families. Generated `WHERE` conjuncts are parenthesized so an `OR` inside one part stays a single top-level conjunct. Boundary literals (float4 `NaN`/`Infinity`) are injected as expression leaves to stress overflow and eval paths. - **DDL**: `CREATE OR REPLACE`, `DROP ... CASCADE`, `CREATE TYPE`, network policies, `ALTER CLUSTER SET`, `ALTER ROLE SET/RESET` session defaults, and broader `COMMENT`. - **Transactions**: parameterized prepared statements, bounded staleness, read-only and DDL transactions. - **Introspection**: system-catalog reads, `EXPLAIN ANALYZE`, and the full `EXPLAIN` matrix. - **Sources and sinks**: load generator sources, sink options, Iceberg `MODE APPEND`, and `CREATE REPLACEMENT MATERIALIZED VIEW` with both the `APPLY REPLACEMENT` and discard paths. - **Other**: RBAC, connection and secret lifecycle, `SHOW`. ## Incident 1136 coverage: replacement lifecycle and a sealed-shard oracle Incident 1136 (a dropped replacement finalizing its live target's shard after an envd restart) needed `CREATE REPLACEMENT` and the resolving `DROP` to happen on different connections across a restart. The old `ReplaceMaterializedViewAction` resolved a replacement on the connection that created it, so it structurally could not hit this. The lifecycle is now split into create/apply/drop actions that pick live replacements from `mz_internal.mz_replacements`, so replacements created by other workers or leaked across kills and 0dt deploys are found and resolved too. Verified to go red in one kill cycle on a build without the #37705 fix. Detection does not rely on the (much later) tombstone panic but on a client-side data-loss oracle, `SealedCollectionCheckAction`: a live user table or materialized view whose write frontier is the empty antichain has had its shard sealed out from under it. Legitimate seals are classified away: REFRESH AT views and bounded (UP TO) load generators seal when they complete, sealing is transitive through readers (a catalog dependency walk), and a view whose optimized plan reads no live storage collections (constant folding, e.g. a join `ON false`, drops inputs the catalog still records) completes on hydration, whatever its refresh schedule says (checked via `EXPLAIN OPTIMIZED PLAN AS JSON`). Candidates are re-checked after 5s since the catalog and `mz_frontiers` are not updated atomically. A guaranteed final oracle pass runs at the end of every run, including on the wedged-worker exit path, so damage from a run's last seconds cannot escape. ## Scale Raises the per-object caps (databases, schemas, tables, views, indexes, roles, sources, sinks, types, network policies, and columns) by roughly 10x, so a run holds many more concurrent objects and exercises deeper dependency graphs. ## Product bugs surfaced The new coverage found several pre-existing bugs. Each is worked around with a TODO pointing at the tracked issue so the action can be re-enabled once fixed: - **[CLO-143](https://linear.app/materializeinc/issue/CLO-143)**: `ALTER NETWORK POLICY` cannot resolve quoted names. - **[SQL-517](https://linear.app/materializeinc/issue/SQL-517)**: coordinator panic when a connection re-alter (via `ALTER CONNECTION`, `GRANT`/`REVOKE`, or `ALTER OWNER`) reshapes a dependent sink's export connection while its key indices are momentarily out of sync. - **[SQL-518](https://linear.app/materializeinc/issue/SQL-518)**: `get_database` panic on a concurrent `DROP DATABASE CASCADE` during a staged create's finish. - **[SQL-519](https://linear.app/materializeinc/issue/SQL-519)**: `EXPLAIN FILTER PUSHDOWN` missing-collection panic when a referenced index/MV is dropped between planning and read-hold acquisition. - **[SQL-520](https://linear.app/materializeinc/issue/SQL-520)**: frontend-peek read-hold vs compaction soft-panic (`enable_frontend_peek_sequencing` pinned off, peeks run the classic path). - **[SS-344](https://linear.app/materializeinc/issue/SS-344)**: `ALTER SINK SET FROM` hang. - **[SS-345](https://linear.app/materializeinc/issue/SS-345)**: `COPY FROM` CSV cannot decode a large-year date that `COPY TO` CSV wrote. - **[SS-347](https://linear.app/materializeinc/issue/SS-347)**: `VALIDATE CONNECTION` transiently reads an empty password during a concurrent `ALTER SECRET` rotation. - **[CLU-169](https://linear.app/materializeinc/issue/CLU-169)**: `SUBSCRIBE ... UP TO` optimizer soft-panic when the object's `as_of` has advanced to `MAX` (e.g. a finished bounded load generator). - **[SQL-595](https://linear.app/materializeinc/issue/SQL-595)**: `CREATE REPLACEMENT MATERIALIZED VIEW` accepts a sealed target (e.g. a constant-folded view, which seals on hydration whatever its refresh schedule says) that `APPLY REPLACEMENT` then refuses forever, leaving an unappliable pending replacement. Found by the sealed-shard oracle. Known negative-accumulation errors (database-issues#9308) from `DELETE ... USING` and `repeat_row` are tolerated in the scenarios that generate them and stay strict elsewhere, so a genuine finding still fails. Coverage for two further directions is landed but left disabled, ready to flip on once the bugs are fixed: a dependency-consistency oracle ([SQL-521](https://linear.app/materializeinc/issue/SQL-521)) and a source read-hold sweep across restart ([SS-346](https://linear.app/materializeinc/issue/SS-346), [PER-49](https://linear.app/materializeinc/issue/PER-49)). Closes: [QAR-139](https://linear.app/materializeinc/issue/QAR-139) --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )