From ca257444e040c6ce25671e19605b7664f26a4efe Mon Sep 17 00:00:00 2001 From: Pranshu Maheshwari Date: Thu, 10 Sep 2026 14:30:17 -0400 Subject: [PATCH 01/11] docs: add historical freshness monitoring section (#38744) --- .../transform-data/monitor-freshness.md | 56 ++++++++++++++----- 1 file changed, 43 insertions(+), 13 deletions(-) diff --git a/doc/user/content/transform-data/monitor-freshness.md b/doc/user/content/transform-data/monitor-freshness.md index 75a80386db5d4..b671f5f66f7a8 100644 --- a/doc/user/content/transform-data/monitor-freshness.md +++ b/doc/user/content/transform-data/monitor-freshness.md @@ -54,22 +54,52 @@ healthy pattern for a lightly loaded object. {{< note >}} -Materialize exposes wallclock lag history through two relations. Which one you -query has a large impact on performance. +[`mz_internal.mz_wallclock_global_lag_recent_history`](/sql/system-catalog/mz_internal/#mz_wallclock_global_lag_recent_history) +is indexed and holds only the past 24 hours of data. Querying it is fast, so it +is the right choice for frequent or interactive monitoring and for dashboards. +Use this relation by default, as in the query above. For older data, see +[Monitor historical freshness](#monitor-historical-freshness). -- [`mz_internal.mz_wallclock_global_lag_recent_history`](/sql/system-catalog/mz_internal/#mz_wallclock_global_lag_recent_history) - is indexed and holds only the past 24 hours of data. Querying it is fast, so - it is the right choice for frequent or interactive monitoring and for - dashboards. Use this relation by default, as in the query above. +{{< /note >}} -- [`mz_internal.mz_wallclock_global_lag_history`](/sql/system-catalog/mz_internal/#mz_wallclock_global_lag_history) - covers the full retention window (at least 30 days) but is unindexed, so it - can be slow to query. A single query can occupy `mz_catalog_server` for - several seconds. Reach for this - relation only when you specifically need data older than 24 hours, and avoid - querying it frequently. +## Monitor historical freshness -{{< /note >}} +`mz_wallclock_global_lag_recent_history` holds only the past 24 hours. To look +further back, query +[`mz_internal.mz_wallclock_global_lag_history`](/sql/system-catalog/mz_internal/#mz_wallclock_global_lag_history) +instead, which covers the full retention window of at least 30 days. The +columns are identical, so any query in this guide works against it by swapping +the relation name and widening the time filter: + +```mzsql +SELECT wl.occurred_at, wl.lag +FROM mz_internal.mz_wallclock_global_lag_history wl +JOIN mz_catalog.mz_objects o ON wl.object_id = o.id +WHERE o.name = '' + AND wl.occurred_at > now() - INTERVAL '7 days' +ORDER BY wl.occurred_at DESC; +``` + +Because the relation is unindexed, a single query can occupy +`mz_catalog_server` for several seconds. Keep the `occurred_at` filter as +narrow as the question allows, and aggregate in the query rather than pulling +raw rows out for a longer window. For example, the following returns one +maximum lag per day over the past 30 days: + +```mzsql +SELECT + date_trunc('day', wl.occurred_at) AS day, + max(wl.lag) AS max_lag +FROM mz_internal.mz_wallclock_global_lag_history wl +JOIN mz_catalog.mz_objects o ON wl.object_id = o.id +WHERE o.name = '' + AND wl.occurred_at > now() - INTERVAL '30 days' +GROUP BY 1 +ORDER BY 1 DESC; +``` + +For recurring or dashboard queries, stay on +`mz_wallclock_global_lag_recent_history`. ## Summarize freshness with a CCDF From 9a48374a1eb5fc23128bb87af04a02f7f49626f3 Mon Sep 17 00:00:00 2001 From: MkByte <18468315+martykulma@users.noreply.github.com> Date: Thu, 10 Sep 2026 15:12:10 -0400 Subject: [PATCH 02/11] SS-58 design: concurrent processing of snapshot and cdc data (#38658) Today's implementation of sources requires that all snapshot data is ingested before CDC events start getting processed. Two places that are impacted by this behavior: 1. Adding a new table/subsource to a source causes all tables to wait for the newly added table to complete snapshotting. 2. When multiple tables/subsources snapshot, all of them must complete together. This prevents smaller tables that finished snapshot from becoming available for read and extends failure window, which causes all tables to repeat the snapshot. The goal was to avoid making changes to persist, if possible. The major changes are in the storage persist sink, where data from the snapshot operator and replication operators will now arrive concurrently. Data needs to be offloaded to blob storage to prevent excessive memory usage, and append to the shard must maintain correctness, ensuring that the snapshot and CDC data are appended and races with other replicas are properly handled. The design relies on a leading window for batches that is larger than the source tick interval, allowing multiple timestamps to be written per batch. Eventually collapsing to the tick interval, once the snapshot is complete and the frontier is no longer pinned. This proposal also splits up exports into separate outputs and capabilities. In the current implementation, all exports move in lockstep. The changes allow each export to move independently, and this has knock on effects for how information about the sources and how `offset_committed` is recorded. Claude combed through the console code to see where assumptions where being made, and those are described in their own section. Besides the storage persist sink and `offset_committed changes`, the other changes are source specific. OLTP sources will be updated independently, starting with Postgres. Kafka and upsert are harder to solve, and I couldn't think of a good way to make this work for them in a reasonable time frame. Kafka sources/sinks create independent clients per worker, which is excessive, and this design would require that to double. There are cases where this can already cause problems with upstream systems, especially with larger cluster sizes. Upsert v2 wouldn't be able to emit any data from the merge batcher to persist due to the pinned frontier, causing data to backup in the upsert operator. Ok to defer for now per discussion with @maheshwarip and @ohbadiah. --- .../20260729_per_export_snapshot_frontiers.md | 636 ++++++++++++++++++ 1 file changed, 636 insertions(+) create mode 100644 doc/developer/design/20260729_per_export_snapshot_frontiers.md diff --git a/doc/developer/design/20260729_per_export_snapshot_frontiers.md b/doc/developer/design/20260729_per_export_snapshot_frontiers.md new file mode 100644 index 0000000000000..3ee97d230fcea --- /dev/null +++ b/doc/developer/design/20260729_per_export_snapshot_frontiers.md @@ -0,0 +1,636 @@ +# Per-Export Frontiers for Source Snapshots + +## The Problem + +Adding a table to a running source (`CREATE TABLE ... FROM SOURCE`, or the legacy +`ALTER SOURCE ... ADD SUBSOURCE`) restarts the ingestion dataflow. On the new incarnation, the +added table has an empty resume upper and is snapshotted. Until that snapshot completes, no +subsource of the source makes visible progress. CDC events for already hydrated tables are not +processed, and their persist uppers do not advance. A snapshot of one large table can block every +other subsource for hours. + +If multiple tables are added together, snapshotting may complete for the smaller tables, but they +cannot make progress until the largest table is also done. Any error during this time restarts +snapshotting for all tables. + +For the rewind-pattern sources (PostgreSQL, MySQL, SQL Server) the lost progress has two +causes, both in the upstream reader operators: + +1. The replication operator drains its rewind input to completion before processing any CDC + events. Rewind requests are only sent after `COPY` (for OLTP) completes, so the CDC stream is + not read while a snapshot runs. +2. All exports share a single data capability. While any rewind request is pending, the capability + cannot be downgraded past the minimum offset, because the operator must still be able to emit + retractions at the minimum timestamp. + +Kafka blocks for a different reason. Multiple tables can also be created from one Kafka source, for +example to accommodate schema changes in the topic. The reader resumes every partition from the +minimum resume upper across all exports, so adding a table makes the single consumer re-read the +topic from the new table's start offsets, and the shared output port means no export commits new +data until the re-read reaches the tip. + +Everything downstream of the snapshot and replication operators is already per export. +- the statistics operator +- reclock +- decode and envelope +- the persist sink + +One thing is not. The shard upper coming back from each persist sink is concatenated into a +single feedback edge, so its frontier is the meet across exports and every export reports the +slowest one's `offset_committed`. See Statistics semantics. + +## Success Criteria + +- While a newly added table snapshots, the already hydrated exports of the same source continue to + process CDC events and their persist uppers continue to advance. +- When multiple tables are added together, each commits its snapshot and begins advancing as soon + as its own snapshot completes. +- Definiteness is preserved. The contents of every collection at each readable timestamp remain a + deterministic function of durable shared facts. +- Exact multiplicities are preserved. No design element requires primary keys or upsert semantics + from a source that does not already have them. +- Memory on the replica during hydration is bounded by the unflushed tail of each open builder, + independent of snapshot duration. +- The number of batches the sink appends is significantly lower than 1 per tick per worker. +- A restart during a snapshot remains correct. + + +## Solution Proposal + +Give each export its own output port and capability on the snapshot/replication operators, and +hold back only the hydrating exports. Sources that use the rewind pattern keep their protocol +unchanged. Kafka would need no reconciliation at all, since a backfill feed and a steady feed +read disjoint offset ranges. Its second consumer is deferred, see the Kafka section and Rollout. + +The work is confined to each source. `SourceRender::render` already returns per-export +collections, reclock derives per-export output frontiers from per-export input frontiers without +changes, and remap bindings are minted from the probe channel independent of data capabilities. + +Steps 1 and 4 apply to all sources. Steps 2 and 3 are the rewind-pattern changes. The Kafka +section sketches their Kafka equivalent for the deferred work. + +### 1. One output port per export + +Replace the multiplexed `(output_index, data)` output and its `partition()` demux with one output +port per export on the snapshot and replication operators. Exports are known at render time, ports +are created in a loop, and each port gets its own capability set. + +This step preserves existing behavior. All port capabilities are downgraded in lockstep, so every +export observes the same frontier as today. + +For existing operators using a single output handle, `handle.give_fueled()` provides a means of +yielding at `MAX_OUTSTANDING_BYTES` to prevent the CDC stream from overwhelming downstream +operators. With the change to multiple output handles, operators must aggregate bytes emitted for +comparison against `MAX_OUTSTANDING_BYTES`. + +There aren't any known issues with the number of input or output ports in Timely. + +### 2. Early snapshot bound + +Modify the snapshot operator to emit rewind requests at the start of the snapshot instead of at +completion. For Postgres, the snapshot LSN is the temporary slot's consistent point and is known +as soon as the slot is created, before any data is copied. The `resume_lsn <= snapshot_lsn + 1` +validation moves to this earlier point. + +The replication operator no longer drains the rewind input before opening the replication stream. +It starts streaming immediately, knowing from the early rewind requests which exports are +hydrating. + +### 3. Per-export capability policy + +**Snapshot operator:** ports of exports the operator will not emit snapshot data on are closed +immediately. That covers exports whose resume upper is past the minimum and exports that carry no +snapshot data at all, such as a source's primary relation. A hydrating export's port holds at the +minimum until its snapshot completes, the `COPY` for PostgreSQL, then closes. + +**Replication operator:** ports of hydrated exports downgrade with the data upper. A hydrating +export's port holds at the minimum until the data upper passes its snapshot offset, because events +at or below the snapshot offset must still be emitted negated at the minimum for that export. Once +the upper passes the snapshot offset, the port downgrades with the data upper like the rest. The +emission of rewind negations remains unchanged. + +Sources still acknowledge at the minimum shard upper across exports. For PG, a hydrating +export continues to pin WAL retention, but no longer pins any other export's frontier. + +### 4. Bounded sink accumulation + +A batch's bounds come from a batch description, which `mint_batch_descriptions` emits when the +collection's frontier advances. While an export snapshots its frontier is pinned, so no +description is minted for the duration. The sink still receives snapshot rows at the pinned time +and CDC events at later times, and has to group them into batches without knowing which +description will cover them. + +Sources have the time domains `F` and `T`, `FromTime` and `MzTime`, respectively. The generic type +names are `FromTime` and `IntoTime`, and in practice `IntoTime` is MZ time. +- The snapshot operator emits data at `F:min`, which is reclocked to `T:c`, where `T:c` is the + captured current time. The snapshot operator pins its capability to `F:min`, and by extension, + `T:c`. +- The replication operator emits data at from-times `f >= F:min`, that are mapped to MZ times + `t >= T:c`. The rewinds are currently the only data emitted by this operator at `F:min`. The + replication operator downgrades caps as today. + +#### Grouping + +The sink strives to write one batch per description, taking every timestamp that description covers +into a single `BatchBuilder`. The grouping cannot be chosen before the bounds are known, and a +`BatchBuilder` cannot be split once a description boundary lands inside it. An update goes into a +`BatchBuilder` as it arrives. The builder is the buffer. + +The data that arrives before the description covering it is the leading edge. A row in the leading +edge is a trailblazer, ahead of its description. Rows that arrive after their builder was closed +are stragglers. Both occur and the sink has to account for them. + +``` +────────────────────┼───────────────┼─────→ leading edge +stragglers committed committed (trailblazers) + lower upper +``` + +`BatchBuilder` writes a part to blob storage every `persist_blob_target_size`, and keeps at most +`persist_batch_builder_max_outstanding_parts` uploads in flight. What sits in memory while the +frontier is pinned is the unflushed tail of each open builder. A batch stays unlinked in blob until +its description is appended, so the sink trades memory for blob writes it may throw away on a +restart. + +Two kinds of builder are open at once. +- A builder per in-flight description, taking data at timestamps in its range. +- A builder per trailblazer timestamp, for data that arrives ahead of the current committed + description. + +A description's builder closes as soon as data moves past it. This keeps the number of builders +from following how long the frontier stays pinned, with one exception. The builder whose description +covers the frontier's own time, which carries the snapshot data, stays open until the frontier +advances and its description is ready. + +Arrival order is an efficiency assumption, not a correctness one. Readiness and the +`operator_batch_lower` declared by new data are both gated on the frontier, so a straggler opens a +new builder under the description it would have landed in, and costs one extra batch. This is +different from trailblazers, where the bounds are unknown and batches must be single-timestamp. + +#### Committed descriptions + +The sink can group without waiting for the frontier if it knows where the next description boundary +falls. So the minter commits to boundaries instead of deriving them. It emits `[a, b)` for a `b` of +its own choosing and then honors it, so a frontier landing inside `[a, b)` is ignored and the next +description still starts at `b`. + +Committing early is safe because a description doesn't assert completeness. The sink appends only +once `desired_frontier` reaches the upper, so a committed description waits in `in_flight_batches` +until then. + +A description has to reach the writers before the rows it covers, since a builder only takes rows +at times it was opened for. The first window `[T:c, T:c + w)` is committed the moment the frontier +pins, before any row arrives, on the knowledge that the snapshot's rows are about to land at `T:c`. +The minter sees the largest timestamp the data has reached and commits the next window once that +timestamp comes within a margin of the committed upper. The margin is the initial width, so every +description keeps a head start over the rows. Each commitment doubles the next width, up to +`storage_persist_sink_description_window_max`, so a short snapshot commits little past its end and +a long one costs few descriptions. + +The minter has two rules for emitting a description. +- Emit `[current_upper, desired_frontier)` whenever the frontier is ahead of the committed upper. + This is steady state and commits to nothing. +- Emit `[current_upper, current_upper + width)` once `max_seen + margin > current_upper`, while the + export's snapshot is in progress. + +Only a commitment ending past the frontier widens the window, so the doubling is driven by the +pinned frontier, not by steady state. Committing costs the minter the coupling between the upper it +emits and the frontier it observes. It downgrades to that upper rather than to the frontier, so +`current_upper > desired_frontier` becomes a state it has to expect. + +The rules interleave in three phases. `F` is the frontier and `C` the committed upper, with the +snapshot pinned at `c` and an initial width of `w`. While the snapshot runs, `F` does not move and +`C` steps ahead of the data, each step on the second rule: + +``` +frontier F C F pinned at c for the whole snapshot +MZ time ----c------c+w---------c+3w----------------c+7w--> + [c,c+w)[c+w, c+3w)[c+3w, c+7w) C steps ahead of the data + on the when data when data + pin nears c+w nears c+3w +``` + +When the snapshot ends, `F` jumps to wherever the source has reached. Every window below it appends +in one pass. The window it lands in binds, and the distance from `F` to `C` is the tail: + +``` +frontier F C F jumps to where the source is +MZ time ----c------------------c+3w----------------c+7w--> + [c, c+3w) appended [c+3w, c+7w) binds shard upper waits at c+3w + |-tail->| until F reaches c+7w +``` + +Once `F` passes `C`, the first rule takes over and every description is derived from the frontier, +so `C` and `F` coincide from then on: + +``` +frontier FC FC FC C rides on F +MZ time ----c+7w-------F1--------F2--------F3------------> + [c+7w, F1) [F1, F2) [F2, F3) each derived from the frontier +``` + +An export snapshots when its resume upper is the minimum from-time, and this is passed into the +persist sink. The test has to be made in the from-time domain. Reclocking a resume upper maps any +MZ time at or below the as_of back to the minimum, which keeps a restart during a snapshot reading +as snapshotting even though its shard upper has moved past `T:min`. The frontier alone cannot tell, +since any restart would look like a snapshot. Exports with the CDCv2 envelope are excluded, as +their MZ times come from the data rather than from reclocking, so a wall-clock width means nothing +there and a committed upper the data never reaches would hold the shard upper forever. + +The frontier determines when the snapshot ends, which relies on a snapshot occupying a single MZ +time. Sources that rewind emit theirs at `F:min`, so it reclocks to `T:c` and the frontier holds +there until the snapshot port closes and the replication port downgrades past the snapshot offset. +Kafka reads real offsets and reaches the same place, see the Kafka section. The minter keeps the +first non-minimum frontier a snapshotting export takes and permits the second rule only while +`desired_frontier` equals it, never before that frontier arrives. Timely does not order progress +ahead of data, so a row can reach the minter under a frontier still at the minimum, and committing +on it would anchor the window at the shard upper instead of `T:c` and mint every window between the +two. On a fresh shard that is every window between zero and the wall clock. + +Catching up costs one append. Every committed description becomes ready in the same pass when the +frontier advances, and `append_batches` combines them into a single `compare_and_append` over the +whole range. Setting `persist_validate_part_bounds_on_write` or +`persist_validate_part_bounds_on_read` gives each description an append of its own instead. + +The commitment's cost is a tail. A commitment is binding, so while `current_upper` sits ahead of +the frontier the first rule mints nothing, and when the snapshot ends the shard upper waits for the +frontier to reach the last committed upper rather than advancing to where the frontier reached. The +last window was committed when the data came within the margin of the previous upper, so the tail +is at most the margin plus the last width, up to the snapshot's own duration and capped by +`storage_persist_sink_description_window_max`. The snapshot gate confines this to exports that are +snapshotting, where the frontier is not advancing anyway. A collection keeping up lags the data by +about one `timestamp_interval` and has nothing to group, so committing there would pay the tail for +nothing. + +An export whose stream is quiet during its snapshot and then receives a burst commits every window +between the last upper and the burst in one pass, each an empty description that still has to be +minted, broadcast, and carried to the append. + +Leaving the window at zero mints descriptions from the frontier alone, so the sink writes one batch +per timestamp exactly as it does today. + +#### Commit timing across workers + +The collection is pre-sharded, so any one worker sees a share of it and its largest timestamp can +trail the collection's. Every worker reports its largest timestamp to the minting worker. + +This makes the trigger independent of how a source distributes rows. PostgreSQL round-robins +replication rows across all workers and MySQL reads its binlog on one, and either way the minter +times the next commitment against the largest timestamp any worker has reached. Without those +reports, a minter that is not the binlog worker would never see a timestamp past `T:c`, no window +after the first would be committed, and everything that worker took in during the snapshot would +degrade to one batch per timestamp. + +#### Concurrent ingestion + +A concurrent writer, another replica of the same ingestion, can raise the shard upper into the +middle of a description. The sink advances that description's lower and re-appends. Persist +registers the batch under the narrowed bounds, as a truncated batch, and filters the out-of-bounds +updates on read. Batches carry their largest timestamp, so ones lying entirely below the new lower +are deleted rather than appended as truncated. + +#### Statistics semantics + +`offset_committed` is reported per export. Each export gets its own feedback edge carrying its +shard upper back from its persist sink, and one operator in the pipeline inverts each of those +uppers through the remap bindings and reports the result for that export alone. +`SourceTimestamp::to_offset_stat` does the frontier-to-offset conversion, which every timestamp +type already had in some form on the source side. What the source acknowledges upstream is still +the meet across exports, which has to respect the slowest one, and `mz_source_statistics` reports +the parent source as `MIN(offset_committed)` over its exports, so the source-level number keeps its +old meaning while the per-export rows tell the truth about each table. + +Two things fall out of that. Every worker has to initialize the gauge, because the controller only +aggregates a gauge once every worker has reported a value for it and renders a failed aggregate as +zero, which single-worker tests will not catch. And PostgreSQL no longer pre-fills +`offset_committed` with the slot's resume LSN at startup, which existed to keep the lag calculation +from looking enormous during an initial snapshot. An export reads zero until its first commit now. +That is the honest per-export lag, but anything subtracting it from `offset_known` will show the +whole LSN as lag while an export snapshots. + +#### Handling for large transactions + +If an upstream transaction contains a large number of rows, they all land at a single timestamp. The +builder holding them spills its parts to blob storage once it passes `persist_blob_target_size`, so +memory is bounded by that rather than by the transaction. A description covering that timestamp puts +the transaction in the same batch, and the same append, as everything else the description covers. + +### Kafka backfill consumer with offset handoff, deferred + +This section is the sketch for the Kafka side, which Rollout defers. Kafka has no rewind machinery. +The "snapshot" of a new export is a re-read of the topic from the export's start offsets, and "CDC" +events are the data after the snapshot, split by Kafka offset. If there are two exports for the same +topic, the hydrated export stops progressing because the shared Kafka consumer has to read from the +other export's start offsets. + +The Kafka source would have up to two consumers, one backfilling every export that needs hydration +and one for steady state. If no hydration is necessary, or all exports need hydration, only a single +consumer is created. This limits the two-consumer case to the situation that motivates it, adding a +new export where one already exists. + +For the mixed-export case, a frontier `B` is determined on startup. The backfill consumer will +capture rows below `B`, and the steady-state consumer captures rows at or above `B`. The two +consumers emit concurrently for hydrating exports. As today with the transition from hydration to +steady state, the handoff at frontier `B` requires no dedup or negation. This is possible because +the set of records for the snapshot and the set of records for steady state are disjoint. The +logical choice for `B` is the minimum `resume_upper` for all non-hydrating exports in the +mixed-export case. For the case where all exports are hydrating, `B` keeps its existing definition, +which is the maximum frontier (maximum offsets of each partition). + +The backfill occupies a single MZ time, which is what the sink's snapshot gate reads. Reclocking +maps an offset to the earliest binding whose source upper exceeds it, and the controller holds the +remap shard's since one step behind the exports' uppers, so the trace a restart reads is compacted +to `T:c` and every offset below the source upper there lands on it. `B` sits within a step of that +bound, so the hydrating export's frontier stays at `T:c` for the backfill's duration and advances +once when the backfill port closes. A source where every export is hydrating gets there by a +different route. The first binding minted into an empty remap shard maps the probed tip to the +minimum MZ time. + +The existing Kafka behavior for reporting snapshot progress is to capture the maximum offsets for +each partition in the topic, sum them, and treat the sum as `snapshot_records_known`. In the +mixed-export case, snapshot and steady-state records are being processed concurrently. To +accurately record statistics, metric definitions are updated. + +1. `snapshot_records_known` would be `sum(B - start_offsets)` +2. `snapshot_records_staged` counts backfill progress only +3. `updates_staged` counts steady-state records (doesn't include backfill) + +This fixes an issue in the existing implementation, which ignores `start_offsets` in the +calculation of `snapshot_records_known`. This doesn't change the issue that `B - start_offsets` +overcounts on compacted topics. + +Upsert v2 is rolling out and is expected to become the default. Correctness holds there, since +snapshot and steady-state records land in the merge batcher and are only emitted on frontier +progression. Unfortunately, updates will accumulate in the upsert merge batcher because the frontier +is pinned, increasing memory utilization. The increase is the replication updates (e.g. messages +at offsets post-snapshot). The batcher pages cold data out of resident memory, but the staged +volume itself grows with the snapshot duration. Additionally, the merge batcher drain is sequential +per timestamp. For a long hydration period, this drain would cover thousands of timestamps, each +making a round-trip through persist. Upsert needs rework to cover the concurrent streams behind +a pinned frontier. This is deferred as it needs more thought and adds a lot more risk around +correctness, so it is best handled as a separate effort. + +### Restart semantics + +A restart during hydration discards the accumulated updates. That now includes the CDC staged +during the snapshot, where before there was only snapshot progress to lose. Multiple tables +snapshotting together come out ahead. Today none of them can complete until all do, so a restart +sends every table back to the start. With independent frontiers a table that finished keeps its +snapshot and only the unfinished ones start over. + +The upstream position the source acknowledges is the meet over exports of their shard uppers, +inverted through the remap bindings. A snapshotting export's shard upper never passes `T:c`, so the +meet stays at the position the source resumed from when the export was added, `P`, and the upstream +retains everything from `P` for the whole snapshot. For PostgreSQL that position is the replication +slot's confirmed flush LSN. And the dataflow `as_of` on restart is the remap since, which the +snapshotting export's read hold keeps at `T:c`, so the new incarnation reclocks that export's shard +upper back to the minimum and snapshots it again even though the empty first description moved the +upper to `T:c`. + +These are notes on the newly possible states and how to test them. The cases below use a hydrated +export `A`, a new table `B` with snapshot offset `s`, and `B`'s appended windows written as +`[c, c+kw)`. Offsets are the source's from-time, LSNs for PostgreSQL. + +1. **Restart during the copy.** `B` snapshots again at a new snapshot offset `s' > s`, stamped + at the same `T:c`. The resume offset is the minimum over exports and `B` contributes `P`, so + the stream restarts at `P` and re-decodes everything `A` appended during the snapshot. Those + rows are dropped below `A`'s shard upper, but `A`'s frontier and its `offset_committed` hold + until the re-read passes `A`'s resume offset. The restart also reads the + remap trace retained since `T:c`, one binding per probe for the snapshot's duration, in both + `reclock_resume_uppers` and `reclock_committed_upper`. Correctness rests on the filtering of + re-read rows below an export's upper in the persist sink and in upsert. +2. **Restart in the tail.** The copy is complete, `B`'s frontier sits inside the last committed + window, and the windows below it are appended, so `B`'s shard upper is a window boundary such as + `c+3w`. When `s` reclocks to a time above that boundary, `B`'s resume offset is below its own + snapshot offset, yet `B` counts as hydrated, so no copy and no rewind request. `B`'s events + between the resume offset and `s` are emitted forward at times at or above `c+3w`, while their + negations at `T:c` are already in the appended batch. The appended prefix is `B`'s state at the + resume offset, so the result is correct. Correctness rests on appends being frontier gated, + which makes any `B` upper past `T:c` imply a complete snapshot. +3. **Two tables added together, one completes first.** `B1` is hydrated and advancing while `B2` + still copies. Only `B2` snapshots again. `B1` resumes from its own upper and keeps its data, as + does `A`. The stream still restarts at `P` because `B2` contributes it, so `B1`'s events between + `P` and its resume offset arrive again and are dropped, and `B2`'s rewind negates `(P, s2']`. +4. **Restart after `B`'s replication port releases but before the copy ends.** The stream has + passed `s`, the rewind entry is gone, and `B`'s forward events already flow into windows. + Nothing is appended past `T:c`, so the restart is case 1, but the negations emitted for `(P, s]` + are discarded with the batches and the new incarnation negates the wider `(P, s']`. The leaked + batches hold that CDC volume as well as the snapshot, see Out of Scope. +5. **Drop of the snapshotting table.** Dropping `B` restarts the dataflow without it. Its read + hold goes, the remap since moves up to `A`'s upper, the resume offset becomes `A`'s, and the + acknowledged position advances from `P`. Dropping `B1` while `B2` still copies must leave it at + `P`, since `B2` still contributes it. + +Cases 1 and 2 are the newly possible states. A stream restarting at `P` while siblings are far +ahead, and a hydrated resume point below the export's own snapshot offset. Cases 3 to 5 are +existing behavior over wider intervals. Each needs a test that restarts the replica in that state +and compares every export against upstream, see Open Questions. + +### Rollout + +PostgreSQL first, then MySQL and SQL Server, which use the same rewind structure adapted to their +offset types. MySQL keys `RewindRequest` by a GTID snapshot upper, SQL Server already tracks a +per-export `initial_lsn` and `snapshot_lsn` pair. In both, the snapshot upper is available when +the snapshot transaction is established, so the same three operator changes apply. Kafka is +deferred, since its backfill consumer is new machinery and upsert needs a rethink. + +The behavior change is gated by the +`storage_source_snapshot_concurrent_replication` feature flag, default off in production and +default on in CI so the new path is exercised by the test suites before it is enabled. + +`storage_persist_sink_description_window` is separate and defaults to zero, which leaves the sink +writing one batch per timestamp. The test configuration sets it to one second with +`storage_persist_sink_description_window_max` at five minutes. Concurrent replication is what makes +a snapshot accumulate many timestamps, so the window has to carry a value before that flag is turned +on. + +In general, a source supports independent export frontiers when it can do four things. +1. emit each export on its own output port with its own capabilities +2. determine the snapshot consistency bound when the snapshot starts rather than when it completes +3. reconcile snapshot and stream either by compensating negated emission at the minimum time or + through an envelope that derives retractions from state +4. derive per-export progress from its own read process + + +## Open Questions + +- **Restart interleavings.** + - Which of the five cases under Restart semantics get a test, and in which framework (platform + checks, testdrive). Case 2 needs a window small enough that the first appended window closes + before the snapshot offset's time. Cases 1 and 4 need a replica killed while a copy runs against + a busy sibling. +- **Trailblazer visibility.** + - Whether the leading edge needs a metric for the batches it produces, so a snapshot whose + trailblazers degrade it toward one batch per timestamp is visible rather than inferred from + persist shard shape. +- **Reader distribution versus pipelining.** + - Whether source readers should round-robin rows to all workers, as PostgreSQL does, or whether + an export should stay on the worker that read it. The source implementations already disagree. + PostgreSQL exchanges and casts rows downstream of the reader, MySQL reads, decodes and emits on + one worker. Neither shape is known to be the faster one. There is no data. + - Pipelining is less complex, and part of what would bound it is persist part uploads. Uploads + already run as spawned tasks, but `persist_batch_builder_max_outstanding_parts` defaults to 2, + so the builder blocks on blob storage long before the append that actually commits, and + `write_stalls` counts it when that happens. Measuring a single export at high volume on one + worker, with `write_stalls` and CPU separated, would show where the bottlenecks are. + - The one-worker shape no longer starves the minter, since every worker exchanges its largest + timestamp to it, so this is a throughput question alone. +- **Multiple concurrent hydrations.** + - Builders spill to blob at `persist_blob_target_size`, so the resident exposure per export is one + unflushed part for the builder covering the frontier's time, one for the current window, and one + per trailblazer timestamp, summed over every export snapshotting at once. Whether that stays + inside a replica's memory when several large tables hydrate together. +- **Tail after long snapshots.** + - A snapshot longer than the window cap ends with a tail of up to the cap plus the margin before + its export is readable. Whether that is acceptable, and what a policy that shortens the last + window as the snapshot nears its end would cost in descriptions, given the sink cannot see the + end coming. + + +## Cross-component impact (Claude discovered) + +- **Console assumes exports move in lockstep.** + - Ingestion lag tracks the freshest export, `max(offset_committed)` over the source and its + tables, in `console/src/api/materialize/source/sourceStatistics.ts` for both the current query + and the pre-0.148 variant. Fine when every export reported the same offset, but exports can + diverge now, so a table that is still snapshotting doesn't show up. + - Combined sources may be overcounting. The console unions the source with its tables in + `sourceStatistics.ts`, and `mz_source_statistics` in `src/catalog/src/builtin/mz_internal.rs` + already rolls the tables up into the source. + - The snapshotting badge is all-or-nothing. `console/src/api/materialize/source/sourceList.ts` + reads the source-level `snapshot_committed`, which is `bool_and` over the exports, so + `console/src/platform/connectors/utils.ts` shows a source as snapshotting while the rest of its + exports are streaming. + - The per-table tab and the queries behind it only know about source status, in + `console/src/api/materialize/source/sourceTables.ts` and + `console/src/platform/sources/SourceTables.tsx`. They should be expanded to show per-export + info, which is where the divergence is most worth seeing. + - `console/src/platform/maintained-objects/SourceDiagnostics.tsx` decides the snapshot is done + from record counts instead of the `snapshot_committed` flag. Possibly a bug, though it may have + been a workaround, in which case we should fix whatever made the flag unusable. + - The maintained objects list in `console/src/platform/maintained-objects/queries.ts` drops + anything with `sourceType == "subsource"` and shows only the top-level source. That is no + longer accurate. The parent says nothing about the subsources under it. + + +## Out of Scope + +The following are follow-on work. The design does not depend on them, and they can land +independently later. + +- **Recovering accumulated work across restarts.** + - A restart during hydration discards the hydrating export's snapshot and CDC data. This will + affect the freshness of already hydrated exports, as the CDC stream has to be re-read. +- **Cleanup of leaked batches.** + - Batches that were written to persist but not linked in the shard are leaked. This is existing + behavior, but was bound by snapshot size, and is now bound by snapshot + CDC during snapshot + size. +- **Upsert storage increase due to concurrent streams.** + - The deferred Kafka design relies on two consumers, a backfill and a steady-state one. In a + mixed-export scenario, some exports hydrating and some in steady state, the hydrating + exports' frontier is pinned due to hydration. Records from both the snapshot and steady state + will collect in the upsert's merge batcher, where before it was only snapshot records. + Addressing this requires additional design around upsert. + + +## Considered but Rejected + +Summary of the rejected options and why. + +- **DBLog-style watermark snapshots.** + - Interleaving chunked snapshot reads with the stream, deduplicating by primary key between + watermarks, produces a per-key upsert stream with no consistent point-in-time state. Rejected + because storage collections require exact multiplicities, the nondeterministic interleaving + violates definiteness, and all sources would require upsert. +- **Persist extensions as the primary mechanism.** + - Gap appends or placeholder batches would make concurrent CDC durable immediately and remove the + sink accumulation problem entirely, and a staged batch registry would make the grouped builders + crash-safe. Rejected for the initial delivery because they change persist shard state, compaction, + and read paths. +- **Out-of-band snapshot committed directly to persist.** + - Run the snapshot as a one-shot job and `compare_and_append` the result, then restart the main + dataflow to pick up the export, which is now past the snapshot, so no other exports are affected. + Rejected because it needs infrastructure this design does not. + - a one-shot job outside the ingestion dataflow + - a durable handoff record + - verification that the slot has not compacted past the recorded LSN + - per-export start points threaded into the dataflow +- **Shadow pipeline with catch-up and handoff.** + - A second temporary pipeline with its own replication slot snapshots the new table, follows CDC + to an agreed LSN, and hands off. Rejected as this does not improve the slot's WAL retention, adds + connection cost on the upstream database, and requires an exactly-once handoff protocol. + +The following were considered for the persist sink, and all share a root cause. Each tries to +choose a batch's grouping before its bounds are known, which is the thing that cannot be done +safely. + +- **Trailing-lag grouping.** + - Hold arriving rows and move them into a grouped builder once they fall a fixed lag `L` behind + the latest data time, rotating that builder when the frontier advances. Rejected because `L` must + exceed the frontier's propagation delay, which is not a quantity the sink can bound, and + a violation is only detected at rotation, where the recovery is a dataflow restart. +- **A coalescing horizon published by the minter.** + - Have `mint_batch_descriptions` broadcast a promise that no future description will end below + some time, so write operators can group updates below it before their description exists. + Rejected as stated, because the promise was derived from the minter's frontier, which is pinned + for exactly the duration of a snapshot. The committed descriptions in step 4 are the corrected + form. The minter commits to boundaries of its own choosing instead of promising something about a + frontier it cannot move, and the data it already passes through triggers the commitment. +- **Source read progress as the coalescing horizon.** + - Feed the source's reclocked read progress into the minter so the promise can advance while the + collection's frontier is pinned. Rejected because a horizon that certifies completeness has to + come from the data path to be sound. Computing it beside the data path lets it announce a time + the reclock operator has not released updates for yet, so it needs a progress output on the + per-export reclock, ordered behind the data that operator emits, and even then it cannot pass the + upsert merge batcher. Allowing several batches under one description drops the requirement from + completeness to a boundary trigger, which the bindings satisfy without touching source + machinery. +- **Splitting the pinned traffic onto its own output.** + - Give the traffic at the minimum offset, the snapshot rows and the rewind retractions, a + separate output so the ongoing replication stream keeps an unpinned frontier. Rejected because + the split is not static. The replication operator's schema-validation errors are emitted at + whatever the port's capability currently holds, which is the minimum only while a rewind is + pending, so routing them has no fixed answer. Envelope processing in `render_source_stream` is + also stateful for upsert and cannot carry a split through in general. +- **Ending descriptions at the data, with a raw stash for the leading edge.** + - Commit `[current_upper, max_seen + 1)` every `N` bytes the minter passes through, so a + description ends at the data instead of ahead of it, and have each writer hold rows for + timestamps no description covers yet as raw rows, draining them into the description's builder + when it arrives and evicting the heaviest timestamps into single-timestamp builders when the + stash overflows. This has real advantages. No tail, since the shard upper lands exactly where the + frontier does when the snapshot ends, boundaries that track volume rather than time, no need to + detect the pin, and snapshot rows that cancel against their rewind retractions before anything is + written. Rejected because a description minted behind the data can only group rows the writer is + still holding, so the stash is load-bearing and needs a budget of its own, kept above the + description budget so commitments drain it before eviction fires. The eviction backstop reclaims + that budget and not memory, because an evicted builder below `persist_blob_target_size` keeps its + rows resident, so a stream spread across many timestamps is invisible to it. And the minter sees + only its own worker's bytes, so the trigger needs every worker to exchange running byte totals to + it. Scaling the budget by the worker count instead assumes a source spreads an export evenly, + which holds for PostgreSQL and not for MySQL, and the sink cannot tell the two shapes apart. That + is two budgets with an ordering constraint, a consolidation heuristic, an eviction policy, and an + exchange, and it still needs the same rule for the builder covering the frontier's time. +- **Pager-backed spill for the leading edge.** + - Spill rows into `mz_ore::pager` chunks rather than into persist batches, keeping the batch count + at one per description regardless of budget. Rejected for now because the pager's default backend + is swap, which is ordinary heap plus a reclaim hint and so does not change the bound. Only the + file backend does, and it depends on a scratch directory and on a process-global setting that + compute owns. +- **Product timestamps to expose stream progress under the pin.** + - Give the ingestion an `(outer, inner)` time so the snapshot sits at `(min, min)` and the stream + at `(f, min)`, with the inner coordinate gating persist. Rejected because `(min, min)` is the + minimum of the product domain and is below every stream time, so the frontier is the single + element `{(min, min)}` for the whole snapshot and carries no more than the plain frontier does. + Exposing stream progress needs the snapshot to be incomparable with the stream, that is, above it + in the gate coordinate. That form would give the minter a global trigger and the writer a + frontier-backed closing rule, but it leaves the persist upper and the bounds-at-add-time trade + untouched, and it is sound only where addition commutes, so not under upsert, where base before + delta is exactly the order that pins. +- **Folding concurrent CDC into the snapshot time.** + - Write the stream's rows at `T:c` alongside the snapshot for as long as it runs, so the whole + snapshot period is one persist time, one spilling builder, no grouping, and upsert state that + collapses per key. Rejected because the export's contents at `T:c` become the state at the offset + where the fold ended, so its history over the interval up to that offset's time disagrees with the + remap bindings and with the source's other exports. A dataflow installed on the export during the + snapshot picks an `as_of` in that interval, observes the disagreement, and holds the since there + so it cannot be advanced past. From e2bd66688e9d665a6c175c7e5b31a69014a2262f Mon Sep 17 00:00:00 2001 From: mtabebe Date: Thu, 10 Sep 2026 15:20:40 -0400 Subject: [PATCH 03/11] adapter: add the first curated metric sinks (sizes and errors) (SQL-556) (#38528) [SQL-555](https://linear.app/materializeinc/issue/SQL-555) landed the machinery to install curated metric sinks on every replica, but `CURATED` was empty, so nothing ran at boot. Solution: Add the two v1 definitions: `mz_metric_arrangement_sizes` (three gauge families) and `mz_metric_dataflow_errors` (one gauge). Both read the raw `..._raw` logs, not the derived views, which re-aggregate expensively. The sizes sink labels each series with the exported object's `GlobalId`, not the timely dataflow id, which churns across re-renders. A dataflow with no export, like a subscribe, falls back to an `unattributable` bucket so its bytes still count. The errors sink reads the raw log rather than `mz_compute_error_counts` because that view joins in `mz_compute_dependencies`, a storage-managed source that `ensure_reads_only_logs` rejects: reading it would couple the sink's frontier to envd. Elapsed stays out of v1, its log churns continuously and cost 14% of a 2-worker replica even raw-sourced. `curated_definitions_plan` only called `plan_source`, so a definition that broke the log-only contract still passed the test but crash-looped the coordinator at boot. It now runs `ensure_reads_only_logs` too. Testing: - Unit tests plan each definition and run the log-only gate. - metric-sink.td gains a curated section and a multi-cluster, multi-replica section. - metric-sink.td covers both the attributable and `unattributable` id paths. - The MetricSink platform check now asserts the curated sinks survive a boot, which covers restart, upgrade, and zero-downtime. --------- Co-authored-by: Moritz Hoffmann --- .../checks/all_checks/metric_sink.py | 10 + src/adapter/src/coord/metric_sink.rs | 192 +++++++++--- src/sql/src/plan.rs | 7 +- src/sql/src/plan/statement/ddl.rs | 67 +++- test/cluster/mzcompose.py | 23 +- test/sqllogictest/metric_sink.slt | 32 ++ test/testdrive/dataflow-cleanup.td | 5 +- .../divergent-dataflow-cancellation.td | 83 ++++- test/testdrive/expected_group_size_tuning.td | 9 + test/testdrive/github-6335.td | 3 +- test/testdrive/metric-sink.td | 294 +++++++++++++++++- 11 files changed, 629 insertions(+), 96 deletions(-) diff --git a/misc/python/materialize/checks/all_checks/metric_sink.py b/misc/python/materialize/checks/all_checks/metric_sink.py index e466ade2a98a7..ed5a3bac50037 100644 --- a/misc/python/materialize/checks/all_checks/metric_sink.py +++ b/misc/python/materialize/checks/all_checks/metric_sink.py @@ -96,9 +96,19 @@ def validate(self) -> Testdrive: > SET cluster_replica = ${replica-name} + # Three user sinks came back. Exclude the curated sinks, which the + # coordinator installs on every replica labeled by name not id. > SELECT count(DISTINCT labels -> 'sink') FROM mz_introspection.mz_cluster_prometheus_metrics WHERE metric_name = 'mz_compute_metric_sink_frontier_ms' + AND labels -> 'sink' NOT IN ('mz_metric_arrangement_sizes', 'mz_metric_dataflow_errors') 3 + # The curated sinks come back too, re-installed from the static list + # rather than re-parsed from a catalog item. + > SELECT count(DISTINCT labels -> 'sink') FROM mz_introspection.mz_cluster_prometheus_metrics + WHERE metric_name = 'mz_compute_metric_sink_frontier_ms' + AND labels -> 'sink' IN ('mz_metric_arrangement_sizes', 'mz_metric_dataflow_errors') + 2 + > RESET cluster_replica """)) diff --git a/src/adapter/src/coord/metric_sink.rs b/src/adapter/src/coord/metric_sink.rs index f0e4a8ea38ea6..040d20f56ca50 100644 --- a/src/adapter/src/coord/metric_sink.rs +++ b/src/adapter/src/coord/metric_sink.rs @@ -20,18 +20,10 @@ //! cluster, and the cost scales with `CURATED`. `coord::introspection` already accepts this for its //! subscribes. //! -//! # Lifecycle -//! -//! * After a new replica is created, the coordinator calls `install_metric_sinks` to install every -//! definition on it. `bootstrap_metric_sinks` does the same for the replicas that already exist -//! when the coordinator starts. -//! * Before a replica is dropped, the coordinator calls `drop_metric_sinks` to drop the sinks -//! installed on it. -//! * A replica that disconnects and reconnects (a crash, an OOM) has its dataflows re-rendered from -//! the controller's state, so unlike an introspection subscribe there is nothing to reinstall. -//! -//! This mirrors [`crate::coord::introspection`], which installs introspection subscribes on the -//! same triggers. +//! `install_metric_sinks` installs every definition on a newly created replica +//! (`bootstrap_metric_sinks` covers the replicas already present at startup), and +//! `drop_metric_sinks` drops them before a replica is dropped. This mirrors +//! [`crate::coord::introspection`], which installs introspection subscribes on the same triggers. use std::collections::{BTreeMap, BTreeSet}; @@ -45,8 +37,8 @@ use mz_repr::optimize::OverrideFrom; use mz_repr::{CatalogItemId, GlobalId, RelationDesc}; use mz_sql::catalog::SessionCatalog; use mz_sql::plan::{ - HirRelationExpr, Params, Plan, SubscribeFrom, SubscribePlan, validate_metric_sink_desc, - validate_metric_sink_prefix, + HirRelationExpr, METRIC_SINK_CURATED_PREFIX_MARKER, Params, Plan, SubscribeFrom, SubscribePlan, + validate_metric_sink_desc, validate_metric_sink_prefix, }; use mz_sql::session::user::{MZ_SYSTEM_ROLE_ID, RoleMetadata}; use mz_sql::session::vars::ENABLE_METRIC_SINK; @@ -78,13 +70,91 @@ pub(super) struct CuratedMetricSink { /// with it. source_sql: &'static str, /// Prepended to every row's `metric_name` to form the published name, exactly as a user's - /// `CREATE METRIC SINK ... WITH (PREFIX = ...)`. Must start with `mz_metric_sink_` so the - /// published families land in the reserved lane (see `validate_metric_sink_prefix`). + /// `CREATE METRIC SINK ... WITH (PREFIX = ...)`. Every definition in [`CURATED`] uses + /// [`METRIC_SINK_CURATED_PREFIX_MARKER`], which user sinks are barred from, so nothing a user + /// publishes can collide with a curated family. prefix: &'static str, } /// The curated metric sinks, installed on every replica. -const CURATED: &[CuratedMetricSink] = &[]; +/// +/// Sources take their measurements from the raw `..._raw` logging relations, never from a derived +/// view that re-aggregates them, like `mz_dataflow_arrangement_sizes`: those churn even on static +/// data, and a per-dataflow size metric off the derived view measured ~30x the raw form. Cheap +/// mapping views over the same logs, `mz_dataflow_operator_dataflows` and `mz_compute_exports`, +/// carry no aggregation and are read freely. +/// +/// Every family sums across workers, so a series carries no `worker_id`, and a multi-process replica +/// reports one number per grouping key rather than one per process. The size families emit one series +/// per dataflow, the errors family one per export. +const CURATED: &[CuratedMetricSink] = &[ + CuratedMetricSink { + name: "mz_metric_arrangement_sizes", + prefix: METRIC_SINK_CURATED_PREFIX_MARKER, + // Key on the export id from `mz_compute_exports`, not `mz_dataflow_global_ids`: a + // materialized view builds under a transient view id and appears only as an export, so a + // global-id label names a `t` that maps to no catalog object and churns on every + // re-render. + // + // Logs are per operator, so map operator -> dataflow -> export id. `min(export_id)` + // collapses a multi-export dataflow to one series (lexicographic, so `min('u10', 'u2')` is + // `'u10'`: arbitrary but stable). The group-size hint stops that `min` from rendering the + // 8-level hierarchy, which would otherwise show up as tuning advice for the sink's own + // dataflow in `mz_expected_group_size_advice`. + // + // Transient exports (subscribes, peeks, metric sinks) and operators with no worker-0 + // mapping fall through to an `unattributable` sentinel, so their bytes still count without a + // churning `t` label growing series without bound. + source_sql: " +WITH ex AS ( + SELECT dataflow_id, min(export_id) AS export_id + FROM mz_introspection.mz_compute_exports + WHERE export_id NOT LIKE 't%' + GROUP BY dataflow_id OPTIONS (AGGREGATE INPUT GROUP SIZE = 1) +) +SELECT 'arrangement_size_bytes'::text AS metric_name, 'gauge'::text AS metric_type, + map_build(LIST[ROW('id', COALESCE(ex.export_id, 'unattributable'))])::map[text=>text] AS labels, + count(*)::double precision AS value, 'arrangement heap size in bytes'::text AS help +FROM mz_introspection.mz_arrangement_heap_size_raw r +LEFT JOIN mz_introspection.mz_dataflow_operator_dataflows dod ON r.operator_id = dod.id +LEFT JOIN ex ON ex.dataflow_id = dod.dataflow_id +GROUP BY COALESCE(ex.export_id, 'unattributable') +UNION ALL +SELECT 'arrangement_records'::text AS metric_name, 'gauge'::text AS metric_type, + map_build(LIST[ROW('id', COALESCE(ex.export_id, 'unattributable'))])::map[text=>text] AS labels, + count(*)::double precision AS value, 'number of records in arrangement heaps'::text AS help +FROM mz_introspection.mz_arrangement_records_raw r +LEFT JOIN mz_introspection.mz_dataflow_operator_dataflows dod ON r.operator_id = dod.id +LEFT JOIN ex ON ex.dataflow_id = dod.dataflow_id +GROUP BY COALESCE(ex.export_id, 'unattributable') +UNION ALL +SELECT 'arrangement_batches'::text AS metric_name, 'gauge'::text AS metric_type, + map_build(LIST[ROW('id', COALESCE(ex.export_id, 'unattributable'))])::map[text=>text] AS labels, + count(*)::double precision AS value, 'number of batches in arrangements'::text AS help +FROM mz_introspection.mz_arrangement_batches_raw r +LEFT JOIN mz_introspection.mz_dataflow_operator_dataflows dod ON r.operator_id = dod.id +LEFT JOIN ex ON ex.dataflow_id = dod.dataflow_id +GROUP BY COALESCE(ex.export_id, 'unattributable')", + }, + CuratedMetricSink { + name: "mz_metric_dataflow_errors", + prefix: METRIC_SINK_CURATED_PREFIX_MARKER, + // Raw log, not the `mz_compute_error_counts` view: the view joins the storage-managed + // `mz_internal.mz_compute_dependencies`, which `ensure_reads_only_logs` rejects. `count` is + // per-worker, so sum per export. `HAVING` drops the healthy ones. + // + // NOTE: direct errors only. The raw log attributes an error to the export that raised it. The + // view also forwards counts onto index-reuse exports, so a broken reuse-index reads 0 here and + // its errors show under the underlying export, undercounting against the view. + source_sql: " +SELECT 'dataflow_error_count'::text AS metric_name, 'gauge'::text AS metric_type, + map_build(LIST[ROW('id', export_id::text)])::map[text=>text] AS labels, + sum(count)::double precision AS value, 'count of errors in the dataflow'::text AS help +FROM mz_introspection.mz_compute_error_counts_raw +GROUP BY export_id +HAVING sum(count) > 0", + }, +]; /// A [`CuratedMetricSink`] installed on one replica. #[derive(Debug)] @@ -109,8 +179,6 @@ pub(super) struct PlannedMetricSink { impl Coordinator { /// Installs the curated metric sinks on all existing replicas. - /// - /// Meant to be invoked during coordinator bootstrapping. pub(super) async fn bootstrap_metric_sinks(&mut self) { for (cluster_id, replica_id) in self.all_cluster_replicas() { self.install_metric_sinks(cluster_id, replica_id).await; @@ -205,14 +273,12 @@ impl Coordinator { } // A user sink's prefix is validated at plan time; a curated one has no such gate, so enforce - // the same contract here. The prefix keeps published families in the reserved lane and - // supplies the leading character the row shaping's name validation needs. A failure is a bug - // in our own definition, hence `soft_panic_or_log!`. + // the same contract here. A failure is a bug in our own definition, hence + // `soft_panic_or_log!`. User-vs-curated collisions need no check: the curated prefix is + // reserved against user sinks in `validate_user_metric_sink_prefix`. // - // NOTE: This checks only the prefix format, not collisions. A curated prefix is not checked - // against user sinks (`ensure_metric_sink_prefix_is_free` cannot see a non-catalog item) nor - // against other curated definitions, yet both share the `mz_metric_sink_` lane and could - // overlap. Deferred until `CURATED` is populated. + // NOTE: curated definitions are not checked against each other; they stay disjoint by + // publishing distinct `metric_name`s under the shared curated prefix. if let Err(err) = validate_metric_sink_prefix(definition.prefix) { soft_panic_or_log!( "invalid curated metric sink prefix (name={}): {err}", @@ -379,11 +445,11 @@ impl Coordinator { let read_holds = self.acquire_read_holds(&id_bundle); df_desc.set_as_of(read_holds.least_valid_read()); - // Record the install now that the dataflow is about to ship, so a definition that fails to - // plan or optimize leaves no entry behind. `drop_metric_sinks` uses this entry to release the - // sink's instance-global collection state when the replica is dropped. Recording after the - // ship (an introspection subscribe records before sequencing) is safe: validity was rechecked - // at this stage and nothing awaits before the ship, so no replica drop can interleave. + // Record the install just before shipping. A failed plan or optimize returns earlier, so it + // leaves no entry behind. `drop_metric_sinks` reads this entry to release the sink's + // instance-global collection state on replica drop. Recording before the ship is safe because + // the coordinator runs one message at a time with no await between the two, so no replica drop + // sees an entry whose dataflow has not shipped. let install = InstalledMetricSink { cluster_id, sink_id, @@ -425,7 +491,7 @@ impl Coordinator { info!(%sink_id, %replica_id, name, "dropping metric sink"); self.metric_sinks.remove(&(replica_id, name)); - // The collection exists (the entry is recorded only after the dataflow ships), so this + // The entry exists only for a shipped dataflow, so its collection is present and this // drop succeeds. Result ignored: a failure during replica teardown is not worth a panic. let _ = self .controller @@ -450,9 +516,7 @@ fn metric_sinks_on_replica( } /// Enforces the introspection-only contract from [`CuratedMetricSink::source_sql`]: every relation -/// the definition reads, walking views transitively, must be a log collection. A storage-backed -/// read would put envd's write frontier on the sink's emission path, the coupling these sinks exist -/// to avoid. +/// the definition reads, walking views transitively, must be a log collection. /// /// Checked here against what the definition reads rather than by import kind after optimization: the /// import split (storage vs index) depends on which indexes the target cluster happens to have, so @@ -570,7 +634,10 @@ mod tests { use mz_cluster_client::ReplicaId; use mz_controller_types::ClusterId; use mz_repr::GlobalId; - use mz_sql::plan::validate_metric_sink_prefix; + use mz_sql::plan::{ + METRIC_SINK_CURATED_PREFIX_MARKER, validate_metric_sink_prefix, + validate_user_metric_sink_prefix, + }; use crate::catalog::Catalog; use crate::coord::metric_sink::{ @@ -620,9 +687,6 @@ mod tests { assert!(metric_sinks_on_replica(&sinks, r(5)).is_empty()); } - /// Every curated definition's prefix must satisfy the same contract a user's `PREFIX` does. - /// Unlike the user path, nothing validates a curated prefix at runtime before this guards it, - /// so a malformed one would escape the reserved lane or break the shaping's name validation. #[mz_ore::test] fn curated_prefixes_are_valid() { for definition in CURATED { @@ -635,6 +699,26 @@ mod tests { } } + #[mz_ore::test] + fn curated_prefixes_are_reserved_against_user_sinks() { + for definition in CURATED { + assert!( + definition + .prefix + .starts_with(METRIC_SINK_CURATED_PREFIX_MARKER), + "curated metric sink {:?} does not use the reserved curated prefix: {:?}", + definition.name, + definition.prefix + ); + assert!( + validate_user_metric_sink_prefix(definition.prefix).is_err(), + "a user could claim curated metric sink {:?}'s prefix {:?}", + definition.name, + definition.prefix + ); + } + } + /// The registry is keyed on the name, so a duplicate would make one definition's install /// unreachable to teardown and both collectors collide on the `sink` label. Guarded at runtime /// (`metric_sink_finish`) too, but caught here at build time before it can ship. @@ -650,23 +734,31 @@ mod tests { } } - /// Every curated definition must plan against the system catalog. A definition that does not - /// would soft-panic at boot, so catch it here as a failing test instead. `CURATED` is empty - /// today, so this iterates nothing until the first definition lands. + /// Runs the two checks `plan_metric_sink` does at boot, where a failure soft-panics (a hard panic + /// under debug assertions, so a CI boot crash-loop). The gate is the load-bearing one: a source + /// can plan cleanly yet still read a storage-backed relation transitively (a builtin view joining + /// in an introspection source), which only `ensure_reads_only_logs` catches. #[mz_ore::test(tokio::test)] #[cfg_attr(miri, ignore)] // unsupported operation: can't call foreign function `TLS_client_method` async fn curated_definitions_plan() { Catalog::with_debug(|catalog| async move { let session_catalog = catalog.for_system_session(); for definition in CURATED { - definition - .plan_source(&session_catalog) - .unwrap_or_else(|err| { - panic!( - "curated metric sink {:?} does not plan: {err}", - definition.name - ) - }); + let (_, _, dependencies) = + definition + .plan_source(&session_catalog) + .unwrap_or_else(|err| { + panic!( + "curated metric sink {:?} does not plan: {err}", + definition.name + ) + }); + ensure_reads_only_logs(&catalog, &dependencies).unwrap_or_else(|err| { + panic!( + "curated metric sink {:?} reads a non-introspection relation: {err}", + definition.name + ) + }); } }) .await diff --git a/src/sql/src/plan.rs b/src/sql/src/plan.rs index f883199ac6204..28dd3347b0732 100644 --- a/src/sql/src/plan.rs +++ b/src/sql/src/plan.rs @@ -116,9 +116,10 @@ pub use query::{ExprContext, QueryContext, QueryLifetime}; pub use scope::Scope; pub use side_effecting_func::SideEffectingFunc; pub use statement::ddl::{ - AlterSourceAddSubsourceOptionExtracted, MySqlConfigOptionExtracted, PgConfigOptionExtracted, - PlannedAlterRoleOption, PlannedRoleAttributes, PlannedRoleVariable, - SqlServerConfigOptionExtracted, validate_metric_sink_desc, validate_metric_sink_prefix, + AlterSourceAddSubsourceOptionExtracted, METRIC_SINK_CURATED_PREFIX_MARKER, + MySqlConfigOptionExtracted, PgConfigOptionExtracted, PlannedAlterRoleOption, + PlannedRoleAttributes, PlannedRoleVariable, SqlServerConfigOptionExtracted, + validate_metric_sink_desc, validate_metric_sink_prefix, validate_user_metric_sink_prefix, }; pub use statement::{ StatementClassification, StatementContext, StatementDesc, describe, plan, plan_copy_from, diff --git a/src/sql/src/plan/statement/ddl.rs b/src/sql/src/plan/statement/ddl.rs index 3c79d12cb003b..0279d5500d022 100644 --- a/src/sql/src/plan/statement/ddl.rs +++ b/src/sql/src/plan/statement/ddl.rs @@ -4262,16 +4262,14 @@ generate_extracted_config!(CreateMetricSinkOption, (Prefix, String)); /// rot as new platform collectors are added. const METRIC_SINK_PREFIX_MARKER: &str = "mz_metric_sink_"; -/// Rejects a prefix that could not start a Prometheus metric name, or that escapes the reserved -/// `mz_metric_sink_` lane (see `METRIC_SINK_PREFIX_MARKER`). -/// -/// The sink prepends this to every name it publishes, so `prefix + name` must stay a legal -/// family name (`[a-zA-Z_:][a-zA-Z0-9_:]*`, the same grammar the runtime checks each row's -/// `metric_name` against). The prefix must therefore be at least one character long. -/// -/// Enforced for a user's `CREATE METRIC SINK` at plan time, and for a coordinator-installed curated -/// sink at install time. Both paths depend on the guarantees this gives the row shaping: the -/// reserved leading character is what lets a bare `metric_name` start with a digit or be empty. +/// The prefix reserved for the curated sinks (`mz_adapter::coord::metric_sink::CURATED`); +/// [`validate_user_metric_sink_prefix`] keeps user prefixes clear of it. +pub const METRIC_SINK_CURATED_PREFIX_MARKER: &str = "mz_metric_sink_curated_"; + +/// Rejects a prefix that is not a legal start of a Prometheus metric name, or that escapes the +/// reserved `mz_metric_sink_` lane (`METRIC_SINK_PREFIX_MARKER`). The reserved leading character +/// is what lets a row's bare `metric_name` start with a digit or be empty. User sinks use +/// [`validate_user_metric_sink_prefix`], which adds the curated reservation. pub fn validate_metric_sink_prefix(prefix: &str) -> Result<(), PlanError> { if prefix.is_empty() { return Err(sql_err!("metric sink prefix must not be empty")); @@ -4299,6 +4297,28 @@ pub fn validate_metric_sink_prefix(prefix: &str) -> Result<(), PlanError> { Ok(()) } +/// [`validate_metric_sink_prefix`] plus the reservation that keeps user sinks clear of the curated +/// families ([`METRIC_SINK_CURATED_PREFIX_MARKER`]). +/// +/// The check runs both ways so it is total: a prefix that starts with the curated prefix is +/// rejected, and so is one the curated prefix starts with (a bare `mz_metric_sink_` plus a +/// `curated_...` row would otherwise publish a curated family, which Prometheus merges silently). +/// It is static rather than a catalog scan like `Coordinator::ensure_metric_sink_prefix_is_free`, +/// because curated sinks are not catalog items. +pub fn validate_user_metric_sink_prefix(prefix: &str) -> Result<(), PlanError> { + validate_metric_sink_prefix(prefix)?; + if prefix.starts_with(METRIC_SINK_CURATED_PREFIX_MARKER) + || METRIC_SINK_CURATED_PREFIX_MARKER.starts_with(prefix) + { + return Err(sql_err!( + "metric sink prefix {:?} overlaps {:?}, which is reserved for the built-in metric sinks", + prefix, + METRIC_SINK_CURATED_PREFIX_MARKER + )); + } + Ok(()) +} + /// Checks that `desc` exposes the canonical metric-sink columns, the contract /// `mz_adapter::optimize::metric_sink`'s row shaping and the compute-side operator both rely on. /// @@ -4348,7 +4368,7 @@ pub fn plan_create_metric_sink( let Some(prefix) = prefix else { sql_bail!("CREATE METRIC SINK requires a PREFIX option"); }; - validate_metric_sink_prefix(&prefix)?; + validate_user_metric_sink_prefix(&prefix)?; let name = scx.allocate_qualified_name(normalize::unresolved_item_name(name.clone())?)?; let full_name = scx.catalog.resolve_full_name(&name); let partial_name = PartialItemName::from(full_name.clone()); @@ -8594,3 +8614,28 @@ fn ensure_cluster_is_not_managed( Ok(()) } } + +#[cfg(test)] +mod tests { + use super::{ + METRIC_SINK_CURATED_PREFIX_MARKER, METRIC_SINK_PREFIX_MARKER, + validate_user_metric_sink_prefix, + }; + + #[mz_ore::test] + fn user_metric_sink_prefix_cannot_reach_the_curated_prefix() { + // Starts with the curated prefix. + assert!(validate_user_metric_sink_prefix(METRIC_SINK_CURATED_PREFIX_MARKER).is_err()); + assert!(validate_user_metric_sink_prefix("mz_metric_sink_curated_x").is_err()); + // A proper prefix of the curated prefix, which a `curated_...` row would complete. + assert!(validate_user_metric_sink_prefix(METRIC_SINK_PREFIX_MARKER).is_err()); + assert!(validate_user_metric_sink_prefix("mz_metric_sink_cur").is_err()); + // Still outside the reserved lane entirely. + assert!(validate_user_metric_sink_prefix("myapp_").is_err()); + // A prefix that diverges from the curated prefix is fine, including one that merely contains + // the marker further along. + assert!(validate_user_metric_sink_prefix("mz_metric_sink_myapp_").is_ok()); + assert!(validate_user_metric_sink_prefix("mz_metric_sink_curatex_").is_ok()); + assert!(validate_user_metric_sink_prefix("mz_metric_sink_my_curated_").is_ok()); + } +} diff --git a/test/cluster/mzcompose.py b/test/cluster/mzcompose.py index bc8c946fea053..7687d3931539e 100644 --- a/test/cluster/mzcompose.py +++ b/test/cluster/mzcompose.py @@ -360,19 +360,34 @@ def find_command_history_metrics(c: Composition) -> tuple[int, int, int, int]: replica_command_count, replica_dataflow_count, ) = find_command_history_metrics(c) + + # Curated metric sinks install one dataflow per definition on every replica, + # so they add to the aggregate history dataflow counts above. That metric + # carries no per-dataflow label to filter on, so subtract the live count of + # curated sink dataflows rather than hardcoding it, keeping the bounds + # correct as the CURATED set grows. Read the count off cluster1's own + # replica, the one whose history the metrics above describe. + with c.sql_cursor() as cursor: + cursor.execute(b"SET cluster = cluster1") + cursor.execute( + b"SELECT count(*) FROM mz_introspection.mz_dataflows" + b" WHERE name LIKE '%metric-sink-%'" + ) + metric_sink_dataflows = int(cursor.fetchall()[0][0]) + assert controller_command_count > 0, "controller history cannot be empty" assert ( controller_dataflow_count > 0 ), "at least one dataflow expected in controller history" assert ( - controller_dataflow_count < 6 + controller_dataflow_count - metric_sink_dataflows < 6 ), "more dataflows than expected in controller history" assert replica_command_count > 0, "replica history cannot be empty" assert ( replica_dataflow_count > 0 ), "at least one dataflow expected in replica history" assert ( - replica_dataflow_count < 6 + replica_dataflow_count - metric_sink_dataflows < 6 ), "more dataflows than expected in replica history" # execute 400 fast- and slow-path peeks @@ -418,7 +433,7 @@ def find_command_history_metrics(c: Composition) -> tuple[int, int, int, int]: controller_dataflow_count > 0 ), f"at least one dataflow expected in controller history, got {controller_dataflow_count}" assert ( - controller_dataflow_count < 6 + controller_dataflow_count - metric_sink_dataflows < 6 ), f"more dataflows than expected in controller history, got {controller_dataflow_count}" assert ( replica_command_count < 100 @@ -427,7 +442,7 @@ def find_command_history_metrics(c: Composition) -> tuple[int, int, int, int]: replica_dataflow_count > 0 ), f"at least one dataflow expected in replica history, got {replica_dataflow_count}" assert ( - replica_dataflow_count < 6 + replica_dataflow_count - metric_sink_dataflows < 6 ), f"more dataflows than expected in replica history, got {replica_dataflow_count}" diff --git a/test/sqllogictest/metric_sink.slt b/test/sqllogictest/metric_sink.slt index 3af6be76fb063..444a9062f55f9 100644 --- a/test/sqllogictest/metric_sink.slt +++ b/test/sqllogictest/metric_sink.slt @@ -125,6 +125,38 @@ CREATE METRIC SINK ms_bad_prefix FROM shaped WITH (PREFIX = '') statement error metric sink prefix "app_" must start with "mz_metric_sink_" CREATE METRIC SINK ms_unmarked FROM shaped WITH (PREFIX = 'app_') +# `mz_metric_sink_curated_` is reserved for the built-in sinks. The reservation is +# bidirectional: a user prefix may neither start with it nor be one it starts with, +# since the published name appends an arbitrary `metric_name`. `mz_metric_sink_` plus +# a row named `curated_arrangement_size_bytes` is exactly the second case. + +statement error metric sink prefix "mz_metric_sink_curated_" overlaps "mz_metric_sink_curated_", which is reserved for the built-in metric sinks +CREATE METRIC SINK ms_curated FROM shaped WITH (PREFIX = 'mz_metric_sink_curated_') + +statement error metric sink prefix "mz_metric_sink_curated_mine_" overlaps "mz_metric_sink_curated_", which is reserved for the built-in metric sinks +CREATE METRIC SINK ms_curated_mine FROM shaped WITH (PREFIX = 'mz_metric_sink_curated_mine_') + +statement error metric sink prefix "mz_metric_sink_" overlaps "mz_metric_sink_curated_", which is reserved for the built-in metric sinks +CREATE METRIC SINK ms_bare FROM shaped WITH (PREFIX = 'mz_metric_sink_') + +statement error metric sink prefix "mz_metric_sink_cur" overlaps "mz_metric_sink_curated_", which is reserved for the built-in metric sinks +CREATE METRIC SINK ms_cur FROM shaped WITH (PREFIX = 'mz_metric_sink_cur') + +# A prefix that diverges from the curated prefix is fine, and so is one that merely +# contains the marker further along. + +statement ok +CREATE METRIC SINK ms_curatex FROM shaped WITH (PREFIX = 'mz_metric_sink_curatex_') + +statement ok +DROP METRIC SINK ms_curatex + +statement ok +CREATE METRIC SINK ms_my_curated FROM shaped WITH (PREFIX = 'mz_metric_sink_my_curated_') + +statement ok +DROP METRIC SINK ms_my_curated + # Prefixes on one cluster have to be prefix-free, not merely distinct: `app_` # plus a row named `svc_x` publishes exactly what `app_svc_` plus a row named # `x` does, and the Prometheus registry merges same-named families silently. diff --git a/test/testdrive/dataflow-cleanup.td b/test/testdrive/dataflow-cleanup.td index ebf43e7120185..9f711177975d2 100644 --- a/test/testdrive/dataflow-cleanup.td +++ b/test/testdrive/dataflow-cleanup.td @@ -19,9 +19,12 @@ # Filter them out so test assertions only see compute operators. $ set storage-id-offset=281474976710656 -# Introspection subscribes add noise to the introspection sources, so disable them. +# Introspection subscribes and curated metric sinks both install dataflows on +# every replica, adding noise to the introspection sources, so disable them +# before creating the cluster below. $ postgres-execute connection=postgres://mz_system:materialize@${testdrive.materialize-internal-sql-addr} ALTER SYSTEM SET enable_introspection_subscribes = false +ALTER SYSTEM SET enable_metric_sink = false # Create a clean replica to inspect dataflow state on. diff --git a/test/testdrive/divergent-dataflow-cancellation.td b/test/testdrive/divergent-dataflow-cancellation.td index bebf2838233d5..36a1065bf8943 100644 --- a/test/testdrive/divergent-dataflow-cancellation.td +++ b/test/testdrive/divergent-dataflow-cancellation.td @@ -22,9 +22,50 @@ $ set storage-id-offset=281474976710656 $ postgres-execute connection=postgres://mz_system:materialize@${testdrive.materialize-internal-sql-addr} ALTER SYSTEM SET enable_introspection_subscribes = false +# Curated metric sinks install one dataflow per definition on every replica. CI +# runs with the feature on, so the assertions at the end filter them out instead +# of disabling it and leaving the shipping path untested. The default cluster's +# replica has run the same set since boot, so read the count from there rather +# than hardcoding it. +> SELECT count(*) > 0 FROM mz_introspection.mz_dataflows + WHERE name LIKE '%metric-sink-%' +true + +$ set-from-sql var=metric-sink-dataflows +SELECT count(*)::text FROM mz_introspection.mz_dataflows WHERE name LIKE '%metric-sink-%' + > CREATE CLUSTER test SIZE 'scale=1,workers=1'; > SET cluster = test; +# Sink install is async; wait for all of them before capturing the baselines +# below, or one landing later looks like a cancelled dataflow that never cleaned up. +> SELECT count(*) = ${metric-sink-dataflows} FROM mz_introspection.mz_dataflows + WHERE name LIKE '%metric-sink-%' +true + +# Capture the sinks' footprint now, while they are the only dataflows here, to +# subtract from the expect-empty assertions at the end. Export ids have no useful +# order, so list them. +$ set-from-sql var=baseline-exports +SELECT string_agg(DISTINCT '''' || export_id || '''', ',') +FROM mz_introspection.mz_compute_exports_per_worker +WHERE export_id NOT LIKE 'si%' + +# Operator and channel ids share one monotonic Timely counter, so a single +# high-water mark separates the sinks from what this test installs next; channels +# show up in mz_dataflow_addresses_per_worker too, so it covers both. Exclude +# storage ids, or the mark exceeds STORAGE_ID_OFFSET and the assertions go vacuous. +# +# NOTE: qualify the ORDER BY column so it sorts the numeric id, not the ::text +# projection (which ranks 999 above 1000). Keep the query fast-path, or it +# allocates ids above the mark it reports. +$ set-from-sql var=baseline-max-id +SELECT id::text AS max_id +FROM mz_introspection.mz_dataflow_addresses_per_worker +WHERE id < ${storage-id-offset} +ORDER BY mz_dataflow_addresses_per_worker.id DESC +LIMIT 1 + > CREATE VIEW divergent AS WITH MUTUALLY RECURSIVE flip(x int) AS (VALUES(1) EXCEPT ALL SELECT * FROM flip) @@ -42,6 +83,13 @@ ALTER SYSTEM SET enable_introspection_subscribes = false WHERE name LIKE '%divergent%' 2 +# The dataflows just installed have to sit above the mark, or the assertions at +# the end filter away everything they are meant to catch. +> SELECT count(*) > 0 + FROM mz_introspection.mz_dataflow_operators_per_worker + WHERE id > ${baseline-max-id} AND id < ${storage-id-offset} +true + # Drop the installed dataflows > DROP INDEX divergent_index @@ -71,33 +119,36 @@ contains: canceling statement due to statement timeout > COMMIT -# Confirm that all dataflows are now cancelled. +# Confirm that all dataflows are now cancelled. The curated metric sinks are +# still running, so each query excludes their baseline rather than expecting a +# bare empty replica: an id above the high-water mark, or an export outside the +# captured list, belongs to a dataflow this test installed and cancelled. # # NOTE: It is important that the below queries are all fast-path queries. # Otherwise they'd introduce new dataflows, confusing the results. # For `_raw` relations, add a limit to protect against huge results. -> SELECT * FROM mz_introspection.mz_arrangement_batches_raw LIMIT 10 -> SELECT * FROM mz_introspection.mz_arrangement_records_raw LIMIT 10 -> SELECT * FROM mz_introspection.mz_arrangement_sharing_raw LIMIT 10 -> SELECT * FROM mz_introspection.mz_compute_error_counts_raw LIMIT 10 +> SELECT * FROM mz_introspection.mz_arrangement_batches_raw WHERE operator_id > ${baseline-max-id} LIMIT 10 +> SELECT * FROM mz_introspection.mz_arrangement_records_raw WHERE operator_id > ${baseline-max-id} LIMIT 10 +> SELECT * FROM mz_introspection.mz_arrangement_sharing_raw WHERE operator_id > ${baseline-max-id} LIMIT 10 +> SELECT * FROM mz_introspection.mz_compute_error_counts_raw WHERE export_id NOT IN (${baseline-exports}) LIMIT 10 > SELECT * FROM mz_introspection.mz_compute_exports_per_worker - WHERE export_id NOT LIKE 'si%' + WHERE export_id NOT LIKE 'si%' AND export_id NOT IN (${baseline-exports}) > SELECT * FROM mz_introspection.mz_compute_frontiers_per_worker - WHERE export_id NOT LIKE 'si%' + WHERE export_id NOT LIKE 'si%' AND export_id NOT IN (${baseline-exports}) > SELECT * FROM mz_introspection.mz_compute_import_frontiers_per_worker - WHERE export_id NOT LIKE 'si%' -> SELECT * FROM mz_introspection.mz_compute_operator_durations_histogram_raw WHERE id < ${storage-id-offset} LIMIT 10 -> SELECT id, worker_id, address::text FROM mz_introspection.mz_dataflow_addresses_per_worker WHERE id < ${storage-id-offset} -> SELECT * FROM mz_introspection.mz_dataflow_channels_per_worker WHERE id < ${storage-id-offset} -> SELECT * FROM mz_introspection.mz_dataflow_operator_reachability_raw WHERE id < ${storage-id-offset} LIMIT 10 -> SELECT * FROM mz_introspection.mz_dataflow_operators_per_worker WHERE id < ${storage-id-offset} -> SELECT * FROM mz_introspection.mz_message_counts_received_raw WHERE channel_id < ${storage-id-offset} LIMIT 10 -> SELECT * FROM mz_introspection.mz_message_counts_sent_raw WHERE channel_id < ${storage-id-offset} LIMIT 10 -> SELECT * FROM mz_introspection.mz_scheduling_elapsed_raw WHERE id < ${storage-id-offset} LIMIT 10 + WHERE export_id NOT LIKE 'si%' AND export_id NOT IN (${baseline-exports}) +> SELECT * FROM mz_introspection.mz_compute_operator_durations_histogram_raw WHERE id > ${baseline-max-id} AND id < ${storage-id-offset} LIMIT 10 +> SELECT id, worker_id, address::text FROM mz_introspection.mz_dataflow_addresses_per_worker WHERE id > ${baseline-max-id} AND id < ${storage-id-offset} +> SELECT * FROM mz_introspection.mz_dataflow_channels_per_worker WHERE id > ${baseline-max-id} AND id < ${storage-id-offset} +> SELECT * FROM mz_introspection.mz_dataflow_operator_reachability_raw WHERE id > ${baseline-max-id} AND id < ${storage-id-offset} LIMIT 10 +> SELECT * FROM mz_introspection.mz_dataflow_operators_per_worker WHERE id > ${baseline-max-id} AND id < ${storage-id-offset} +> SELECT * FROM mz_introspection.mz_message_counts_received_raw WHERE channel_id > ${baseline-max-id} AND channel_id < ${storage-id-offset} LIMIT 10 +> SELECT * FROM mz_introspection.mz_message_counts_sent_raw WHERE channel_id > ${baseline-max-id} AND channel_id < ${storage-id-offset} LIMIT 10 +> SELECT * FROM mz_introspection.mz_scheduling_elapsed_raw WHERE id > ${baseline-max-id} AND id < ${storage-id-offset} LIMIT 10 # Cleanup. > DROP CLUSTER test CASCADE diff --git a/test/testdrive/expected_group_size_tuning.td b/test/testdrive/expected_group_size_tuning.td index f77db50889fc8..983d55c0ed96e 100644 --- a/test/testdrive/expected_group_size_tuning.td +++ b/test/testdrive/expected_group_size_tuning.td @@ -115,6 +115,15 @@ $ set-sql-timeout duration=240s "Dataflow: materialize.public.lineitem_by_suppkey" ReduceHierarchical 8 5 4095 "Dataflow: materialize.public.lineitem_by_suppkey" TopK 8 5 4095 +# The curated metric sinks run on this replica too, and this view has no +# internal-dataflow filter. Their `min` aggregate carries a group-size hint, so it +# renders one reduce stage and offers nothing to cut. Without the hint each sink +# dataflow would add a row here, in a view users are pointed at for tuning advice. +> SELECT count(*) + FROM mz_introspection.mz_expected_group_size_advice + WHERE dataflow_name LIKE '%metric-sink-%'; +0 + # Validate the reported memory savings for the entries above. The `savings` column # sums arrangement heap sizes (mz_arrangement_sizes.size), which are logged # asynchronously and best-effort, lagging the structural columns above and sometimes diff --git a/test/testdrive/github-6335.td b/test/testdrive/github-6335.td index 404f0a63f216d..447c7635aab11 100644 --- a/test/testdrive/github-6335.td +++ b/test/testdrive/github-6335.td @@ -51,5 +51,6 @@ $ set-sql-timeout duration=360s JOIN mz_introspection.mz_dataflows d ON d.id = e.dataflow_id WHERE e.export_id LIKE 't%' AND d.name NOT LIKE '%introspection-subscribe%' AND - d.name NOT LIKE '%oneshot-select%' + d.name NOT LIKE '%oneshot-select%' AND + d.name NOT LIKE '%metric-sink%' 0 diff --git a/test/testdrive/metric-sink.td b/test/testdrive/metric-sink.td index 06498ea31a482..5cf9fecf9bb95 100644 --- a/test/testdrive/metric-sink.td +++ b/test/testdrive/metric-sink.td @@ -95,12 +95,17 @@ true 2 # Read the sink's id off the companion gauge it stamps with a `sink` label. -# `LIMIT 1` guards the single-row contract of `set-from-sql`: today `s` is the -# only metric sink, but a second one (user or builtin) sharing this scrape -# window would otherwise make the capture fail. +# The curated metric sinks (installed on every replica) stamp this gauge with +# their names too, so exclude those to isolate `s`, the only user sink here. +# `LIMIT 1` guards the single-row contract of `set-from-sql`. +# +# NOTE: `CURATED` in src/adapter/src/coord/metric_sink.rs is the source of truth +# for these names. The same exclusion list is repeated later in this file and in +# checks/all_checks/metric_sink.py, so a new curated sink means updating them all. $ set-from-sql var=sink-id SELECT labels -> 'sink' FROM mz_introspection.mz_cluster_prometheus_metrics WHERE metric_name = 'mz_compute_metric_sink_frontier_ms' + AND labels -> 'sink' NOT IN ('mz_metric_arrangement_sizes', 'mz_metric_dataflow_errors') LIMIT 1 # The companion gauges exist, are labeled with this sink's id, and read healthy @@ -216,10 +221,11 @@ contains:still depended upon by metric sink "s" > CREATE METRIC SINK s_skip IN CLUSTER quickstart FROM lv WITH (PREFIX = 'mz_metric_sink_skip_') # Both rows are skipped and stay distinct (a null value is not the same identity as an empty one), -# so the gauge reads 2, not 1. This retrying read also waits for the scrape to observe the sink; -# `s_skip` is the only metric sink here, so no `sink`-label filter is needed. +# so the gauge reads 2, not 1. This retrying read also waits for the scrape to observe the sink. +# `s_skip` is the only user sink here, but the curated sinks stamp this gauge too, so exclude them. > SELECT value FROM mz_introspection.mz_cluster_prometheus_metrics WHERE metric_name = 'mz_compute_metric_sink_skipped' + AND labels -> 'sink' NOT IN ('mz_metric_arrangement_sizes', 'mz_metric_dataflow_errors') 2 # Neither row publishes a series under the sink's metric name. @@ -229,12 +235,153 @@ contains:still depended upon by metric sink "s" > DROP TABLE lt CASCADE +# +# Curated metric sinks. The coordinator installs these on every replica (not via +# CREATE METRIC SINK), so with `enable_metric_sink` on they run on quickstart's r1 +# from boot. Reads target r1 (cluster_replica, set at the top of the file). +# + +# An index gives the sizes sink a dataflow arrangement to measure. +> CREATE TABLE arr_t (k text NOT NULL, v int) + +> INSERT INTO arr_t VALUES ('a', 1), ('b', 2) + +> CREATE INDEX arr_idx ON arr_t (k) + +# Each size family publishes one gauge series per dataflow, tagged with a +# representative exported object's catalog id, or `unattributable` for a dataflow +# that exports no catalog object. Values churn, so assert shape rather than magnitude. +> SELECT count(*) > 0 FROM mz_introspection.mz_cluster_prometheus_metrics + WHERE metric_name = 'mz_metric_sink_curated_arrangement_size_bytes' + AND metric_type = 'gauge' AND labels -> 'id' IS NOT NULL +true + +> SELECT count(*) > 0 FROM mz_introspection.mz_cluster_prometheus_metrics + WHERE metric_name = 'mz_metric_sink_curated_arrangement_records' + AND metric_type = 'gauge' AND labels -> 'id' IS NOT NULL +true + +> SELECT count(*) > 0 FROM mz_introspection.mz_cluster_prometheus_metrics + WHERE metric_name = 'mz_metric_sink_curated_arrangement_batches' + AND metric_type = 'gauge' AND labels -> 'id' IS NOT NULL +true + +# The attributable path, on the index's own catalog id rather than any `u%` label: +# an index is a dataflow export, so its arrangement bytes carry its id. +$ set-from-sql var=arr-idx-id +SELECT id FROM mz_catalog.mz_indexes WHERE name = 'arr_idx' + +> SELECT count(*) > 0 FROM mz_introspection.mz_cluster_prometheus_metrics + WHERE metric_name = 'mz_metric_sink_curated_arrangement_size_bytes' + AND labels -> 'id' = '${arr-idx-id}' +true + +# A materialized view is only an export of its dataflow: what the dataflow *builds* is a +# transient internal view, so an id read off the build list would label these bytes with a +# `t` that resolves to no catalog object. The reduce gives the view an arrangement to +# measure. +> CREATE MATERIALIZED VIEW arr_mv AS SELECT k, count(*) AS n FROM arr_t GROUP BY k + +$ set-from-sql var=arr-mv-id +SELECT id FROM mz_catalog.mz_materialized_views WHERE name = 'arr_mv' + +> SELECT count(*) > 0 FROM mz_introspection.mz_cluster_prometheus_metrics + WHERE metric_name = 'mz_metric_sink_curated_arrangement_size_bytes' + AND labels -> 'id' = '${arr-mv-id}' +true + +# The sentinel path: the replica's own introspection subscribes and metric sinks export +# only transient ids, which are folded into the sentinel rather than labelled, so their +# arrangements still count. These run on every replica, so the bucket is always present. +> SELECT count(*) > 0 FROM mz_introspection.mz_cluster_prometheus_metrics + WHERE metric_name = 'mz_metric_sink_curated_arrangement_size_bytes' + AND labels -> 'id' = 'unattributable' +true + +# No series carries a transient id. Those are re-minted on every re-render, so labelling +# them would grow the sink's series set without bound. +> SELECT count(*) FROM mz_introspection.mz_cluster_prometheus_metrics + WHERE metric_name = 'mz_metric_sink_curated_arrangement_size_bytes' + AND labels -> 'id' LIKE 't%' +0 + +# Series count equals the sink's own operator -> dataflow -> export id mapping over the +# raw heap-size log, the export id when the dataflow exports a catalog object and the +# sentinel when it does not. Also confirms the per-worker rows collapsed. +> SELECT + (SELECT count(*) FROM mz_introspection.mz_cluster_prometheus_metrics + WHERE metric_name = 'mz_metric_sink_curated_arrangement_size_bytes') + = + (SELECT count(DISTINCT COALESCE(ex.export_id, 'unattributable')) + FROM mz_introspection.mz_arrangement_heap_size_raw r + LEFT JOIN mz_introspection.mz_dataflow_operator_dataflows dod ON r.operator_id = dod.id + LEFT JOIN (SELECT dataflow_id, min(export_id) AS export_id + FROM mz_introspection.mz_compute_exports + WHERE export_id NOT LIKE 't%' GROUP BY dataflow_id) ex + ON ex.dataflow_id = dod.dataflow_id) +true + +# Both curated sinks report healthy companion gauges: frontier advancing, no errors. +> SELECT value > 0 FROM mz_introspection.mz_cluster_prometheus_metrics + WHERE metric_name = 'mz_compute_metric_sink_frontier_ms' + AND labels -> 'sink' = 'mz_metric_arrangement_sizes' +true + +> SELECT value > 0 FROM mz_introspection.mz_cluster_prometheus_metrics + WHERE metric_name = 'mz_compute_metric_sink_frontier_ms' + AND labels -> 'sink' = 'mz_metric_dataflow_errors' +true + +> SELECT value = 0 FROM mz_introspection.mz_cluster_prometheus_metrics + WHERE metric_name = 'mz_compute_metric_sink_errors' + AND labels -> 'sink' = 'mz_metric_arrangement_sizes' +true + +> SELECT value = 0 FROM mz_introspection.mz_cluster_prometheus_metrics + WHERE metric_name = 'mz_compute_metric_sink_errors' + AND labels -> 'sink' = 'mz_metric_dataflow_errors' +true + +# The errors sink's HAVING drops zero-error exports, so a healthy cluster +# publishes no error series. +> SELECT count(*) FROM mz_introspection.mz_cluster_prometheus_metrics + WHERE metric_name = 'mz_metric_sink_curated_dataflow_error_count' +0 + +# A divide-by-zero materialized view errors in its dataflow, so the raw error +# log gains a row and the errors sink publishes a series for it. +> CREATE TABLE err_t (x int) + +> INSERT INTO err_t VALUES (0) + +> CREATE MATERIALIZED VIEW err_mv IN CLUSTER quickstart AS SELECT 1 / x AS v FROM err_t + +> SELECT count(*) > 0 FROM mz_introspection.mz_cluster_prometheus_metrics + WHERE metric_name = 'mz_metric_sink_curated_dataflow_error_count' + AND metric_type = 'gauge' AND labels -> 'id' IS NOT NULL +true + +# Once the erroring dataflow is gone the error series is retracted. +> DROP MATERIALIZED VIEW err_mv + +> SELECT count(*) FROM mz_introspection.mz_cluster_prometheus_metrics + WHERE metric_name = 'mz_metric_sink_curated_dataflow_error_count' +0 + +> DROP INDEX arr_idx + +> DROP MATERIALIZED VIEW arr_mv + +> DROP TABLE arr_t + +> DROP TABLE err_t + # Replica churn drives the coordinator's curated metric-sink install and -# teardown (`install_metric_sinks` / `drop_metric_sinks`). The curated list is -# empty, so there is no curated series to assert on: what is under test is that -# both hooks run over an empty list without erroring, and that a replica added -# and removed under a live sink leaves that sink publishing on the replica that -# remains. +# teardown (`install_metric_sinks` / `drop_metric_sinks`): the curated sinks +# install on churn_c's added replica and are torn down when it is dropped. The +# curated series themselves are asserted on quickstart above; here the focus is +# that a user sink added and removed under replica churn keeps publishing on the +# replica that remains, while the curated install/teardown runs on each change. # # On its own cluster, so the churn does not disturb the replica serving the rest # of this file. `quickstart` is a managed cluster anyway, which rejects @@ -286,3 +433,130 @@ contains:still depended upon by metric sink "s" > DROP CLUSTER churn_c CASCADE > DROP TABLE churn_t CASCADE + +# +# Curated sinks across a multi-cluster, multi-replica topology. The coordinator +# installs the curated set on every replica of every cluster, so a regression that +# skipped a replica, or only worked on quickstart, leaves one of the four replicas +# below with no curated series. Each read targets one replica by name, so a missing +# install shows as a mismatch rather than being masked by another replica's series. +# +> CREATE CLUSTER ma_c (SIZE '${arg.default-replica-size}', REPLICATION FACTOR 2) + +> CREATE CLUSTER mb_c (SIZE '${arg.default-replica-size}', REPLICATION FACTOR 2) + +> CREATE TABLE mrep_t (x int) + +> INSERT INTO mrep_t VALUES (0), (1), (2) + +# An arrangement (index) and an erroring dataflow (divide by zero) on each cluster. +> CREATE INDEX mrep_ix_a IN CLUSTER ma_c ON mrep_t (x) + +> CREATE MATERIALIZED VIEW mrep_mv_a IN CLUSTER ma_c AS SELECT 1 / x AS v FROM mrep_t + +> CREATE INDEX mrep_ix_b IN CLUSTER mb_c ON mrep_t (x) + +> CREATE MATERIALIZED VIEW mrep_mv_b IN CLUSTER mb_c AS SELECT 1 / x AS v FROM mrep_t + +# Every replica of both clusters publishes the size family, the error family (the +# erroring MV), and both curated sinks' health frontier gauges. +> SET cluster = ma_c + +> SET cluster_replica = r1 + +> SELECT count(*) > 0 FROM mz_introspection.mz_cluster_prometheus_metrics + WHERE metric_name = 'mz_metric_sink_curated_arrangement_size_bytes' AND labels -> 'id' IS NOT NULL +true + +> SELECT count(*) > 0 FROM mz_introspection.mz_cluster_prometheus_metrics + WHERE metric_name = 'mz_metric_sink_curated_dataflow_error_count' AND labels -> 'id' IS NOT NULL +true + +> SELECT value > 0 FROM mz_introspection.mz_cluster_prometheus_metrics + WHERE metric_name = 'mz_compute_metric_sink_frontier_ms' AND labels -> 'sink' = 'mz_metric_arrangement_sizes' +true + +> SELECT value > 0 FROM mz_introspection.mz_cluster_prometheus_metrics + WHERE metric_name = 'mz_compute_metric_sink_frontier_ms' AND labels -> 'sink' = 'mz_metric_dataflow_errors' +true + +# One series per dataflow, not per worker: holds for any worker count, and under +# a multi-worker config it is the assertion that the per-worker raw log collapsed. +> SELECT + (SELECT count(*) FROM mz_introspection.mz_cluster_prometheus_metrics + WHERE metric_name = 'mz_metric_sink_curated_arrangement_size_bytes') + = + (SELECT count(DISTINCT COALESCE(ex.export_id, 'unattributable')) + FROM mz_introspection.mz_arrangement_heap_size_raw r + LEFT JOIN mz_introspection.mz_dataflow_operator_dataflows dod ON r.operator_id = dod.id + LEFT JOIN (SELECT dataflow_id, min(export_id) AS export_id + FROM mz_introspection.mz_compute_exports + WHERE export_id NOT LIKE 't%' GROUP BY dataflow_id) ex + ON ex.dataflow_id = dod.dataflow_id) +true + +> SET cluster_replica = r2 + +> SELECT count(*) > 0 FROM mz_introspection.mz_cluster_prometheus_metrics + WHERE metric_name = 'mz_metric_sink_curated_arrangement_size_bytes' AND labels -> 'id' IS NOT NULL +true + +> SELECT count(*) > 0 FROM mz_introspection.mz_cluster_prometheus_metrics + WHERE metric_name = 'mz_metric_sink_curated_dataflow_error_count' AND labels -> 'id' IS NOT NULL +true + +> SELECT value > 0 FROM mz_introspection.mz_cluster_prometheus_metrics + WHERE metric_name = 'mz_compute_metric_sink_frontier_ms' AND labels -> 'sink' = 'mz_metric_arrangement_sizes' +true + +> SELECT value > 0 FROM mz_introspection.mz_cluster_prometheus_metrics + WHERE metric_name = 'mz_compute_metric_sink_frontier_ms' AND labels -> 'sink' = 'mz_metric_dataflow_errors' +true + +> SET cluster = mb_c + +> SET cluster_replica = r1 + +> SELECT count(*) > 0 FROM mz_introspection.mz_cluster_prometheus_metrics + WHERE metric_name = 'mz_metric_sink_curated_arrangement_size_bytes' AND labels -> 'id' IS NOT NULL +true + +> SELECT count(*) > 0 FROM mz_introspection.mz_cluster_prometheus_metrics + WHERE metric_name = 'mz_metric_sink_curated_dataflow_error_count' AND labels -> 'id' IS NOT NULL +true + +> SELECT value > 0 FROM mz_introspection.mz_cluster_prometheus_metrics + WHERE metric_name = 'mz_compute_metric_sink_frontier_ms' AND labels -> 'sink' = 'mz_metric_arrangement_sizes' +true + +> SELECT value > 0 FROM mz_introspection.mz_cluster_prometheus_metrics + WHERE metric_name = 'mz_compute_metric_sink_frontier_ms' AND labels -> 'sink' = 'mz_metric_dataflow_errors' +true + +> SET cluster_replica = r2 + +> SELECT count(*) > 0 FROM mz_introspection.mz_cluster_prometheus_metrics + WHERE metric_name = 'mz_metric_sink_curated_arrangement_size_bytes' AND labels -> 'id' IS NOT NULL +true + +> SELECT count(*) > 0 FROM mz_introspection.mz_cluster_prometheus_metrics + WHERE metric_name = 'mz_metric_sink_curated_dataflow_error_count' AND labels -> 'id' IS NOT NULL +true + +> SELECT value > 0 FROM mz_introspection.mz_cluster_prometheus_metrics + WHERE metric_name = 'mz_compute_metric_sink_frontier_ms' AND labels -> 'sink' = 'mz_metric_arrangement_sizes' +true + +> SELECT value > 0 FROM mz_introspection.mz_cluster_prometheus_metrics + WHERE metric_name = 'mz_compute_metric_sink_frontier_ms' AND labels -> 'sink' = 'mz_metric_dataflow_errors' +true + +> RESET cluster_replica + +> RESET cluster + +> DROP CLUSTER ma_c CASCADE + +> DROP CLUSTER mb_c CASCADE + +> DROP TABLE mrep_t CASCADE From 1799791cb968aacfec924a8cf6e23504a185d0b8 Mon Sep 17 00:00:00 2001 From: Sang Jun Bak Date: Thu, 10 Sep 2026 15:56:08 -0400 Subject: [PATCH 04/11] sql-499: print ids for types in DOC ON positions (#38612) **Context:** DOC ON TYPE x and DOC ON COLUMN x.c resolve a name that may denote a type or a relation, and both spellings persist in the sink's create_sql. A relation in these positions prints as [id AS name], but resolution suppressed ids for types, so a type persisted as a bare qualified name. A bare name hides the sink's dependency on the type from create_sql reference extraction: mz_object_dependencies files the bare name under named_relations, whose join excludes Type rows, so the edge silently drops out of the view even though the in-memory dependency graph (which blocks DROP TYPE) still records it. The id form would also keep the reference valid if renames of types were ever added; today no rename strands a bare name, since ALTER TYPE has no RENAME and ALTER SCHEMA RENAME rewrites bare names too. Force ids in the two places that construct DOC ON references: the name resolver's DOC ON folds, and sink purification, which injects DOC ON options for every commented item the sink references. An AST migration rewrites existing catalogs to match. The migration binds bare names to types only, since a type may share (schema, name) with a secret, connection, sink, or function, and resolves builtin types through the system object mappings. ### Motivation Part of sql-499 ### Verification Added platform check to test the upgrade and modified a td to assert it. The platform check creates a secret with the type's name first, so the migration must bind the type rather than the older same-named item. --- .../checks/all_checks/kafka_sink.py | 78 +++++++++ src/adapter/src/catalog/migrate.rs | 162 ++++++++++++++++++ src/sql-parser/src/ast/item_refs.rs | 19 ++ src/sql/src/names.rs | 47 ++--- src/sql/src/pure.rs | 5 +- .../kafka-avro-sinks-doc-comments.td | 15 ++ 6 files changed, 301 insertions(+), 25 deletions(-) diff --git a/misc/python/materialize/checks/all_checks/kafka_sink.py b/misc/python/materialize/checks/all_checks/kafka_sink.py index dc833c7590355..a7b5a4a409ac1 100644 --- a/misc/python/materialize/checks/all_checks/kafka_sink.py +++ b/misc/python/materialize/checks/all_checks/kafka_sink.py @@ -791,6 +791,84 @@ def validate(self) -> Testdrive: """)) +class KafkaSinkCommentsOnType(Check): + """An Avro sink over a relation with a commented custom-type column + records a dependency edge on the type. Purification injects a + `DOC ON TYPE` reference for every commented type the sinked relation + uses, and the persisted reference must keep resolving across restarts + and upgrades and stay visible to `mz_object_dependencies`.""" + + def initialize(self) -> Testdrive: + return Testdrive(dedent(""" + # A secret sharing the type's name, created first: + # the DOC ON migration must bind the type, not + # whichever same-named item is older. + > CREATE SECRET sink_comments_point AS 'x' + > CREATE TYPE sink_comments_point AS (x integer, y integer) + > COMMENT ON TYPE sink_comments_point IS 'comment on type sink_comments_point' + > CREATE TABLE sink_comments_type_tbl (a int) + > INSERT INTO sink_comments_type_tbl VALUES (1) + > CREATE MATERIALIZED VIEW sink_comments_type_view AS + SELECT ROW(a, a)::sink_comments_point AS p, a FROM sink_comments_type_tbl + + > CREATE SINK sink_comments_type1 FROM sink_comments_type_view + INTO KAFKA CONNECTION kafka_conn (TOPIC 'sink-comments-type1') + KEY (a) NOT ENFORCED + FORMAT AVRO USING CONFLUENT SCHEMA REGISTRY CONNECTION csr_conn + ENVELOPE UPSERT + """)) + + def manipulate(self) -> list[Testdrive]: + return [ + Testdrive(dedent(s)) + for s in [ + """ + $ postgres-execute connection=postgres://mz_system@${testdrive.materialize-internal-sql-addr} + GRANT SELECT ON sink_comments_type_view TO materialize + GRANT USAGE ON CONNECTION kafka_conn TO materialize + GRANT USAGE ON CONNECTION csr_conn TO materialize + + > CREATE SINK sink_comments_type2 FROM sink_comments_type_view + INTO KAFKA CONNECTION kafka_conn (TOPIC 'sink-comments-type2') + KEY (a) NOT ENFORCED + FORMAT AVRO USING CONFLUENT SCHEMA REGISTRY CONNECTION csr_conn + ENVELOPE UPSERT + """, + """ + $ postgres-execute connection=postgres://mz_system@${testdrive.materialize-internal-sql-addr} + GRANT SELECT ON sink_comments_type_view TO materialize + GRANT USAGE ON CONNECTION kafka_conn TO materialize + GRANT USAGE ON CONNECTION csr_conn TO materialize + + > CREATE SINK sink_comments_type3 FROM sink_comments_type_view + INTO KAFKA CONNECTION kafka_conn (TOPIC 'sink-comments-type3') + KEY (a) NOT ENFORCED + FORMAT AVRO USING CONFLUENT SCHEMA REGISTRY CONNECTION csr_conn + ENVELOPE UPSERT + """, + ] + ] + + def validate(self) -> Testdrive: + return Testdrive(dedent(""" + # Every sink depends on the commented type its injected DOC ON + # reference names, including sinks whose create_sql predates + # types printing ids in DOC ON positions. + # + # NOTE: the version guard must name the first release that + # prints ids in DOC ON positions. + >[version>=2604200] SELECT s.name + FROM mz_internal.mz_object_dependencies d + JOIN mz_sinks s ON s.id = d.object_id + WHERE d.referenced_object_id IN + (SELECT id FROM mz_types WHERE name = 'sink_comments_point') + ORDER BY 1 + sink_comments_type1 + sink_comments_type2 + sink_comments_type3 + """)) + + @externally_idempotent(False) class KafkaSinkAutoCreatedTopicConfig(Check): """Check on a sink with auto-created topic configuration""" diff --git a/src/adapter/src/catalog/migrate.rs b/src/adapter/src/catalog/migrate.rs index e5b0014066c55..e70d7f1aedc57 100644 --- a/src/adapter/src/catalog/migrate.rs +++ b/src/adapter/src/catalog/migrate.rs @@ -154,6 +154,7 @@ pub(crate) async fn migrate( ast_rewrite_create_sink_partition_strategy(stmt)?; ast_rewrite_sql_server_constraints(stmt)?; ast_rewrite_add_missing_index_ids(tx, stmt)?; + ast_rewrite_add_missing_doc_on_ids(tx, stmt)?; ast_rewrite_kafka_metadata_refresh_intervals(stmt)?; ast_rewrite_small_commit_intervals(stmt)?; ast_rewrite_strip_builtin_version_pins(stmt)?; @@ -1042,6 +1043,108 @@ fn ast_rewrite_add_missing_index_ids( Ok(()) } +/// Add missing item IDs to `DOC ON` references in CREATE SINK statements. +/// +/// `DOC ON TYPE x` and `DOC ON COLUMN x.c` used to persist a type reference +/// as a bare qualified name, unlike a relation in the same position, because +/// name resolution suppressed ids for types. Resolution now prints the id +/// (see `NameResolver::resolve_doc_on_name`); this rewrites stored statements +/// to match, so `create_sql` reference extraction (`mz_object_dependencies`) +/// recovers the sink's edge to the type. +/// +/// References that already carry an id are skipped, so this is idempotent and +/// safe to run every boot. +fn ast_rewrite_add_missing_doc_on_ids( + tx: &Transaction<'_>, + stmt: &mut Statement, +) -> Result<(), anyhow::Error> { + if !matches!(stmt, Statement::CreateSink(_)) { + return Ok(()); + } + rewrite_doc_on_ids(stmt, |name| { + let parts = &name.0; + let (db_name, schema_name, item_name) = match parts.len() { + 3 => (Some(&parts[0]), &parts[1], &parts[2]), + // Names in ambient schemas denote builtin types, which are resolved + // through the system object mappings. + 2 => (None, &parts[0], &parts[1]), + _ => panic!("invalid doc on reference: {name:?}"), + }; + let db_id = db_name.map(|db_name| { + let db = tx.get_databases().find(|db| db.name == db_name.as_str()); + let db = db.unwrap_or_else(|| panic!("missing database in doc on reference: {name:?}")); + db.id + }); + let schema = tx + .get_schemas() + .find(|s| s.name == schema_name.as_str() && s.database_id == db_id); + let schema = + schema.unwrap_or_else(|| panic!("missing schema in doc on reference: {name:?}")); + + let user_type = tx.get_items().find(|i| { + i.name == item_name.as_str() + && i.schema_id == schema.id + // A type may share its (schema, name) with a secret, connection, sink, + // or function, so explicitly filter on Type. + && i.item_type() == CatalogItemType::Type + }); + if let Some(item) = user_type { + return Some(item.id.to_string()); + } + // A bare name in an ambient schema denotes a builtin type (an explicit + // `DOC ON TYPE int4`, or a DOC ON injected by purification after mz_system + // commented on one). Builtin types are not durable items, so the lookup + // above cannot find them; their ids live in the system object mappings. + let builtin_type = tx.get_system_object_mappings().find(|m| { + m.description.schema_name == schema.name + && m.description.object_type == CatalogItemType::Type + && m.description.object_name == item_name.as_str() + }); + let builtin_type = + builtin_type.unwrap_or_else(|| panic!("missing type in doc on reference: {name:?}")); + Some(builtin_type.unique_identifier.catalog_id.to_string()) + }); + Ok(()) +} + +/// Replaces every bare `DOC ON` reference in `stmt` for which `lookup` +/// returns an id with the `[id AS name]` form. `None` leaves the reference as +/// a name. +fn rewrite_doc_on_ids( + stmt: &mut Statement, + lookup: impl FnMut(&mz_sql::ast::UnresolvedItemName) -> Option, +) { + use mz_sql::ast::visit_mut::{VisitMut, VisitMutNode}; + use mz_sql::ast::{ColumnName, DocOnIdentifier, RawItemName, UnresolvedItemName}; + + struct Rewriter { + lookup: F, + } + + impl<'ast, F> VisitMut<'ast, Raw> for Rewriter + where + F: FnMut(&UnresolvedItemName) -> Option, + { + fn visit_doc_on_identifier_mut(&mut self, node: &mut DocOnIdentifier) { + let name = match node { + DocOnIdentifier::Type(name) => name, + DocOnIdentifier::Column(ColumnName { + relation, + column: _, + }) => relation, + }; + if let RawItemName::Name(unresolved) = name { + if let Some(id) = (self.lookup)(unresolved) { + let unresolved = unresolved.clone(); + *name = RawItemName::Id(id, unresolved, None); + } + } + } + } + + stmt.visit_mut(&mut Rewriter { lookup }); +} + /// Strips the `VERSION` qualifier from by-id references to non-user (builtin) /// items. /// @@ -1230,4 +1333,63 @@ mod tests { assert!(!out.contains("VERSION"), "unexpected version: {out}"); assert!(out.contains("s518"), "reference dropped: {out}"); } + + fn add_doc_on_ids( + sql: &str, + lookup: impl FnMut(&mz_sql::ast::UnresolvedItemName) -> Option, + ) -> String { + let mut stmt = mz_sql::parse::parse(sql) + .expect("test sql parses") + .into_element() + .ast; + rewrite_doc_on_ids(&mut stmt, lookup); + stmt.to_ast_string_stable() + } + + #[mz_ore::test] + #[cfg_attr(miri, ignore)] // unsupported operation: can't call foreign function `rust_psm_stack_pointer` (SQL parser stack growth) + fn adds_ids_to_bare_doc_on_references() { + // The persisted shape of a sink from before types printed ids in DOC + // ON positions: relations carry ids, the type and its column carry + // bare names. + let out = add_doc_on_ids( + r#"CREATE SINK "materialize"."public"."s" FROM [u1 AS "materialize"."public"."t"] INTO KAFKA CONNECTION [u2 AS "materialize"."public"."kc"] (TOPIC = 'top') FORMAT AVRO USING CONFLUENT SCHEMA REGISTRY CONNECTION [u3 AS "materialize"."public"."csr"] (DOC ON TYPE "materialize"."public"."point" = 'p', DOC ON COLUMN "materialize"."public"."point"."x" = 'x', DOC ON TYPE "pg_catalog"."int4" = 'i') ENVELOPE UPSERT"#, + |name| match name.0.len() { + 3 => { + assert_eq!(name.0[2].as_str(), "point", "unexpected lookup: {name:?}"); + Some("u9".into()) + } + 2 => { + assert_eq!(name.0[1].as_str(), "int4", "unexpected lookup: {name:?}"); + Some("s23".into()) + } + _ => panic!("unexpected lookup: {name:?}"), + }, + ); + assert!( + out.contains(r#"DOC ON TYPE [u9 AS "materialize"."public"."point"]"#), + "type reference not rewritten: {out}" + ); + assert!( + out.contains(r#"DOC ON COLUMN [u9 AS "materialize"."public"."point"]."x""#), + "column reference not rewritten: {out}" + ); + assert!( + out.contains(r#"DOC ON TYPE [s23 AS "pg_catalog"."int4"]"#), + "builtin type reference not rewritten: {out}" + ); + } + + #[mz_ore::test] + #[cfg_attr(miri, ignore)] // unsupported operation: can't call foreign function `rust_psm_stack_pointer` (SQL parser stack growth) + fn leaves_id_doc_on_references_untouched() { + let out = add_doc_on_ids( + r#"CREATE SINK "materialize"."public"."s" FROM [u1 AS "materialize"."public"."t"] INTO KAFKA CONNECTION [u2 AS "materialize"."public"."kc"] (TOPIC = 'top') FORMAT AVRO USING CONFLUENT SCHEMA REGISTRY CONNECTION [u3 AS "materialize"."public"."csr"] (DOC ON TYPE [u4 AS "materialize"."public"."point"] = 'p', DOC ON COLUMN [u1 AS "materialize"."public"."t"]."c1" = 'c') ENVELOPE UPSERT"#, + |name| panic!("id reference must not be resolved: {name:?}"), + ); + assert!( + out.contains(r#"DOC ON TYPE [u4 AS "materialize"."public"."point"]"#), + "id reference altered: {out}" + ); + } } diff --git a/src/sql-parser/src/ast/item_refs.rs b/src/sql-parser/src/ast/item_refs.rs index aa76b97be4bea..dfdf0f4040616 100644 --- a/src/sql-parser/src/ast/item_refs.rs +++ b/src/sql-parser/src/ast/item_refs.rs @@ -378,6 +378,25 @@ mod tests { assert_eq!(refs.ids, strings(&["u5", "u7"])); } + #[mz_ore::test] + fn doc_on_references() { + // DOC ON TYPE and DOC ON COLUMN references persist with ids, so a + // sink's dependency on a commented type is recovered from the ids + // bucket like any other id reference. + let refs = collect( + r#"CREATE SINK s + FROM [u1 AS "materialize"."public"."t"] + INTO KAFKA CONNECTION [u2 AS "materialize"."public"."kc"] (TOPIC 'top') + FORMAT AVRO USING CONFLUENT SCHEMA REGISTRY CONNECTION [u3 AS "materialize"."public"."csr"] + (DOC ON TYPE [u4 AS "materialize"."public"."point"] = 'point doc', + DOC ON COLUMN [u4 AS "materialize"."public"."point"].x = 'x doc') + ENVELOPE UPSERT"#, + ); + assert_eq!(refs.ids, strings(&["u1", "u2", "u3", "u4"])); + assert!(refs.named_relations.is_empty()); + assert!(refs.named_types.is_empty()); + } + #[mz_ore::test] fn index_references() { let refs = collect( diff --git a/src/sql/src/names.rs b/src/sql/src/names.rs index 1fd703a9d8885..01e3b7f80bacc 100644 --- a/src/sql/src/names.rs +++ b/src/sql/src/names.rs @@ -1668,6 +1668,30 @@ impl<'a> NameResolver<'a> { version, } } + + /// Resolves an item name in a `DOC ON` position (`DOC ON TYPE x`, + /// `DOC ON COLUMN x.c`), where the name may denote a type or a relation. Also used for `COMMENT + /// ON COLUMN`. + fn resolve_doc_on_name(&mut self, name: RawItemName) -> ResolvedItemName { + let mut name = self.resolve_item_name( + name, + // The name can refer to either a type or a relation. + // + // It's possible this will get simpler once database-issues#7142 is + // fixed. See the comment on `ItemResolutionConfig` for details. + ItemResolutionConfig { + functions: false, + types: true, + relations: true, + }, + ); + if let ResolvedItemName::Item { print_id, .. } = &mut name { + // These references persist in a sink's `create_sql`, so the resolved name must print + // its id, otherwise it can't recover it when recreating + *print_id = true; + } + name + } } impl<'a> Fold for NameResolver<'a> { @@ -1798,14 +1822,7 @@ impl<'a> Fold for NameResolver<'a> { } fn fold_column_name(&mut self, column_name: ast::ColumnName) -> ast::ColumnName { - let item_name = self.resolve_item_name( - column_name.relation, - ItemResolutionConfig { - functions: false, - types: true, - relations: true, - }, - ); + let item_name = self.resolve_doc_on_name(column_name.relation); match &item_name { ResolvedItemName::Item { @@ -2302,19 +2319,7 @@ impl<'a> Fold for NameResolver<'a> { fn fold_doc_on_identifier(&mut self, node: DocOnIdentifier) -> DocOnIdentifier { match node { DocOnIdentifier::Column(name) => DocOnIdentifier::Column(self.fold_column_name(name)), - DocOnIdentifier::Type(name) => DocOnIdentifier::Type(self.resolve_item_name( - name, - // In `DOC ON TYPE ...`, the type can refer to either a type or - // a relation. - // - // It's possible this will get simpler once database-issues#7142 is fixed. See - // the comment on `ItemResolutionConfig` for details. - ItemResolutionConfig { - functions: false, - types: true, - relations: true, - }, - )), + DocOnIdentifier::Type(name) => DocOnIdentifier::Type(self.resolve_doc_on_name(name)), } } diff --git a/src/sql/src/pure.rs b/src/sql/src/pure.rs index e366a4779e5b0..f1d8ed5bfee53 100644 --- a/src/sql/src/pure.rs +++ b/src/sql/src/pure.rs @@ -429,10 +429,7 @@ pub(crate) fn purify_create_sink_avro_doc_on_options( id: *object_id, qualifiers: item.name().qualifiers.clone(), full_name: catalog.resolve_full_name(item.name()), - print_id: !matches!( - item.item_type(), - CatalogItemType::Func | CatalogItemType::Type - ), + print_id: !matches!(item.item_type(), CatalogItemType::Func), version: RelationVersionSelector::Latest, }; diff --git a/test/testdrive/kafka-avro-sinks-doc-comments.td b/test/testdrive/kafka-avro-sinks-doc-comments.td index 5bcc8b32725b3..c44c5e8973b1a 100644 --- a/test/testdrive/kafka-avro-sinks-doc-comments.td +++ b/test/testdrive/kafka-avro-sinks-doc-comments.td @@ -69,6 +69,21 @@ $ schema-registry-verify schema-type=avro subject=testdrive-sink1-${testdrive.se $ schema-registry-verify schema-type=avro subject=testdrive-sink1-${testdrive.seed}-key {"type":"record","name":"row","doc":"key doc on t","fields":[{"name":"c2","type":"string","doc":"key doc on t.c2"}]} +# The sink's DOC ON references record dependency edges on the commented type +# and on the sinked table. The type reference persists by id, so reference +# extraction (mz_object_dependencies) recovers the edge; a bare name would +# silently drop out. The guard names the first release that prints ids in +# DOC ON positions +>[version>=2604200] SELECT count(*) FROM mz_internal.mz_object_dependencies + WHERE object_id = (SELECT id FROM mz_sinks WHERE name = 'sink1') + AND referenced_object_id = (SELECT id FROM mz_types WHERE name = 'point'); +1 + +> SELECT count(*) FROM mz_internal.mz_object_dependencies + WHERE object_id = (SELECT id FROM mz_sinks WHERE name = 'sink1') + AND referenced_object_id = (SELECT id FROM mz_tables WHERE name = 't'); +1 + > CREATE CLUSTER sink2_cluster SIZE '${arg.default-storage-size}'; > CREATE SINK sink2 IN CLUSTER sink2_cluster From 72009713143753aec9cc8ab495fa7d91f6b2a7bb Mon Sep 17 00:00:00 2001 From: Nicholas McAvoy Date: Thu, 10 Sep 2026 15:57:15 -0400 Subject: [PATCH 05/11] sql: fork chrono-tz to deliver tzdata 2026c (#38735) Ship IANA tzdata 2026c by pointing the workspace at a MaterializeInc fork of chrono-tz. ### Motivation Upstream chrono-tz stops at 0.10.4 with tzdata 2025b and its maintainer is winding the project down, so newer IANA data will not arrive from crates.io. Under 2025b, Materialize starts giving wrong answers about the present on 2026-09-20, when Morocco moves to permanent +00, and again on 2026-11-01, when Alberta and British Columbia would appear to fall back but no longer do. ### The fork [MaterializeInc/chrono-tz](https://github.com/MaterializeInc/chrono-tz) is forked at the v0.10.4 tag. Its `mz_changes` branch carries exactly two commits: bump the `tz` submodule to the IANA 2026c tag, and regenerate the crate's prebuilt data with its own codegen test. No code changes, same API, same phf 0.12, so the existing `deny.toml` exception and every call site are untouched. Upstream `main` has unreleased semver-breaking changes, which is why the fork is cut from the tag rather than from `main`. The `[patch.crates-io]` entry pins the rev. chrono-tz-build and parse-zoneinfo are path dependencies inside that repo, so they follow automatically. ### Data changes 2025b to 2026c Transition-level diff of every zone the prebuilt data touches: | Zones | Change | Past instants affected | |---|---|---| | Africa/Casablanca, Africa/El_Aaiun | Permanent +00 from 2026-09-20 | none | | America/Edmonton, America/Yellowknife, Canada/Mountain | Permanent -06, no 2026-11-01 fall-back, abbreviation CST | none | | America/Vancouver, Canada/Pacific | Permanent -07, no 2026-11-01 fall-back, abbreviation MST | none | | Europe/Chisinau, Europe/Tiraspol | EU transition instants since 2022, one hour later than before | nine one-hour windows, 2022-03-27 to 2026-03-29 | | America/Tijuana and links | DST observed 1953 and 1961 to 1975 | 32 transitions, all before 1976 | The zone directory is unchanged, so no zone appears, disappears, or renames. Co-authored-by: Claude Fable 5.1 --- Cargo.lock | 9 +-- Cargo.toml | 6 ++ .../testdata/timezones/names-2026-12-15.csv | 14 ++--- .../tests/testdata/timezones/regenerate.sh | 2 +- test/sqllogictest/timezone.slt | 57 +++++++++++++++++++ 5 files changed, 74 insertions(+), 14 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 80bc64224e23a..f69d00392e722 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2234,8 +2234,7 @@ dependencies = [ [[package]] name = "chrono-tz" version = "0.10.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a6139a8597ed92cf816dfb33f5dd6cf0bb93a6adc938f11039f371bc5bcd26c3" +source = "git+https://github.com/MaterializeInc/chrono-tz.git?rev=5999cdd1b971694f834ec3467d11ef9147fc297c#5999cdd1b971694f834ec3467d11ef9147fc297c" dependencies = [ "chrono", "chrono-tz-build", @@ -2247,8 +2246,7 @@ dependencies = [ [[package]] name = "chrono-tz-build" version = "0.5.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c7d8d1efd5109b9c1cd3b7966bd071cdfb53bb6eb0b22a473a68c2f70a11a1eb" +source = "git+https://github.com/MaterializeInc/chrono-tz.git?rev=5999cdd1b971694f834ec3467d11ef9147fc297c#5999cdd1b971694f834ec3467d11ef9147fc297c" dependencies = [ "parse-zoneinfo", "phf_codegen 0.12.1", @@ -10319,8 +10317,7 @@ dependencies = [ [[package]] name = "parse-zoneinfo" version = "0.4.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e3c406c9e2aa74554e662d2c2ee11cd3e73756988800be7e6f5eddb16fed4699" +source = "git+https://github.com/MaterializeInc/chrono-tz.git?rev=5999cdd1b971694f834ec3467d11ef9147fc297c#5999cdd1b971694f834ec3467d11ef9147fc297c" [[package]] name = "paste" diff --git a/Cargo.toml b/Cargo.toml index d6de0f6ce2336..15d779565b774 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -648,6 +648,12 @@ postgres-types = { git = "https://github.com/MaterializeInc/rust-postgres" } postgres-openssl = { git = "https://github.com/MaterializeInc/rust-postgres" } postgres_array = { git = "https://github.com/MaterializeInc/rust-postgres-array" } +# Upstream chrono-tz is unmaintained and stops at tzdata 2025b. The fork's +# `mz_changes` branch carries current IANA data only, no code changes. To bump +# tzdata, update the fork, then follow the bump ceremony documented in +# src/environmentd/tests/testdata/timezones/regenerate.sh. +chrono-tz = { git = "https://github.com/MaterializeInc/chrono-tz.git", rev = "5999cdd1b971694f834ec3467d11ef9147fc297c" } + # Waiting on https://github.com/MaterializeInc/serde-value/pull/35. serde-value = { git = "https://github.com/MaterializeInc/serde-value.git" } diff --git a/src/environmentd/tests/testdata/timezones/names-2026-12-15.csv b/src/environmentd/tests/testdata/timezones/names-2026-12-15.csv index f74cbc1d46d22..a70d1bcb76355 100644 --- a/src/environmentd/tests/testdata/timezones/names-2026-12-15.csv +++ b/src/environmentd/tests/testdata/timezones/names-2026-12-15.csv @@ -12,14 +12,14 @@ Africa/Blantyre,CAT,02:00:00,f Africa/Brazzaville,WAT,01:00:00,f Africa/Bujumbura,CAT,02:00:00,f Africa/Cairo,EET,02:00:00,f -Africa/Casablanca,+01,01:00:00,f +Africa/Casablanca,+00,00:00:00,f Africa/Ceuta,CET,01:00:00,f Africa/Conakry,GMT,00:00:00,f Africa/Dakar,GMT,00:00:00,f Africa/Dar_es_Salaam,EAT,03:00:00,f Africa/Djibouti,EAT,03:00:00,f Africa/Douala,WAT,01:00:00,f -Africa/El_Aaiun,+01,01:00:00,f +Africa/El_Aaiun,+00,00:00:00,f Africa/Freetown,GMT,00:00:00,f Africa/Gaborone,CAT,02:00:00,f Africa/Harare,CAT,02:00:00,f @@ -107,7 +107,7 @@ America/Dawson_Creek,MST,-07:00:00,f America/Denver,MST,-07:00:00,f America/Detroit,EST,-05:00:00,f America/Dominica,AST,-04:00:00,f -America/Edmonton,MST,-07:00:00,f +America/Edmonton,CST,-06:00:00,f America/Eirunepe,-05,-05:00:00,f America/El_Salvador,CST,-06:00:00,f America/Ensenada,PST,-08:00:00,f @@ -215,12 +215,12 @@ America/Thunder_Bay,EST,-05:00:00,f America/Tijuana,PST,-08:00:00,f America/Toronto,EST,-05:00:00,f America/Tortola,AST,-04:00:00,f -America/Vancouver,PST,-08:00:00,f +America/Vancouver,MST,-07:00:00,f America/Virgin,AST,-04:00:00,f America/Whitehorse,MST,-07:00:00,f America/Winnipeg,CST,-06:00:00,f America/Yakutat,AKST,-09:00:00,f -America/Yellowknife,MST,-07:00:00,f +America/Yellowknife,CST,-06:00:00,f Antarctica/Casey,+08,08:00:00,f Antarctica/Davis,+07,07:00:00,f Antarctica/DumontDUrville,+10,10:00:00,f @@ -375,9 +375,9 @@ Brazil/West,-04,-04:00:00,f Canada/Atlantic,AST,-04:00:00,f Canada/Central,CST,-06:00:00,f Canada/Eastern,EST,-05:00:00,f -Canada/Mountain,MST,-07:00:00,f +Canada/Mountain,CST,-06:00:00,f Canada/Newfoundland,NST,-03:30:00,f -Canada/Pacific,PST,-08:00:00,f +Canada/Pacific,MST,-07:00:00,f Canada/Saskatchewan,CST,-06:00:00,f Canada/Yukon,MST,-07:00:00,f CET,CET,01:00:00,f diff --git a/src/environmentd/tests/testdata/timezones/regenerate.sh b/src/environmentd/tests/testdata/timezones/regenerate.sh index a1cae366cff29..6d0013dccbb9d 100755 --- a/src/environmentd/tests/testdata/timezones/regenerate.sh +++ b/src/environmentd/tests/testdata/timezones/regenerate.sh @@ -29,7 +29,7 @@ set -euo pipefail cd "$(dirname "$0")" -EXPECTED_TZDATA=2025b +EXPECTED_TZDATA=2026c IMAGE=mz-timezone-snapshot DATES=("$@") diff --git a/test/sqllogictest/timezone.slt b/test/sqllogictest/timezone.slt index 53026cadaa1cf..1c53b705565ad 100644 --- a/test/sqllogictest/timezone.slt +++ b/test/sqllogictest/timezone.slt @@ -358,6 +358,44 @@ SELECT timezone_offset('Asia/Almaty', '2026-07-15 00:00:00 UTC'), timezone_offse (+05,05:00:00,00:00:00) (EEST,02:00:00,01:00:00) +# Zones whose rules changed in tzdata 2025c through 2026c: Morocco moved to +# permanent +00 on 2026-09-20, Alberta and British Columbia abolished DST on +# their 2026 fall-back dates and keep the summer offset, and Moldova follows +# EU transition instants. Both Canadian zones now render a neighbouring +# region's standard-time abbreviation for their permanent offset. + +statement ok +SET unsafe_new_transaction_wall_time = '2026-12-15 00:00:00 UTC'; + +query TTTT +SELECT * FROM pg_catalog.pg_timezone_names +WHERE name IN ('Africa/Casablanca', 'America/Edmonton', 'America/Vancouver', 'Europe/Chisinau') +ORDER BY name +---- +Africa/Casablanca ++00 +00:00:00 +false +America/Edmonton +CST +-06:00:00 +false +America/Vancouver +MST +-07:00:00 +false +Europe/Chisinau +EET +02:00:00 +false + +query TTT +SELECT timezone_offset('Africa/Casablanca', '2026-09-21 00:00:00 UTC'), timezone_offset('America/Edmonton', '2026-11-15 00:00:00 UTC'), timezone_offset('America/Vancouver', '2026-11-15 00:00:00 UTC') +---- +(+00,00:00:00,00:00:00) +(CST,-06:00:00,00:00:00) +(MST,-07:00:00,00:00:00) + # These pin historical rules: a timestamp from before a zone's rule change # must resolve under the rules in force at that instant, not current rules. @@ -385,3 +423,22 @@ query T SELECT timezone_offset('Asia/Choibalsan', '1995-06-15 00:00:00 UTC') ---- (+09,08:00:00,01:00:00) + +# Moldova has switched at the EU instant, 01:00 UTC, since 2022. Older data +# had it one hour earlier, so for one hour around each transition the two +# disagree. This asserts the current belief about the past. If a data update +# breaks it, IANA rewrote history. + +query T +SELECT timezone_offset('Europe/Chisinau', '2023-03-26 00:30:00 UTC') +---- +(EET,02:00:00,00:00:00) + +# Baja California observed DST in 1953 and 1961 through 1975, following +# California, where earlier data had none. This asserts the current belief +# about the past. If a data update breaks it, IANA rewrote history. + +query T +SELECT timezone_offset('America/Tijuana', '1970-07-15 12:00:00 UTC') +---- +(PDT,-08:00:00,01:00:00) From b0b39841b1c51a277956aaded40781a4678ef830 Mon Sep 17 00:00:00 2001 From: Pranshu Maheshwari Date: Thu, 10 Sep 2026 16:44:11 -0400 Subject: [PATCH 06/11] docs: run historical freshness queries from a separate cluster (#38746) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### Motivation Follow-up to #38744. On that PR, a reviewer noted that regularly querying the unindexed `mz_wallclock_global_lag_history` can bog down `mz_catalog_server`, which is risky because the catalog server also backs the Console and `SHOW`/catalog queries across the whole environment. Slack thread: https://materializeinc.slack.com/archives/CPNFV9CVB/p1789065229460309?thread_ts=1789064976.189309&cid=CPNFV9CVB ### Description Adds guidance to the "Monitor historical freshness" section of `doc/user/content/transform-data/monitor-freshness.md` to run recurring historical freshness queries from a separate cluster instead of the default `mz_catalog_server`. Because these queries read only system catalog relations, they auto-route to `mz_catalog_server` regardless of the session's active cluster, so the note also instructs disabling `auto_route_catalog_queries` so the query actually runs on the dedicated cluster. ### Verification - Docs-only prose change; `bin/format-docs` runs clean. - Validated the documented flow against a local Materialize emulator (v26.41.0): `CREATE CLUSTER ... (SIZE = '25cc')`, `SET cluster = ...`, `SET auto_route_catalog_queries = false`, and both freshness queries all run without error. Confirmed via the statement activity log that with auto-routing left on the query executes on `mz_catalog_server`, and only with `auto_route_catalog_queries = false` does it execute on the separate cluster. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01Vihj1JSR7cd8CW3SKhquoD --- _Generated by [Claude Code](https://claude.ai/code/session_01Vihj1JSR7cd8CW3SKhquoD)_ Co-authored-by: Claude --- .../content/transform-data/monitor-freshness.md | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/doc/user/content/transform-data/monitor-freshness.md b/doc/user/content/transform-data/monitor-freshness.md index b671f5f66f7a8..a21da7f7339e9 100644 --- a/doc/user/content/transform-data/monitor-freshness.md +++ b/doc/user/content/transform-data/monitor-freshness.md @@ -98,9 +98,24 @@ GROUP BY 1 ORDER BY 1 DESC; ``` -For recurring or dashboard queries, stay on +For recurring or dashboard queries that fit within the last 24 hours, stay on `mz_wallclock_global_lag_recent_history`. +If you need historical data on a recurring basis, run the query from a separate +cluster instead of the default `mz_catalog_server`. A slow scan of the unindexed +relation can tie up `mz_catalog_server` for seconds at a time, and because it +also serves the `SHOW` commands and catalog lookups behind the Console and +interactive tooling, that slows catalog queries across your whole environment. +These queries read only system catalog relations, so Materialize routes them to +`mz_catalog_server` by default. Point the session at your own cluster and turn +off catalog auto-routing so the query actually runs there: + +```mzsql +CREATE CLUSTER freshness_monitoring (SIZE = '25cc'); +SET cluster = freshness_monitoring; +SET auto_route_catalog_queries = false; +``` + ## Summarize freshness with a CCDF A raw time series is hard to summarize. A **complementary cumulative From bdb21878e5bd16e89c2f75666ba3fd61ff25b30c Mon Sep 17 00:00:00 2001 From: Patrick Butler Date: Thu, 10 Sep 2026 17:12:05 -0400 Subject: [PATCH 07/11] SS-452 Add sql option for iceberg storage provider to iceberg catalog connection (#38732) Adds an option to the `CREATE CONNECTION ... TO ICEBERG CATALOG` statement to specify a storage provider for the catalog, and then consumes this option when building the opendal storage interface to create the correct one. Unblocks vended credential support for GCS and ADLS backed iceberg catalogs --- Cargo.lock | 52 ++++++++++ Cargo.toml | 2 +- deny.toml | 2 + src/sql-lexer/src/keywords.txt | 1 + src/sql-parser/src/ast/defs/ddl.rs | 3 + src/sql-parser/src/parser.rs | 5 + src/sql-parser/tests/testdata/ddl | 16 ++- src/sql/src/plan/statement/ddl/connection.rs | 35 ++++++- src/sql/src/plan/with_options.rs | 31 +++++- src/storage-types/src/connections.rs | 51 +++++++++- test/iceberg/mzcompose.py | 13 +++ test/iceberg/storage-provider.td | 100 +++++++++++++++++++ 12 files changed, 303 insertions(+), 8 deletions(-) create mode 100644 test/iceberg/storage-provider.td diff --git a/Cargo.lock b/Cargo.lock index f69d00392e722..8709869696b74 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -9845,6 +9845,7 @@ dependencies = [ "opendal-core", "opendal-layer-retry", "opendal-layer-timeout", + "opendal-service-azdls", "opendal-service-gcs", "opendal-service-s3", ] @@ -9898,6 +9899,36 @@ dependencies = [ "tokio", ] +[[package]] +name = "opendal-service-azdls" +version = "0.57.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6dea4908d490143a9b0b7f7a790e139ff829b06a023f670455ed3d44f664b361" +dependencies = [ + "base64 0.22.1", + "bytes", + "http 1.4.2", + "log", + "opendal-core", + "opendal-service-azure-common", + "quick-xml 0.39.4", + "reqsign-azure-storage", + "reqsign-core", + "reqsign-file-read-tokio", + "serde", + "serde_json", +] + +[[package]] +name = "opendal-service-azure-common" +version = "0.57.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9b489f13c42e69d69bdd72952b634356ec43a7881a20259b38b540fcecdf4051" +dependencies = [ + "http 1.4.2", + "opendal-core", +] + [[package]] name = "opendal-service-gcs" version = "0.57.0" @@ -11682,6 +11713,27 @@ dependencies = [ "serde", ] +[[package]] +name = "reqsign-azure-storage" +version = "3.1.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2824e7da3c2cc42ac3406c674eb57c89127fdcd97f3a73c608cfc680505ea134" +dependencies = [ + "anyhow", + "base64 0.23.1", + "bytes", + "form_urlencoded", + "http 1.4.2", + "log", + "pem 3.0.6", + "percent-encoding", + "reqsign-core", + "rsa", + "serde", + "serde_json", + "sha1 0.11.0", +] + [[package]] name = "reqsign-core" version = "3.3.0" diff --git a/Cargo.toml b/Cargo.toml index 15d779565b774..1de960d735215 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -383,7 +383,7 @@ hyper-util = "0.1.20" tower-service = "0.3.3" iceberg = "0.10.1" iceberg-catalog-rest = "0.10.1" -iceberg-storage-opendal = { version = "0.10.1", default-features = false, features = ["opendal-s3", "opendal-gcs"] } +iceberg-storage-opendal = { version = "0.10.1", default-features = false, features = ["opendal-s3", "opendal-gcs", "opendal-azdls"] } imbl = { version = "7.0.0", features = ["serde"] } include_dir = "0.7.4" indexmap = { version = "2.10.0", default-features = false, features = ["std"] } diff --git a/deny.toml b/deny.toml index ec3704b00ef8a..0a72f4bde8c0a 100644 --- a/deny.toml +++ b/deny.toml @@ -235,6 +235,7 @@ wrappers = [ "native-tls", "opendal-core", "opendal-layer-retry", + "opendal-service-azdls", "opendal-service-gcs", "opendal-service-s3", "os_info", @@ -245,6 +246,7 @@ wrappers = [ "rdkafka", "reqsign-aws-core", "reqsign-aws-v4", + "reqsign-azure-storage", "reqsign-core", "reqsign-google", "reqwest", diff --git a/src/sql-lexer/src/keywords.txt b/src/sql-lexer/src/keywords.txt index a69287cc3ce23..18763dd47871e 100644 --- a/src/sql-lexer/src/keywords.txt +++ b/src/sql-lexer/src/keywords.txt @@ -380,6 +380,7 @@ Progress Projection Protobuf Protocol +Provider Public Publication Pushdown diff --git a/src/sql-parser/src/ast/defs/ddl.rs b/src/sql-parser/src/ast/defs/ddl.rs index 5a591458609b7..c3626f9631ffa 100644 --- a/src/sql-parser/src/ast/defs/ddl.rs +++ b/src/sql-parser/src/ast/defs/ddl.rs @@ -931,6 +931,7 @@ pub enum ConnectionOptionName { SslMode, SessionToken, CatalogType, + StorageProvider, Url, User, Warehouse, @@ -978,6 +979,7 @@ impl ConnectionOptionName { | ConnectionOptionName::SshTunnel | ConnectionOptionName::SslMode | ConnectionOptionName::CatalogType + | ConnectionOptionName::StorageProvider | ConnectionOptionName::Url | ConnectionOptionName::Warehouse => false, } @@ -1027,6 +1029,7 @@ impl AstDisplay for ConnectionOptionName { ConnectionOptionName::SslMode => "SSL MODE", ConnectionOptionName::SessionToken => "SESSION TOKEN", ConnectionOptionName::CatalogType => "CATALOG TYPE", + ConnectionOptionName::StorageProvider => "STORAGE PROVIDER", ConnectionOptionName::Url => "URL", ConnectionOptionName::User => "USER", ConnectionOptionName::Warehouse => "WAREHOUSE", diff --git a/src/sql-parser/src/parser.rs b/src/sql-parser/src/parser.rs index 96e4ba1efad03..76af586641bf9 100644 --- a/src/sql-parser/src/parser.rs +++ b/src/sql-parser/src/parser.rs @@ -2989,6 +2989,7 @@ impl<'a> Parser<'a> { SESSION, SSH, SSL, + STORAGE, URL, USER, USERNAME, @@ -3102,6 +3103,10 @@ impl<'a> Parser<'a> { MODE => ConnectionOptionName::SslMode, _ => unreachable!(), }, + STORAGE => { + self.expect_keyword(PROVIDER)?; + ConnectionOptionName::StorageProvider + } URL => ConnectionOptionName::Url, // TYPE => ConnectionOptionName::CatalogType, WAREHOUSE => ConnectionOptionName::Warehouse, diff --git a/src/sql-parser/tests/testdata/ddl b/src/sql-parser/tests/testdata/ddl index 37c4c04cb1711..cfeaf705a4337 100644 --- a/src/sql-parser/tests/testdata/ddl +++ b/src/sql-parser/tests/testdata/ddl @@ -627,6 +627,20 @@ CREATE CONNECTION icebergconn TO ICEBERG CATALOG (CATALOG TYPE = 'rest', URL = ' => CreateConnection(CreateConnectionStatement { name: UnresolvedItemName([Ident("icebergconn")]), connection_type: IcebergCatalog, if_not_exists: false, values: [ConnectionOption { name: CatalogType, value: Some(Value(String("rest"))) }, ConnectionOption { name: Url, value: Some(Value(String("https://example/api/catalog"))) }, ConnectionOption { name: Credential, value: Some(Value(String("id:secret"))) }, ConnectionOption { name: Oauth2ServerUrl, value: Some(Value(String("https://example/oidc/v1/token"))) }, ConnectionOption { name: Scope, value: Some(Value(String("all-apis"))) }, ConnectionOption { name: Warehouse, value: Some(Value(String("wh"))) }], with_options: [] }) +parse-statement +CREATE CONNECTION icebergconn TO ICEBERG CATALOG (CATALOG TYPE 'rest', URL 'https://example/api/catalog', CREDENTIAL 'id:secret', STORAGE PROVIDER 'gcs', WAREHOUSE 'wh') +---- +CREATE CONNECTION icebergconn TO ICEBERG CATALOG (CATALOG TYPE = 'rest', URL = 'https://example/api/catalog', CREDENTIAL = 'id:secret', STORAGE PROVIDER = 'gcs', WAREHOUSE = 'wh') +=> +CreateConnection(CreateConnectionStatement { name: UnresolvedItemName([Ident("icebergconn")]), connection_type: IcebergCatalog, if_not_exists: false, values: [ConnectionOption { name: CatalogType, value: Some(Value(String("rest"))) }, ConnectionOption { name: Url, value: Some(Value(String("https://example/api/catalog"))) }, ConnectionOption { name: Credential, value: Some(Value(String("id:secret"))) }, ConnectionOption { name: StorageProvider, value: Some(Value(String("gcs"))) }, ConnectionOption { name: Warehouse, value: Some(Value(String("wh"))) }], with_options: [] }) + +parse-statement +CREATE CONNECTION icebergconn TO ICEBERG CATALOG (CATALOG TYPE 'rest', URL 'https://example/api/catalog', CREDENTIAL 'id:secret', STORAGE 'gcs') +---- +error: Expected PROVIDER, found string literal "gcs" +CREATE CONNECTION icebergconn TO ICEBERG CATALOG (CATALOG TYPE 'rest', URL 'https://example/api/catalog', CREDENTIAL 'id:secret', STORAGE 'gcs') + ^ + parse-statement CREATE CONNECTION privatelinkconn TO AWS PRIVATELINK (SERVICE NAME 'com.amazonaws.vpce.us-east-1.vpce-svc-0e123abc123198abc', AVAILABILITY ZONES ('use1-az1', 'use1-az4')) ---- @@ -3082,7 +3096,7 @@ CreateConnection(CreateConnectionStatement { name: UnresolvedItemName([Ident("my parse-statement CREATE CONNECTION my_ssh_tunnel FOR SSH TUNNEL (PUBLIC KEY 3 = nope) ---- -error: Expected one of ACCESS or ASSUME or AVAILABILITY or AWS or BROKER or BROKERS or CATALOG or CREDENTIAL or DATABASE or ENDPOINT or GCP or HOST or OAUTH2 or PASSWORD or PORT or PUBLIC or PROGRESS or REGION or REGISTRY or SASL or SCOPE or SECRET or SECURITY or SERVICE or SESSION or SSH or SSL or URL or USER or USERNAME or WAREHOUSE, found left parenthesis +error: Expected one of ACCESS or ASSUME or AVAILABILITY or AWS or BROKER or BROKERS or CATALOG or CREDENTIAL or DATABASE or ENDPOINT or GCP or HOST or OAUTH2 or PASSWORD or PORT or PUBLIC or PROGRESS or REGION or REGISTRY or SASL or SCOPE or SECRET or SECURITY or SERVICE or SESSION or SSH or SSL or STORAGE or URL or USER or USERNAME or WAREHOUSE, found left parenthesis CREATE CONNECTION my_ssh_tunnel FOR SSH TUNNEL (PUBLIC KEY 3 = nope) ^ diff --git a/src/sql/src/plan/statement/ddl/connection.rs b/src/sql/src/plan/statement/ddl/connection.rs index cf333d0b1ab93..eccbfed25b732 100644 --- a/src/sql/src/plan/statement/ddl/connection.rs +++ b/src/sql/src/plan/statement/ddl/connection.rs @@ -36,9 +36,10 @@ use mz_storage_types::connections::{ AwsPrivatelink, AwsPrivatelinkConnection, AwsPrivatelinkRule, CsrConnection, CsrConnectionHttpAuth, GlueSchemaRegistryConnection, IcebergAccessDelegation, IcebergCatalogAuth, IcebergCatalogConnection, IcebergCatalogImpl, IcebergCatalogType, - KafkaConnection, KafkaSaslConfig, KafkaTlsConfig, KafkaTopicOptions, MySqlConnection, - MySqlSslMode, PostgresConnection, RestIcebergCatalog, S3TablesRestIcebergCatalog, - SqlServerConnectionDetails, SshConnection, SshTunnel, TlsIdentity, Tunnel, + IcebergStorageProvider, KafkaConnection, KafkaSaslConfig, KafkaTlsConfig, KafkaTopicOptions, + MySqlConnection, MySqlSslMode, PostgresConnection, RestIcebergCatalog, + S3TablesRestIcebergCatalog, SqlServerConnectionDetails, SshConnection, SshTunnel, TlsIdentity, + Tunnel, }; use crate::names::Aug; @@ -50,6 +51,7 @@ use crate::session::vars; generate_extracted_config!( ConnectionOption, (AccessDelegation, IcebergAccessDelegation), + (StorageProvider, IcebergStorageProvider), (AccessKeyId, StringOrSecret), (AssumeRoleArn, String), (AssumeRoleSessionName, String), @@ -199,6 +201,7 @@ pub(super) fn validate_options_per_connection_type( GcpConnection, Oauth2ServerUrl, Scope, + StorageProvider, Url, Warehouse, ], @@ -711,6 +714,11 @@ impl ConnectionOptionExtracted { "invalid CONNECTION: ICEBERG s3tablesrest connections do not support ACCESS DELEGATION" ); } + if self.storage_provider.is_some() { + sql_bail!( + "invalid CONNECTION: ICEBERG s3tablesrest connections do not support STORAGE PROVIDER" + ); + } let Some(warehouse) = warehouse else { sql_bail!( "invalid CONNECTION: ICEBERG s3tablesrest connections must specify WAREHOUSE" @@ -753,6 +761,18 @@ impl ConnectionOptionExtracted { "invalid CONNECTION: ICEBERG GCP CONNECTION does not support ACCESS DELEGATION" ); } + // A GCP connection authenticates to GCS, so the + // store is already determined. Reject a + // contradicting value rather than silently + // overriding it. + if let Some(provider) = self.storage_provider + && provider != IcebergStorageProvider::Gcs + { + sql_bail!( + "invalid CONNECTION: ICEBERG GCP CONNECTION implies STORAGE PROVIDER 'gcs', not '{}'", + provider.as_str() + ); + } /// All BigLake Iceberg REST Catalogs use the same catalog URI. const BIGLAKE_CATALOG_URI: &str = "https://biglake.googleapis.com/iceberg/v1/restcatalog"; @@ -769,10 +789,19 @@ impl ConnectionOptionExtracted { ), }; + // A GCP connection only authenticates to GCS, so it + // fixes the store regardless of what was written. + let storage_provider = if matches!(auth, IcebergCatalogAuth::Gcp(_)) { + IcebergStorageProvider::Gcs + } else { + self.storage_provider.unwrap_or_default() + }; + IcebergCatalogImpl::Rest(RestIcebergCatalog { auth, warehouse, access_delegation: self.access_delegation, + storage_provider, }) } }; diff --git a/src/sql/src/plan/with_options.rs b/src/sql/src/plan/with_options.rs index 2c7342b0ae7b6..0bf582c02fb05 100644 --- a/src/sql/src/plan/with_options.rs +++ b/src/sql/src/plan/with_options.rs @@ -21,7 +21,9 @@ use mz_sql_parser::ast::{ NetworkPolicyRuleDefinition, RefreshOptionValue, ReplicaDefinition, }; use mz_storage_types::connections::string_or_secret::StringOrSecret; -use mz_storage_types::connections::{IcebergAccessDelegation, IcebergCatalogType}; +use mz_storage_types::connections::{ + IcebergAccessDelegation, IcebergCatalogType, IcebergStorageProvider, +}; use serde::{Deserialize, Serialize}; use crate::ast::{AstInfo, UnresolvedItemName, Value, WithOptionValue}; @@ -68,6 +70,33 @@ impl ImpliedValue for IcebergCatalogType { } } +impl TryFromValue> for IcebergStorageProvider { + fn try_from_value(v: WithOptionValue) -> Result { + match String::try_from_value(v)? { + s if s.eq_ignore_ascii_case("s3") => Ok(IcebergStorageProvider::S3), + s if s.eq_ignore_ascii_case("gcs") => Ok(IcebergStorageProvider::Gcs), + s if s.eq_ignore_ascii_case("adls") => Ok(IcebergStorageProvider::Adls), + _ => sql_bail!("invalid iceberg storage provider, expected 's3', 'gcs', or 'adls'"), + } + } + + fn try_into_value(self, _catalog: &dyn SessionCatalog) -> Option> { + Some(WithOptionValue::Value(Value::String( + self.as_str().to_string(), + ))) + } + + fn name() -> String { + "iceberg storage provider".to_string() + } +} + +impl ImpliedValue for IcebergStorageProvider { + fn implied_value() -> Result { + sql_bail!("must provide an iceberg storage provider") + } +} + impl TryFromValue> for IcebergAccessDelegation { fn try_from_value(v: WithOptionValue) -> Result { match String::try_from_value(v)? { diff --git a/src/storage-types/src/connections.rs b/src/storage-types/src/connections.rs index aa48f6a599c23..628af8478aba5 100644 --- a/src/storage-types/src/connections.rs +++ b/src/storage-types/src/connections.rs @@ -671,6 +671,11 @@ pub struct RestIcebergCatalog { /// a catalog can reject our whole request, /// even if we have our own storage credentials to fall back on. pub access_delegation: Option, + /// Which object store the catalog's tables live in. + /// + /// Defaults to S3. A REST catalog does not tell us this, so a table backed + /// by GCS or ADLS is unreadable until the connection says so. + pub storage_provider: IcebergStorageProvider, } /// The value Materialize sends in the Iceberg REST `X-Iceberg-Access-Delegation` @@ -725,6 +730,7 @@ impl IntoInlineConnection auth: self.auth.into_inline_connection(&r), warehouse: self.warehouse, access_delegation: self.access_delegation, + storage_provider: self.storage_provider, } } } @@ -746,6 +752,36 @@ pub enum IcebergCatalogType { S3TablesRest, } +/// Which object store holds the data files of a REST catalog's tables. +/// +/// The catalog protocol says nothing about this: a REST catalog hands back +/// storage locations and credentials, and the client has to already know how to +/// talk to that store. So it is configured per connection rather than +/// discovered. +#[derive(Clone, Copy, Debug, Eq, PartialEq, Hash, Serialize, Deserialize)] +pub enum IcebergStorageProvider { + S3, + Gcs, + Adls, +} + +impl IcebergStorageProvider { + /// The name as spelled in SQL. + pub fn as_str(&self) -> &'static str { + match self { + IcebergStorageProvider::S3 => "s3", + IcebergStorageProvider::Gcs => "gcs", + IcebergStorageProvider::Adls => "adls", + } + } +} + +impl Default for IcebergStorageProvider { + fn default() -> Self { + IcebergStorageProvider::S3 + } +} + #[derive(Clone, Debug, Eq, PartialEq, Hash, Serialize, Deserialize)] pub enum IcebergCatalogImpl { Rest(RestIcebergCatalog), @@ -1109,14 +1145,25 @@ impl IcebergCatalogConnection { }; ( - OpenDalStorageFactory::S3 { + // The catalog tells us where the data lives but not what + // kind of store it is, so the connection has to say. + match rest.storage_provider { // When used with MinIO, Polaris returns a config with: // s3.access-key-id, s3.secret-access-key, s3.endpoint, ... // `iceberg-rust` forwards these props to `opendal`. When the catalog // vends instead, it returns per-table `storage-credentials` that // `iceberg-rust` wires into the same FileIO. // N.B. This is not confirmed to work with other catalog & storage implementations. - customized_credential_load, + IcebergStorageProvider::S3 => OpenDalStorageFactory::S3 { + customized_credential_load, + }, + // Both take their credentials from the catalog's + // config, which `iceberg-rust` forwards to `opendal` + // the same way. Neither has an equivalent of the S3 + // credential loader, so vended credentials for these + // stores only work through those props. + IcebergStorageProvider::Gcs => OpenDalStorageFactory::Gcs, + IcebergStorageProvider::Adls => OpenDalStorageFactory::Azdls, }, // NOTE: We construct our own OAuth authenticator for the Catalog client instead of using the one built in. // This means we ignore auth overrides from `/v1/config` (e.g. `oauth2-server-uri`). diff --git a/test/iceberg/mzcompose.py b/test/iceberg/mzcompose.py index d0da220de564e..f681670208f37 100644 --- a/test/iceberg/mzcompose.py +++ b/test/iceberg/mzcompose.py @@ -130,6 +130,19 @@ def workflow_oauth2_server_url(c: Composition) -> None: ) +def workflow_storage_provider(c: Composition) -> None: + """STORAGE PROVIDER names the object store behind a REST catalog's tables. + The catalog protocol does not report it, so a table on GCS or ADLS is + unreadable until the connection says so. This exercises connection planning + only and needs no Iceberg backend.""" + c.down(destroy_volumes=True) + c.up("materialized") + + c.run_testdrive_files( + "storage-provider.td", + ) + + def workflow_mode_append(c: Composition) -> None: key = _setup(c) diff --git a/test/iceberg/storage-provider.td b/test/iceberg/storage-provider.td new file mode 100644 index 0000000000000..b09a99efdb5e6 --- /dev/null +++ b/test/iceberg/storage-provider.td @@ -0,0 +1,100 @@ +# Copyright Materialize, Inc. and contributors. All rights reserved. +# +# Use of this software is governed by the Business Source License +# included in the LICENSE file at the root of this repository. +# +# As of the Change Date specified in that file, in accordance with +# the Business Source License, use of this software will be governed +# by the Apache License, Version 2.0. + +# STORAGE PROVIDER names the object store holding a REST catalog's data files. +# The catalog protocol does not report this: it hands back locations and +# credentials and expects the client to already know how to reach that store. +# +# Everything asserted here is decided at plan time, so skip the +# connect-on-create validation and point at hosts that do not exist. +$ postgres-execute connection=postgres://mz_system:materialize@${testdrive.materialize-internal-sql-addr} +ALTER SYSTEM SET enable_default_connection_validation = false + +> CREATE SECRET sp_cred AS 'client-id:client-secret' + +# Omitting the option leaves the default of S3 in place. It is not echoed back, +# so an unset provider stays unset in the catalog rather than being normalized +# to an explicit 's3'. +> CREATE CONNECTION sp_default TO ICEBERG CATALOG ( + CATALOG TYPE = 'REST', + URL = 'https://example.invalid/api/catalog', + CREDENTIAL = SECRET sp_cred, + WAREHOUSE = 'wh' + ); + +> SHOW CREATE CONNECTION sp_default +name create_sql +--------------------------------- +materialize.public.sp_default "CREATE CONNECTION materialize.public.sp_default TO ICEBERG CATALOG (CATALOG TYPE = 'rest', CREDENTIAL = SECRET materialize.public.sp_cred, URL = 'https://example.invalid/api/catalog', WAREHOUSE = 'wh');" + +# Each accepted value round-trips through the catalog. +> CREATE CONNECTION sp_s3 TO ICEBERG CATALOG ( + CATALOG TYPE = 'REST', + URL = 'https://example.invalid/api/catalog', + CREDENTIAL = SECRET sp_cred, + STORAGE PROVIDER = 's3', + WAREHOUSE = 'wh' + ); + +> CREATE CONNECTION sp_gcs TO ICEBERG CATALOG ( + CATALOG TYPE = 'REST', + URL = 'https://example.invalid/api/catalog', + CREDENTIAL = SECRET sp_cred, + STORAGE PROVIDER = 'gcs', + WAREHOUSE = 'wh' + ); + +> CREATE CONNECTION sp_adls TO ICEBERG CATALOG ( + CATALOG TYPE = 'REST', + URL = 'https://example.invalid/api/catalog', + CREDENTIAL = SECRET sp_cred, + STORAGE PROVIDER = 'adls', + WAREHOUSE = 'wh' + ); + +> SELECT name FROM mz_connections WHERE name LIKE 'sp\_%' ORDER BY name +sp_adls +sp_default +sp_gcs +sp_s3 + +# The value is case-insensitive, like CATALOG TYPE. +> CREATE CONNECTION sp_upper TO ICEBERG CATALOG ( + CATALOG TYPE = 'REST', + URL = 'https://example.invalid/api/catalog', + CREDENTIAL = SECRET sp_cred, + STORAGE PROVIDER = 'GCS', + WAREHOUSE = 'wh' + ); + +! CREATE CONNECTION sp_bad TO ICEBERG CATALOG ( + CATALOG TYPE = 'REST', + URL = 'https://example.invalid/api/catalog', + CREDENTIAL = SECRET sp_cred, + STORAGE PROVIDER = 'azure', + WAREHOUSE = 'wh' + ); +contains:invalid iceberg storage provider, expected 's3', 'gcs', or 'adls' + +# S3 Tables is always backed by S3, so the option is meaningless there rather +# than merely redundant. +! CREATE CONNECTION sp_on_s3tables TO ICEBERG CATALOG ( + CATALOG TYPE = 's3tablesrest', + URL = 'https://example.invalid/api/catalog', + STORAGE PROVIDER = 's3', + WAREHOUSE = 'wh' + ); +contains:s3tablesrest connections do not support STORAGE PROVIDER + +> DROP CONNECTION sp_upper +> DROP CONNECTION sp_adls +> DROP CONNECTION sp_gcs +> DROP CONNECTION sp_s3 +> DROP CONNECTION sp_default +> DROP SECRET sp_cred From 8ceeee9a3a254a5968ff05fa7efdf9b73f0f93fd Mon Sep 17 00:00:00 2001 From: Patrick Butler Date: Thu, 10 Sep 2026 17:12:05 -0400 Subject: [PATCH 08/11] SS-470 Add support for vended credentials for GCS backed Iceberg catalogs (#38733) Updates cargo.toml to point at the new revision of our iceberg fork, containing the custom credential loader support for GCS. Adds logic to opendal storage interface construction to build custom credential loaders for GCS when the access delegation option is specified, and removes the restriction on using `ACCESS DELEGATION` when creating a GCP biglake iceberg catalog connection. --- Cargo.lock | 7 +- Cargo.toml | 6 +- src/sql/src/plan/statement/ddl/connection.rs | 5 - src/storage-types/src/connections.rs | 191 ++++++++--- .../src/connections/iceberg_credentials.rs | 322 ++++++++++++++---- 5 files changed, 416 insertions(+), 115 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8709869696b74..629bcc067ae1e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4932,7 +4932,7 @@ dependencies = [ [[package]] name = "iceberg" version = "0.10.1" -source = "git+https://github.com/MaterializeInc/iceberg-rust.git?rev=958ac5b3c318026aeb362485c593e4956aeabe95#958ac5b3c318026aeb362485c593e4956aeabe95" +source = "git+https://github.com/MaterializeInc/iceberg-rust.git?rev=985929c6c5cc7083bac30c881e7ec43ec4d57b64#985929c6c5cc7083bac30c881e7ec43ec4d57b64" dependencies = [ "aes-gcm", "anyhow", @@ -4989,7 +4989,7 @@ dependencies = [ [[package]] name = "iceberg-catalog-rest" version = "0.10.1" -source = "git+https://github.com/MaterializeInc/iceberg-rust.git?rev=958ac5b3c318026aeb362485c593e4956aeabe95#958ac5b3c318026aeb362485c593e4956aeabe95" +source = "git+https://github.com/MaterializeInc/iceberg-rust.git?rev=985929c6c5cc7083bac30c881e7ec43ec4d57b64#985929c6c5cc7083bac30c881e7ec43ec4d57b64" dependencies = [ "async-trait", "chrono", @@ -5008,7 +5008,7 @@ dependencies = [ [[package]] name = "iceberg-storage-opendal" version = "0.10.1" -source = "git+https://github.com/MaterializeInc/iceberg-rust.git?rev=958ac5b3c318026aeb362485c593e4956aeabe95#958ac5b3c318026aeb362485c593e4956aeabe95" +source = "git+https://github.com/MaterializeInc/iceberg-rust.git?rev=985929c6c5cc7083bac30c881e7ec43ec4d57b64#985929c6c5cc7083bac30c881e7ec43ec4d57b64" dependencies = [ "anyhow", "async-trait", @@ -5019,6 +5019,7 @@ dependencies = [ "opendal", "reqsign-aws-v4", "reqsign-core", + "reqsign-google", "serde", "typetag", "url", diff --git a/Cargo.toml b/Cargo.toml index 1de960d735215..27422005f8eb2 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -676,9 +676,9 @@ async-compression = { git = "https://github.com/MaterializeInc/async-compression # NOTE: The `[workspace.dependencies]` version requirement above must stay # semver-compatible with this revision's crate version, otherwise Cargo drops # these patches into `[[patch.unused]]` and silently builds against crates.io. -iceberg = { git = "https://github.com/MaterializeInc/iceberg-rust.git", rev = "958ac5b3c318026aeb362485c593e4956aeabe95" } -iceberg-catalog-rest = { git = "https://github.com/MaterializeInc/iceberg-rust.git", rev = "958ac5b3c318026aeb362485c593e4956aeabe95" } -iceberg-storage-opendal = { git = "https://github.com/MaterializeInc/iceberg-rust.git", rev = "958ac5b3c318026aeb362485c593e4956aeabe95" } +iceberg = { git = "https://github.com/MaterializeInc/iceberg-rust.git", rev = "985929c6c5cc7083bac30c881e7ec43ec4d57b64" } +iceberg-catalog-rest = { git = "https://github.com/MaterializeInc/iceberg-rust.git", rev = "985929c6c5cc7083bac30c881e7ec43ec4d57b64" } +iceberg-storage-opendal = { git = "https://github.com/MaterializeInc/iceberg-rust.git", rev = "985929c6c5cc7083bac30c881e7ec43ec4d57b64" } # Custom duckdb crate to support mz needs # All changes should go to the `mz_changes` branch. diff --git a/src/sql/src/plan/statement/ddl/connection.rs b/src/sql/src/plan/statement/ddl/connection.rs index eccbfed25b732..1ec8ed6b178f4 100644 --- a/src/sql/src/plan/statement/ddl/connection.rs +++ b/src/sql/src/plan/statement/ddl/connection.rs @@ -756,11 +756,6 @@ impl ConnectionOptionExtracted { "invalid CONNECTION: OAUTH2 SERVER URL applies to CREDENTIAL auth, not GCP CONNECTION" ); } - if self.access_delegation.is_some() { - sql_bail!( - "invalid CONNECTION: ICEBERG GCP CONNECTION does not support ACCESS DELEGATION" - ); - } // A GCP connection authenticates to GCS, so the // store is already determined. Reject a // contradicting value rather than silently diff --git a/src/storage-types/src/connections.rs b/src/storage-types/src/connections.rs index 628af8478aba5..036536d0abb62 100644 --- a/src/storage-types/src/connections.rs +++ b/src/storage-types/src/connections.rs @@ -37,7 +37,8 @@ use iceberg_catalog_rest::{ RestCatalogBuilder, TokenProvider, }; use iceberg_storage_opendal::{ - AwsCredential, CustomAwsCredentialLoader, OpenDalStorageFactory, ProvideCredential, + AwsCredential, CustomAwsCredentialLoader, CustomGcsCredentialLoader, OpenDalStorageFactory, + ProvideCredential, }; use itertools::Itertools; use mz_ccsr::tls::{Certificate, Identity}; @@ -103,9 +104,11 @@ pub mod string_or_secret; const OAUTH2_PARAM_SCOPE: &str = "scope"; const REST_CATALOG_PROP_OAUTH2_SERVER_URI: &str = "oauth2-server-uri"; -/// Requests catalog-vended storage credentials. `iceberg-rust` turns `header.*` catalog -/// properties into headers on every REST request, the same way the Iceberg Java client -/// carries this one. +/// The prefix marking a catalog property that `iceberg-rust` turns into a header on every REST +/// request, the same convention the Iceberg Java client uses. +const REST_CATALOG_HEADER_PROP_PREFIX: &str = "header."; +/// Requests catalog-vended storage credentials, carried as a header by way of +/// [`REST_CATALOG_HEADER_PROP_PREFIX`]. const REST_CATALOG_PROP_ACCESS_DELEGATION: &str = "header.X-Iceberg-Access-Delegation"; /// A credential loader that wraps an aws-sdk-rust credentials provider for use with @@ -1018,6 +1021,79 @@ impl IcebergCatalogConnection { Ok(Arc::new(catalog)) } + /// Collects the headers `iceberg-rust` puts on every catalog request out of the `header.*` + /// props it takes them from. + /// + /// The credential endpoints Materialize calls directly bypass the catalog client, so without + /// this they would reach the same server missing headers it may require, `x-goog-user-project` + /// on a GCP-hosted catalog among them. + fn catalog_headers(props: &BTreeMap) -> Result { + props + .iter() + .filter_map(|(k, v)| { + k.strip_prefix(REST_CATALOG_HEADER_PROP_PREFIX) + .map(|name| (name, v)) + }) + .map(|(name, value)| { + let name = HeaderName::try_from(name) + .with_context(|| format!("invalid Iceberg catalog header name: {name}"))?; + let value = HeaderValue::try_from(value) + .with_context(|| format!("invalid Iceberg catalog header value for {name}"))?; + Ok((name, value)) + }) + .collect() + } + + /// Resolves the endpoint that vends storage credentials for `table`, or `None` if this + /// connection has no use for one. + /// + /// A loader built on the returned endpoint takes sole responsibility for storage credentials, + /// so `None` is the signal to leave the catalog's static `storage-credentials` props in force. + /// It means either that the connection did not ask for delegation, or that the caller named no + /// table, and the specification scopes vended credentials to a single table. + async fn vended_credential_endpoint( + &self, + rest: &RestIcebergCatalog, + client: &reqwest::Client, + token: &Arc, + headers: &HeaderMap, + table: Option<&TableIdent>, + ) -> Result, anyhow::Error> { + match (&rest.access_delegation, table) { + (Some(IcebergAccessDelegation::VendedCredentials), Some(table)) => Ok(Some( + iceberg_credentials::table_credentials_endpoint( + &self.uri, + client, + token, + headers, + rest.warehouse.as_deref(), + table, + ) + .await?, + )), + _ => Ok(None), + } + } + + /// Builds a GCS storage factory, refreshing vended credentials from `endpoint` if there is one. + fn gcs_storage_factory( + endpoint: Option, + client: &reqwest::Client, + token: &Arc, + headers: &HeaderMap, + ) -> OpenDalStorageFactory { + OpenDalStorageFactory::Gcs { + customized_credential_load: endpoint.map(|endpoint| { + CustomGcsCredentialLoader::new(iceberg_credentials::VendedCredentialLoader::new( + client.clone(), + endpoint, + Arc::clone(token), + headers.clone(), + )) + }), + } + } + async fn connect_rest( &self, rest: &RestIcebergCatalog, @@ -1118,31 +1194,10 @@ impl IcebergCatalogConnection { oauth_params.into_iter().collect(), )); - // Installing a loader hands it sole responsibility for S3 credentials: OpenDAL - // replaces its whole provider chain, including the static keys parsed out of the - // catalog's vended `storage-credentials` props. So only install one when the - // connection asked for delegation and we know which table to refresh, and let the - // static props serve every other case. - let customized_credential_load = match (&rest.access_delegation, table) { - (Some(IcebergAccessDelegation::VendedCredentials), Some(table)) => { - let endpoint = iceberg_credentials::table_credentials_endpoint( - &self.uri, - &client, - &token, - rest.warehouse.as_deref(), - table, - ) - .await?; - Some(CustomAwsCredentialLoader::new( - iceberg_credentials::VendedCredentialLoader::new( - client.clone(), - endpoint, - Arc::clone(&token), - ), - )) - } - _ => None, - }; + let headers = Self::catalog_headers(&props)?; + let endpoint = self + .vended_credential_endpoint(rest, &client, &token, &headers, table) + .await?; ( // The catalog tells us where the data lives but not what @@ -1155,14 +1210,26 @@ impl IcebergCatalogConnection { // `iceberg-rust` wires into the same FileIO. // N.B. This is not confirmed to work with other catalog & storage implementations. IcebergStorageProvider::S3 => OpenDalStorageFactory::S3 { - customized_credential_load, + customized_credential_load: endpoint.map(|endpoint| { + CustomAwsCredentialLoader::new( + iceberg_credentials::VendedCredentialLoader::new( + client.clone(), + endpoint, + Arc::clone(&token), + headers.clone(), + ), + ) + }), }, - // Both take their credentials from the catalog's - // config, which `iceberg-rust` forwards to `opendal` - // the same way. Neither has an equivalent of the S3 - // credential loader, so vended credentials for these - // stores only work through those props. - IcebergStorageProvider::Gcs => OpenDalStorageFactory::Gcs, + IcebergStorageProvider::Gcs => { + Self::gcs_storage_factory(endpoint, &client, &token, &headers) + } + // ADLS takes its credentials from the catalog's config, which + // `iceberg-rust` forwards to `opendal` the same way. OpenDAL's Azure + // service does not go through reqsign's credential provider + // abstraction, so there is no hook to wrap: vended credentials for + // ADLS work only through those static props, and stop working when + // they expire. IcebergStorageProvider::Adls => OpenDalStorageFactory::Azdls, }, // NOTE: We construct our own OAuth authenticator for the Catalog client instead of using the one built in. @@ -1193,18 +1260,31 @@ impl IcebergCatalogConnection { ); } + // The service account authenticates catalog requests whether or not the catalog + // vends storage credentials, and doubles as the token source for refreshing them. + let token: Arc = Arc::new(GcpTokenProvider { service_account }); + let headers = Self::catalog_headers(&props)?; + let endpoint = self + .vended_credential_endpoint(rest, &client, &token, &headers, table) + .await?; + ( - OpenDalStorageFactory::Gcs, - Some(iceberg_catalog_rest::BearerTokenAuthenticator::new( - Arc::new(GcpTokenProvider { service_account }), - )), + // A GCP-hosted catalog vends GCS credentials, so the storage provider is not + // in question here the way it is for a generic REST catalog. + // + // NOTE: with delegation the service account key above stops governing storage + // and only authenticates the catalog. That is not a change in precedence: + // OpenDAL already preferred the vended token over a credential file, and a + // loader only keeps that token from expiring. + Self::gcs_storage_factory(endpoint, &client, &token, &headers), + Some(iceberg_catalog_rest::BearerTokenAuthenticator::new(token)), ) } }; - // `iceberg-rust` turns `header.*` props into headers on every REST request, so - // connection asked for delegation. - // than falling back to their configured storage credentials. + // Inserted after the storage factory is built, so that the loaders above carry only the + // headers the catalog client would send on an ordinary request. Each adds this one itself, + // since the credentials endpoint is the one request that always asks for delegation. if let Some(delegation) = &rest.access_delegation { props.insert( REST_CATALOG_PROP_ACCESS_DELEGATION.to_string(), @@ -3540,6 +3620,31 @@ impl AwsPrivatelinkConnection { mod tests { use super::*; + #[mz_ore::test] + fn test_catalog_headers() { + let props = BTreeMap::from_iter( + [ + (REST_CATALOG_PROP_URI, "https://catalog.example"), + (REST_CATALOG_PROP_WAREHOUSE, "wh"), + ("header.x-goog-user-project", "some-project"), + (REST_CATALOG_PROP_ACCESS_DELEGATION, "vended-credentials"), + ] + .map(|(k, v)| (k.to_string(), v.to_string())), + ); + + // Only `header.*` props become headers, and the prefix is stripped. Header names are + // matched case-insensitively, so the delegation prop's mixed-case spelling still lands. + let headers = IcebergCatalogConnection::catalog_headers(&props).expect("valid headers"); + assert_eq!(headers.len(), 2); + assert_eq!(headers["x-goog-user-project"], "some-project"); + assert_eq!(headers["x-iceberg-access-delegation"], "vended-credentials"); + + // A prop whose name is not a legal header is an error rather than a dropped header: it + // would otherwise mean silently talking to the catalog differently than asked. + let props = BTreeMap::from([("header.bad name".to_string(), "v".to_string())]); + assert!(IcebergCatalogConnection::catalog_headers(&props).is_err()); + } + #[mz_ore::test] fn test_check_service_name() { // Customer-owned and AWS-managed endpoint services are both accepted, diff --git a/src/storage-types/src/connections/iceberg_credentials.rs b/src/storage-types/src/connections/iceberg_credentials.rs index 0c446e8890bcc..bc839385e8240 100644 --- a/src/storage-types/src/connections/iceberg_credentials.rs +++ b/src/storage-types/src/connections/iceberg_credentials.rs @@ -21,14 +21,16 @@ //! builds the catalog but keeps it private, so this module asks the server directly. use std::collections::BTreeMap; +use std::fmt::Debug; use std::sync::Arc; use std::time::{Duration, Instant, SystemTime}; use anyhow::{Context, anyhow}; +use http::{HeaderMap, HeaderName, HeaderValue}; use iceberg::TableIdent; -use iceberg::io::{S3_ACCESS_KEY_ID, S3_SECRET_ACCESS_KEY, S3_SESSION_TOKEN}; +use iceberg::io::{GCS_TOKEN, S3_ACCESS_KEY_ID, S3_SECRET_ACCESS_KEY, S3_SESSION_TOKEN}; use iceberg_catalog_rest::{StorageCredential, TokenProvider}; -use iceberg_storage_opendal::{AwsCredential, ProvideCredential}; +use iceberg_storage_opendal::{AwsCredential, GcsCredential, GcsToken, ProvideCredential}; use mz_ore::error::ErrorExt; use reqsign_core::time::Timestamp; use reqwest::StatusCode; @@ -41,49 +43,152 @@ use crate::connections::IcebergAccessDelegation; /// The `X-Iceberg-Access-Delegation` header, spelled for requests Materialize issues itself /// rather than through the catalog client, which takes it as a `header.*` catalog property. -const ICEBERG_ACCESS_DELEGATION_HEADER: &str = "X-Iceberg-Access-Delegation"; +const ICEBERG_ACCESS_DELEGATION_HEADER: HeaderName = + HeaderName::from_static("x-iceberg-access-delegation"); /// Property name for the most common way catalogs report when vended S3 credentials expire. /// Catalogs are not required to send it. const S3_SESSION_TOKEN_EXPIRES_AT_MS: &str = "s3.session-token-expires-at-ms"; +/// Property name for when a vended GCS token expires, in epoch milliseconds. +/// +/// NOTE: no `-ms` suffix, unlike [`S3_SESSION_TOKEN_EXPIRES_AT_MS`], even though both carry +/// milliseconds. The spelling comes from Iceberg's `GCPProperties`, not from analogy with the S3 +/// property. +const GCS_TOKEN_EXPIRES_AT: &str = "gcs.oauth2.token-expires-at"; + /// How far ahead of a reported expiry to re-fetch a vended credential. +/// +/// NOTE: must stay comfortably above reqsign's own refresh buffers, which are 120s for a GCS +/// token. A GCS token handed back inside that window fails signing outright, because a vended +/// credential carries no service account for reqsign to fall back on. const VENDED_CREDENTIAL_REFRESH_BUFFER: Duration = Duration::from_secs(900); /// How long to trust a vended credential that reports no expiry. /// -/// A catalog that omits [`S3_SESSION_TOKEN_EXPIRES_AT_MS`] leaves nothing to schedule against, and -/// a credential held past its real lifetime fails every S3 request until the dataflow restarts. So +/// A catalog that omits the expiry property leaves nothing to schedule against, and a credential +/// held past its real lifetime fails every storage request until the dataflow restarts. So /// re-fetch on a short interval instead: each one is a single REST call against a credential the /// sink is already using. // TODO SS-449: make this a dyncfg const VENDED_CREDENTIAL_DEFAULT_TTL: Duration = Duration::from_secs(300); +/// A storage credential that a REST catalog can vend. +/// +/// Implementors carry the object store's notion of a credential and know which +/// `storage-credentials` properties encode it. Everything else about vending, including the fetch, +/// the caching, and the refresh schedule, is shared by [`VendedCredentialLoader`]. +pub(super) trait VendedCredential: Clone + Debug + Send + Sync + Unpin + 'static { + /// The object store this credential authenticates against, for diagnostics. + const STORE: &'static str; + + /// Builds the credential from one entry of a catalog's `storage-credentials`. + fn from_vended(vended: &StorageCredential) -> reqsign_core::Result; + + /// When the credential expires, if the catalog reported it. + fn expires_at(&self) -> Option; +} + +impl VendedCredential for AwsCredential { + const STORE: &'static str = "S3"; + + fn from_vended(vended: &StorageCredential) -> reqsign_core::Result { + Ok(AwsCredential { + access_key_id: required(vended, S3_ACCESS_KEY_ID)?, + secret_access_key: required(vended, S3_SECRET_ACCESS_KEY)?, + session_token: vended.config.get(S3_SESSION_TOKEN).cloned(), + expires_in: vended_expires_at(vended, S3_SESSION_TOKEN_EXPIRES_AT_MS)?, + }) + } + + fn expires_at(&self) -> Option { + self.expires_in + } +} + +impl VendedCredential for GcsCredential { + const STORE: &'static str = "GCS"; + + fn from_vended(vended: &StorageCredential) -> reqsign_core::Result { + Ok(GcsCredential::with_token(GcsToken { + access_token: required(vended, GCS_TOKEN)?, + expires_at: vended_expires_at(vended, GCS_TOKEN_EXPIRES_AT)?, + })) + } + + fn expires_at(&self) -> Option { + self.token.as_ref().and_then(|token| token.expires_at) + } +} + +/// Reads a property a vended credential cannot do without. +fn required(vended: &StorageCredential, prop: &str) -> reqsign_core::Result { + vended.config.get(prop).cloned().ok_or_else(|| { + reqsign_core::Error::credential_invalid(format!( + "vended Iceberg storage credential for prefix {} is missing {prop}", + vended.prefix + )) + }) +} + +/// Reads an expiry property, which catalogs are not required to send. +fn vended_expires_at( + vended: &StorageCredential, + prop: &str, +) -> reqsign_core::Result> { + let Some(raw) = vended.config.get(prop) else { + debug!( + prefix = vended.prefix, + prop, "Iceberg catalog vended a storage credential with no expiry" + ); + return Ok(None); + }; + let millis = raw.parse::().map_err(|e| { + reqsign_core::Error::credential_invalid(format!( + "vended Iceberg storage credential for prefix {} has an unparseable {prop}", + vended.prefix + )) + .with_source(e) + })?; + Ok(Some(Timestamp::from_millisecond(millis)?)) +} + #[derive(Debug)] -pub(super) struct VendedCredentialLoader { +pub(super) struct VendedCredentialLoader { client: reqwest::Client, credential_endpoint: Url, token: Arc, - cached: Mutex>, + /// Every header this request needs beyond the bearer token, resolved once. + headers: HeaderMap, + cached: Mutex>, } -impl VendedCredentialLoader { +impl VendedCredentialLoader { + /// `headers` are the headers the catalog client puts on its own requests, which this one + /// bypasses. The delegation header is added here rather than expected in `headers`, since + /// asking the catalog to vend is the whole point of this request. pub(super) fn new( client: reqwest::Client, credential_endpoint: Url, token: Arc, + mut headers: HeaderMap, ) -> Self { + headers.insert( + ICEBERG_ACCESS_DELEGATION_HEADER, + HeaderValue::from_static(IcebergAccessDelegation::VendedCredentials.as_header_value()), + ); Self { client, credential_endpoint, token, + headers, cached: Mutex::new(None), } } /// Fetches a fresh credential from the catalog, paired with the instant at which it should be /// re-fetched. - async fn fetch(&self) -> reqsign_core::Result<(AwsCredential, Instant)> { + async fn fetch(&self) -> reqsign_core::Result<(C, Instant)> { let token = self.token.token().await.map_err(|e| { reqsign_core::Error::credential_invalid( "failed to obtain a catalog token for vended Iceberg storage credentials", @@ -95,10 +200,7 @@ impl VendedCredentialLoader { .client .get(self.credential_endpoint.clone()) .bearer_auth(token) - .header( - ICEBERG_ACCESS_DELEGATION_HEADER, - IcebergAccessDelegation::VendedCredentials.as_header_value(), - ) + .headers(self.headers.clone()) .send() .await .map_err(|e| { @@ -149,7 +251,7 @@ impl VendedCredentialLoader { "Iceberg catalog vended multiple storage credentials; using the longest prefix" ); } - let credential = response + let vended = response .storage_credentials .into_iter() .max_by_key(|credential| credential.prefix.len()) @@ -160,53 +262,14 @@ impl VendedCredentialLoader { )) })?; - let missing = |prop: &str| { - reqsign_core::Error::credential_invalid(format!( - "vended Iceberg storage credential for prefix {} is missing {prop}", - credential.prefix - )) - }; - let access_key_id = credential - .config - .get(S3_ACCESS_KEY_ID) - .ok_or_else(|| missing(S3_ACCESS_KEY_ID))? - .clone(); - let secret_access_key = credential - .config - .get(S3_SECRET_ACCESS_KEY) - .ok_or_else(|| missing(S3_SECRET_ACCESS_KEY))? - .clone(); - - let expires_in = credential - .config - .get(S3_SESSION_TOKEN_EXPIRES_AT_MS) - .map(|raw| { - let millis = raw.parse::().map_err(|e| { - reqsign_core::Error::credential_invalid(format!( - "vended Iceberg storage credential for prefix {} has an unparseable \ - {S3_SESSION_TOKEN_EXPIRES_AT_MS}", - credential.prefix - )) - .with_source(e) - })?; - Timestamp::from_millisecond(millis) - }) - .transpose()?; - - Ok(( - AwsCredential { - access_key_id, - secret_access_key, - session_token: credential.config.get(S3_SESSION_TOKEN).cloned(), - expires_in, - }, - refresh_deadline(expires_in), - )) + let credential = C::from_vended(&vended)?; + let refresh_at = refresh_deadline(credential.expires_at()); + Ok((credential, refresh_at)) } } -impl ProvideCredential for VendedCredentialLoader { - type Credential = AwsCredential; +impl ProvideCredential for VendedCredentialLoader { + type Credential = C; async fn provide_credential( &self, @@ -215,7 +278,7 @@ impl ProvideCredential for VendedCredentialLoader { // The lock is deliberately held across the fetch. `create_operator` builds a fresh // OpenDAL `Operator` for every file operation, so reqsign's own credential cache never // outlives a single call and this cache is all that stands between the sink and one - // catalog round trip per S3 request. Serializing here means a stale entry costs one + // catalog round trip per storage request. Serializing here means a stale entry costs one // refetch rather than one per in-flight operation. let mut cached = self.cached.lock().await; @@ -225,7 +288,16 @@ impl ProvideCredential for VendedCredentialLoader { return Ok(Some(credential.clone())); } - let (credential, refresh_at) = self.fetch().await?; + // The chain reqsign consults reports only that credential loading failed, without a + // cause, so log the real one here or it is lost. + let (credential, refresh_at) = self.fetch().await.inspect_err(|e| { + warn!( + store = C::STORE, + endpoint = %self.credential_endpoint, + error = %e, + "failed to refresh vended Iceberg storage credentials" + ); + })?; *cached = Some((credential.clone(), refresh_at)); Ok(Some(credential)) } @@ -327,11 +399,13 @@ fn table_credentials_url( /// Resolves the REST endpoint that vends storage credentials for `table`. /// /// Takes a round trip to the catalog's `config` endpoint, because the resource path carries a -/// request prefix that only the server knows. +/// request prefix that only the server knows. `headers` are the headers the catalog client puts +/// on its own requests, which this one bypasses. pub(super) async fn table_credentials_endpoint( uri: &Url, client: &reqwest::Client, token: &Arc, + headers: &HeaderMap, warehouse: Option<&str>, table: &TableIdent, ) -> Result { @@ -344,6 +418,7 @@ pub(super) async fn table_credentials_endpoint( let response = client .get(config_endpoint.clone()) .bearer_auth(bearer) + .headers(headers.clone()) .send() .await .with_context(|| { @@ -389,6 +464,131 @@ mod tests { } } + fn vended(prefix: &str, config: &[(&str, &str)]) -> StorageCredential { + StorageCredential { + prefix: prefix.to_string(), + config: config + .iter() + .map(|(k, v)| (k.to_string(), v.to_string())) + .collect(), + } + } + + #[mz_ore::test] + fn test_aws_credential_from_vended() { + let credential = AwsCredential::from_vended(&vended( + "s3://bucket/warehouse", + &[ + (S3_ACCESS_KEY_ID, "key"), + (S3_SECRET_ACCESS_KEY, "secret"), + (S3_SESSION_TOKEN, "session"), + (S3_SESSION_TOKEN_EXPIRES_AT_MS, "1700000000000"), + ], + )) + .expect("complete credential"); + assert_eq!(credential.access_key_id, "key"); + assert_eq!(credential.secret_access_key, "secret"); + assert_eq!(credential.session_token.as_deref(), Some("session")); + assert_eq!( + credential.expires_at(), + Some(Timestamp::from_millisecond(1700000000000).unwrap()) + ); + + // A catalog need not report an expiry, and long-lived static keys have no session token. + let credential = AwsCredential::from_vended(&vended( + "s3://bucket/warehouse", + &[(S3_ACCESS_KEY_ID, "key"), (S3_SECRET_ACCESS_KEY, "secret")], + )) + .expect("credential without expiry"); + assert_eq!(credential.session_token, None); + assert_eq!(credential.expires_at(), None); + + // A credential missing either key is unusable, and says which one. + let err = AwsCredential::from_vended(&vended( + "s3://bucket/warehouse", + &[(S3_ACCESS_KEY_ID, "key")], + )) + .expect_err("incomplete credential"); + assert!( + err.to_string().contains(S3_SECRET_ACCESS_KEY), + "unexpected error: {err}" + ); + + // A malformed expiry is an error rather than a silently ignored property: treating it as + // absent would fall back to the default TTL and hide a catalog bug. + let err = AwsCredential::from_vended(&vended( + "s3://bucket/warehouse", + &[ + (S3_ACCESS_KEY_ID, "key"), + (S3_SECRET_ACCESS_KEY, "secret"), + (S3_SESSION_TOKEN_EXPIRES_AT_MS, "soon"), + ], + )) + .expect_err("unparseable expiry"); + assert!( + err.to_string().contains("unparseable"), + "unexpected error: {err}" + ); + } + + #[mz_ore::test] + fn test_gcs_credential_from_vended() { + let credential = GcsCredential::from_vended(&vended( + "gs://bucket/warehouse", + &[ + (GCS_TOKEN, "ya29.token"), + (GCS_TOKEN_EXPIRES_AT, "1700000000000"), + ], + )) + .expect("complete credential"); + // A vended credential is a bare token: reqsign has no service account to fall back on. + assert!(credential.service_account.is_none()); + let token = credential.token.as_ref().expect("token"); + assert_eq!(token.access_token, "ya29.token"); + assert_eq!( + credential.expires_at(), + Some(Timestamp::from_millisecond(1700000000000).unwrap()) + ); + + let credential = + GcsCredential::from_vended(&vended("gs://bucket/warehouse", &[(GCS_TOKEN, "tok")])) + .expect("credential without expiry"); + assert_eq!(credential.expires_at(), None); + + let err = GcsCredential::from_vended(&vended( + "gs://bucket/warehouse", + &[(GCS_TOKEN_EXPIRES_AT, "1700000000000")], + )) + .expect_err("credential without a token"); + assert!( + err.to_string().contains(GCS_TOKEN), + "unexpected error: {err}" + ); + } + + #[mz_ore::test] + fn test_refresh_deadline() { + // No reported expiry falls back to the fixed interval. + let deadline = refresh_deadline(None); + assert!(deadline > Instant::now()); + assert!(deadline <= Instant::now() + VENDED_CREDENTIAL_DEFAULT_TTL); + + // A comfortably distant expiry is refreshed a buffer ahead of it. + let lifetime = Duration::from_secs(3600); + let deadline = refresh_deadline(Some(Timestamp::now() + lifetime)); + let expected = Instant::now() + lifetime - VENDED_CREDENTIAL_REFRESH_BUFFER; + assert!( + deadline <= expected && deadline > expected - Duration::from_secs(60), + "deadline is not a buffer ahead of the expiry" + ); + + // An expiry inside the buffer, or already past, is refreshed on the next call rather + // than yielding a deadline reqsign would reject anyway. + for offset in [VENDED_CREDENTIAL_REFRESH_BUFFER / 2, Duration::from_secs(0)] { + assert!(refresh_deadline(Some(Timestamp::now() + offset)) <= Instant::now()); + } + } + #[mz_ore::test] fn test_announced_prefix() { // Overrides win, matching how the catalog client merges the two. From ac9b7bd446c730fc3ba81eca99adc5735c764f41 Mon Sep 17 00:00:00 2001 From: jazz-mo Date: Fri, 11 Sep 2026 06:43:24 +0900 Subject: [PATCH 09/11] orchestratord: allow setting a PriorityClass on environmentd and clusterd pods (#38457) ## Why The operator sets no priority class on environmentd or clusterd pods. Higher priority pods evict them when a node fills up. One eviction aborted a deploy mid-hydration. ## What Users set `environmentd.priorityClassName` and `clusterd.priorityClassName` to apply priority classes to those pods. Both values default to unset, which renders no argument and changes no pod spec. The clusterd argument requires environmentd v26.40 or newer. This matches how `clusterd.affinity` and `clusterd.tolerations` are already gated. Older environmentd versions exit on unrecognized arguments. We exclude balancerd and console because they hold no state and run multiple replicas. ## Before you set it * The values do not reach running pods until a user requests a rollout. * Kubernetes caps a user-defined class at 1000000000 and `system-cluster-critical` is 2000000000, so these pods remain vulnerable to critical preemptors. * Prefer a class with `preemptionPolicy: Never` to prevent new pods from evicting serving pods during the rollout. ## Testing Helm unit tests use `notMatchRegex` for negative assertions. The `notContains` function compares whole list elements, so it never fails against a flag rendered as `--flag=value`. The pre-existing `schedulerName` assertion has the same defect and is fixed here. An mzcompose test asserts the resolved integer priority to prove Kubernetes admitted the class. Open: I omitted the Chart.yaml version bump because a script manages it and PR #38406 omitted it, but which rule is live? --------- Co-authored-by: Jesse Luehrs --- .../materialize_operator_chart_parameter.yml | 114 +++++++++++------- misc/helm-charts/operator/README.md | 2 + .../operator/templates/deployment.yaml | 6 + .../operator/tests/deployment_test.yaml | 34 +++++- misc/helm-charts/operator/values.yaml | 8 ++ src/environmentd/src/environmentd/main.rs | 4 + src/orchestrator-kubernetes/src/lib.rs | 3 + src/orchestratord/src/bin/orchestratord.rs | 6 + .../src/controller/materialize.rs | 4 + .../src/controller/materialize/generation.rs | 20 +++ test/orchestratord/mzcompose.py | 86 +++++++++++++ test/orchestratord/priorityclass.yaml | 21 ++++ 12 files changed, 260 insertions(+), 48 deletions(-) create mode 100644 test/orchestratord/priorityclass.yaml diff --git a/doc/user/data/self_managed/materialize_operator_chart_parameter.yml b/doc/user/data/self_managed/materialize_operator_chart_parameter.yml index 62780a51224c0..fd23bc762238c 100644 --- a/doc/user/data/self_managed/materialize_operator_chart_parameter.yml +++ b/doc/user/data/self_managed/materialize_operator_chart_parameter.yml @@ -7,7 +7,7 @@ description: "" section: "" column: 3 - linenumber: 359 + linenumber: 367 dependency: "" isglobal: false - key: balancerd.defaultResources.limits @@ -19,7 +19,7 @@ description: "" section: "" column: 5 - linenumber: 368 + linenumber: 376 dependency: "" isglobal: false - key: balancerd.defaultResources.requests @@ -31,7 +31,7 @@ description: "" section: "" column: 5 - linenumber: 372 + linenumber: 380 dependency: "" isglobal: false - key: balancerd.enabled @@ -43,7 +43,7 @@ description: "" section: "" column: 3 - linenumber: 351 + linenumber: 359 dependency: "" isglobal: false - key: balancerd.nodeSelector @@ -55,7 +55,7 @@ description: "" section: "" column: 3 - linenumber: 355 + linenumber: 363 dependency: "" isglobal: false - key: balancerd.tolerations @@ -67,7 +67,7 @@ description: "" section: "" column: 3 - linenumber: 363 + linenumber: 371 dependency: "" isglobal: false - key: clusterd.affinity @@ -79,7 +79,7 @@ description: "" section: "" column: 3 - linenumber: 343 + linenumber: 347 dependency: "" isglobal: false - key: clusterd.nodeSelector @@ -91,7 +91,19 @@ description: "" section: "" column: 3 - linenumber: 327 + linenumber: 331 + dependency: "" + isglobal: false +- key: clusterd.priorityClassName + type: string + notationtype: "" + autodefault: "" + default: '`nil`' + autodescription: PriorityClass to use for clusterd pods spawned by the operator. The PriorityClass must already exist. Kubernetes rejects a pod that names one it cannot resolve, so a typo here stops these pods being created at all. + description: PriorityClass to use for clusterd pods spawned by the operator. The PriorityClass must already exist. Kubernetes rejects a pod that names one it cannot resolve, so a typo here stops these pods being created at all. + section: "" + column: 3 + linenumber: 355 dependency: "" isglobal: false - key: clusterd.scratchfsNodeSelector @@ -103,7 +115,7 @@ description: "" section: "" column: 3 - linenumber: 332 + linenumber: 336 dependency: "" isglobal: false - key: clusterd.swapNodeSelector @@ -115,7 +127,7 @@ description: "" section: "" column: 3 - linenumber: 338 + linenumber: 342 dependency: "" isglobal: false - key: clusterd.tolerations @@ -127,7 +139,7 @@ description: "" section: "" column: 3 - linenumber: 347 + linenumber: 351 dependency: "" isglobal: false - key: console.affinity @@ -139,7 +151,7 @@ description: "" section: "" column: 3 - linenumber: 388 + linenumber: 396 dependency: "" isglobal: false - key: console.defaultResources.limits @@ -151,7 +163,7 @@ description: "" section: "" column: 5 - linenumber: 397 + linenumber: 405 dependency: "" isglobal: false - key: console.defaultResources.requests @@ -163,7 +175,7 @@ description: "" section: "" column: 5 - linenumber: 401 + linenumber: 409 dependency: "" isglobal: false - key: console.enabled @@ -175,7 +187,7 @@ description: "" section: "" column: 3 - linenumber: 378 + linenumber: 386 dependency: "" isglobal: false - key: console.imageTagMapOverride @@ -187,7 +199,7 @@ description: "" section: "" column: 3 - linenumber: 380 + linenumber: 388 dependency: "" isglobal: false - key: console.nodeSelector @@ -199,7 +211,7 @@ description: "" section: "" column: 3 - linenumber: 384 + linenumber: 392 dependency: "" isglobal: false - key: console.tolerations @@ -211,7 +223,7 @@ description: "" section: "" column: 3 - linenumber: 392 + linenumber: 400 dependency: "" isglobal: false - key: environmentd.affinity @@ -235,7 +247,7 @@ description: "" section: "" column: 5 - linenumber: 313 + linenumber: 317 dependency: "" isglobal: false - key: environmentd.defaultResources.requests @@ -247,7 +259,7 @@ description: "" section: "" column: 5 - linenumber: 317 + linenumber: 321 dependency: "" isglobal: false - key: environmentd.nodeSelector @@ -262,6 +274,18 @@ linenumber: 300 dependency: "" isglobal: false +- key: environmentd.priorityClassName + type: string + notationtype: "" + autodefault: "" + default: '`nil`' + autodescription: PriorityClass to use for environmentd pods spawned by the operator. The PriorityClass must already exist. Kubernetes rejects a pod that names one it cannot resolve, so a typo here stops these pods being created at all. + description: PriorityClass to use for environmentd pods spawned by the operator. The PriorityClass must already exist. Kubernetes rejects a pod that names one it cannot resolve, so a typo here stops these pods being created at all. + section: "" + column: 3 + linenumber: 312 + dependency: "" + isglobal: false - key: environmentd.tolerations type: k8s/tolerations notationtype: k8s/tolerations @@ -283,7 +307,7 @@ description: "" section: "" column: 5 - linenumber: 461 + linenumber: 469 dependency: "" isglobal: false - key: networkPolicies.egress.enabled @@ -295,7 +319,7 @@ description: "" section: "" column: 5 - linenumber: 459 + linenumber: 467 dependency: "" isglobal: false - key: networkPolicies.enabled @@ -307,7 +331,7 @@ description: "" section: "" column: 3 - linenumber: 446 + linenumber: 454 dependency: "" isglobal: false - key: networkPolicies.ingress.cidrs @@ -319,7 +343,7 @@ description: "" section: "" column: 5 - linenumber: 455 + linenumber: 463 dependency: "" isglobal: false - key: networkPolicies.ingress.enabled @@ -331,7 +355,7 @@ description: "" section: "" column: 5 - linenumber: 453 + linenumber: 461 dependency: "" isglobal: false - key: networkPolicies.internal.enabled @@ -343,7 +367,7 @@ description: "" section: "" column: 5 - linenumber: 449 + linenumber: 457 dependency: "" isglobal: false - key: observability.enabled @@ -355,7 +379,7 @@ description: "" section: "" column: 3 - linenumber: 427 + linenumber: 435 dependency: "" isglobal: false - key: observability.podMetrics.enabled @@ -367,7 +391,7 @@ description: "" section: "" column: 5 - linenumber: 432 + linenumber: 440 dependency: "" isglobal: false - key: observability.prometheus.scrapeAnnotations.enabled @@ -379,7 +403,7 @@ description: "" section: "" column: 7 - linenumber: 436 + linenumber: 444 dependency: "" isglobal: false - key: operator.additionalMaterializeCRDColumns @@ -943,7 +967,7 @@ description: "" section: "" column: 3 - linenumber: 408 + linenumber: 416 dependency: "" isglobal: false - key: schedulerName @@ -955,7 +979,7 @@ description: Optionally use a non-default kubernetes scheduler. section: "" column: 1 - linenumber: 411 + linenumber: 419 dependency: "" isglobal: false - key: serviceAccount.annotations @@ -967,7 +991,7 @@ description: "" section: "" column: 3 - linenumber: 422 + linenumber: 430 dependency: "" isglobal: false - key: serviceAccount.create @@ -979,7 +1003,7 @@ description: "" section: "" column: 3 - linenumber: 416 + linenumber: 424 dependency: "" isglobal: false - key: serviceAccount.name @@ -991,7 +1015,7 @@ description: "" section: "" column: 3 - linenumber: 418 + linenumber: 426 dependency: "" isglobal: false - key: storage.storageClass.allowVolumeExpansion @@ -1003,7 +1027,7 @@ description: "" section: "" column: 5 - linenumber: 502 + linenumber: 510 dependency: "" isglobal: false - key: storage.storageClass.create @@ -1015,7 +1039,7 @@ description: "" section: "" column: 5 - linenumber: 488 + linenumber: 496 dependency: "" isglobal: false - key: storage.storageClass.name @@ -1027,7 +1051,7 @@ description: "" section: "" column: 5 - linenumber: 491 + linenumber: 499 dependency: "" isglobal: false - key: storage.storageClass.parameters @@ -1039,7 +1063,7 @@ description: "" section: "" column: 5 - linenumber: 497 + linenumber: 505 dependency: "" isglobal: false - key: storage.storageClass.provisioner @@ -1051,7 +1075,7 @@ description: "" section: "" column: 5 - linenumber: 494 + linenumber: 502 dependency: "" isglobal: false - key: storage.storageClass.reclaimPolicy @@ -1063,7 +1087,7 @@ description: "" section: "" column: 5 - linenumber: 503 + linenumber: 511 dependency: "" isglobal: false - key: storage.storageClass.volumeBindingMode @@ -1075,7 +1099,7 @@ description: "" section: "" column: 5 - linenumber: 504 + linenumber: 512 dependency: "" isglobal: false - key: telemetry.enabled @@ -1087,7 +1111,7 @@ description: "" section: "" column: 3 - linenumber: 439 + linenumber: 447 dependency: "" isglobal: false - key: telemetry.segmentApiKey @@ -1099,7 +1123,7 @@ description: "" section: "" column: 3 - linenumber: 440 + linenumber: 448 dependency: "" isglobal: false - key: telemetry.segmentClientSide @@ -1111,7 +1135,7 @@ description: "" section: "" column: 3 - linenumber: 441 + linenumber: 449 dependency: "" isglobal: false - key: tls.defaultCertificateSpecs @@ -1123,6 +1147,6 @@ description: "" section: "" column: 3 - linenumber: 465 + linenumber: 473 dependency: "" isglobal: false diff --git a/misc/helm-charts/operator/README.md b/misc/helm-charts/operator/README.md index 0d702cc638f45..98df0d5745ede 100644 --- a/misc/helm-charts/operator/README.md +++ b/misc/helm-charts/operator/README.md @@ -115,6 +115,7 @@ The following table lists the configurable parameters of the Materialize operato | `balancerd.tolerations` | Tolerations to use for balancerd pods spawned by the operator | ``{}`` | | `clusterd.affinity` | Affinity to use for clusterd pods spawned by the operator | ``{}`` | | `clusterd.nodeSelector` | Node selector to use for all clusterd pods spawned by the operator | ``{}`` | +| `clusterd.priorityClassName` | PriorityClass to use for clusterd pods spawned by the operator. The PriorityClass must already exist. Kubernetes rejects a pod that names one it cannot resolve, so a typo here stops these pods being created at all. | ``nil`` | | `clusterd.scratchfsNodeSelector` | Additional node selector to use for clusterd pods when using an LVM scratch disk. This will be merged with the values in `nodeSelector`. | ``{"materialize.cloud/scratch-fs": "true"}`` | | `clusterd.swapNodeSelector` | Additional node selector to use for clusterd pods when using swap. This will be merged with the values in `nodeSelector`. | ``{"materialize.cloud/swap": "true"}`` | | `clusterd.tolerations` | Tolerations to use for clusterd pods spawned by the operator | ``{}`` | @@ -129,6 +130,7 @@ The following table lists the configurable parameters of the Materialize operato | `environmentd.defaultResources.limits` | Default resource limits for environmentd's CPU and memory if not set in the Materialize CR | ``{"memory":"4Gi"}`` | | `environmentd.defaultResources.requests` | Default resources requested for environmentd's CPU and memory if not set in the Materialize CR | ``{"cpu":"1","memory":"4095Mi"}`` | | `environmentd.nodeSelector` | Node selector to use for environmentd pods spawned by the operator | ``{}`` | +| `environmentd.priorityClassName` | PriorityClass to use for environmentd pods spawned by the operator. The PriorityClass must already exist. Kubernetes rejects a pod that names one it cannot resolve, so a typo here stops these pods being created at all. | ``""`` | | `environmentd.tolerations` | Tolerations to use for environmentd pods spawned by the operator | ``{}`` | | `networkPolicies.egress.cidrs` | CIDR blocks to allow egress to | ``["0.0.0.0/0"]`` | | `networkPolicies.egress.enabled` | Whether to enable egress network policies to sources and sinks | ``false`` | diff --git a/misc/helm-charts/operator/templates/deployment.yaml b/misc/helm-charts/operator/templates/deployment.yaml index b0c21a92779a2..ef0fde436e00a 100644 --- a/misc/helm-charts/operator/templates/deployment.yaml +++ b/misc/helm-charts/operator/templates/deployment.yaml @@ -192,6 +192,9 @@ spec: - '--environmentd-toleration={{ toJson $toleration }}' {{- end }} {{- end }} + {{- if .Values.environmentd.priorityClassName }} + - "--environmentd-priority-class-name={{ .Values.environmentd.priorityClassName }}" + {{- end }} {{- if .Values.environmentd.defaultResources }} - '--environmentd-default-resources={{ toJson .Values.environmentd.defaultResources }}' {{- end }} @@ -208,6 +211,9 @@ spec: - '--clusterd-toleration={{ toJson $toleration }}' {{- end }} {{- end }} + {{- if .Values.clusterd.priorityClassName }} + - "--clusterd-priority-class-name={{ .Values.clusterd.priorityClassName }}" + {{- end }} {{- if .Values.balancerd.nodeSelector }} {{- range $key, $value := .Values.balancerd.nodeSelector }} - "--balancerd-node-selector={{ $key }}={{ $value }}" diff --git a/misc/helm-charts/operator/tests/deployment_test.yaml b/misc/helm-charts/operator/tests/deployment_test.yaml index 6ca708148d4fe..b3139e33d3df2 100644 --- a/misc/helm-charts/operator/tests/deployment_test.yaml +++ b/misc/helm-charts/operator/tests/deployment_test.yaml @@ -274,9 +274,9 @@ tests: - it: should not pass the scheduler when not configured asserts: - - notContains: - path: spec.template.spec.containers[0].args - content: "--scheduler-name" + - notMatchRegex: + path: spec.template.spec.containers[0].args[*] + pattern: "^--scheduler-name" - it: should pass the scheduler when configured set: @@ -286,6 +286,34 @@ tests: path: spec.template.spec.containers[0].args content: "--scheduler-name=my-scheduler" +- it: should not pass the environmentd priority class when not configured + asserts: + - notMatchRegex: + path: spec.template.spec.containers[0].args[*] + pattern: "^--environmentd-priority-class-name" + +- it: should pass the environmentd priority class when configured + set: + environmentd.priorityClassName: mz-environmentd-high + asserts: + - contains: + path: spec.template.spec.containers[0].args + content: "--environmentd-priority-class-name=mz-environmentd-high" + +- it: should not pass the clusterd priority class when not configured + asserts: + - notMatchRegex: + path: spec.template.spec.containers[0].args[*] + pattern: "^--clusterd-priority-class-name" + +- it: should pass the clusterd priority class when configured + set: + clusterd.priorityClassName: mz-clusterd-high + asserts: + - contains: + path: spec.template.spec.containers[0].args + content: "--clusterd-priority-class-name=mz-clusterd-high" + - it: should have license key checks disabled by default asserts: - contains: diff --git a/misc/helm-charts/operator/values.yaml b/misc/helm-charts/operator/values.yaml index 99e52e80b4ba5..7658eff4754b6 100644 --- a/misc/helm-charts/operator/values.yaml +++ b/misc/helm-charts/operator/values.yaml @@ -306,6 +306,10 @@ environmentd: # @notationType -- k8s/tolerations # @default -- ``{}`` tolerations: {} + # -- PriorityClass to use for environmentd pods spawned by the operator. The + # PriorityClass must already exist. Kubernetes rejects a pod that names one + # it cannot resolve, so a typo here stops these pods being created at all. + priorityClassName: "" # @notationType -- k8s/resources defaultResources: # -- Default resource limits for environmentd's CPU and memory @@ -345,6 +349,10 @@ clusterd: # @notationType -- k8s/tolerations # @default -- ``{}`` tolerations: {} + # -- PriorityClass to use for clusterd pods spawned by the operator. The + # PriorityClass must already exist. Kubernetes rejects a pod that names one + # it cannot resolve, so a typo here stops these pods being created at all. + priorityClassName: balancerd: # -- Flag to indicate whether to create balancerd pods for the environments diff --git a/src/environmentd/src/environmentd/main.rs b/src/environmentd/src/environmentd/main.rs index 73d1ea5350307..630b045bb4ec1 100644 --- a/src/environmentd/src/environmentd/main.rs +++ b/src/environmentd/src/environmentd/main.rs @@ -181,6 +181,9 @@ pub struct Args { /// Name of a non-default Kubernetes scheduler, if any. #[structopt(long, env = "ORCHESTRATOR_KUBERNETES_SCHEDULER_NAME")] orchestrator_kubernetes_scheduler_name: Option, + /// Name of a `PriorityClass` to assign to services, if any. + #[structopt(long, env = "ORCHESTRATOR_KUBERNETES_PRIORITY_CLASS_NAME")] + orchestrator_kubernetes_priority_class_name: Option, /// Annotations to apply to all services created by the Kubernetes orchestrator /// in the form `KEY=VALUE`. #[structopt(long, env = "ORCHESTRATOR_KUBERNETES_SERVICE_ANNOTATION")] @@ -824,6 +827,7 @@ fn run(mut args: Args) -> Result<(), anyhow::Error> { .block_on(KubernetesOrchestrator::new(KubernetesOrchestratorConfig { context: args.orchestrator_kubernetes_context.clone(), scheduler_name: args.orchestrator_kubernetes_scheduler_name, + priority_class_name: args.orchestrator_kubernetes_priority_class_name, service_annotations: args .orchestrator_kubernetes_service_annotation .into_iter() diff --git a/src/orchestrator-kubernetes/src/lib.rs b/src/orchestrator-kubernetes/src/lib.rs index 37d570e5f19c2..3515b7d07b495 100644 --- a/src/orchestrator-kubernetes/src/lib.rs +++ b/src/orchestrator-kubernetes/src/lib.rs @@ -76,6 +76,8 @@ pub struct KubernetesOrchestratorConfig { pub context: String, /// The name of a non-default Kubernetes scheduler to use, if any. pub scheduler_name: Option, + /// The name of a `PriorityClass` to assign to services, if any. + pub priority_class_name: Option, /// Annotations to install on every service created by the orchestrator. pub service_annotations: BTreeMap, /// Labels to install on every service created by the orchestrator. @@ -1200,6 +1202,7 @@ impl NamespacedOrchestrator for NamespacedKubernetesOrchestrator { security_context, node_selector: Some(node_selector), scheduler_name: self.config.scheduler_name.clone(), + priority_class_name: self.config.priority_class_name.clone(), service_account: self.config.service_account.clone(), affinity: Some(affinity), topology_spread_constraints: topology_spread, diff --git a/src/orchestratord/src/bin/orchestratord.rs b/src/orchestratord/src/bin/orchestratord.rs index 59caf93d713c9..00624afe76323 100644 --- a/src/orchestratord/src/bin/orchestratord.rs +++ b/src/orchestratord/src/bin/orchestratord.rs @@ -236,12 +236,16 @@ pub struct Args { #[clap(long, value_parser = parse_resources)] environmentd_default_resources: Option, #[clap(long)] + environmentd_priority_class_name: Option, + #[clap(long)] clusterd_node_selector: Vec>, #[clap(long, value_parser = parse_affinity)] clusterd_affinity: Option, #[clap(long = "clusterd-toleration", value_parser = parse_tolerations)] clusterd_tolerations: Option>, #[clap(long)] + clusterd_priority_class_name: Option, + #[clap(long)] balancerd_node_selector: Vec>, #[clap(long, value_parser = parse_affinity)] balancerd_affinity: Option, @@ -666,9 +670,11 @@ async fn run(args: Args) -> Result<(), anyhow::Error> { environmentd_affinity: args.environmentd_affinity, environmentd_tolerations: args.environmentd_tolerations, environmentd_default_resources: args.environmentd_default_resources, + environmentd_priority_class_name: args.environmentd_priority_class_name, clusterd_node_selector: args.clusterd_node_selector, clusterd_affinity: args.clusterd_affinity, clusterd_tolerations: args.clusterd_tolerations, + clusterd_priority_class_name: args.clusterd_priority_class_name, image_pull_policy: args.image_pull_policy, network_policies_internal_enabled: args.network_policies_internal_enabled, network_policies_ingress_enabled: args.network_policies_ingress_enabled, diff --git a/src/orchestratord/src/controller/materialize.rs b/src/orchestratord/src/controller/materialize.rs index 177b339834b9f..8ea9f50bdf2b3 100644 --- a/src/orchestratord/src/controller/materialize.rs +++ b/src/orchestratord/src/controller/materialize.rs @@ -89,9 +89,13 @@ pub struct Config { pub environmentd_affinity: Option, pub environmentd_tolerations: Option>, pub environmentd_default_resources: Option, + /// The name of a `PriorityClass` to assign to environmentd pods, if any. + pub environmentd_priority_class_name: Option, pub clusterd_node_selector: Vec>, pub clusterd_affinity: Option, pub clusterd_tolerations: Option>, + /// The name of a `PriorityClass` to assign to clusterd pods, if any. + pub clusterd_priority_class_name: Option, pub image_pull_policy: KubernetesImagePullPolicy, pub network_policies_internal_enabled: bool, pub network_policies_ingress_enabled: bool, diff --git a/src/orchestratord/src/controller/materialize/generation.rs b/src/orchestratord/src/controller/materialize/generation.rs index c99c43c7b1765..aca41ee505d91 100644 --- a/src/orchestratord/src/controller/materialize/generation.rs +++ b/src/orchestratord/src/controller/materialize/generation.rs @@ -73,6 +73,18 @@ static V154_DEV0: LazyLock = LazyLock::new(|| Version { }); pub const V161: Version = Version::new(0, 161, 0); +/// Version at which `environmentd` learned +/// `--orchestrator-kubernetes-priority-class-name`. Older images reject the +/// unknown argument and fail to start, so the flag is only forwarded at or +/// above this version. +static V26_40_0: LazyLock = LazyLock::new(|| Version { + major: 26, + minor: 40, + patch: 0, + pre: Prerelease::new("dev.0").expect("dev.0 is valid prerelease"), + build: BuildMetadata::new("").expect("empty string is valid buildmetadata"), +}); + static V26_1_0: LazyLock = LazyLock::new(|| Version { major: 26, minor: 1, @@ -804,6 +816,13 @@ fn create_environmentd_statefulset_object( scheduler_name )); } + if mz.meets_minimum_version(&V26_40_0) { + if let Some(priority_class_name) = &config.clusterd_priority_class_name { + args.push(format!( + "--orchestrator-kubernetes-priority-class-name={priority_class_name}" + )); + } + } if mz.meets_minimum_version(&V154_DEV0) { args.extend( mz.spec @@ -1246,6 +1265,7 @@ fn create_environmentd_statefulset_object( ), affinity: config.environmentd_affinity.clone(), scheduler_name: config.scheduler_name.clone(), + priority_class_name: config.environmentd_priority_class_name.clone(), service_account_name: Some(mz.service_account_name()), volumes: Some(volumes), security_context: Some(PodSecurityContext { diff --git a/test/orchestratord/mzcompose.py b/test/orchestratord/mzcompose.py index 1b5a88b08d58c..1b5fa55cea98e 100644 --- a/test/orchestratord/mzcompose.py +++ b/test/orchestratord/mzcompose.py @@ -770,6 +770,84 @@ def check() -> None: retry(check, 240) +# Must match test/orchestratord/priorityclass.yaml. +PRIORITY_CLASS_NAME = "mz-test-priority" +PRIORITY_CLASS_VALUE = 1000000000 +# Release in which environmentd learned +# --orchestrator-kubernetes-priority-class-name. +PRIORITY_CLASS_VERSION = "v26.40.0" + + +def assert_priority_class(pod: dict[str, Any], expected: str | None) -> None: + """Assert a pod's priority class by name and by resolved value. + + `spec.priority` is filled in by the API server from the named class, so + checking it is what distinguishes a class Kubernetes actually resolved from + a string the operator copied into the spec. + """ + spec = pod["spec"] + name = pod["metadata"]["name"] + actual = spec.get("priorityClassName") + priority = spec.get("priority") + if expected is None: + assert not actual, f"{name}: unexpected priorityClassName {actual}" + assert not priority, f"{name}: unexpected priority {priority}" + else: + assert actual == expected, f"{name}: expected {expected}, got {actual}" + assert ( + priority == PRIORITY_CLASS_VALUE + ), f"{name}: expected priority {PRIORITY_CLASS_VALUE}, got {priority}" + + +class PriorityClassName(Modification): + @classmethod + def values(cls, version: MzVersion) -> list[Any]: + return [None, PRIORITY_CLASS_NAME] + + @classmethod + def default(cls) -> Any: + return None + + def modify(self, definition: dict[str, Any]) -> None: + if not self.value: + return + definition["operator"]["environmentd"]["priorityClassName"] = self.value + definition["operator"]["clusterd"]["priorityClassName"] = self.value + # environmentd and clusterd only pick up generation-affecting changes on + # a requested rollout, so without this the pods keep the spec they were + # created with and the assertions below check the wrong generation. + request = str(uuid.uuid4()) + if definition["materialize"]["apiVersion"] == "materialize.cloud/v1alpha1": + definition["materialize"]["spec"]["requestRollout"] = request + definition["materialize"]["spec"]["forceRollout"] = request + + def validate(self, mods: dict[type[Modification], Any]) -> None: + version = MzVersion.parse_mz(mods[EnvironmentdImageRef]) + # The operator sets environmentd's class directly, but forwards + # clusterd's to environmentd behind a version gate, so an older image + # gets one and not the other. + clusterd_expected = ( + self.value + if version >= MzVersion.parse_mz(PRIORITY_CLASS_VERSION) + else None + ) + + def check() -> None: + environmentd = get_environmentd_data()["items"] + clusterd = get_clusterd_data()["items"] + # A class the API server cannot resolve yields no pod at all, which + # an assertion over an empty list would otherwise call a pass. + assert environmentd, "no environmentd pods" + assert clusterd, "no clusterd pods" + for pod in environmentd: + assert_priority_class(pod, self.value) + for pod in clusterd: + assert_priority_class(pod, clusterd_expected) + + # Clusterd is recreated by the rollout and can take a while + retry(check, 240) + + class ConsoleEnabled(Modification): @classmethod def values(cls, version: MzVersion) -> list[Any]: @@ -4573,6 +4651,14 @@ def setup(c: Composition, args) -> dict[str, Any]: MZ_ROOT / "test" / "orchestratord" / "storageclass.yaml", ] ) + spawn.runv( + [ + "kubectl", + "apply", + "-f", + MZ_ROOT / "test" / "orchestratord" / "priorityclass.yaml", + ] + ) if not args.tag: services = [ diff --git a/test/orchestratord/priorityclass.yaml b/test/orchestratord/priorityclass.yaml new file mode 100644 index 0000000000000..0ac8919da0115 --- /dev/null +++ b/test/orchestratord/priorityclass.yaml @@ -0,0 +1,21 @@ +# Copyright Materialize, Inc. and contributors. All rights reserved. +# +# Use of this software is governed by the Business Source License +# included in the LICENSE file at the root of this repository. +# +# As of the Change Date specified in that file, in accordance with +# the Business Source License, use of this software will be governed +# by the Apache License, Version 2.0. + +# test/orchestratord/priorityclass.yaml, applied beside storageclass.yaml (:4571). +# `value` is required. `globalDefault: false` keeps the unset baseline testable: +# a global default would put a class on every pod, including the None case. +# `preemptionPolicy: Never` keeps the test from evicting anything in the cluster. +apiVersion: scheduling.k8s.io/v1 +kind: PriorityClass +metadata: + name: mz-test-priority +value: 1000000000 +globalDefault: false +preemptionPolicy: Never +description: "Test fixture for the priorityClassName chart values." From 8886055272519fd56c0da22b52d560c1309e6686 Mon Sep 17 00:00:00 2001 From: doy-materialize <110416385+doy-materialize@users.noreply.github.com> Date: Thu, 10 Sep 2026 18:23:58 -0400 Subject: [PATCH 10/11] fix the version gate for --orchestrator-kubernetes-priority-class-name (#38756) ### Motivation fixes the code added in https://github.com/MaterializeInc/materialize/pull/38457 since this wasn't merged until after the v26.41 release --- src/orchestratord/src/controller/materialize/generation.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/orchestratord/src/controller/materialize/generation.rs b/src/orchestratord/src/controller/materialize/generation.rs index aca41ee505d91..886cb8b61f174 100644 --- a/src/orchestratord/src/controller/materialize/generation.rs +++ b/src/orchestratord/src/controller/materialize/generation.rs @@ -77,9 +77,9 @@ pub const V161: Version = Version::new(0, 161, 0); /// `--orchestrator-kubernetes-priority-class-name`. Older images reject the /// unknown argument and fail to start, so the flag is only forwarded at or /// above this version. -static V26_40_0: LazyLock = LazyLock::new(|| Version { +static V26_42_0: LazyLock = LazyLock::new(|| Version { major: 26, - minor: 40, + minor: 42, patch: 0, pre: Prerelease::new("dev.0").expect("dev.0 is valid prerelease"), build: BuildMetadata::new("").expect("empty string is valid buildmetadata"), @@ -816,7 +816,7 @@ fn create_environmentd_statefulset_object( scheduler_name )); } - if mz.meets_minimum_version(&V26_40_0) { + if mz.meets_minimum_version(&V26_42_0) { if let Some(priority_class_name) = &config.clusterd_priority_class_name { args.push(format!( "--orchestrator-kubernetes-priority-class-name={priority_class_name}" From 3ae6e5e798cf9cef326e1be99cb86c78b7bd9367 Mon Sep 17 00:00:00 2001 From: Materialize Date: Fri, 11 Sep 2026 00:00:00 +0000 Subject: [PATCH 11/11] LICENSE: update change date --- LICENSE | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/LICENSE b/LICENSE index 5fd88deb4ad4d..0cc0ff1ff8409 100644 --- a/LICENSE +++ b/LICENSE @@ -13,7 +13,7 @@ Business Source License 1.1 Licensor: Materialize, Inc. -Licensed Work: Materialize Version 20260910 +Licensed Work: Materialize Version 20260911 The Licensed Work is © 2026 Materialize, Inc. Additional Use Grant: Within a single installation of the Licensed Work, @@ -39,7 +39,7 @@ Additional Use Grant: Within a single installation of the Licensed Work, whose definitions are controlled by such third parties. -Change Date: September 10, 2030 +Change Date: September 11, 2030 Change License: Apache License, Version 2.0