From 38447d1ee07d71308e63026a3b72610630bbe44f Mon Sep 17 00:00:00 2001 From: Moritz Hoffmann Date: Mon, 24 Aug 2026 14:16:19 +0200 Subject: [PATCH 1/8] transform: cancel union branches exposed by physical RelationCSE (#37270) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `UnionBranchCancellation` cancels two branches only when they are direct siblings of the same `Union` and structurally equal (see `compare_branches`). A `Get x` sitting under a `Map` next to a `Union` one level down does not qualify, even when that nested union holds a matching `Negate (Get x)`. The transform ran only in the logical optimizer and the logical cleanup pass, so a plan that reaches the flat sibling shape later kept the cancellable union all the way into the optimized plan. The shape becomes cancellable inside `fixpoint_physical_01`. `LiteralLifting` hoists a literal `Map` that wrapped *both* branches out of the union, turning `union(map(k, x), map(k, union(negate(x), y)))` into `map(k, union(x, union(negate(x), y)))`, and flattening that union-of-union makes `x` and `negate(x)` siblings for the first time. `LiteralLifting` also runs in `fixpoint_logical_02`, but `logical_cleanup_pass` never repeats it, so no earlier `UnionBranchCancellation` call gets to see the flattened form. The transform logic is correct; the gap is pipeline coverage. This change runs `fusion::union::Union` followed by `UnionBranchCancellation` after the final `RelationCSE` in the physical optimizer. The explicit union fusion establishes the flat shape on purpose instead of relying on `FoldConstants`, which flattens a union-of-union only as a side effect of decomposing a `Union` and rebuilding it with `union_many`. The trailing `fold_constants_fixpoint` then fuses the resulting empty constants and `NormalizeLets` drops the now unused binding. Both calls sit behind `enable_union_cancellation_after_relation_cse`, cluster-scoped and default on, so the optimization can be switched off for a user who hits an unexpected regression. Cleanup of the flag is tracked in [STG-41](https://linear.app/materializeinc/issue/STG-41). The motivating cases are a query in `test/sqllogictest/joins.slt` and the second query in `test/sqllogictest/transform/demand.slt`, both of which previously left `union(get l0, negate(get l0), constant)` in the plan and now fold to a constant fast path. The `demand.slt` query no longer reaches a plan that could carry a `dummy`, so it stops covering the scenario its first comment paragraph describes; the comment now records that. Every `test/sqllogictest` file containing `EXPLAIN` (137 files, excluding the upstream `sqlite` and `cockroach` trees) passes unchanged apart from those two goldens. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- misc/python/materialize/mzcompose/__init__.py | 6 ++++ .../materialize/parallel_workload/action.py | 3 ++ src/repr/src/optimize.rs | 2 ++ src/sql-lexer/src/keywords.txt | 1 + src/sql-parser/src/ast/defs/statement.rs | 8 +++-- src/sql/src/plan/statement/ddl.rs | 9 ++++++ src/sql/src/plan/statement/dml.rs | 7 +++++ src/sql/src/session/vars/definitions.rs | 11 +++++++ src/transform/src/lib.rs | 20 +++++++++++++ .../mzcompose.py | 1 + test/sqllogictest/joins.slt | 16 +++------- test/sqllogictest/transform/demand.slt | 30 +++++++------------ 12 files changed, 80 insertions(+), 34 deletions(-) diff --git a/misc/python/materialize/mzcompose/__init__.py b/misc/python/materialize/mzcompose/__init__.py index 322c6f05cf0fa..0731ed7ea5c6e 100644 --- a/misc/python/materialize/mzcompose/__init__.py +++ b/misc/python/materialize/mzcompose/__init__.py @@ -118,6 +118,7 @@ def get_minimal_system_parameters( "enable_storage_introspection_logs": "true", "enable_compute_error_distinct": "true", "enable_compute_temporal_bucketing": "true", + "enable_union_cancellation_after_relation_cse": "true", "enable_variadic_left_join_lowering": "true", "enable_worker_core_affinity": "true", "grpc_client_http2_keep_alive_timeout": "5s", @@ -297,6 +298,11 @@ def get_variable_system_parameters( "true", ["true", "false"], ), + VariableSystemParameter( + "enable_union_cancellation_after_relation_cse", + "true", + ["true", "false"], + ), VariableSystemParameter( "enable_upsert_v2", "false", diff --git a/misc/python/materialize/parallel_workload/action.py b/misc/python/materialize/parallel_workload/action.py index 58de840777d63..868907a41cc8c 100644 --- a/misc/python/materialize/parallel_workload/action.py +++ b/misc/python/materialize/parallel_workload/action.py @@ -3015,6 +3015,9 @@ def __init__( "false", ] self.flags_with_values["enable_case_literal_transform"] = BOOLEAN_FLAG_VALUES + self.flags_with_values["enable_union_cancellation_after_relation_cse"] = ( + BOOLEAN_FLAG_VALUES + ) self.flags_with_values["enable_cast_elimination"] = BOOLEAN_FLAG_VALUES self.flags_with_values["enable_fixed_correlated_cte_lowering"] = ( BOOLEAN_FLAG_VALUES diff --git a/src/repr/src/optimize.rs b/src/repr/src/optimize.rs index d0330d7ec8147..8f9d8809cdccd 100644 --- a/src/repr/src/optimize.rs +++ b/src/repr/src/optimize.rs @@ -122,6 +122,8 @@ optimizer_feature_flags!({ // See the feature flag of the same name. enable_projection_pushdown_after_relation_cse: bool, // See the feature flag of the same name. + enable_union_cancellation_after_relation_cse: bool, + // See the feature flag of the same name. enable_less_reduce_in_eqprop: bool, // See the feature flag of the same name. enable_dequadratic_eqprop_map: bool, diff --git a/src/sql-lexer/src/keywords.txt b/src/sql-lexer/src/keywords.txt index 20eff3d836bae..ad04025ccbaff 100644 --- a/src/sql-lexer/src/keywords.txt +++ b/src/sql-lexer/src/keywords.txt @@ -74,6 +74,7 @@ Broker Brokers By Bytes +Cancellation Capture Cardinality Cascade diff --git a/src/sql-parser/src/ast/defs/statement.rs b/src/sql-parser/src/ast/defs/statement.rs index be3901ecd7835..305fa8d19eee6 100644 --- a/src/sql-parser/src/ast/defs/statement.rs +++ b/src/sql-parser/src/ast/defs/statement.rs @@ -2506,6 +2506,7 @@ pub enum ClusterFeatureName { EnableLetrecFixpointAnalysis, EnableJoinPrioritizeArranged, EnableProjectionPushdownAfterRelationCse, + EnableUnionCancellationAfterRelationCse, } impl WithOptionName for ClusterFeatureName { @@ -2522,7 +2523,8 @@ impl WithOptionName for ClusterFeatureName { | Self::EnableVariadicLeftJoinLowering | Self::EnableLetrecFixpointAnalysis | Self::EnableJoinPrioritizeArranged - | Self::EnableProjectionPushdownAfterRelationCse => false, + | Self::EnableProjectionPushdownAfterRelationCse + | Self::EnableUnionCancellationAfterRelationCse => false, } } } @@ -4191,6 +4193,7 @@ pub enum ExplainPlanOptionName { EnableJoinPrioritizeArranged, EnableProjectionPushdownAfterRelationCse, EnableFixedCorrelatedCteLowering, + EnableUnionCancellationAfterRelationCse, } impl WithOptionName for ExplainPlanOptionName { @@ -4228,7 +4231,8 @@ impl WithOptionName for ExplainPlanOptionName { | Self::EnableLetrecFixpointAnalysis | Self::EnableJoinPrioritizeArranged | Self::EnableProjectionPushdownAfterRelationCse - | Self::EnableFixedCorrelatedCteLowering => false, + | Self::EnableFixedCorrelatedCteLowering + | Self::EnableUnionCancellationAfterRelationCse => false, } } } diff --git a/src/sql/src/plan/statement/ddl.rs b/src/sql/src/plan/statement/ddl.rs index cdaafdfac1a26..e8dbf023512f3 100644 --- a/src/sql/src/plan/statement/ddl.rs +++ b/src/sql/src/plan/statement/ddl.rs @@ -5047,6 +5047,11 @@ generate_extracted_config!( EnableProjectionPushdownAfterRelationCse, Option, Default(None) + ), + ( + EnableUnionCancellationAfterRelationCse, + Option, + Default(None) ) ); @@ -5190,6 +5195,7 @@ pub fn plan_create_cluster_inner( enable_letrec_fixpoint_analysis, enable_join_prioritize_arranged, enable_projection_pushdown_after_relation_cse, + enable_union_cancellation_after_relation_cse, seen: _, } = ClusterFeatureExtracted::try_from(features)?; let optimizer_feature_overrides = OptimizerFeatureOverrides { @@ -5200,6 +5206,7 @@ pub fn plan_create_cluster_inner( enable_letrec_fixpoint_analysis, enable_join_prioritize_arranged, enable_projection_pushdown_after_relation_cse, + enable_union_cancellation_after_relation_cse, ..Default::default() }; @@ -5326,6 +5333,7 @@ pub fn unplan_create_cluster( enable_letrec_fixpoint_analysis, enable_join_prioritize_arranged, enable_projection_pushdown_after_relation_cse, + enable_union_cancellation_after_relation_cse, enable_less_reduce_in_eqprop: _, enable_dequadratic_eqprop_map: _, enable_eq_classes_withholding_errors: _, @@ -5349,6 +5357,7 @@ pub fn unplan_create_cluster( enable_letrec_fixpoint_analysis, enable_join_prioritize_arranged, enable_projection_pushdown_after_relation_cse, + enable_union_cancellation_after_relation_cse, }; let features = features_extracted.into_values(scx.catalog); let availability_zones = if availability_zones.is_empty() { diff --git a/src/sql/src/plan/statement/dml.rs b/src/sql/src/plan/statement/dml.rs index 2272eff32ec62..cc7726aad9ac7 100644 --- a/src/sql/src/plan/statement/dml.rs +++ b/src/sql/src/plan/statement/dml.rs @@ -579,6 +579,11 @@ generate_extracted_config!( EnableFixedCorrelatedCteLowering, Option, Default(None) + ), + ( + EnableUnionCancellationAfterRelationCse, + Option, + Default(None) ) ); @@ -631,6 +636,8 @@ impl TryFrom for ExplainConfig { enable_join_prioritize_arranged: v.enable_join_prioritize_arranged, enable_projection_pushdown_after_relation_cse: v .enable_projection_pushdown_after_relation_cse, + enable_union_cancellation_after_relation_cse: v + .enable_union_cancellation_after_relation_cse, enable_less_reduce_in_eqprop: Default::default(), enable_dequadratic_eqprop_map: Default::default(), enable_eq_classes_withholding_errors: Default::default(), diff --git a/src/sql/src/session/vars/definitions.rs b/src/sql/src/session/vars/definitions.rs index a4b2ba40919d3..908339f1d5452 100644 --- a/src/sql/src/session/vars/definitions.rs +++ b/src/sql/src/session/vars/definitions.rs @@ -2285,6 +2285,13 @@ feature_flags!( enable_for_item_parsing: false, scope: ParameterScope::Cluster, }, + { + name: enable_union_cancellation_after_relation_cse, + desc: "Run UnionBranchCancellation one more time after the last RelationCSE.", + default: true, + enable_for_item_parsing: false, + scope: ParameterScope::Cluster, + }, { name: enable_less_reduce_in_eqprop, desc: "Run MSE::reduce in EquivalencePropagation only if reduce_expr changed something.", @@ -2395,6 +2402,8 @@ impl From<&super::SystemVars> for OptimizerFeatures { enable_join_prioritize_arranged: vars.enable_join_prioritize_arranged(), enable_projection_pushdown_after_relation_cse: vars .enable_projection_pushdown_after_relation_cse(), + enable_union_cancellation_after_relation_cse: vars + .enable_union_cancellation_after_relation_cse(), enable_less_reduce_in_eqprop: vars.enable_less_reduce_in_eqprop(), enable_dequadratic_eqprop_map: vars.enable_dequadratic_eqprop_map(), enable_eq_classes_withholding_errors: vars.enable_eq_classes_withholding_errors(), @@ -2446,6 +2455,7 @@ mod tests { reoptimize_imported_views, enable_join_prioritize_arranged, enable_projection_pushdown_after_relation_cse, + enable_union_cancellation_after_relation_cse, enable_less_reduce_in_eqprop, enable_dequadratic_eqprop_map, enable_fast_path_plan_insights, @@ -2478,6 +2488,7 @@ mod tests { let _ = reoptimize_imported_views; // no corresponding var set_var!(enable_join_prioritize_arranged); set_var!(enable_projection_pushdown_after_relation_cse); + set_var!(enable_union_cancellation_after_relation_cse); set_var!(enable_less_reduce_in_eqprop); set_var!(enable_dequadratic_eqprop_map); set_var!(enable_fast_path_plan_insights); diff --git a/src/transform/src/lib.rs b/src/transform/src/lib.rs index 36f988d6a641d..712913a035ce8 100644 --- a/src/transform/src/lib.rs +++ b/src/transform/src/lib.rs @@ -877,6 +877,26 @@ impl Optimizer { Box::new(CanonicalizeMfp), // Identifies common relation subexpressions. Box::new(cse::relation_cse::RelationCSE::new(false)), + // `UnionBranchCancellation` only cancels two branches that are direct siblings of the + // same `Union` and literally structurally equal (see `compare_branches`); a + // `Get x` under a `Map` sibling to a `Union` nested one level down does not qualify, + // even if that nested union contains a matching `Negate (Get x)`. + // + // In `fixpoint_physical_01` above, `LiteralLifting` can hoist a `Map` literal that was + // wrapping *both* branches of such a union out of the union entirely, turning + // `union(map(k, x), map(k, union(negate(x), y)))` into `map(k, union(x, union(negate(x), y)))`. + // Flattening that union-of-union is what makes `x` and `negate(x)` direct siblings, so + // run `UnionFusion` first to establish the flat shape `UnionBranchCancellation` needs. + // `LiteralLifting` also runs in the logical optimizer's `fixpoint_logical_02`, but + // `logical_cleanup_pass` (which runs after cross-view normalization, closer to where this + // shape tends to arise) never repeats it, so no earlier `UnionBranchCancellation` call + // gets a chance to see the flattened form. Run it again here, after the last + // `RelationCSE`, so the trailing `fold_constants_fixpoint` can fuse the resulting empty + // constants and drop the now-unused binding. + Box::new(fusion::union::Union); + if ctx.features.enable_union_cancellation_after_relation_cse, + Box::new(UnionBranchCancellation); + if ctx.features.enable_union_cancellation_after_relation_cse, // `RelationCSE` can create new points of interest for `ProjectionPushdown`: If an MFP // is cut in half by `RelationCSE`, then we'd like to push projections behind the new // Get as much as possible. This is because a fork in the plan involves copying the diff --git a/test/launchdarkly-flag-consistency/mzcompose.py b/test/launchdarkly-flag-consistency/mzcompose.py index bd8d9cb6baac4..483e593d0af1c 100644 --- a/test/launchdarkly-flag-consistency/mzcompose.py +++ b/test/launchdarkly-flag-consistency/mzcompose.py @@ -284,6 +284,7 @@ enable_simplify_from_less_existence enable_simplify_quantified_comparisons enable_time_at_time_zone + enable_union_cancellation_after_relation_cse enable_unlimited_retain_history enable_will_distinct_propagation enable_with_ordinality_legacy_fallback diff --git a/test/sqllogictest/joins.slt b/test/sqllogictest/joins.slt index 10dd5d1fe5447..325a0b7b70837 100644 --- a/test/sqllogictest/joins.slt +++ b/test/sqllogictest/joins.slt @@ -1176,10 +1176,6 @@ WHERE EXISTS (SELECT FROM c AS ref_7 WHERE EXISTS (SELECT ref_0."?column?" AS c2 Explained Query: With cte l0 = - Distinct project=[] // { arity: 0 } - Project () // { arity: 0 } - ReadStorage materialize.public.c // { arity: 1 } - cte l1 = Union // { arity: 1 } Project (#2) // { arity: 1 } Map (null) // { arity: 3 } @@ -1195,21 +1191,17 @@ Explained Query: implementation %0[×] » %1[×] ArrangeBy keys=[[]] // { arity: 0 } - Union // { arity: 0 } - Get l0 // { arity: 0 } - Negate // { arity: 0 } - Get l0 // { arity: 0 } - Constant // { arity: 0 } - - () + Constant // { arity: 0 } + - () ArrangeBy keys=[[]] // { arity: 1 } Union // { arity: 1 } - Get l1 // { arity: 1 } + Get l0 // { arity: 1 } Map (null) // { arity: 1 } Union // { arity: 0 } Negate // { arity: 0 } Distinct project=[] // { arity: 0 } Project () // { arity: 0 } - Get l1 // { arity: 1 } + Get l0 // { arity: 1 } Constant // { arity: 0 } - () diff --git a/test/sqllogictest/transform/demand.slt b/test/sqllogictest/transform/demand.slt index 6d0873ff1be70..4230afcbad00c 100644 --- a/test/sqllogictest/transform/demand.slt +++ b/test/sqllogictest/transform/demand.slt @@ -29,6 +29,13 @@ select x from (select x, 1/a from (select 2 as x), t); # A `dummy` used to occur in the following plan before putting an extra call to `ProjectionPushdown` after the `Demand` # call in the physical optimizer, because at the time of the first `Demand` call, the column that is later dummied in # the second call is still being used in a join constraint, but this join is then eliminated by `RedundantJoin`. +# +# This query also exercises `UnionBranchCancellation` running again after the final `RelationCSE`: it folds a +# `union(get l0, negate(get l0), constant)` left behind by the earlier passes into the fast path below. +# +# NOTE: That cancellation collapses the whole query to a constant fast path, so this query no longer exercises the +# `dummy` scenario described in the first paragraph. It is kept for the `UnionBranchCancellation` coverage, and the +# original `dummy` coverage would need a different query. query T multiline EXPLAIN OPTIMIZED PLAN WITH(humanized expressions, arity, join implementations) AS VERBOSE TEXT FOR select @@ -87,27 +94,10 @@ from where true limit coalesce(43, 120); ---- -Explained Query: +Explained Query (fast path): Finish limit=43 output=[#0..=#3] - With - cte l0 = - Distinct project=[] // { arity: 0 } - TopK limit=13 // { arity: 0 } - Filter error("timestamp out of range") // { arity: 0 } - Project () // { arity: 0 } - ReadIndex on=mz_columns mz_columns_ind=[*** full scan ***] // { arity: 8 } - Return // { arity: 4 } - Map (833564499, null, null, 2024-12-18 12:54:29.994 UTC) // { arity: 4 } - TopK limit=90 // { arity: 0 } - Union // { arity: 0 } - Get l0 // { arity: 0 } - Negate // { arity: 0 } - Get l0 // { arity: 0 } - Constant // { arity: 0 } - - () - -Used Indexes: - - mz_catalog.mz_columns_ind (*** full scan ***) + Constant + - (833564499, null, null, 2024-12-18 12:54:29.994 UTC) Target cluster: mz_catalog_server From 24f26138f0574ea0c63f254a70da6736f709063e Mon Sep 17 00:00:00 2001 From: Moritz Hoffmann Date: Mon, 24 Aug 2026 15:19:08 +0200 Subject: [PATCH 2/8] sql: graduate the bounded staleness isolation level (#38414) ### Motivation `bounded staleness ` shipped in v26.29 behind `enable_bounded_staleness_isolation`, which has been defaulting on since. The level is documented as public preview and still carries the gating machinery that a preview needs. This graduates it: the flag goes away and the docs drop the preview annotation, so bounded staleness is a supported part of the `transaction_isolation` surface. ### Description * Removes `enable_bounded_staleness_isolation` from the feature-flag table. `check_transaction_isolation_feature_flag` now gates only `strong session serializable`, which is still flagged behind `enable_session_timelines`. The shared check and the session-default scrub in `command_handler` therefore stay in place; only the bounded staleness arm and the comment's example change. * An environment that persisted the removed parameter via `ALTER SYSTEM` is unaffected: an unknown system parameter in catalog storage is warned about and ignored at startup (`catalog/apply.rs`, `catalog/open.rs`). * Drops `{{< public-preview />}}` and the **Public preview.** cell from the isolation-level reference. The `{{< if-released "v26.29" >}}` guards are kept, since older Self-Managed versions still lack the level entirely. * Parallel workload no longer gates `BoundedStalenessReadAction` on the flag or flips the flag, and the flag leaves the LaunchDarkly consistency allowlist (it never had an LD flag, so it was listed under `KNOWN_MISSING_FROM_LD`). Not included: a release-note entry in `doc/user/content/releases/_index.md`. That file has no section for the in-flight version, so the GA note belongs with the release-notes commit for whichever release this lands in. ### Verification * `test/sqllogictest/bounded_staleness.slt` loses the feature-flag sections and keeps the role-default coverage: `ALTER ROLE ... SET` of the level, and a fresh session picking it up as its starting isolation. * `test/testdrive/bounded-staleness.td` no longer enables the flag; the happy-path coverage is otherwise unchanged. * The `mz-sql` unit test for the shared isolation gate now exercises `strong session serializable` (rejected with the flag off, accepted with it on) and asserts bounded staleness passes ungated. Release note: This release makes the bounded staleness isolation level generally available. --- _Generated by [Claude Code](https://claude.ai/code/session_01PfeCp5oXaZ9RayUSdD5aJb)_ Co-authored-by: Claude --- doc/user/content/reference/isolation-level.md | 4 +- .../materialize/parallel_workload/action.py | 8 --- src/adapter/src/coord/command_handler.rs | 6 +- src/sql/src/session/vars.rs | 53 ++++++++------- src/sql/src/session/vars/definitions.rs | 6 -- .../mzcompose.py | 1 - test/sqllogictest/bounded_staleness.slt | 64 ++----------------- test/testdrive/bounded-staleness.td | 3 - 8 files changed, 34 insertions(+), 111 deletions(-) diff --git a/doc/user/content/reference/isolation-level.md b/doc/user/content/reference/isolation-level.md index 228068de9121c..7e1d45dfc51de 100644 --- a/doc/user/content/reference/isolation-level.md +++ b/doc/user/content/reference/isolation-level.md @@ -33,7 +33,7 @@ against Materialize: | --- | --- | | [**Strict Serializable**](#strict-serializable) | **Default.** Provides serializability and linearizability. | | [**Serializable**](#serializable) | Provides serializability but not linearizability. | -| [**Bounded Staleness ``**](#bounded-staleness) | **Public preview.** Serves reads at a timestamp at most `` stale; never blocks, errors if the bound cannot be met. | +| [**Bounded Staleness ``**](#bounded-staleness) | Serves reads at a timestamp at most `` stale; never blocks, errors if the bound cannot be met. | | Read Uncommitted, Read Committed, Repeatable Read | Accepted for compatibility; treated as Serializable. | {{< /if-released >}} @@ -156,8 +156,6 @@ made available to us (e.g., querying PostgreSQL for the replication slot's LSN). {{< if-released "v26.29" >}} ## Bounded Staleness -{{< public-preview />}} - The Bounded Staleness isolation level lets you trade exact freshness for predictable latency. A query under bounded staleness is served at a timestamp that is at most `` stale—but never blocks waiting for input diff --git a/misc/python/materialize/parallel_workload/action.py b/misc/python/materialize/parallel_workload/action.py index 868907a41cc8c..619fb5be810ec 100644 --- a/misc/python/materialize/parallel_workload/action.py +++ b/misc/python/materialize/parallel_workload/action.py @@ -2679,15 +2679,10 @@ class BoundedStalenessReadAction(Action): restored afterwards, since bounded staleness is read-only and would break writes.""" - def applicable(self, exe: Executor) -> bool: - return exe.db.flags.get("enable_bounded_staleness_isolation", "FALSE") == "TRUE" - def errors_to_ignore(self, exe: Executor) -> list[str]: result = super().errors_to_ignore(exe) result.extend( [ - # The flag was flipped off between applicable() and run(). - "is not available", # The freshness bound could not be met. Bounded staleness # never blocks, it errors instead. "not been materialized", @@ -2988,9 +2983,6 @@ def __init__( ) self.flags_with_values["enable_compute_error_distinct"] = BOOLEAN_FLAG_VALUES self.flags_with_values["enable_alter_table_add_column"] = BOOLEAN_FLAG_VALUES - self.flags_with_values["enable_bounded_staleness_isolation"] = ( - BOOLEAN_FLAG_VALUES - ) self.flags_with_values["enable_arrangement_dictionary_compression_alpha"] = ( BOOLEAN_FLAG_VALUES ) diff --git a/src/adapter/src/coord/command_handler.rs b/src/adapter/src/coord/command_handler.rs index 0cbbb3df99ee1..4b334b3a8f31f 100644 --- a/src/adapter/src/coord/command_handler.rs +++ b/src/adapter/src/coord/command_handler.rs @@ -1084,9 +1084,9 @@ impl Coordinator { // If the resolved `transaction_isolation` default names a feature-flagged // isolation level whose flag is now disabled (e.g. a role default set - // while `bounded staleness` was enabled, then the flag turned off), drop - // it so the session falls back to the built-in default rather than - // silently using a gated level. + // while `strong session serializable` was enabled, then the flag turned + // off), drop it so the session falls back to the built-in default rather + // than silently using a gated level. if let Some(value) = session_defaults.get(TRANSACTION_ISOLATION_VAR_NAME) { if check_transaction_isolation_feature_flag( TRANSACTION_ISOLATION_VAR_NAME, diff --git a/src/sql/src/session/vars.rs b/src/sql/src/session/vars.rs index 9e457f5041860..f6ad7a9446eb7 100644 --- a/src/sql/src/session/vars.rs +++ b/src/sql/src/session/vars.rs @@ -1023,8 +1023,7 @@ fn compat_translate_name(name: &str) -> &str { } /// Enforces feature-flag gating for `transaction_isolation` levels that sit -/// behind a flag (`bounded staleness ` and -/// `strong session serializable`). +/// behind a flag (`strong session serializable`). /// /// Returns `Ok(())` for any other variable, and for an unparseable value /// (parse errors surface on the actual set). This is shared by every path that @@ -1045,9 +1044,6 @@ pub fn check_transaction_isolation_feature_flag( }; match level { IsolationLevel::StrongSessionSerializable => ENABLE_SESSION_TIMELINES.require(system_vars), - IsolationLevel::BoundedStaleness(_) => { - ENABLE_BOUNDED_STALENESS_ISOLATION.require(system_vars) - } _ => Ok(()), } } @@ -2613,46 +2609,49 @@ mod isolation_feature_flag_tests { use super::*; #[mz_ore::test] - fn gates_bounded_staleness_value() { + fn gates_strong_session_serializable_value() { let mut system_vars = SystemVars::new(); - // Default-on: the value passes the gate. - check_transaction_isolation_feature_flag( - TRANSACTION_ISOLATION_VAR_NAME, - VarInput::Flat("bounded staleness 5s"), - &system_vars, - ) - .expect("flag on by default"); - - // Turn the flag off: the value is rejected regardless of the letter case - // of the variable name. This covers `SET`, `SET "TRANSACTION_ISOLATION"`, - // `ALTER ROLE ... SET`, and connection options, which all route through - // `SessionVars::set` and this shared check. - system_vars - .set("enable_bounded_staleness_isolation", VarInput::Flat("off")) - .expect("set flag"); + // The flag defaults off: the value is rejected regardless of the letter + // case of the variable name. This covers `SET`, + // `SET "TRANSACTION_ISOLATION"`, `ALTER ROLE ... SET`, and connection + // options, which all route through `SessionVars::set` and this shared + // check. for name in ["transaction_isolation", "TRANSACTION_ISOLATION"] { let err = check_transaction_isolation_feature_flag( name, - VarInput::Flat("bounded staleness 5s"), + VarInput::Flat("strong session serializable"), &system_vars, ) - .expect_err("flag off rejects bounded staleness"); + .expect_err("flag off rejects strong session serializable"); assert!(matches!(err, VarError::RequiresFeatureFlag { .. })); } - // Non-gated levels are unaffected. + // Ungated levels pass regardless of the flag. + for level in ["serializable", "bounded staleness 5s"] { + check_transaction_isolation_feature_flag( + TRANSACTION_ISOLATION_VAR_NAME, + VarInput::Flat(level), + &system_vars, + ) + .expect("ungated level always allowed"); + } + + // With the flag on, the gated value passes too. + system_vars + .set("enable_session_timelines", VarInput::Flat("on")) + .expect("set flag"); check_transaction_isolation_feature_flag( TRANSACTION_ISOLATION_VAR_NAME, - VarInput::Flat("serializable"), + VarInput::Flat("strong session serializable"), &system_vars, ) - .expect("serializable always allowed"); + .expect("flag on"); // Unrelated variables are ignored, even with a gated-looking value. check_transaction_isolation_feature_flag( CLUSTER.name(), - VarInput::Flat("bounded staleness 5s"), + VarInput::Flat("strong session serializable"), &system_vars, ) .expect("unrelated var ignored"); diff --git a/src/sql/src/session/vars/definitions.rs b/src/sql/src/session/vars/definitions.rs index 908339f1d5452..73015b562dd9c 100644 --- a/src/sql/src/session/vars/definitions.rs +++ b/src/sql/src/session/vars/definitions.rs @@ -2380,12 +2380,6 @@ feature_flags!( default: true, enable_for_item_parsing: false, }, - { - name: enable_bounded_staleness_isolation, - desc: "the `bounded staleness ` transaction isolation level", - default: true, - enable_for_item_parsing: false, - }, ); impl From<&super::SystemVars> for OptimizerFeatures { diff --git a/test/launchdarkly-flag-consistency/mzcompose.py b/test/launchdarkly-flag-consistency/mzcompose.py index 483e593d0af1c..bbb1859e01372 100644 --- a/test/launchdarkly-flag-consistency/mzcompose.py +++ b/test/launchdarkly-flag-consistency/mzcompose.py @@ -239,7 +239,6 @@ enable_background_alter_cluster enable_statement_arrival_logging enable_binary_date_bin - enable_bounded_staleness_isolation enable_coalesce_case_transform enable_compute_half_join2 enable_compute_render_fueled_as_specific_collection diff --git a/test/sqllogictest/bounded_staleness.slt b/test/sqllogictest/bounded_staleness.slt index 68c4244319349..b855ff1be8c8f 100644 --- a/test/sqllogictest/bounded_staleness.slt +++ b/test/sqllogictest/bounded_staleness.slt @@ -13,11 +13,6 @@ mode cockroach reset-server -simple conn=mz_system,user=mz_system -ALTER SYSTEM SET enable_bounded_staleness_isolation = true; ----- -COMPLETE 0 - # --- parse and SHOW round-trip ----------------------------------------------- statement ok @@ -229,70 +224,19 @@ RESET transaction_isolation statement ok DROP TABLE bs_copy -# --- feature-flag gating ------------------------------------------------------ -# -# The flag defaults to `true` in public preview; explicitly turn it off here to -# exercise the gating path. - -simple conn=mz_system,user=mz_system -ALTER SYSTEM SET enable_bounded_staleness_isolation = false; ----- -COMPLETE 0 - -statement error the `bounded staleness ` transaction isolation level is not available -SET transaction_isolation TO 'bounded staleness 5s' - -# The gate must not be bypassable via a quoted, upper-cased variable name. -statement error the `bounded staleness ` transaction isolation level is not available -SET "TRANSACTION_ISOLATION" TO 'bounded staleness 5s' - -# ... nor via `ALTER ROLE ... SET`. -statement error the `bounded staleness ` transaction isolation level is not available -ALTER ROLE materialize SET transaction_isolation = 'bounded staleness 5s' - -simple conn=mz_system,user=mz_system -ALTER SYSTEM SET enable_bounded_staleness_isolation = true; ----- -COMPLETE 0 - -statement ok -SET transaction_isolation TO 'bounded staleness 5s' - -statement ok -RESET transaction_isolation - -# --- role-default fallback when the flag is later disabled -------------------- +# --- role default ------------------------------------------------------------- # -# A role default set while the flag is on must not silently take effect once the -# flag is turned off; fresh sessions fall back to the built-in default. +# `ALTER ROLE ... SET` accepts the level, and a fresh session picks it up as its +# starting isolation. statement ok ALTER ROLE materialize SET transaction_isolation = 'bounded staleness 5s' -# A fresh session picks up the role default while the flag is on. -simple conn=role_on,user=materialize +simple conn=role_default,user=materialize SHOW transaction_isolation ---- bounded staleness 5s COMPLETE 1 -simple conn=mz_system,user=mz_system -ALTER SYSTEM SET enable_bounded_staleness_isolation = false; ----- -COMPLETE 0 - -# With the flag off, a fresh session falls back to the built-in default rather -# than applying the gated role default. -simple conn=role_off,user=materialize -SHOW transaction_isolation ----- -strict serializable -COMPLETE 1 - -simple conn=mz_system,user=mz_system -ALTER SYSTEM SET enable_bounded_staleness_isolation = true; ----- -COMPLETE 0 - statement ok ALTER ROLE materialize RESET transaction_isolation diff --git a/test/testdrive/bounded-staleness.td b/test/testdrive/bounded-staleness.td index 0a176d370a4ad..23d6a953fd0a6 100644 --- a/test/testdrive/bounded-staleness.td +++ b/test/testdrive/bounded-staleness.td @@ -9,9 +9,6 @@ # Bounded staleness isolation — happy-path coverage. -$ postgres-execute connection=postgres://mz_system:materialize@${testdrive.materialize-internal-sql-addr} -ALTER SYSTEM SET enable_bounded_staleness_isolation = true - > CREATE TABLE bs_t (a int); > INSERT INTO bs_t VALUES (1), (2), (3); From fb86c94c64ea75664c5119adeb089b176dc52939 Mon Sep 17 00:00:00 2001 From: Moritz Hoffmann Date: Mon, 24 Aug 2026 15:26:38 +0200 Subject: [PATCH 3/8] mz: don't require write access to load an existing config file (#37566) ### Motivation `mz` fails on read-only commands (e.g. `mz sql`) when `mz.toml` sits on a read-only mount, such as a sandbox that bind-mounts the config file read-only. The command never intends to write anything, so this is an unnecessary failure. ### Description `ConfigFile::load` always opened `mz.toml` with `write(true).create(true)`, even though every mutation (`add_profile`, `remove_profile`, `set_param`, `set_profile_param`) writes the file independently afterward via `fs::write`. The write-mode open at load time was only ever needed to create the file when missing. `load` no longer writes to the file system at all. A missing file loads as an empty configuration, and the first mutation creates whatever is missing. Since `fs::write` creates the file but not its parent directory, all mutations now go through a private `ConfigFile::write` helper that calls `create_dir_all` first, taking over the directory creation that `load` used to perform. Read-only commands therefore work both when `mz.toml` sits on a read-only mount and when it does not exist yet. Dropping the write-mode open changes when an unwritable configuration is reported. `mz profile init` creates a remote app password, and on macOS a keychain entry, before `add_profile` reaches its write. Failing only at that write would leave a credential behind that no profile records, so `ConfigFile::ensure_writable` establishes write access and `init` calls it before logging in. The check creates the parent directory and opens the file with `create(true)`, because whether a missing file can be written depends on the parent hierarchy and no probe settles that as reliably as performing the creation the later write needs anyway. Unit tests cover loading a missing file, loading a read-only file, a missing file inside a read-only directory, and the parent directory creation that moved out of `load`. The permission-dependent assertions skip themselves when the file system does not enforce the permission bits for the process, as is the case under root. --------- Co-authored-by: Claude Sonnet 5 --- Cargo.lock | 1 + src/mz/Cargo.toml | 1 + src/mz/src/command/profile.rs | 4 + src/mz/src/config_file.rs | 185 +++++++++++++++++++++++++++++----- src/mz/src/error.rs | 5 + 5 files changed, 173 insertions(+), 23 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b7df075307db4..bb087a9b6c6e5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6083,6 +6083,7 @@ dependencies = [ "serde-aux", "serde_json", "tabled", + "tempfile", "termcolor", "thiserror 2.0.18", "time", diff --git a/src/mz/Cargo.toml b/src/mz/Cargo.toml index 51e26137c7182..099b22ccaf87a 100644 --- a/src/mz/Cargo.toml +++ b/src/mz/Cargo.toml @@ -44,6 +44,7 @@ url.workspace = true [dev-dependencies] assert_cmd.workspace = true +tempfile.workspace = true [target.'cfg(target_os = "macos")'.dependencies] security-framework = "3.7.0" diff --git a/src/mz/src/command/profile.rs b/src/mz/src/command/profile.rs index be05351b8a0c5..f4ab5e7b85236 100644 --- a/src/mz/src/command/profile.rs +++ b/src/mz/src/command/profile.rs @@ -170,6 +170,10 @@ pub async fn init( } } + // Fail before logging in, so that an unwritable configuration file doesn't + // leave behind an app password that no profile records. + config_file.ensure_writable().await?; + let app_password = match no_browser { true => init_without_browser(admin_endpoint.clone()).await?, false => init_with_browser(cloud_endpoint.clone()).await?, diff --git a/src/mz/src/config_file.rs b/src/mz/src/config_file.rs index 5954ca0e3f636..436b99dd69188 100644 --- a/src/mz/src/config_file.rs +++ b/src/mz/src/config_file.rs @@ -15,8 +15,6 @@ //! Configuration file management. -use std::fs::OpenOptions; -use std::io::Read; use std::path::PathBuf; use std::sync::LazyLock; use std::{collections::BTreeMap, str::FromStr}; @@ -84,24 +82,17 @@ impl ConfigFile { } /// Loads a configuration file from the specified path. + /// + /// A missing file loads as an empty configuration. Loading never creates + /// the file or its parent directory, and never requires write access, so + /// commands that only read the configuration work when `mz.toml` lives on + /// a read-only mount. The first mutation creates whatever is missing. pub async fn load(path: PathBuf) -> Result { - // Create the parent directory if it doesn't exist - if let Some(parent) = path.parent() { - if !parent.exists() { - fs::create_dir_all(parent).await?; - } - } - - // Create the file if it doesn't exist - let mut file = OpenOptions::new() - .read(true) - .write(true) - .create(true) - .truncate(false) - .open(&path)?; - - let mut buffer = String::new(); - file.read_to_string(&mut buffer)?; + let buffer = match fs::read_to_string(&path).await { + Ok(buffer) => buffer, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => String::new(), + Err(e) => return Err(e.into()), + }; let parsed = toml_edit::de::from_str(&buffer)?; let editable = buffer.parse()?; @@ -113,6 +104,50 @@ impl ConfigFile { }) } + /// Writes `contents` to the configuration file, creating the parent + /// directory if it doesn't exist. + async fn write(&self, contents: String) -> Result<(), Error> { + if let Some(parent) = self.path.parent() { + fs::create_dir_all(parent).await?; + } + fs::write(&self.path, contents).await?; + + Ok(()) + } + + /// Errors unless the configuration file can be written, creating the file + /// and its parent directory if they are missing. + /// + /// Commands that cause external side effects before mutating the + /// configuration, such as creating an app password or writing to the + /// keychain, must call this beforehand. Otherwise an unwritable + /// configuration file fails the command only after those side effects have + /// happened, leaving them unrecorded. + /// + /// Creating what is missing is what makes the check conclusive: whether a + /// missing file can be written depends on the parent hierarchy, which no + /// amount of probing establishes as reliably as performing the creation + /// that the later write needs anyway. + pub async fn ensure_writable(&self) -> Result<(), Error> { + let result = async { + if let Some(parent) = self.path.parent() { + fs::create_dir_all(parent).await?; + } + fs::OpenOptions::new() + .write(true) + .create(true) + .truncate(false) + .open(&self.path) + .await?; + + Ok::<(), std::io::Error>(()) + }; + + result + .await + .map_err(|e| Error::ConfigFileNotWritable(self.path.clone(), e)) + } + /// Loads a profile from the configuration file. /// Panics if the profile is not found. pub fn load_profile<'a>(&'a self, name: &'a str) -> Result, Error> { @@ -161,7 +196,7 @@ impl ConfigFile { editable["profile"] = editable.entry("profile").or_insert(value(name)).clone(); // TODO: I don't know why it creates an empty [profiles] table - fs::write(&self.path, editable.to_string()).await?; + self.write(editable.to_string()).await?; Ok(()) } @@ -209,7 +244,7 @@ impl ConfigFile { .ok_or(Error::ProfilesMissing)?; profiles.remove(name); - fs::write(&self.path, editable.to_string()).await?; + self.write(editable.to_string()).await?; Ok(()) } @@ -290,7 +325,7 @@ impl ConfigFile { Some(value) => editable["profiles"][profile_name][name] = toml_edit::value(value), } - fs::write(&self.path, editable.to_string()).await?; + self.write(editable.to_string()).await?; Ok(()) } @@ -324,7 +359,7 @@ impl ConfigFile { } Some(value) => editable[name] = toml_edit::value(value), } - fs::write(&self.path, editable.to_string()).await?; + self.write(editable.to_string()).await?; Ok(()) } } @@ -530,3 +565,107 @@ pub struct TomlProfile { /// A custom cloud endpoint used for development. pub cloud_endpoint: Option, } + +#[cfg(test)] +mod tests { + use tempfile::TempDir; + + use super::*; + + /// Returns whether the file system enforces `path`'s permission bits for + /// this process, which is not the case when running as root. + fn permissions_are_enforced(path: &PathBuf) -> bool { + std::fs::OpenOptions::new().write(true).open(path).is_err() + } + + /// Makes `path` read-only and returns whether the file system enforces + /// that for this process, which is not the case when running as root. + fn make_read_only(path: &PathBuf) -> bool { + let mut permissions = std::fs::metadata(path).unwrap().permissions(); + permissions.set_readonly(true); + std::fs::set_permissions(path, permissions).unwrap(); + + if path.is_dir() { + let probe = path.join("probe"); + let enforced = std::fs::write(&probe, "").is_err(); + let _ = std::fs::remove_file(probe); + enforced + } else { + permissions_are_enforced(path) + } + } + + /// Restores write access so that the temporary directory can be cleaned up. + fn make_writable(path: &PathBuf) { + let mut permissions = std::fs::metadata(path).unwrap().permissions(); + #[allow(clippy::permissions_set_readonly_false)] + permissions.set_readonly(false); + std::fs::set_permissions(path, permissions).unwrap(); + } + + #[mz_ore::test(tokio::test)] + #[cfg_attr(miri, ignore)] // unsupported operation: can't call foreign function `mkdir` + async fn test_load_missing_file() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("missing").join("mz.toml"); + + let config = ConfigFile::load(path.clone()).await.unwrap(); + + assert!(config.profiles().is_none()); + // Loading must not create anything on disk. + assert!(!path.exists()); + assert!(!path.parent().unwrap().exists()); + } + + #[mz_ore::test(tokio::test)] + #[cfg_attr(miri, ignore)] // unsupported operation: can't call foreign function `mkdir` + async fn test_load_read_only_file() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("mz.toml"); + std::fs::write(&path, "profile = \"default\"\n").unwrap(); + let enforced = make_read_only(&path); + + let config = ConfigFile::load(path.clone()).await.unwrap(); + + assert_eq!(config.profile(), "default"); + if enforced { + assert!(config.ensure_writable().await.is_err()); + } + + make_writable(&path); + } + + #[mz_ore::test(tokio::test)] + #[cfg_attr(miri, ignore)] // unsupported operation: can't call foreign function `mkdir` + async fn test_missing_file_in_read_only_directory() { + let dir = TempDir::new().unwrap(); + let parent = dir.path().join("materialize"); + std::fs::create_dir(&parent).unwrap(); + let enforced = make_read_only(&parent); + + let config = ConfigFile::load(parent.join("mz.toml")).await.unwrap(); + + // A missing file is only writable if its parent hierarchy accepts it. + if enforced { + assert!(config.ensure_writable().await.is_err()); + } + + make_writable(&parent); + } + + #[mz_ore::test(tokio::test)] + #[cfg_attr(miri, ignore)] // unsupported operation: can't call foreign function `mkdir` + async fn test_write_creates_parent_directory() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("materialize").join("mz.toml"); + + let config = ConfigFile::load(path.clone()).await.unwrap(); + config.ensure_writable().await.unwrap(); + // The writability check creates what the later write needs. + assert!(path.exists()); + + config.set_param("profile", Some("default")).await.unwrap(); + + assert_eq!(ConfigFile::load(path).await.unwrap().profile(), "default"); + } +} diff --git a/src/mz/src/error.rs b/src/mz/src/error.rs index f0d0864a5df2d..1d2380a4b9ca8 100644 --- a/src/mz/src/error.rs +++ b/src/mz/src/error.rs @@ -16,6 +16,8 @@ //! [`Error`](`enum@Error`) is a custom error type containing multiple variants //! for erros produced by the self crate, internal crates and external crates. +use std::path::PathBuf; + use hyper::header::{InvalidHeaderValue, ToStrError}; use thiserror::Error; use url::ParseError; @@ -88,6 +90,9 @@ pub enum Error { /// I/O Error #[error(transparent)] IOError(#[from] std::io::Error), + /// Error raised when the configuration file exists but cannot be written. + #[error("Error: The configuration file {0} is not writable: {1}")] + ConfigFileNotWritable(PathBuf, #[source] std::io::Error), /// I/O Error #[error(transparent)] CSVParseError(#[from] csv::Error), From d7b0d9eb908baa30d8dc8926ee1c2e2be846cf62 Mon Sep 17 00:00:00 2001 From: mtabebe Date: Mon, 24 Aug 2026 09:37:29 -0400 Subject: [PATCH 4/8] metric sink: skip rows with a null label value (SQL-645) (#38375) Problem: A `map[text=>text]` has no per-value nullability, so a metric sink's `labels` map can hold nulls. Such a row hit `extract_row`, which unwrapped the value as a string and panicked the worker. That takes down clusterd, and the sink re-renders over the same persisted row on restart, crash-looping the whole cluster. Separately, `drop_optimizer_notices` handled only `Index` and `MaterializedView`, so a dropped sink's notices were never retracted. Solution: Skip a row whose label set is not representable, counting it in `mz_compute_metric_sink_skipped`. A null value has nothing to encode, and an empty string is not a stand-in either, since Prometheus reads it as absent and would fold `{a => ''}` into `{}`. Retract a dropped sink's notices through a new `dataflow_metainfo_mut`, the mutable twin of the existing `dataflow_metainfo` getter, shared by both drop sites. Testing: - unit tests - a sqllogictest over a filtered, indexed view that exercises both the panic and the notice retraction on `DROP METRIC SINK`. Closes: [SQL-645](https://linear.app/materializeinc/issue/SQL-645) --- src/adapter/src/catalog/apply.rs | 22 +--- src/catalog/src/memory/objects.rs | 10 ++ src/compute/src/sink/metric_sink.rs | 185 ++++++++++++++++++++++------ test/sqllogictest/metric_sink.slt | 103 +++++++--------- test/testdrive/metric-sink.td | 33 +++++ 5 files changed, 238 insertions(+), 115 deletions(-) diff --git a/src/adapter/src/catalog/apply.rs b/src/adapter/src/catalog/apply.rs index 4a4ca65a423c4..90bf99fc34d0b 100644 --- a/src/adapter/src/catalog/apply.rs +++ b/src/adapter/src/catalog/apply.rs @@ -1598,12 +1598,7 @@ impl CatalogState { // Extract notices directly from the owned dropped entries. for mut entry in dropped_entries { drop_ids.extend(entry.global_ids()); - let metainfo = match entry.item_mut() { - CatalogItem::Index(idx) => idx.dataflow_metainfo.take(), - CatalogItem::MaterializedView(mv) => mv.dataflow_metainfo.take(), - _ => None, - }; - if let Some(mut metainfo) = metainfo { + if let Some(metainfo) = entry.item_mut().dataflow_metainfo_mut() { soft_assert_or_log!( metainfo.optimizer_notices.iter().all_unique(), "should have been pushed there by \ @@ -1639,19 +1634,8 @@ impl CatalogState { if let Some(entry) = self.try_get_entry_by_global_id(item_id) { let catalog_item_id = entry.id(); let entry = self.get_entry_mut(&catalog_item_id); - let item = entry.item_mut(); - match item { - CatalogItem::Index(idx) => { - if let Some(ref mut m) = idx.dataflow_metainfo { - m.optimizer_notices.retain(|x| &n != x); - } - } - CatalogItem::MaterializedView(mv) => { - if let Some(ref mut m) = mv.dataflow_metainfo { - m.optimizer_notices.retain(|x| &n != x); - } - } - _ => {} + if let Some(m) = entry.item_mut().dataflow_metainfo_mut() { + m.optimizer_notices.retain(|x| &n != x); } } } diff --git a/src/catalog/src/memory/objects.rs b/src/catalog/src/memory/objects.rs index 200eb401441fe..39a84ca35c32a 100644 --- a/src/catalog/src/memory/objects.rs +++ b/src/catalog/src/memory/objects.rs @@ -1852,6 +1852,16 @@ impl CatalogItem { } } + /// Returns a mutable reference to the dataflow metainfo, if this item has one. + pub fn dataflow_metainfo_mut(&mut self) -> Option<&mut DataflowMetainfo>> { + match self { + CatalogItem::Index(idx) => idx.dataflow_metainfo.as_mut(), + CatalogItem::MaterializedView(mv) => mv.dataflow_metainfo.as_mut(), + CatalogItem::MetricSink(ms) => ms.dataflow_metainfo.as_mut(), + _ => None, + } + } + /// Returns mutable references to the plan fields (`optimized_plan`, /// `physical_plan`, `dataflow_metainfo`) on plan-bearing items /// (`Index`, `MaterializedView`), or `None` for diff --git a/src/compute/src/sink/metric_sink.rs b/src/compute/src/sink/metric_sink.rs index 5c0a73ecfce97..355e74cf7c3a3 100644 --- a/src/compute/src/sink/metric_sink.rs +++ b/src/compute/src/sink/metric_sink.rs @@ -197,8 +197,9 @@ impl<'scope> SinkRender<'scope> for MetricSinkConnection { /// The source relation exposes `metric_name`, `labels`, `value`, and `help` of the required types, /// and `shape_metric_sink_source` adds the `metric_kind` and `name_valid` columns this reads. /// `resolve` panics if a column is missing. No tree caller enforces this column contract yet; the -/// SQL planner will. `metric_name` and `value` may still be `Datum::Null`; -/// the rest are non-null by construction. Column position within the row is unconstrained. +/// SQL planner will. `metric_name`, `value`, and the values of the `labels` map may still be +/// `Datum::Null` (a `map[text=>text]` has no per-value nullability); the columns themselves are +/// non-null by construction. Column position within the row is unconstrained. struct ColumnIndices { metric_name: usize, labels: usize, @@ -231,6 +232,10 @@ impl ColumnIndices { /// /// The planner already did the row-wise shaping. /// +/// A null label value stays `None` rather than being coerced to a string. Neither a null nor a +/// genuine empty-string value is a representable Prometheus label (an empty value reads as absent), +/// so a row carrying either is skipped downstream instead of published. +/// /// Strings borrow from `datums`, so the caller must own what it needs /// (see `SinkState::stage_ok`) before the row backing `datums` is dropped. fn extract_row<'a>( @@ -240,7 +245,7 @@ fn extract_row<'a>( &'a str, Option, bool, - Vec<(&'a str, &'a str)>, + Vec<(&'a str, Option<&'a str>)>, Option, &'a str, ) { @@ -250,10 +255,10 @@ fn extract_row<'a>( }; let metric_kind = MetricKind::from_datum(datums[cols.metric_kind]); let name_valid = matches!(datums[cols.name_valid], Datum::True); - let mut labels: Vec<(&str, &str)> = datums[cols.labels] + let mut labels: Vec<(&str, Option<&str>)> = datums[cols.labels] .unwrap_map() .iter() - .map(|(k, v)| (k, v.unwrap_str())) + .map(|(k, v)| (k, (!v.is_null()).then(|| v.unwrap_str()))) .collect(); labels.sort(); let value = match datums[cols.value] { @@ -269,7 +274,8 @@ fn extract_row<'a>( /// /// A null value is its own distinct row identity, not a stand-in for any particular number, /// so it is kept apart from every `Some(_)` identity rather than coerced to -/// one. +/// one. A null label value is likewise distinct from a `''` value, so `{a => NULL}` and +/// `{a => ''}` retract only against their own inserts even though both are unpublishable. /// The name and labels lead the tuple so that a `BTreeMap` keeps all rows of one /// `(metric_name, labels)` series adjacent. /// @@ -279,7 +285,7 @@ fn extract_row<'a>( /// granularity of the `skipped` count for rows that are never published either way. type RowKey = ( String, - Vec<(String, String)>, + Vec<(String, Option)>, Option, Option, bool, @@ -363,6 +369,13 @@ fn is_valid_label_name(name: &str) -> bool { } } +/// Whether a label value can be published as-is. A null (`None`) has no value to encode, and an +/// empty string reads as absent to Prometheus, so `{a => ''}` would fold into `{}`. Both make the +/// row that carries them unpublishable. +fn is_publishable_label_value(value: Option<&str>) -> bool { + matches!(value, Some(v) if !v.is_empty()) +} + impl SinkState { /// Buffers one ok-collection update under its timestamp. /// @@ -375,7 +388,7 @@ impl SinkState { metric_name: &str, metric_kind: Option, name_valid: bool, - labels: &[(&str, &str)], + labels: &[(&str, Option<&str>)], value: Option, help: &str, time: Timestamp, @@ -385,7 +398,7 @@ impl SinkState { metric_name.to_string(), labels .iter() - .map(|&(k, v)| (k.to_string(), v.to_string())) + .map(|&(k, v)| (k.to_string(), v.map(str::to_string))) .collect(), value.map(f64::to_bits), metric_kind, @@ -459,8 +472,8 @@ impl SinkState { } } -/// Counts live working rows dropped for an unsupported `metric_type` or an invalid Prometheus -/// metric or label name. +/// Counts live working rows dropped for an unsupported `metric_type`, an invalid Prometheus +/// metric or label name, or a null or empty label value. fn count_skipped(working: &BTreeMap) -> u64 { let mut skipped = 0u64; for ((_name, labels, _bits, metric_kind, name_valid, _help), acc) in working { @@ -468,7 +481,10 @@ fn count_skipped(working: &BTreeMap) -> u64 { continue; } let unsupported = metric_kind.is_none(); - let invalid = !name_valid || !labels.iter().all(|(k, _)| is_valid_label_name(k)); + let invalid = !name_valid + || !labels + .iter() + .all(|(k, v)| is_publishable_label_value(v.as_deref()) && is_valid_label_name(k)); if unsupported || invalid { skipped += 1; } @@ -479,6 +495,9 @@ fn count_skipped(working: &BTreeMap) -> u64 { /// Collapses the live, representable rows of `working` into one published entry per /// `(metric_name, labels)` series and counts colliding and null-suppressed series. /// +/// A row whose label set is not representable (an invalid label name, or a null or empty label +/// value) is dropped here and counted by [`count_skipped`] instead. +/// /// A series collides when more than one distinct live non-null value exists for its /// `(metric_name, labels)`: a genuine conflict of two source rows, unlike an ordinary /// value update whose old row is retracted and new row inserted within the same closed @@ -501,13 +520,31 @@ fn rebuild_published( let Some(kind) = metric_kind else { continue; }; + // A null or empty label value has no representable Prometheus label (an empty value reads + // as absent, folding `{a => ''}` into `{}`), so the whole row is unpublishable. Drop it + // rather than emit a series the source never had. + let Some(labels) = labels + .iter() + .map(|(k, v)| { + is_publishable_label_value(v.as_deref()).then(|| { + ( + k.clone(), + v.as_ref().expect("publishable is non-null").clone(), + ) + }) + }) + .collect::>>() + else { + continue; + }; if !name_valid || !labels.iter().all(|(k, _)| is_valid_label_name(k)) { continue; } - grouped - .entry((name.clone(), labels.clone())) - .or_default() - .push((bits.map(f64::from_bits), *kind, help.clone())); + grouped.entry((name.clone(), labels)).or_default().push(( + bits.map(f64::from_bits), + *kind, + help.clone(), + )); } let mut published = BTreeMap::new(); @@ -660,7 +697,7 @@ impl SinkCollector { ), skipped_gauge: gauge( "mz_compute_metric_sink_skipped", - "The number of input rows skipped for an unsupported metric type or an invalid name.", + "The number of input rows skipped for an unsupported metric type, an invalid name, or a null or empty label value.", ), conflicts_gauge: gauge( "mz_compute_metric_sink_conflicts", @@ -727,12 +764,35 @@ mod tests { } /// `label_a()`, borrowed: what `stage_ok` now takes (see `extract_row`). - const LABEL_A: &[(&str, &str)] = &[("a", "1")]; + const LABEL_A: &[(&str, Option<&str>)] = &[("a", Some("1"))]; fn key_m() -> PublishedKey { ("m".into(), label_a()) } + /// Mirrors the shaped relation `shape_metric_sink_source` builds: `labels`/`help` are + /// non-null by construction, `metric_name`/`value` stay nullable, and `metric_kind`/ + /// `name_valid` are the planner's computed classification columns. + fn shaped_desc() -> RelationDesc { + use mz_repr::SqlScalarType; + + RelationDesc::builder() + .with_column("metric_name", SqlScalarType::String.nullable(true)) + .with_column( + "labels", + SqlScalarType::Map { + value_type: Box::new(SqlScalarType::String), + custom_id: None, + } + .nullable(false), + ) + .with_column("value", SqlScalarType::Float64.nullable(true)) + .with_column("help", SqlScalarType::String.nullable(false)) + .with_column("metric_kind", SqlScalarType::Int32.nullable(true)) + .with_column("name_valid", SqlScalarType::Bool.nullable(true)) + .finish() + } + /// Stages one gauge update for the `(m, {a:1})` series. fn stage_m(st: &mut SinkState, value: f64, time: u64, diff: i64) { st.stage_ok( @@ -891,26 +951,7 @@ mod tests { #[mz_ore::test] fn extract_row_normalizes_null_datums() { - use mz_repr::SqlScalarType; - - // Mirrors the shaped relation `shape_metric_sink_source` builds: `labels`/`help` are - // non-null by construction, `metric_name`/`value` stay nullable, and `metric_kind`/ - // `name_valid` are the planner's computed classification columns. - let desc = RelationDesc::builder() - .with_column("metric_name", SqlScalarType::String.nullable(true)) - .with_column( - "labels", - SqlScalarType::Map { - value_type: Box::new(SqlScalarType::String), - custom_id: None, - } - .nullable(false), - ) - .with_column("value", SqlScalarType::Float64.nullable(true)) - .with_column("help", SqlScalarType::String.nullable(false)) - .with_column("metric_kind", SqlScalarType::Int32.nullable(true)) - .with_column("name_valid", SqlScalarType::Bool.nullable(true)) - .finish(); + let desc = shaped_desc(); let cols = ColumnIndices::resolve(&desc); let mut row = Row::default(); @@ -929,11 +970,77 @@ mod tests { assert_eq!(name, ""); assert_eq!(metric_kind, None); assert!(!name_valid); - assert_eq!(labels, Vec::<(&str, &str)>::new()); + assert_eq!(labels, Vec::new()); assert_eq!(value, None); assert_eq!(help, ""); } + /// A null map value must survive extraction as `None`; unwrapping it as a string panicked the + /// worker and took down the replica. + #[mz_ore::test] + fn extract_row_keeps_null_label_values() { + let desc = shaped_desc(); + let cols = ColumnIndices::resolve(&desc); + + let mut row = Row::default(); + { + let mut packer = row.packer(); + packer.push(Datum::String("m")); + packer.push_dict_with(|row| { + row.push(Datum::String("bad")); + row.push(Datum::Null); + row.push(Datum::String("good")); + row.push(Datum::String("1")); + }); + packer.push(Datum::Float64(1.0.into())); + packer.push(Datum::String("h")); + packer.push(Datum::Int32(0)); + packer.push(Datum::True); + } + + let datums: Vec = row.iter().collect(); + let (_name, _metric_kind, _name_valid, labels, _value, _help) = extract_row(&cols, &datums); + assert_eq!(labels, vec![("bad", None), ("good", Some("1"))]); + } + + #[mz_ore::test] + fn null_or_empty_label_value_skips_row() { + let mut st = SinkState::default(); + // A null label value has no representable label set: publishes nothing, counts as skipped. + st.stage_ok( + "m", + Some(MetricKind::Gauge), + true, + &[("a", None)], + Some(1.0), + "h", + Timestamp::from(1), + 1, + ); + st.integrate(&frontier(2)); + st.publish_if_healthy(); + assert!(st.published.is_empty()); + assert_eq!(st.skipped, 1); + + // An empty label value folds to `{}` in Prometheus, so it is skipped too rather than + // published as a bare `{}` series. It is a distinct identity from the null row, so both + // stay live and count: `skipped` reaches 2, not 1. + st.stage_ok( + "m", + Some(MetricKind::Gauge), + true, + &[("a", Some(""))], + Some(1.0), + "h", + Timestamp::from(3), + 1, + ); + st.integrate(&frontier(4)); + st.publish_if_healthy(); + assert!(st.published.is_empty()); + assert_eq!(st.skipped, 2); + } + fn pkey(name: &str, labels: &[(&str, &str)]) -> PublishedKey { ( name.to_string(), diff --git a/test/sqllogictest/metric_sink.slt b/test/sqllogictest/metric_sink.slt index b5b76ffdddf59..aa41331d4c262 100644 --- a/test/sqllogictest/metric_sink.slt +++ b/test/sqllogictest/metric_sink.slt @@ -389,63 +389,52 @@ CREATE METRIC SINK rbac_ms FROM rbac_src WITH (PREFIX = 'mz_metric_sink_rbac_') ---- COMPLETE 0 -# TODO(linear#SQL-645): re-enable the block below once the sink operator skips a row whose -# `labels` map carries a null value. Today `extract_row` unwraps such a value as a string and -# panics, which aborts clusterd, so the test reproduces the crash instead of asserting the -# behavior it describes. -# -# # A `map[text=>text]` legally holds null *values*, and the shaping coalesces only -# # a null `labels` map as a whole, so such a row reaches the sink operator, which -# # has no label set it can represent and must skip it. Panicking instead aborts the -# # whole clusterd process, and the sink re-renders over the same persisted row on -# # restart, crash-looping every dataflow on the cluster. -# # -# # The same sink covers notice bookkeeping: `LiteralConstraints` attaches an "index -# # too wide" notice for the `metric_name = 'm'` filter over `nulls_idx`, keyed by -# # the sink's own id, and `DROP METRIC SINK` has to retract it again. -# -# statement ok -# CREATE TABLE nulls (metric_name text, metric_type text, labels map[text=>text], value double, help text) -# -# statement ok -# INSERT INTO nulls VALUES ('m', 'gauge', '{good=>1, bad=>NULL}'::map[text=>text], 1, 'h') -# -# statement ok -# CREATE INDEX nulls_idx ON nulls (metric_name, metric_type) -# -# statement ok -# CREATE VIEW nulls_v AS SELECT * FROM nulls WHERE metric_name = 'm' -# -# statement ok -# CREATE METRIC SINK ms_nulls FROM nulls_v WITH (PREFIX = 'mz_metric_sink_nulls_') -# -# # The replica folded that row and still answers: `count(*)` needs a dataflow on -# # `quickstart`, the cluster the sink runs on. -# -# query I -# SELECT count(*) FROM nulls -# ---- -# 1 -# -# # `mz_optimizer_notices` is readable only by `mz_monitor`, hence `mz_system`. -# -# simple conn=mz_system,user=mz_system -# SELECT count(*) FROM mz_internal.mz_optimizer_notices WHERE message LIKE '%nulls_idx%' -# ---- -# 1 -# COMPLETE 1 -# -# statement ok -# DROP METRIC SINK ms_nulls -# -# simple conn=mz_system,user=mz_system -# SELECT count(*) FROM mz_internal.mz_optimizer_notices WHERE message LIKE '%nulls_idx%' -# ---- -# 0 -# COMPLETE 1 -# -# statement ok -# DROP TABLE nulls CASCADE +# A `map[text=>text]` can hold null *values*, so such a row reaches the sink, which has no label +# set to represent and must skip it. Panicking instead crash-loops every dataflow on the cluster. +# This sink also covers notice bookkeeping: `LiteralConstraints` attaches an "index too wide" notice +# keyed by the sink's own id that `DROP METRIC SINK` must retract. + +statement ok +CREATE TABLE nulls (metric_name text, metric_type text, labels map[text=>text], value double, help text) + +statement ok +INSERT INTO nulls VALUES ('m', 'gauge', '{good=>1, bad=>NULL}'::map[text=>text], 1, 'h') + +statement ok +CREATE INDEX nulls_idx ON nulls (metric_name, metric_type) + +statement ok +CREATE VIEW nulls_v AS SELECT * FROM nulls WHERE metric_name = 'm' + +statement ok +CREATE METRIC SINK ms_nulls FROM nulls_v WITH (PREFIX = 'mz_metric_sink_nulls_') + +# The replica folded that row and still answers a `count(*)` on the sink's cluster. + +query I +SELECT count(*) FROM nulls +---- +1 + +# `mz_optimizer_notices` is readable only by `mz_monitor`, hence `mz_system`. + +simple conn=mz_system,user=mz_system +SELECT count(*) FROM mz_internal.mz_optimizer_notices WHERE message LIKE '%nulls_idx%' +---- +1 +COMPLETE 1 + +statement ok +DROP METRIC SINK ms_nulls + +simple conn=mz_system,user=mz_system +SELECT count(*) FROM mz_internal.mz_optimizer_notices WHERE message LIKE '%nulls_idx%' +---- +0 +COMPLETE 1 + +statement ok +DROP TABLE nulls CASCADE # With the flag off the statement is refused, and the error says which flag. diff --git a/test/testdrive/metric-sink.td b/test/testdrive/metric-sink.td index 870da103caef9..cb6f05705bbd3 100644 --- a/test/testdrive/metric-sink.td +++ b/test/testdrive/metric-sink.td @@ -191,3 +191,36 @@ contains:still depended upon by metric sink "s" 0 > DROP TABLE t + +# A null or empty label *value* has no representable Prometheus label set: a null has nothing to +# encode, and an empty value reads as absent to Prometheus, which would fold `{a => ''}` into `{}`. +# The sink skips the whole row and counts it in `mz_compute_metric_sink_skipped` rather than +# publish a mislabeled series. `map_build` keeps a null value (it drops only null keys), so the two +# rows reach the sink as `{a => NULL}` and `{a => ''}`, distinct identities that are each skipped. +> CREATE TABLE lt (lv text) + +> INSERT INTO lt VALUES (NULL), ('') + +> CREATE VIEW lv AS + SELECT 'skip_test'::text AS metric_name, + 'gauge'::text AS metric_type, + map_build(LIST[ROW('a', lv)])::map[text=>text] AS labels, + 1::double AS value, + 'a metric skipped by the metric sink e2e test'::text AS help + FROM lt + +> 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. +> SELECT value FROM mz_introspection.mz_cluster_prometheus_metrics + WHERE metric_name = 'mz_compute_metric_sink_skipped' +2 + +# Neither row publishes a series under the sink's metric name. +> SELECT count(*) FROM mz_introspection.mz_cluster_prometheus_metrics + WHERE metric_name = 'mz_metric_sink_skip_skip_test' +0 + +> DROP TABLE lt CASCADE From ed7294b842a4883db2355ac0dd8bfa4152aee8b8 Mon Sep 17 00:00:00 2001 From: mtabebe Date: Mon, 24 Aug 2026 10:35:37 -0400 Subject: [PATCH 5/8] adapter: hydrate migrated builtin MVs before 0dt cut-over (#38399) Problem: A builtin schema migration using the `Replacement` mechanism hands the new deployment a fresh persist shard. Nothing writes that shard while the deployment is read-only, because `ComputeController::allow_writes` no-ops in read-only mode, so the MV's write frontier never advances and the 0dt readiness gate has to drop it and everything downstream of it from the caught-up check. The deployment then promotes with those collections unhydrated, and they all hydrate at once at cut-over, spiking catalog-server CPU and degrading catalog queries. Solution: The replacement shard is exclusively owned by this deployment, so the MV can write it while we are still read-only. Do that, and keep the MV in the readiness gate, so it hydrates before cut-over instead of at it. Previously we didn't do this for builtins derived from mz_catalog because catalog shard frontier was not held. However, as of 26.17 it is, so gate the behaviour on the version of the old leader. Testing: - New environmentd integration test `test_0dt_migrated_builtin_mv_hydrates_before_promotion` boots a read-only generation with a forced `replacement` migration and asserts the migrated builtin MVs `mz_databases` and `mz_clusters` are readable before it reports `ReadyToPromote`, proving they hydrate before cut-over rather than at it. - Extended the `0dt` mzcompose workflow `builtin-schema-migrations-replacement` to read those MVs from the read-only generation before promotion, guarding the same invariant through a real cross-version upgrade. --- .../20251015_builtin_schema_migration.md | 12 +- misc/python/materialize/mzcompose/__init__.py | 1 + .../materialize/parallel_workload/action.py | 1 + src/adapter-types/src/dyncfgs.rs | 14 ++ src/adapter/src/catalog.rs | 1 + src/adapter/src/catalog/open.rs | 34 +++- .../catalog/open/builtin_schema_migration.rs | 14 +- src/adapter/src/coord.rs | 107 ++++++++--- src/adapter/src/coord/caught_up.rs | 61 +++++- src/catalog-debug/src/main.rs | 2 +- src/catalog/src/builtin.rs | 10 + src/compute-client/src/controller.rs | 56 ++++++ src/environmentd/src/test_util.rs | 11 +- src/environmentd/tests/server.rs | 181 ++++++++++++++++++ src/storage-controller/src/lib.rs | 4 + test/0dt/mzcompose.py | 19 ++ 16 files changed, 482 insertions(+), 46 deletions(-) diff --git a/doc/developer/design/20251015_builtin_schema_migration.md b/doc/developer/design/20251015_builtin_schema_migration.md index 478c0d96ae8ca..d1e87bdd5798d 100644 --- a/doc/developer/design/20251015_builtin_schema_migration.md +++ b/doc/developer/design/20251015_builtin_schema_migration.md @@ -58,7 +58,8 @@ Doing so requires no writes to durable state, and therefore doesn't interfere wi In the subsequent read-only bootstrap phase, the process creates persist read and write handles using the new schema. Read handles perform transparent migration of any data updates that flow through them, so dataflow hydration can proceed using the new schema. -Write handles only require a matching registered shard schema when writing batches, which is something a read-only process doesn't do. +Write handles only require a matching registered shard schema when writing batches, which a read-only process performing schema evolution doesn't do: the shard is the leader's live one, so it stays read-only until promotion. +(Shard replacement is different: there the read-only process does write batches, but to a shard it created for itself. See below.) Once the read-only process gets promoted to a leader, and runs the builtin schema migration mechanism again, it this time registers the new schema with the persist shard. Doing so fences out any processes that planned to evolve the schema to an earlier version. @@ -76,6 +77,15 @@ Notably, it ignores any entries at different versions or deploy generations, to Depending on the existing migration shard entries, the process either decides to use the existing replacement shard, or to create the replacement shard and write its ID into the migration shard, at the current version. It sets the new shard ID as the migrated collection's shard in its in-memory catalog and commences bootstrapping using the replacement shard. +Because this environment exclusively owns the replacement shard, the read-only process force-writes it during bootstrap rather than leaving it read-only until promotion. +This lets a migrated builtin materialized view and its dependents hydrate before cut-over instead of all at once at cut-over. +It is safe only for the self-owned replacement shard, never a shard the leader is still serving, which is why it applies to shard replacement and not schema evolution. + +The force-write is conditional on two things. +First, the leader must be at v26.17 or later, because every builtin materialized view reads the catalog shard and only leaders from that version on keep its frontier advancing with the current time; against an older leader the dataflow would sit at a stale frontier. +Second, the `enable_0dt_hydrate_migrated_builtin_mvs` feature flag must be on; it exists as a break-glass revert. +When either condition does not hold, the migrated materialized views and their dependents are instead excluded from the 0dt caught-up check, which is the older behaviour: promotion proceeds without them and they hydrate at cut-over. + A leader process performing shard replacement performs the same steps as in read-only mode. Additionally, it cleans up durable state written by earlier versions and/or deploy generations by: - arranging for the previous shards used by the migrated storage collections to be finalized diff --git a/misc/python/materialize/mzcompose/__init__.py b/misc/python/materialize/mzcompose/__init__.py index 0731ed7ea5c6e..63b8ce5312bb1 100644 --- a/misc/python/materialize/mzcompose/__init__.py +++ b/misc/python/materialize/mzcompose/__init__.py @@ -750,6 +750,7 @@ def get_default_system_parameters( "with_0dt_caught_up_check_cutoff", "enable_0dt_caught_up_replica_status_check", "enable_0dt_caught_up_stability_check", + "enable_0dt_hydrate_migrated_builtin_mvs", "plan_insights_notice_fast_path_clusters_optimize_duration", "enable_expression_cache", "mz_metrics_lgalloc_map_refresh_interval", diff --git a/misc/python/materialize/parallel_workload/action.py b/misc/python/materialize/parallel_workload/action.py index 619fb5be810ec..b1bb873b81ded 100644 --- a/misc/python/materialize/parallel_workload/action.py +++ b/misc/python/materialize/parallel_workload/action.py @@ -3284,6 +3284,7 @@ def __init__( "with_0dt_caught_up_check_cutoff", "with_0dt_caught_up_check_stability_period", "enable_0dt_caught_up_stability_check", + "enable_0dt_hydrate_migrated_builtin_mvs", "enable_statement_lifecycle_logging", "enable_introspection_subscribes", "plan_insights_notice_fast_path_clusters_optimize_duration", diff --git a/src/adapter-types/src/dyncfgs.rs b/src/adapter-types/src/dyncfgs.rs index 18a3b2bdcc12b..259972ef21973 100644 --- a/src/adapter-types/src/dyncfgs.rs +++ b/src/adapter-types/src/dyncfgs.rs @@ -90,6 +90,19 @@ pub const WITH_0DT_CAUGHT_UP_CHECK_STABILITY_PERIOD: Config = Config:: ParameterScope::Environment, ); +pub const ENABLE_0DT_HYDRATE_MIGRATED_BUILTIN_MVS: Config = Config::new( + "enable_0dt_hydrate_migrated_builtin_mvs", + true, + "Write-enable replacement-migrated builtin materialized views while read-only during a 0dt \ + deployment, so they hydrate before cut-over and keep gating promotion. Emergency break-glass \ + flag: disabling excludes migrated MVs (and their dependents) from the caught-up check again, \ + so promotion proceeds with them unhydrated. Not an exact revert: a collection with no live \ + leader frontier must be hydrated either way. Only takes effect when the leader is new enough \ + for the write to make progress, and is read once at startup, so changing it means setting it \ + on the leader and restarting the new deployment.", + ParameterScope::Environment, +); + /// Enable logging of statement lifecycle events in mz_internal.mz_statement_lifecycle_history. pub const ENABLE_STATEMENT_LIFECYCLE_LOGGING: Config = Config::new( "enable_statement_lifecycle_logging", @@ -511,6 +524,7 @@ pub fn all_dyncfgs(configs: ConfigSet) -> ConfigSet { .add(&ENABLE_0DT_CAUGHT_UP_REPLICA_STATUS_CHECK) .add(&ENABLE_0DT_CAUGHT_UP_STABILITY_CHECK) .add(&WITH_0DT_CAUGHT_UP_CHECK_STABILITY_PERIOD) + .add(&ENABLE_0DT_HYDRATE_MIGRATED_BUILTIN_MVS) .add(&ENABLE_STATEMENT_LIFECYCLE_LOGGING) .add(&ENABLE_INTROSPECTION_SUBSCRIBES) .add(&ENABLE_FRONTEND_SUBSCRIBES) diff --git a/src/adapter/src/catalog.rs b/src/adapter/src/catalog.rs index 4b06ecb55e559..f21271b1d89f3 100644 --- a/src/adapter/src/catalog.rs +++ b/src/adapter/src/catalog.rs @@ -648,6 +648,7 @@ impl Catalog { let OpenCatalogResult { catalog, + last_seen_version: _, migrated_storage_collections_0dt: _, new_builtin_collections: _, builtin_table_updates: _, diff --git a/src/adapter/src/catalog/open.rs b/src/adapter/src/catalog/open.rs index e6801620cc15a..af7046dc84d50 100644 --- a/src/adapter/src/catalog/open.rs +++ b/src/adapter/src/catalog/open.rs @@ -61,6 +61,7 @@ use mz_sql::session::user::{MZ_SYSTEM_ROLE_ID, SYSTEM_USER}; use mz_sql::session::vars::{SessionVars, SystemVars, VarError, VarInput}; use mz_storage_client::controller::{StorageMetadata, StorageTxn}; use mz_storage_client::storage_collections::StorageCollections; +use semver::Version; use tracing::{Instrument, info, warn}; use uuid::Uuid; @@ -73,14 +74,26 @@ use crate::catalog::{BuiltinTableUpdate, Catalog, CatalogState, Config, is_reser pub struct InitializeStateResult { /// An initialized [`CatalogState`]. pub state: CatalogState, - /// A set of new shards that may need to be initialized (only used by 0dt migration). + /// Items whose builtin schema migration allocated a fresh, self-owned persist shard. Only used + /// by 0dt migration. + /// + /// `Replacement`-migrated items only: derived by filtering to items with a global id in + /// `MigrationRunResult::new_shards`, which only `migrate_replace` populates. `Evolution` + /// migrates in place and reuses the leader's shard, so it never lands here. Read-only write + /// paths depend on that: force-writing a shard while read-only is safe only because we + /// exclusively own it (see `ComputeController::allow_writes_in_read_only`). pub migrated_storage_collections_0dt: BTreeSet, /// A set of new builtin items. pub new_builtin_collections: BTreeSet, /// A list of builtin table updates corresponding to the initialized state. pub builtin_table_updates: Vec, - /// The version of the catalog that existed before initializing the catalog. - pub last_seen_version: String, + /// The version of the binary that last committed catalog migrations, or `None` for a newly + /// initialized catalog. + /// + /// While this environment is read-only during a 0dt deployment, this is the version of the + /// leader environment: the read-only catalog transaction is a savepoint, so our own bump of + /// the setting never lands. + pub last_seen_version: Option, /// A handle to the expression cache if it's enabled. pub expr_cache_handle: Option, /// The global expressions that were cached in `expr_cache_handle`. @@ -92,7 +105,10 @@ pub struct InitializeStateResult { pub struct OpenCatalogResult { /// An opened [`Catalog`]. pub catalog: Catalog, - /// A set of new shards that may need to be initialized. + /// See [`InitializeStateResult::last_seen_version`]. + pub last_seen_version: Option, + /// See [`InitializeStateResult::migrated_storage_collections_0dt`]; `Replacement`-migrated + /// items only. pub migrated_storage_collections_0dt: BTreeSet, /// A set of new builtin items. pub new_builtin_collections: BTreeSet, @@ -430,8 +446,7 @@ impl Catalog { .await; builtin_table_updates.extend(builtin_table_update); - let last_seen_version = - get_migration_version(&txn).map_or_else(|| "new".into(), |v| v.to_string()); + let last_seen_version = get_migration_version(&txn); let mz_authentication_mock_nonce = txn.get_authentication_mock_nonce().ok_or_else(|| { @@ -453,7 +468,9 @@ impl Catalog { .await .map_err(|e| { Error::new(ErrorKind::FailedCatalogMigration { - last_seen_version: last_seen_version.clone(), + last_seen_version: last_seen_version + .as_ref() + .map_or_else(|| "new".to_string(), |v| v.to_string()), this_version: config.build_info.version, cause: e.to_string(), }) @@ -566,7 +583,7 @@ impl Catalog { migrated_storage_collections_0dt, new_builtin_collections, mut builtin_table_updates, - last_seen_version: _, + last_seen_version, expr_cache_handle, cached_global_exprs, uncached_local_exprs, @@ -624,6 +641,7 @@ impl Catalog { Ok(OpenCatalogResult { catalog, + last_seen_version, migrated_storage_collections_0dt, new_builtin_collections, builtin_table_updates, diff --git a/src/adapter/src/catalog/open/builtin_schema_migration.rs b/src/adapter/src/catalog/open/builtin_schema_migration.rs index 8af544edca8b5..36dc0e75b9850 100644 --- a/src/adapter/src/catalog/open/builtin_schema_migration.rs +++ b/src/adapter/src/catalog/open/builtin_schema_migration.rs @@ -37,8 +37,9 @@ use futures::future::BoxFuture; use mz_build_info::{BuildInfo, DUMMY_BUILD_INFO}; use mz_catalog::builtin::{ BUILTIN_LOOKUP, Builtin, Fingerprint, MZ_CATALOG_RAW, MZ_CATALOG_RAW_DESCRIPTION, - MZ_OBJECT_ARRANGEMENT_SIZE_HISTORY_DESCRIPTION, MZ_STORAGE_USAGE_BY_SHARD, - MZ_STORAGE_USAGE_BY_SHARD_DESCRIPTION, RUNTIME_ALTERABLE_FINGERPRINT_SENTINEL, + MZ_CLUSTER_REPLICA_FRONTIERS_DESCRIPTION, MZ_OBJECT_ARRANGEMENT_SIZE_HISTORY_DESCRIPTION, + MZ_STORAGE_USAGE_BY_SHARD, MZ_STORAGE_USAGE_BY_SHARD_DESCRIPTION, + RUNTIME_ALTERABLE_FINGERPRINT_SENTINEL, }; use mz_catalog::config::BuiltinItemMigrationConfig; use mz_catalog::durable::objects::SystemObjectUniqueIdentifier; @@ -758,6 +759,15 @@ impl Migration { "mz_catalog_raw cannot be migrated" ); + // The 0dt caught-up gate reads the leader's `mz_cluster_replica_frontiers` shard for + // the live frontiers it checks every collection against. Migrating it via `Replacement` + // hands us a fresh shard we write ourselves, so the gate would compare us against + // ourselves instead of against the leader. + assert_ne!( + &*MZ_CLUSTER_REPLICA_FRONTIERS_DESCRIPTION, object, + "mz_cluster_replica_frontiers cannot be migrated or else the 0dt caught-up gate loses its live-frontier reference" + ); + let Some(object_info) = self.system_objects.get(object) else { panic!("migration step for non-existent builtin: {object:?}"); }; diff --git a/src/adapter/src/coord.rs b/src/adapter/src/coord.rs index f621713c9272a..0018adb4471d7 100644 --- a/src/adapter/src/coord.rs +++ b/src/adapter/src/coord.rs @@ -93,7 +93,8 @@ use mz_adapter_types::compaction::CompactionWindow; use mz_adapter_types::connection::ConnectionId; use mz_adapter_types::dyncfgs::FRONTEND_READ_THEN_WRITE; use mz_adapter_types::dyncfgs::{ - USER_ID_POOL_BATCH_SIZE, WITH_0DT_DEPLOYMENT_CAUGHT_UP_CHECK_INTERVAL, + ENABLE_0DT_HYDRATE_MIGRATED_BUILTIN_MVS, USER_ID_POOL_BATCH_SIZE, + WITH_0DT_DEPLOYMENT_CAUGHT_UP_CHECK_INTERVAL, }; use mz_auth::password::Password; use mz_build_info::BuildInfo; @@ -171,6 +172,7 @@ use mz_storage_types::sources::{IngestionDescription, SourceExport, Timeline}; use mz_timestamp_oracle::{TimestampOracleConfig, WriteTimestamp}; use mz_transform::dataflow::DataflowMetainfo; use opentelemetry::trace::TraceContextExt; +use semver::Version; use serde::Serialize; use thiserror::Error; use timely::progress::{Antichain, Timestamp as _}; @@ -243,6 +245,17 @@ mod privatelink_status; mod sql; mod validity; +/// The oldest leader version against which a replacement-migrated builtin materialized view may +/// write its new persist shard while this environment is still read-only. +/// +/// Every builtin materialized view reads `mz_internal.mz_catalog_raw`, so its dataflow only makes +/// progress up to the catalog shard's frontier. Holding that frontier at the current time is the +/// leader's job, and leaders only started doing it in v26.17 (PR #35402). Write-enable such an MV +/// against an older leader and it sits at a stale frontier and never reports caught up, which +/// blocks promotion outright instead of merely leaving the collection cold at cut-over. We still +/// support upgrading from before v26.17, so that leader is a real case, not a hypothetical. +const MIN_LEADER_VERSION_FOR_MIGRATED_MV_WRITES: Version = Version::new(26, 17, 0); + /// A pool of pre-allocated user IDs to avoid per-DDL persist writes. /// /// IDs in the range `[next, upper)` are available for allocation. @@ -2450,6 +2463,7 @@ impl Coordinator { &mut self, boot_ts: Timestamp, migrated_storage_collections_0dt: BTreeSet, + hydrate_migrated_mvs: bool, mut builtin_table_updates: Vec, cached_global_exprs: BTreeMap, uncached_local_exprs: BTreeMap, @@ -2785,10 +2799,30 @@ impl Coordinator { self.ship_dataflow(df_desc, mview.cluster_id, mview.target_replica) .await; - // If this is a replacement MV, it must remain read-only until the replacement - // gets applied. + // A pending `REPLACEMENT FOR` MV must stay read-only until + // `ALTER ... APPLY REPLACEMENT` swaps it in. Unrelated to the + // builtin-migration `Replacement` mechanism below. if mview.replacement_target.is_none() { - self.allow_writes(mview.cluster_id, mview.global_id_writes()); + let gid = mview.global_id_writes(); + if hydrate_migrated_mvs + && migrated_storage_collections_0dt.contains(&entry.id()) + { + // `migrated_storage_collections_0dt` is `Replacement`-migrated items + // only, so this is a fresh shard we own: nothing else writes it, and + // writing it while read-only hydrates the MV and its dependents before + // cut-over. An `Evolution`-migrated MV reuses the leader's live shard + // and must never reach here. + // + // A *new* builtin MV gets no such treatment: its shard allocation + // lives only in this read-only savepoint, so the promoted leader + // allocates a different shard and discards whatever we wrote. + self.controller + .compute + .allow_writes_in_read_only(mview.cluster_id, gid) + .unwrap_or_terminate("allow_writes cannot fail"); + } else { + self.allow_writes(mview.cluster_id, gid); + } } } CatalogItem::MetricSink(metric_sink) => { @@ -5036,6 +5070,7 @@ pub fn serve( ; let OpenCatalogResult { mut catalog, + last_seen_version, migrated_storage_collections_0dt, new_builtin_collections, builtin_table_updates, @@ -5091,6 +5126,22 @@ pub fn serve( catalog_open_start.elapsed() ); + // Whether replacement-migrated builtin MVs may write their new shards before cut-over. + // Both `bootstrap` and the readiness gate below read this, and they have to agree. + // `MIN_LEADER_VERSION_FOR_MIGRATED_MV_WRITES` explains why the leader's version settles it. + // + // While we are read-only, `last_seen_version` is that leader's version: our catalog + // transaction is a savepoint, so our own bump of the setting never lands. `None` means a + // freshly initialized catalog, with nothing migrated and no leader to be compatible with. + // + // `ENABLE_0DT_HYDRATE_MIGRATED_BUILTIN_MVS` is the break-glass revert: off falls back to + // excluding migrated MVs from the caught-up gate, no redeploy needed. + let hydrate_migrated_mvs = ENABLE_0DT_HYDRATE_MIGRATED_BUILTIN_MVS + .get(catalog.system_config().dyncfgs()) + && last_seen_version + .as_ref() + .is_none_or(|version| *version >= MIN_LEADER_VERSION_FOR_MIGRATED_MV_WRITES); + let coord_thread_start = Instant::now(); info!("startup: coordinator init: coordinator thread start beginning"); @@ -5141,34 +5192,31 @@ pub fn serve( // A collection that can't advance its write frontier in read-only mode // stalls its transitive dependents too, so exclude those from the caught-up - // check as well. That's migrated MVs (their dataflows don't write in - // read-only mode) and new builtin MVs (their fresh shard has no writer until - // this deployment promotes). An excluded dependent may still be hydrating - // right after promotion, a brief blip we accept because these MVs are small - // and get a writer at cut-over. + // check as well. That's every *new* builtin collection, whose fresh shard has no + // writer until this deployment promotes, plus migrated MVs whenever the leader is + // too old for them to write. An excluded dependent may still be hydrating right + // after promotion, a brief blip we accept because these collections are small and + // get a writer at cut-over. // - // TODO: Consider sending `allow_writes` for the dataflows of migrated MVs, which - // would allow them to make progress even in read-only mode. This doesn't - // work for MVs based on `mz_catalog_raw`, if the leader's version is less - // than v26.17, since before that version the catalog shard's frontier wasn't - // kept up-to-date with the current time. So this workaround has to remain in - // place upgrades from a version less than v26.17 are no longer supported. - let new_builtin_mvs = new_builtin_collections - .iter() - .map(|global_id| { - catalog - .state() - .try_get_entry_by_global_id(global_id) - .expect("new builtin collections have catalog entries") - }) - .filter(|entry| entry.is_materialized_view()) - .map(|entry| entry.id()); - let mut todo: Vec<_> = migrated_storage_collections_0dt + // Seeded from all of `new_builtin_collections`, not just the MVs: a new builtin + // table or source has no read-only writer either (`register_table_collections` + // retains only *migrated* tables), so an MV reading one never advances past its + // empty frontier. A *migrated* table is the opposite case, even though a builtin + // MV can read one (`mz_clusters` joins `mz_cluster_replica_size_internal`): + // `read_only_mode_table_worker` keeps advancing migrated tables' uppers. + let new_builtin_items = new_builtin_collections.iter().map(|global_id| { + catalog + .state() + .try_get_entry_by_global_id(global_id) + .expect("new builtin collections have catalog entries") + .id() + }); + let frozen_migrated_mvs = migrated_storage_collections_0dt .iter() .copied() - .filter(|id| catalog.state().get_entry(id).is_materialized_view()) - .chain(new_builtin_mvs) - .collect(); + .filter(|_| !hydrate_migrated_mvs) + .filter(|id| catalog.state().get_entry(id).is_materialized_view()); + let mut todo: Vec<_> = new_builtin_items.chain(frozen_migrated_mvs).collect(); while let Some(item_id) = todo.pop() { let entry = catalog.state().get_entry(&item_id); exclude_collections.extend(entry.global_ids()); @@ -5341,6 +5389,7 @@ pub fn serve( .bootstrap( boot_ts, migrated_storage_collections_0dt, + hydrate_migrated_mvs, builtin_table_updates, cached_global_exprs, uncached_local_exprs, diff --git a/src/adapter/src/coord/caught_up.rs b/src/adapter/src/coord/caught_up.rs index 33692571e944c..37228f8682ea0 100644 --- a/src/adapter/src/coord/caught_up.rs +++ b/src/adapter/src/coord/caught_up.rs @@ -271,6 +271,12 @@ impl Coordinator { // `snapshot_latest` requires that the collection consolidates to a // set. `mz_cluster_replica_frontiers` is a controller-managed builtin // written with ±1 diffs, so it satisfies that invariant. + // + // NOTE: these are the leader's frontiers only because we read the leader's shard. + // `validate_migration_steps` forbids migrating `mz_cluster_replica_frontiers` for this + // reason, so a declared migration can't reach here. A test forcing replacement across all + // builtins bypasses that guard, hands us a shard we write ourselves, and the lag check + // below then compares this deployment against itself. let live_frontiers = self .controller .storage_collections @@ -614,10 +620,57 @@ impl Coordinator { let live_write_frontier = match live_frontiers.get(&id) { Some(frontier) => frontier, None => { - // The collection didn't previously exist, so consider - // ourselves hydrated as long as our write_ts is > 0. - tracing::info!(?write_frontier, "collection {id} not in live frontiers"); - if write_frontier.less_equal(&Timestamp::minimum()) { + // No live frontier to compare against, either because the collection didn't + // exist on the leader or because the leader hosts it as something + // `mz_cluster_replica_frontiers` doesn't track. A table→MV conversion is the + // latter: it keeps the table's `GlobalId`, still a table on the leader, so the + // new MV lands here instead of the strong path below. + // + // Require hydration, not just a write frontier past the minimum. A fresh MV's + // sink reaches frontier 1 after one batch, which would otherwise look caught + // up mid-hydration and bring back the cut-over spike this gate prevents. + let collection_hydrated = match collection_type { + CollectionType::Compute => { + self.controller + .compute + .collection_hydrated(cluster.id, id) + .await? + } + CollectionType::Storage => { + self.controller.storage.collection_hydrated(id)? + } + }; + + // Also require the frontier to be within the allowed lag, the bound the + // live-frontier path applies, with `now` standing in for the missing live + // frontier. Hydration is one-shot: a collection that hydrated and then stalled + // would otherwise satisfy this branch forever. + // + // NOTE: there is deliberately no `cutoff` escape hatch here. A frontier frozen + // at the minimum is exactly what this gate must catch, so a collection stuck + // here blocks promotion until `with_0dt_deployment_max_wait` elapses. + let write_frontier_plus_allowed_lag = Antichain::from_iter( + write_frontier + .iter() + .map(|t| t.step_forward_by(&allowed_lag)), + ); + let within_lag = PartialOrder::less_equal( + &Antichain::from_elem(now), + &write_frontier_plus_allowed_lag, + ); + + tracing::info!( + ?write_frontier, + %collection_hydrated, + %within_lag, + ?allowed_lag, + ?now, + "collection {id} not in live frontiers" + ); + if write_frontier.less_equal(&Timestamp::minimum()) + || !collection_hydrated + || !within_lag + { all_caught_up = false; } continue; diff --git a/src/catalog-debug/src/main.rs b/src/catalog-debug/src/main.rs index 9a8ad5a4d5c2f..48fa96d4eacd6 100644 --- a/src/catalog-debug/src/main.rs +++ b/src/catalog-debug/src/main.rs @@ -709,7 +709,7 @@ async fn upgrade_check( let msg = format!( "catalog upgrade from {} to {} would succeed in about {} ms", - last_seen_version, + last_seen_version.map_or_else(|| "new".into(), |v| v.to_string()), BUILD_INFO.human_version(None), dur.as_millis(), ); diff --git a/src/catalog/src/builtin.rs b/src/catalog/src/builtin.rs index e7eeb0ccd017f..3428608461df8 100644 --- a/src/catalog/src/builtin.rs +++ b/src/catalog/src/builtin.rs @@ -836,6 +836,16 @@ pub static MZ_OBJECT_ARRANGEMENT_SIZE_HISTORY_DESCRIPTION: LazyLock = + LazyLock::new(|| SystemObjectDescription { + schema_name: MZ_CLUSTER_REPLICA_FRONTIERS.schema.to_string(), + object_type: CatalogItemType::Source, + object_name: MZ_CLUSTER_REPLICA_FRONTIERS.name.to_string(), + }); pub const MZ_SYSTEM_ROLE: BuiltinRole = BuiltinRole { id: MZ_SYSTEM_ROLE_ID, name: SYSTEM_USER_NAME, diff --git a/src/compute-client/src/controller.rs b/src/compute-client/src/controller.rs index f3ca5b1e9dd99..405cb3aa417fa 100644 --- a/src/compute-client/src/controller.rs +++ b/src/compute-client/src/controller.rs @@ -53,6 +53,7 @@ use mz_ore::cast::CastFrom; use mz_ore::metrics::MetricsRegistry; use mz_ore::now::NowFn; use mz_ore::soft_assert_or_log; +use mz_ore::soft_panic_or_log; use mz_ore::tracing::OpenTelemetryContext; use mz_persist_types::PersistLocation; use mz_repr::{GlobalId, RelationDesc, Row, Timestamp}; @@ -1177,6 +1178,61 @@ impl ComputeController { return Ok(()); } + self.allow_writes_inner(instance_id, collection_id) + } + + /// Like [`Self::allow_writes`], but takes effect even in read-only mode. + /// + /// The caller must guarantee that no leader environment writes the collection's output shard. + /// In a 0dt deployment that means a shard this environment created for itself, the replacement + /// shard of a `Replacement`-migrated builtin collection, never one the leader is still serving + /// from. `Evolution` migrates in place and reuses the leader's shard, so it must not reach this + /// path. Violating the guarantee races two writers on one shard. + /// + /// NOTE: ownership is exclusive per (build version, deploy generation), not per process: the + /// migration shard entry naming the shard is keyed by that pair and a read-only catalog open is + /// a savepoint, so two read-only processes of one generation both write it. Same shape as a + /// multi-replica materialized view, which the self-correcting persist sink tolerates (see the + /// `mz_compute::sink::materialized_view` module docs). + /// + /// This is the compute-side counterpart to the storage controller's `force_writable` handling + /// of migrated storage collections. Migrated builtin tables are storage collections that + /// storage force-writes read-only; migrated builtin MVs are compute collections that only this + /// path can force-write. Both rest on the same guarantee (this environment exclusively owns the + /// replacement shard) but run on separate write paths, so each needs its own bypass. + /// + /// NOTE: the replica-side handler enables persist compaction process-wide on the clusterd + /// (`ComputeState::handle_allow_writes`), which this path is the first to trigger inside a + /// read-only deployment. + pub fn allow_writes_in_read_only( + &mut self, + instance_id: ComputeInstanceId, + collection_id: GlobalId, + ) -> Result<(), CollectionUpdateError> { + // Every builtin eligible for this bypass has a system id, so a non-system id means the + // caller's `Replacement`-only invariant broke. No-op rather than risk writing a shard the + // leader still serves. The collection then sits in the caught-up gate on an unwritten + // shard and blocks promotion, which is the loud, safe direction to fail. + // + // Storage asserts the same invariant hard, in + // `StorageController::register_introspection_collection`. The asymmetry is deliberate: a + // soft panic keeps a caller bug visible in CI and Sentry without downing production. + if self.read_only && !collection_id.is_system() { + soft_panic_or_log!( + "allow_writes_in_read_only called for non-system collection {collection_id}; \ + falling back to read-only no-op" + ); + return Ok(()); + } + + self.allow_writes_inner(instance_id, collection_id) + } + + fn allow_writes_inner( + &mut self, + instance_id: ComputeInstanceId, + collection_id: GlobalId, + ) -> Result<(), CollectionUpdateError> { let instance = self.instance_mut(instance_id)?; // Validation diff --git a/src/environmentd/src/test_util.rs b/src/environmentd/src/test_util.rs index 86fe89ebcb24c..24d0d17749ba6 100644 --- a/src/environmentd/src/test_util.rs +++ b/src/environmentd/src/test_util.rs @@ -137,6 +137,7 @@ pub struct TestHarness { internal_console_redirect_url: Option, metrics_registry: Option, code_version: semver::Version, + force_builtin_schema_migration: Option, capture: Option, pub environment_id: EnvironmentId, } @@ -242,6 +243,7 @@ impl Default for TestHarness { ..Default::default() }, code_version: crate::BUILD_INFO.semver_version(), + force_builtin_schema_migration: None, environment_id: EnvironmentId::for_tests(), capture: None, } @@ -692,6 +694,13 @@ impl TestHarness { self } + /// Forces every builtin storage collection through the given migration mechanism, + /// `"evolution"` or `"replacement"`. + pub fn with_force_builtin_schema_migration(mut self, mechanism: &str) -> Self { + self.force_builtin_schema_migration = Some(mechanism.into()); + self + } + pub fn with_capture(mut self, storage: SharedStorage) -> Self { self.capture = Some(storage); self @@ -944,7 +953,7 @@ impl Listeners { helm_chart_version: None, license_key: ValidatedLicenseKey::for_tests(), external_login_password_mz_system: config.external_login_password_mz_system, - force_builtin_schema_migration: None, + force_builtin_schema_migration: config.force_builtin_schema_migration, }) .await?; diff --git a/src/environmentd/tests/server.rs b/src/environmentd/tests/server.rs index acfd4cb088da8..e0e8b7b01e91d 100644 --- a/src/environmentd/tests/server.rs +++ b/src/environmentd/tests/server.rs @@ -11,6 +11,7 @@ #![recursion_limit = "256"] +use std::cell::Cell; use std::collections::{BTreeMap, BTreeSet, VecDeque}; use std::fmt::Write; use std::io::Write as _; @@ -2542,6 +2543,186 @@ async fn test_leader_promotion_mixed_code_version() { client_this.simple_query("SELECT 1").await.unwrap(); } +/// The migrated builtin MVs that the 0dt hydration tests read. +/// +/// `mz_clusters` is the interesting one: it joins the `mz_cluster_replica_size_internal` builtin +/// table, whose replacement shard is written once at bootstrap and thereafter only kept moving by +/// `read_only_mode_table_worker`. +const FORCED_BUILTIN_MIGRATION_MVS: &[&str] = + &["mz_catalog.mz_databases", "mz_catalog.mz_clusters"]; + +/// How long a single read of a migrated builtin MV may take before it counts as unreadable. +const MIGRATED_BUILTIN_MV_READ_TIMEOUT: Duration = Duration::from_secs(3); + +/// Reads every MV in [`FORCED_BUILTIN_MIGRATION_MVS`], reporting whether all of them returned rows. +/// +/// Each read gets its own connection, dropped on timeout so the session ends and takes the peek +/// with it. `statement_timeout` bounds INSERT/UPDATE/DELETE, not a `SELECT` waiting on a frontier, +/// so it would leave the poll loop hanging on an unwritten replacement shard. +#[allow(clippy::disallowed_methods)] +async fn migrated_builtin_mvs_readable(server: &test_util::TestServer) -> bool { + for relation in FORCED_BUILTIN_MIGRATION_MVS { + let client = server.connect().await.unwrap(); + let query = format!("SELECT count(*) FROM {relation}"); + let read = tokio::time::timeout( + MIGRATED_BUILTIN_MV_READ_TIMEOUT, + client.query_one(&query, &[]), + ); + match read.await { + Ok(Ok(row)) => { + let count: i64 = row.get(0); + if count == 0 { + tracing::info!("`{query}` returned 0 rows"); + return false; + } + } + Ok(Err(err)) => { + tracing::info!("`{query}` errored: {err}"); + return false; + } + Err(_) => { + tracing::info!( + "`{query}` did not return within {MIGRATED_BUILTIN_MV_READ_TIMEOUT:?}" + ); + return false; + } + } + } + true +} + +/// Boots a leader deployment plus a read-only deployment whose builtins were force-migrated by +/// shard replacement, and reports whether the migrated builtin MVs had become readable by the time +/// the read-only deployment first said `ReadyToPromote`. +/// +/// Status and readability are polled in the same loop on purpose. Reading the MVs only *after* +/// observing `ReadyToPromote` proves nothing about ordering: a peek against an unwritten +/// replacement shard blocks, but so does a peek against an MV that is merely late. +#[allow(clippy::disallowed_methods)] +async fn migrated_builtin_mvs_readable_at_ready_to_promote(hydrate_migrated_mvs: bool) -> bool { + let tmpdir = TempDir::new().unwrap(); + let harness = test_util::TestHarness::default() + .unsafe_mode() + .data_directory(tmpdir.path()) + .with_deploy_generation(1) + // Tick often, and tolerate far less lag than production, so the test finishes quickly and + // actually exercises the gate. The allowed lag has to stay *below* the stability period: a + // frozen collection looks caught up for as long as the tolerance lasts, so it must fall + // out of tolerance before an uninterrupted streak can complete. + .with_system_parameter_default( + "0dt_deployment_hydration_check_interval".to_string(), + "1s".to_string(), + ) + .with_system_parameter_default( + "with_0dt_caught_up_check_allowed_lag".to_string(), + "5s".to_string(), + ) + .with_system_parameter_default( + "with_0dt_caught_up_check_stability_period".to_string(), + "10s".to_string(), + ) + .with_system_parameter_default( + "enable_0dt_hydrate_migrated_builtin_mvs".to_string(), + hydrate_migrated_mvs.to_string(), + ); + + // The leader deployment. + let server_leader = harness.clone().start().await; + let client_leader = server_leader.connect().await.unwrap(); + client_leader.simple_query("SELECT 1").await.unwrap(); + + // The new deployment, booting read-only. Forcing the `replacement` mechanism gives every + // builtin storage collection a fresh shard, which is what a release carrying a + // `MigrationStep::replacement` does for the collections it names. + let server_new = harness + .clone() + .with_deploy_generation(2) + .with_force_builtin_schema_migration("replacement") + .start() + .await; + + let status_url = Url::parse(&format!( + "http://{}/api/leader/status", + server_new.internal_http_local_addr() + )) + .unwrap(); + + // Readability is monotonic, so latching the first `true` can only understate how early the MVs + // hydrated, never overstate it, and a readable-mid-tick race can't fail the test spuriously. + // The budget stays inside nextest's 240s kill for this package, so a gate that never opens + // fails the assertion rather than timing the test out. + let mvs_readable = Cell::new(false); + Retry::default() + .max_duration(Duration::from_secs(120)) + .retry_async(|_state| { + let status_url = status_url.clone(); + let server_new = &server_new; + let mvs_readable = &mvs_readable; + async move { + if !mvs_readable.get() { + mvs_readable.set(migrated_builtin_mvs_readable(server_new).await); + } + + let res = reqwest::Client::new().get(status_url).send().await.unwrap(); + assert_eq!(res.status(), StatusCode::OK); + let response = res.text().await.unwrap(); + tracing::info!( + mvs_readable = mvs_readable.get(), + "leader status of the new deployment: {response}" + ); + assert_ne!(response, r#"{"status":"IsLeader"}"#); + if response == r#"{"status":"ReadyToPromote"}"# { + Ok(()) + } else { + Err(()) + } + } + }) + .await + .expect("new deployment never reported ReadyToPromote"); + + // NOTE: we never promote. Cut-over `halt!`s the process, taking the test with it. + mvs_readable.get() +} + +/// Regression test for builtin MVs migrated by shard replacement: they must be hydrated by the +/// time the read-only deployment reports `ReadyToPromote`. +/// +/// A replacement migration hands the new deployment a fresh shard that no other environment +/// writes. Leave the MV's dataflow read-only and that shard stays empty, so the MV and everything +/// downstream of it drop out of the 0dt caught-up gate and then all hydrate at once at cut-over. +/// +/// Reaching `ReadyToPromote` at all is the other half of the assertion. Write-enabling puts these +/// MVs back *into* the gate, so anything they cannot catch up to now blocks promotion instead of +/// being waved through. +/// +/// NOTE: this covers the ordering, not the gate's lag comparison. Forcing `replacement` replaces +/// `mz_cluster_replica_frontiers` too, and the gate reads its "live" frontiers out of that +/// collection, so here it compares this deployment against itself. +#[mz_ore::test(tokio::test(flavor = "multi_thread"))] +#[cfg_attr(miri, ignore)] // too slow +async fn test_0dt_migrated_builtin_mv_hydrates_before_promotion() { + assert!( + migrated_builtin_mvs_readable_at_ready_to_promote(true).await, + "migrated builtin MVs were not readable when the new deployment reported ReadyToPromote" + ); +} + +/// The break-glass half of [`test_0dt_migrated_builtin_mv_hydrates_before_promotion`]: with +/// `enable_0dt_hydrate_migrated_builtin_mvs` off, the migrated MVs are excluded from the caught-up +/// gate again, so the deployment reports `ReadyToPromote` with them still unhydrated. +/// +/// The two tests differ only in the flag, so a change that reaches `ReadyToPromote` without +/// hydrating cannot pass both. +#[mz_ore::test(tokio::test(flavor = "multi_thread"))] +#[cfg_attr(miri, ignore)] // too slow +async fn test_0dt_migrated_builtin_mv_flag_off_promotes_unhydrated() { + assert!( + !migrated_builtin_mvs_readable_at_ready_to_promote(false).await, + "migrated builtin MVs hydrated even with enable_0dt_hydrate_migrated_builtin_mvs off" + ); +} + // Test that websockets observe cancellation. #[mz_ore::test] #[cfg_attr(miri, ignore)] // unsupported operation: can't call foreign function `epoll_wait` on OS `linux` diff --git a/src/storage-controller/src/lib.rs b/src/storage-controller/src/lib.rs index da25a62036c7a..d790f581c1206 100644 --- a/src/storage-controller/src/lib.rs +++ b/src/storage-controller/src/lib.rs @@ -3181,6 +3181,10 @@ where // In read-only mode we create a new shard for all migrated storage collections. So we // "trick" the write task into thinking that it's not in read-only mode so something is // advancing this new shard. + // + // This is the storage-side of forcing writes to a self-owned replacement shard while + // read-only. Migrated builtin MVs need the same treatment on their own write path; see + // `ComputeController::allow_writes_in_read_only`. let force_writable = self.read_only && self.migrated_storage_collections.contains(&id); if force_writable { assert!(id.is_system(), "unexpected non-system global id: {id:?}"); diff --git a/test/0dt/mzcompose.py b/test/0dt/mzcompose.py index f20afd9a30d11..f20f6f1957bcc 100644 --- a/test/0dt/mzcompose.py +++ b/test/0dt/mzcompose.py @@ -1813,6 +1813,25 @@ def get_persist_shard_id(item_id: str, service: str) -> str: ): c.up("mz_new") c.await_mz_deployment_status(DeploymentStatus.READY_TO_PROMOTE, "mz_new") + + # The new generation is ready to promote while still read-only, so its migrated builtin MVs + # must already be hydrated off their replacement shards. If they were excluded from the + # caught-up gate they would hydrate at cut-over instead, spiking catalog-server CPU. Reading + # them here from the read-only generation asserts the write-enable-while-read-only path + # actually filled those shards. + # + # NOTE: this covers hydration, not the caught-up gate's lag comparison. + # `force_migrations="replacement"` replaces `mz_cluster_replica_frontiers` too, and the gate + # reads the leader's "live" frontiers out of that collection, so here it compares this + # generation against itself. + for relation in ["mz_databases", "mz_clusters"]: + count = c.sql_query( + f"SELECT count(*) FROM mz_catalog.{relation}", + service="mz_new", + reuse_connection=False, + )[0][0] + assert count > 0, f"{relation} returned {count} on the read-only generation" + c.promote_mz("mz_new") c.await_mz_deployment_status(DeploymentStatus.IS_LEADER, "mz_new") From cb255f06282bc02e2fe66d49b808f80599aae80e Mon Sep 17 00:00:00 2001 From: Tyler Hartwig Date: Mon, 24 Aug 2026 11:06:45 -0400 Subject: [PATCH 6/8] docs: add guide for ingesting PostgreSQL partitioned tables (#38199) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Motivation Partitioned PostgreSQL tables come up repeatedly with customers and we have no docs for them. PostgreSQL expands a partitioned table to its leaf partitions in a publication, and the PostgreSQL source does not support `publish_via_partition_root`, so users need a documented strategy. Adds a guide under **Ingest data → PostgreSQL**, plus a "Partitioned tables" entry in the PostgreSQL source considerations that warns against `publish_via_partition_root` and links to it. ## What's covered **Approach 1 — ingest the leaf partitions.** One table per partition, unioned into a *materialized* view so that partitions can be added and removed with `CREATE REPLACEMENT MATERIALIZED VIEW` / `ALTER MATERIALIZED VIEW ... APPLY REPLACEMENT`, without recreating dependent objects. (Per @sjwiesman's and Nate Stewart's suggestion — using a plain view means every rollover breaks downstream objects.) **Approach 2 — flatten onto a dedicated replica.** Written as an extension of the existing [dedicated replica guide](https://materialize.com/docs/ingest-data/postgres/logical-replica/) rather than a duplicate: only the partition-specific deltas (`publish_via_partition_root = true` on the primary→replica hop, flat table with the parent's primary key on the replica, mirroring retention with `DELETE`). It also explains *how* the flattening actually happens, since that was the least obvious part in review: logical replication matches publisher to subscriber by schema-qualified name, and `publish_via_partition_root` changes which name goes on the wire, so partitions are never named and the subscriber's table can be an ordinary one. ## Please verify before merge Three claims come from local testing (Postgres 17 → Materialize v26.37.0), not from existing docs. Flagging them so review effort lands in the right place: 1. **"Materialize does not support `publish_via_partition_root` ... can produce incorrect results."** Reproduced locally: the source is created and appears to work, then diverges once partitions are attached/detached, and an `ATTACH` of a populated partition followed by an `UPDATE` of one of its rows produced a negative accumulation (`Non-positive multiplicity in DistinctBy`). Matches [SS-384](https://linear.app/materializeinc/issue/SS-384) and [SS-394](https://linear.app/materializeinc/issue/SS-394). **Verify:** is "can produce incorrect results" the wording we want, and should this instead be a hard error at `CREATE SOURCE`? 2. **The add/remove-partition runbooks.** Verified end to end on v26.37.0, including that dependent materialized views survive both operations and that retiring a partition in Materialize before dropping it upstream retracts the rows rather than stranding them. **Verify:** the ordering in "Remove a partition" is the sequence we want to recommend. 3. **`REPLICA IDENTITY FULL` does not cascade to partitions**, and a partition with neither a replica identity nor a primary key causes PostgreSQL to reject `UPDATE`/`DELETE` against it *on the primary*. Verified on PostgreSQL 17. ## Open question Approach 1 leans on **replacement views, which are Public Preview** and currently annotated "only recommended for use in development environments." That sits awkwardly with recommending them as the primary path in an ingest guide. Options: ship as-is with the preview annotation (current state), soften Approach 1 to plain views and note replacement views as the better option once GA, or lead with Approach 2. Happy to take direction here. ## Testing - `hugo` builds clean; all shortcodes expand and internal links resolve. - Reviewed against `doc/user/.prompts/docs_pre_pr_review.md`. --------- Co-authored-by: Claude Opus 5 (1M context) --- .../postgres/partitioned-tables.md | 350 ++++++++++++++++++ doc/user/data/postgres_source_details.yml | 27 ++ 2 files changed, 377 insertions(+) create mode 100644 doc/user/content/ingest-data/postgres/partitioned-tables.md diff --git a/doc/user/content/ingest-data/postgres/partitioned-tables.md b/doc/user/content/ingest-data/postgres/partitioned-tables.md new file mode 100644 index 0000000000000..4c925eba72286 --- /dev/null +++ b/doc/user/content/ingest-data/postgres/partitioned-tables.md @@ -0,0 +1,350 @@ +--- +title: "Guide: Ingest from partitioned tables" +description: "How to ingest data from a declaratively partitioned PostgreSQL table into Materialize." +menu: + main: + parent: "postgresql" + name: "Ingest from partitioned tables" + weight: 60 +--- + +This guide shows you how to ingest data from a [declaratively partitioned +PostgreSQL table](https://www.postgresql.org/docs/current/ddl-partitioning.html) +into Materialize. + +## How PostgreSQL publishes a partitioned table + +When you add a partitioned table to a publication, PostgreSQL expands it to the +table's **leaf partitions**. The parent table is not itself replicated: + +```sql +-- On PostgreSQL, where orders is partitioned by range +CREATE PUBLICATION mz_source FOR TABLE orders; + +SELECT tablename FROM pg_publication_tables WHERE pubname = 'mz_source'; +-- tablename +-- ---------------- +-- orders_2026_01 +-- orders_2026_02 +``` + +Materialize therefore ingests one table per partition, and you reassemble the +parent table with a `UNION ALL` over them. + +{{< warning >}} + +Do **not** create a publication with [`publish_via_partition_root = +true`](https://www.postgresql.org/docs/current/sql-createpublication.html) for a +Materialize source. Materialize does not support ingesting from a publication +that uses this option, and doing so can produce incorrect results. + +The configuration is accepted rather than rejected: the source is created, the +initial snapshot is correct, and inserts, updates, and deletes appear to +replicate normally. Problems surface only once partitions are added, attached, +or detached. Use one of the approaches on this page instead. + +{{< /warning >}} + +## Approach 1: Ingest the leaf partitions + +Ingest each partition as its own table, then union them into a **materialized +view**. Use a materialized view rather than a view so that you can add and remove +partitions later by [replacing the materialized +view](/transform-data/updating-materialized-views/replace-materialized-view/), +without recreating the objects that depend on it. + +{{% include-headless "/headless/replacement-views/public-preview-annotation" %}} + +### 1. Set `REPLICA IDENTITY FULL` on each partition + +Materialize requires `REPLICA IDENTITY FULL` on every table it ingests. Setting +it on the parent table does **not** cascade to the partitions, so set it on each +leaf partition: + +```sql +-- On PostgreSQL +ALTER TABLE orders_2026_01 REPLICA IDENTITY FULL; +ALTER TABLE orders_2026_02 REPLICA IDENTITY FULL; +``` + +### 2. Create the source and one table per partition + +Using an existing [PostgreSQL connection](/sql/create-connection/#postgresql), +create the source, then create one table per partition: + +```mzsql +CREATE SOURCE pg_src + FROM POSTGRES CONNECTION pg_connection (PUBLICATION 'mz_source'); + +CREATE TABLE orders_2026_01 FROM SOURCE pg_src (REFERENCE orders_2026_01); +CREATE TABLE orders_2026_02 FROM SOURCE pg_src (REFERENCE orders_2026_02); +``` + +### 3. Union the partitions into a materialized view + +```mzsql +CREATE MATERIALIZED VIEW orders AS + SELECT * FROM orders_2026_01 + UNION ALL + SELECT * FROM orders_2026_02; +``` + +Build your downstream objects on `orders`, not on the individual partitions. + +### Add a partition + +New partitions are **not** ingested automatically. When a partition is created +upstream, add it to the publication, create a table for it, and then replace the +materialized view to include it. + +1. On PostgreSQL, prepare the new partition and add it to the publication: + + ```sql + ALTER TABLE orders_2026_03 REPLICA IDENTITY FULL; + ALTER PUBLICATION mz_source ADD TABLE orders_2026_03; + ``` + +1. In Materialize, create a table for the new partition: + + ```mzsql + CREATE TABLE orders_2026_03 FROM SOURCE pg_src (REFERENCE orders_2026_03); + ``` + +1. Create a replacement view that includes the new partition: + + ```mzsql + CREATE REPLACEMENT MATERIALIZED VIEW orders_v2 FOR orders AS + SELECT * FROM orders_2026_01 + UNION ALL + SELECT * FROM orders_2026_02 + UNION ALL + SELECT * FROM orders_2026_03; + ``` + +1. Before applying the replacement view, verify that it is hydrated, to avoid + downtime: + + ```mzsql + SELECT mv.name, h.hydrated + FROM mz_catalog.mz_materialized_views AS mv + JOIN mz_internal.mz_hydration_statuses AS h ON (mv.id = h.object_id) + WHERE mv.name = 'orders_v2'; + ``` + +1. Apply the replacement: + + ```mzsql + ALTER MATERIALIZED VIEW orders APPLY REPLACEMENT orders_v2; + ``` + +1. Verify that rows from the new partition are now served by `orders`: + + ```mzsql + SELECT count(*) FROM orders WHERE order_date >= '2026-03-01'; + ``` + +Objects that depend on `orders` do not need to be recreated. They do have to +process the diff emitted by the replacement: + +{{% include-from-yaml data="examples/alter_materialized_view" +name="cpu-memory-considerations" %}} + +### Remove a partition + +Dropping or detaching a partition upstream removes it from the publication. If +Materialize is still ingesting that partition when this happens, the +corresponding table becomes inaccessible, and the materialized view that unions +it stops answering queries until you rebuild it. + +To avoid this, retire the partition in Materialize **first**, then upstream: + +1. Create a replacement view that excludes the partition, and apply it: + + ```mzsql + CREATE REPLACEMENT MATERIALIZED VIEW orders_v3 FOR orders AS + SELECT * FROM orders_2026_02 + UNION ALL + SELECT * FROM orders_2026_03; + + ALTER MATERIALIZED VIEW orders APPLY REPLACEMENT orders_v3; + ``` + +1. Drop the table in Materialize, which now has no dependents: + + ```mzsql + DROP TABLE orders_2026_01; + ``` + +1. On PostgreSQL, remove the partition from the publication and retire it: + + ```sql + ALTER PUBLICATION mz_source DROP TABLE orders_2026_01; + ALTER TABLE orders DETACH PARTITION orders_2026_01; + DROP TABLE orders_2026_01; + ``` + +Because the partition was removed from the materialized view before it was +retired upstream, its rows are retracted rather than left behind. To confirm, +compare the row count against the parent table upstream: + +```mzsql +SELECT count(*) FROM orders; +``` + +## Approach 2: Flatten the table on a dedicated replica + +If you would rather not perform the steps above on every partition rollover, you +can replicate the partitioned table into a **dedicated PostgreSQL replica** where +it is an ordinary, non-partitioned table, and point Materialize at that replica. +New partitions then flow through without any changes in Materialize. + +This is an extension of the [dedicated replica +guide](/ingest-data/postgres/logical-replica/), and carries the same trade-offs: +an extra system to operate, and an extra hop of replication lag. + +### How the flattening works + +PostgreSQL logical replication matches the publisher's relation to the +subscriber's relation **by schema-qualified name**. The +`publish_via_partition_root` option changes which name the publisher advertises: +with the option on, changes written to any partition are published as though +they came from the parent table. + +So if `public.orders` is partitioned on the primary and the publication uses +`publish_via_partition_root = true`, the subscriber receives changes for +`public.orders` and applies them to *its* `public.orders` — which you create as +an ordinary, non-partitioned table. The subscriber is never told that +partitioning exists, and the partitions themselves are never named on the wire: + +```sql +-- On the primary, with publish_via_partition_root = true +SELECT tablename FROM pg_publication_tables WHERE pubname = 'repl_to_replica'; +-- tablename +-- ----------- +-- orders -- the parent table, not orders_2026_01, orders_2026_02, ... +``` + +This is also why new partitions need no action: a partition added upstream +publishes under the same parent name, so it lands in the same table on the +replica. + +### Set up the primary → replica hop + +Follow the [dedicated replica guide](/ingest-data/postgres/logical-replica/) for +the base setup on both instances (`wal_level = logical`, a replication user, and +network access), with the following differences: + +1. On the **primary**, create the publication with `publish_via_partition_root = + true`: + + ```sql + CREATE PUBLICATION repl_to_replica FOR TABLE orders + WITH (publish_via_partition_root = true); + ``` + + List the **parent** table, not the individual partitions. + + {{< note >}} This option is safe on the `primary → replica` hop, which is + native PostgreSQL logical replication. Do not use it on the publication that + Materialize reads from. {{< /note >}} + +1. On the **replica**, create `orders` as a plain table with the same name and + columns as the parent table upstream, but without `PARTITION BY`: + + ```sql + CREATE TABLE orders ( + id bigint NOT NULL, + order_date date NOT NULL, + -- ... + PRIMARY KEY (id, order_date) + ); + + ALTER TABLE orders REPLICA IDENTITY FULL; + ``` + + Recreate the parent table's primary key, or another unique index, on the + replica. The subscriber uses it to locate rows for `UPDATE` and `DELETE`; + without a suitable index, each replicated change requires a sequential scan + of the table. + +1. On the **replica**, subscribe to the primary's publication: + + ```sql + CREATE SUBSCRIPTION orders_sub + CONNECTION 'host= port=5432 dbname= user=repuser password=' + PUBLICATION repl_to_replica; + ``` + + This statement does not name any tables, because the set of replicated tables + is defined by the publication on the primary. By default, it also creates a + replication slot on the primary, copies the existing rows out of every + partition, and then begins streaming ongoing changes. + + {{< warning >}} Create the table on the replica **before** the subscription. + PostgreSQL does not replicate DDL, so the subscriber never creates tables + itself. If `orders` does not already exist on the replica, the subscription + fails with `logical replication target relation "public.orders" does not + exist` and stops applying changes until you create it. {{< /warning >}} + +1. Verify that the flattening worked. The replica should have a single, ordinary + table holding the rows from every partition: + + ```sql + -- On the replica + SELECT relname, relkind FROM pg_class WHERE relname = 'orders'; + -- relname | relkind + -- ---------+--------- + -- orders | r -- an ordinary table, not 'p' for partitioned + + SELECT count(*) FROM orders; -- compare against the parent table upstream + ``` + +### Connect Materialize to the replica + +From here the replica is an ordinary, non-partitioned PostgreSQL database. +Follow [Connect Materialize to the +replica](/ingest-data/postgres/logical-replica/#c-connect-materialize-to-the-replica) +to create the publication that Materialize reads and the source itself. That +publication is a plain one: + +```sql +-- On the replica +CREATE PUBLICATION mz_source FOR TABLE orders; +``` + +New partitions created upstream are replicated to the replica automatically, as +long as the partitioned table has a primary key. If it does not, set `REPLICA +IDENTITY FULL` on each new partition as it is created; otherwise PostgreSQL +rejects `UPDATE` and `DELETE` against that partition on the primary. + +Partition maintenance is still not replicated. When you drop or detach a +partition upstream, mirror it on the replica with a matching `DELETE` so that +Materialize retracts the rows: + +```sql +-- On the replica +DELETE FROM orders WHERE order_date >= '2026-01-01' AND order_date < '2026-02-01'; +``` + +## Things to watch out for + +- **`REPLICA IDENTITY FULL` does not cascade.** Setting it on the parent table + leaves the partitions unchanged. Set it on each partition, including new ones. + +- **Attaching a populated partition.** `ATTACH PARTITION` does not replicate the + rows the table already contains, because they were never written to the WAL. + Load rows through the parent table, or resynchronize afterwards. + +- **Don't `TRUNCATE`.** Truncating an upstream table that Materialize reads makes + the corresponding table in Materialize inaccessible until it is recreated. + Note that `TRUNCATE` of the parent table propagates to a dedicated replica, so + it affects both approaches. Use an unqualified `DELETE` instead, including when + resynchronizing a replica: + + ```sql + DELETE FROM orders; + ``` + +- **Reconcile periodically.** Because partition maintenance is invisible to + logical replication, a scheduled row count or checksum comparison between the + upstream parent table and Materialize is the most reliable way to catch drift. diff --git a/doc/user/data/postgres_source_details.yml b/doc/user/data/postgres_source_details.yml index 993c0e819538d..ae7977b920191 100644 --- a/doc/user/data/postgres_source_details.yml +++ b/doc/user/data/postgres_source_details.yml @@ -158,6 +158,25 @@ SOURCE` and create a new view (materialized or non-) that unions the new table. +- name: postgres-partitioned-tables + content: | + When you add a [declaratively partitioned + table](https://www.postgresql.org/docs/current/ddl-partitioning.html) to a + publication, PostgreSQL expands it to the table's leaf partitions; the parent + table is not itself replicated. Materialize ingests one table per partition, + which you can reassemble into the parent table using `UNION ALL`. + + Materialize does **not** support ingesting from a publication created with + [`publish_via_partition_root = + true`](https://www.postgresql.org/docs/current/sql-createpublication.html), + and doing so can produce incorrect results. + +- name: postgres-partitioned-tables-action + content: | + See [Ingest from partitioned + tables](/ingest-data/postgres/partitioned-tables/) for the supported + approaches, including how to add and remove partitions over time. + - name: postgres-compatible-schema-changes content: | The use of [`CREATE SOURCE`](/sql/create-source/postgres-v2/) with `CREATE @@ -219,6 +238,14 @@ {{% include-from-yaml data="postgres_source_details" name="postgres-inherited-tables-action" %}} + ### Partitioned tables + + {{% include-from-yaml data="postgres_source_details" + name="postgres-partitioned-tables" %}} + + {{% include-from-yaml data="postgres_source_details" + name="postgres-partitioned-tables-action" %}} + ### Replication slots {{% include-from-yaml data="postgres_source_details" From 048109004e4032c29d4f87dadc5fcd81c7eddf6a Mon Sep 17 00:00:00 2001 From: Dov Alperin Date: Mon, 24 Aug 2026 11:25:52 -0400 Subject: [PATCH 7/8] timely-util: spill young chunk generations uncompressed (#38253) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### Motivation Spilled chunk bodies currently compress with lz4 unconditionally. Fresh chunks (generation depth 0) are usually consumed by their first merge moments after they spill, so compressing them buys little swap savings and pays encode plus decode on the hottest path. The August benchmark campaign measured this as roughly half of the spill-on co-tenant tail at 50cc: sweeping the floor from compress-everything to never-compress cut the co-tenant probe p99 from 16.6 s to 7.5 s under an ~18k rows/s upsert load (details in the campaign notes, results reproduced across two runs). This adds a generational compression floor: bodies below `column_chunk_compress_min_depth` (default 1, replica-scoped) spill through a new identity codec in `mz_ore::pool`. They stay budget-accounted and swap-backed, but encode and decode reduce to copies. Keeping young bodies out of the pool entirely was rejected because exempted bytes would be invisible to the budget. ### Tips for reviewer * The codec choice happens once at spill time (`codec_for_depth`), so a flag change affects newly spilled bodies only. * `0` restores compress-everything, `255` is effectively never-compress. ### Checklist - [ ] This PR has adequate test coverage / QA involvement has been duly considered. ([trigger-ci for additional test/nightly runs](https://trigger-ci.dev.materialize.com/)) - [ ] This PR has an associated up-to-date [design doc](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/design/README.md), is a design doc ([template](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/design/00000000_template.md)), or is sufficiently small to not require a design. - [ ] If this PR evolves [an existing `$T ⇔ Proto$T` mapping](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/command-and-response-binary-encoding.md) (possibly in a backwards-incompatible way), then it is tagged with a `T-proto` label. - [ ] If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label ([example](https://github.com/MaterializeInc/cloud/pull/5021)). - [ ] If this PR includes major [user-facing behavior changes](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/guide-changes.md#what-changes-require-a-release-note), I have pinged the relevant PM to schedule a changelog post. --- misc/python/materialize/mzcompose/__init__.py | 1 + .../materialize/parallel_workload/action.py | 5 + src/compute-types/src/dyncfgs.rs | 23 + src/compute/src/compute_state.rs | 7 + src/ore/src/pool.rs | 40 ++ src/timely-util/src/columnar/chunk.rs | 469 ++++++++++++++++-- 6 files changed, 497 insertions(+), 48 deletions(-) diff --git a/misc/python/materialize/mzcompose/__init__.py b/misc/python/materialize/mzcompose/__init__.py index 63b8ce5312bb1..fd544c851bbff 100644 --- a/misc/python/materialize/mzcompose/__init__.py +++ b/misc/python/materialize/mzcompose/__init__.py @@ -595,6 +595,7 @@ def get_default_system_parameters( "linear_join_yielding", "enable_column_paged_batcher", "enable_column_paged_batcher_spill", + "column_chunk_compress_min_depth", "column_paged_batcher_budget_fraction", "column_paged_batcher_lz4", "column_paged_batcher_swap_pageout", diff --git a/misc/python/materialize/parallel_workload/action.py b/misc/python/materialize/parallel_workload/action.py index b1bb873b81ded..5cba73171e282 100644 --- a/misc/python/materialize/parallel_workload/action.py +++ b/misc/python/materialize/parallel_workload/action.py @@ -3055,6 +3055,11 @@ def __init__( "0.02", ] self.flags_with_values["enable_upsert_paged_spill"] = BOOLEAN_FLAG_VALUES + self.flags_with_values["column_chunk_compress_min_depth"] = [ + "0", # compress every spilled body + "1", # the default: fresh chunks store uncompressed + "4", # exempt several young generations + ] # 0 forces the estimated-size path for every table, the default forces # the exact COUNT(*) path for workload-sized tables. self.flags_with_values["mysql_source_snapshot_exact_count_max_rows"] = [ diff --git a/src/compute-types/src/dyncfgs.rs b/src/compute-types/src/dyncfgs.rs index ff03b29b64aed..8b516d22ca5ab 100644 --- a/src/compute-types/src/dyncfgs.rs +++ b/src/compute-types/src/dyncfgs.rs @@ -86,6 +86,28 @@ pub const ENABLE_COLUMN_PAGED_BATCHER_SPILL: Config = Config::new( ParameterScope::Replica, ); +/// The youngest chunk generation whose spilled bodies are compressed. +/// +/// A chunk at generational depth `d` is rewritten with frequency +/// proportional to `2^-d` under geometric merging, so compressing shallow +/// generations buys pool bytes back for only a short stay at a guaranteed +/// near-term codec round-trip. Generations below the floor spill under the +/// identity codec: fully budgeted and swap-backed, with encode and decode +/// reduced to copies. The default exempts only fresh (depth 0) chunks. A +/// chunk that outlives a merge untouched ages a generation regardless, and +/// an identity-coded body at or past the floor is re-spilled compressed at +/// its next survival, so key-disjoint input cannot hold its backlog +/// uncompressed indefinitely. Lowering this at runtime therefore migrates +/// bodies that already spilled, rather than applying only to new ones. +/// `0` compresses every spilled body. +pub const COLUMN_CHUNK_COMPRESS_MIN_DEPTH: Config = Config::new( + "column_chunk_compress_min_depth", + 1, + "The youngest chunk generation whose spilled bodies are lz4-compressed in the buffer \ + pool; younger generations store uncompressed. 0 compresses every spilled body.", + ParameterScope::Replica, +); + /// Resident-bytes budget fraction for chunk spilling. Two consumers read /// it: the column pager's tiered policy multiplies it against the /// announced memory limit, and the buffer pool (`mz_ore::pool`) @@ -691,4 +713,5 @@ pub fn all_dyncfgs(configs: ConfigSet) -> ConfigSet { .add(&COLUMN_PAGED_BATCHER_SPILL_WORKER_COUNT) .add(&COLUMN_PAGED_BATCHER_EAGER_BACKING) .add(&COLUMN_PAGED_BATCHER_POOL_RSS_TARGET_FRACTION) + .add(&COLUMN_CHUNK_COMPRESS_MIN_DEPTH) } diff --git a/src/compute/src/compute_state.rs b/src/compute/src/compute_state.rs index 3a2f7b42e2697..1569592ede151 100644 --- a/src/compute/src/compute_state.rs +++ b/src/compute/src/compute_state.rs @@ -428,6 +428,13 @@ impl ComputeState { warn!("chunk spill: buffer pool unavailable; chunks stay resident"); } } + + // The generational depth floor below which spilled bodies store + // uncompressed. Subsystem-independent, so applied here alongside + // the rest of the process-wide chunk configuration. + let compress_min_depth = + u8::try_from(COLUMN_CHUNK_COMPRESS_MIN_DEPTH.get(config)).unwrap_or(u8::MAX); + mz_timely_util::columnar::chunk::set_compress_min_depth(compress_min_depth); } // Remember the maintenance interval locally to avoid reading it from the config set on diff --git a/src/ore/src/pool.rs b/src/ore/src/pool.rs index 7518877e959d6..6b739c74aa9d7 100644 --- a/src/ore/src/pool.rs +++ b/src/ore/src/pool.rs @@ -126,6 +126,28 @@ pub trait ExtentCodec: std::fmt::Debug + Send + Sync { fn decode(&self, stored: &[u8], body: &mut [u8]); } +/// The identity [`ExtentCodec`]: the stored form is the body. Encode and +/// decode are copies, and range reads copy the range directly, so a chunk +/// stored under this codec pays no compression work in either direction +/// while remaining fully budgeted and swap-backed like any other extent. +#[derive(Debug)] +pub struct IdentityCodec; + +/// The [`IdentityCodec`] instance to pass to [`Pool::insert_with`]. +pub static IDENTITY_CODEC: IdentityCodec = IdentityCodec; + +impl ExtentCodec for IdentityCodec { + fn encode(&self, body: &[u8], out: &mut Vec) { + out.clear(); + out.extend_from_slice(body); + } + + fn decode(&self, stored: &[u8], body: &mut [u8]) { + assert_eq!(stored.len(), body.len(), "identity stored form is the body"); + body.copy_from_slice(stored); + } +} + /// The largest stored form [`ExtentCodec::encode`] may produce for a /// `body_len`-byte body: an incompressible-input expansion matching lz4's /// worst case plus a four-byte length prefix. The extent store's size-class @@ -3703,6 +3725,24 @@ mod tests { assert_eq!(pool.stats().resident_bytes, 0); } + /// The identity codec stores the body verbatim: eviction and reads, + /// whole and by range, reconstruct it unchanged. + #[mz_ore::test] + fn identity_codec_round_trips() { + let pool = test_pool(usize::MAX); + let want = payload(SMALL, 601); + let h = pool.insert_with(SMALL, ChunkHints::default(), &IDENTITY_CODEC, |dst| { + dst.copy_from_slice(&want); + }); + assert_eq!(read(&h), want); + pool.evict(&h); + assert_eq!(read(&h), want, "round-trips through the extent"); + pool.evict(&h); + let mut range = Vec::new(); + h.read_range_into(8..24, &mut range); + assert_eq!(range, want[8..24], "range reads copy the range directly"); + } + #[mz_ore::test] fn insert_with_fills_in_place() { let pool = test_pool(usize::MAX); diff --git a/src/timely-util/src/columnar/chunk.rs b/src/timely-util/src/columnar/chunk.rs index 2b05f4e8cbf16..1048f46d9852a 100644 --- a/src/timely-util/src/columnar/chunk.rs +++ b/src/timely-util/src/columnar/chunk.rs @@ -44,10 +44,12 @@ //! resident fence metadata so a probe set faults only the chunk bodies it //! actually touches. +#[cfg(test)] +use std::cell::Cell; use std::cell::RefCell; use std::collections::VecDeque; use std::rc::Rc; -use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::atomic::{AtomicBool, AtomicU8, Ordering}; use columnar::bytes::indexed; use columnar::{Borrow, BorrowedOf, Columnar, Container as _, FromBytes, Index, Len, Push as _}; @@ -55,7 +57,7 @@ use differential_dataflow::difference::Semigroup; use differential_dataflow::lattice::Lattice; use differential_dataflow::trace::chunk::Chunk; use mz_ore::cast::CastFrom; -use mz_ore::pool::{ChunkHandle, ChunkHints, ExtentCodec, Pool}; +use mz_ore::pool::{ChunkHandle, ChunkHints, ExtentCodec, IDENTITY_CODEC, Pool}; use timely::Accountable; use timely::container::{ContainerBuilder, PushInto}; use timely::dataflow::channels::ContainerBytes; @@ -78,6 +80,12 @@ thread_local! { /// pool without touching process-global state. static SPILL_OVERRIDE: RefCell> = const { RefCell::new(None) }; + /// A thread-scoped depth-floor override, taking precedence over the + /// global value. Lets tests pin the floor without racing concurrently + /// running tests on the process-global state. + #[cfg(test)] + static COMPRESS_MIN_DEPTH_OVERRIDE: Cell> = const { Cell::new(None) }; + /// Reusable staging for call-scoped reads of spilled bodies. static READ_SCRATCH: RefCell> = const { RefCell::new(Vec::new()) }; } @@ -113,6 +121,62 @@ pub fn set_spill_override(pool: Option) { SPILL_OVERRIDE.with(|cell| *cell.borrow_mut() = pool); } +/// The youngest generational depth whose spilled bodies are compressed. See +/// [`set_compress_min_depth`]. +static COMPRESS_MIN_DEPTH: AtomicU8 = AtomicU8::new(DEFAULT_COMPRESS_MIN_DEPTH); + +/// Set the youngest generational depth whose spilled bodies are compressed. +/// +/// A chunk at depth `d` is rewritten (merged, extracted, advanced) with +/// frequency proportional to `2^-d` under geometric merging, so compressing +/// a shallow chunk buys a short stay in the pool at the cost of a guaranteed +/// near-term codec round-trip: the body is encoded only to be read back and +/// decoded by the next rewrite. Generations below the floor spill under the +/// identity codec instead: still budgeted and swap-backed like every extent, +/// but encode and decode are copies. The floor never exempts a body from the +/// pool, so it cannot grow unbudgeted resident state. +/// +/// The floor cannot strand a long-lived body uncompressed: a chunk that a +/// merge carries forward untouched also ages a generation, and its body is +/// re-spilled under the compressing codec when it crosses the floor. Without +/// that, key-disjoint input (a monotonically increasing key) would hold its +/// entire spilled backlog identity-coded for the backlog's lifetime. +/// +/// `0` compresses every spilled body. Consulted at every commit, so changes +/// apply to running dataflows. +pub fn set_compress_min_depth(depth: u8) { + COMPRESS_MIN_DEPTH.store(depth, Ordering::Relaxed); +} + +/// Set or unset a thread-scoped depth-floor override, taking precedence over +/// [`set_compress_min_depth`]. Tests run concurrently and must not race on +/// the process-global floor. +#[cfg(test)] +pub fn set_compress_min_depth_override(depth: Option) { + COMPRESS_MIN_DEPTH_OVERRIDE.with(|cell| cell.set(depth)); +} + +/// The depth floor in effect for this thread's commits. +fn compress_min_depth() -> u8 { + #[cfg(test)] + if let Some(depth) = COMPRESS_MIN_DEPTH_OVERRIDE.with(|cell| cell.get()) { + return depth; + } + COMPRESS_MIN_DEPTH.load(Ordering::Relaxed) +} + +/// The codec a body at `depth` stores under, identity below the compression +/// floor and lz4 at and past it, paired with whether that codec compresses. +/// One read of the floor, so the pair cannot disagree with itself when the +/// floor moves under a concurrent commit. +fn codec_for_depth(depth: u8) -> (&'static dyn ExtentCodec, bool) { + if depth < compress_min_depth() { + (&IDENTITY_CODEC, false) + } else { + (&LZ4_CODEC, true) + } +} + /// The pool committed chunks spill to, if any. fn spill_pool() -> Option { if let Some(pool) = SPILL_OVERRIDE.with(|cell| cell.borrow().clone()) { @@ -161,6 +225,16 @@ const COMMIT_BYTES: usize = 2 << 20; /// unbudgeted heap, and no accounting here would catch it. const SPILL_MIN_BYTES: usize = 64 << 10; +/// The default compression depth floor: fresh (depth 0) bodies spill +/// uncompressed. +/// +/// A fresh chunk is consumed by its first merge with certainty, so +/// compressing it can never save pool bytes for longer than one merge +/// cadence and always costs a full encode plus decode. Depth 1 and beyond +/// have survived a merge and wait geometrically longer for the next, so +/// their compression amortizes. +const DEFAULT_COMPRESS_MIN_DEPTH: u8 = 1; + /// Whether a column is big enough to commit on its own. A monotone /// threshold, so settle's carry, which grows by whole chunks, cannot step /// over it. @@ -193,33 +267,45 @@ pub struct SpilledBody { /// container rather than two singletons, so the leaf allocations are not /// duplicated per fence. fences: D::Container, - /// The chunk's generational depth, mirrored into the pool's - /// [`ChunkHints`] at spill time. - depth: u8, + /// Whether the body was inserted under the compressing codec. The pool + /// stores the codec itself and reads decode through it, so this is the + /// only handle chunk code has on what a body is stored as, and it is + /// what `survive_merge` consults to decide a body wants migrating. + /// Deriving that from depth instead would tie it to a single transition + /// and miss every path that skips it. + compressed: bool, /// The pool chunk holding the serialized column. handle: ChunkHandle, } /// A sorted, consolidated run of `(D, T, R)` updates, resident or spilled. /// -/// Every chunk carries a generational depth, fixed at creation: fresh chunks -/// are depth 0, a merge output is one generation past its deepest input -/// (saturating at `u8::MAX`, where remerged long-lived chunks stay), and -/// rewrites within a generation (extract, advance, settle coalescing) -/// preserve depth. At spill time the depth becomes the pool's [`ChunkHints`], -/// so repeatedly merged (older, colder) data lands in deeper eviction bands. +/// Every chunk carries a generational depth counting the merge cadences it +/// has lived through: fresh chunks are depth 0, a merge output is one +/// generation past its deepest input (saturating at `u8::MAX`, where +/// remerged long-lived chunks stay), a chunk a merge carries forward +/// untouched also gains a generation (see `survive_merge`), and rewrites +/// within a generation (extract, advance, settle coalescing) preserve +/// depth. +/// +/// Depth belongs to the chunk, not to the body: a body outlives the chunks +/// that share it, and aging must not depend on whether a caller happens to +/// hold the only reference. At spill time the depth becomes the pool's +/// [`ChunkHints`], so repeatedly merged (older, colder) data lands in deeper +/// eviction bands. Hints are fixed at insert, so a chunk aged without a +/// re-spill keeps the band it spilled into. pub enum ColumnChunk { /// Body on the heap, shared via `Rc`, with its generational depth. Resident(Rc>, u8), - /// Body in the pool. See [`SpilledBody`]. - Spilled(Rc>), + /// Body in the pool, with its generational depth. See [`SpilledBody`]. + Spilled(Rc>, u8), } impl Clone for ColumnChunk { fn clone(&self) -> Self { match self { ColumnChunk::Resident(col, depth) => ColumnChunk::Resident(Rc::clone(col), *depth), - ColumnChunk::Spilled(body) => ColumnChunk::Spilled(Rc::clone(body)), + ColumnChunk::Spilled(body, depth) => ColumnChunk::Spilled(Rc::clone(body), *depth), } } } @@ -251,7 +337,7 @@ impl ColumnChunk { ColumnChunk::Resident(col, _) => { Rc::try_unwrap(col).unwrap_or_else(|shared| copy_column(&shared)) } - ColumnChunk::Spilled(body) => { + ColumnChunk::Spilled(body, _) => { let mut words = Vec::new(); body.handle.read_into(&mut words); Column::Align(words) @@ -261,22 +347,21 @@ impl ColumnChunk { /// True when the body lives in the pool. pub fn is_spilled(&self) -> bool { - matches!(self, ColumnChunk::Spilled(_)) + matches!(self, ColumnChunk::Spilled(_, _)) } /// The number of updates, from resident state only. fn records(&self) -> usize { match self { ColumnChunk::Resident(col, _) => col.borrow().len(), - ColumnChunk::Spilled(body) => body.records, + ColumnChunk::Spilled(body, _) => body.records, } } /// The generational depth, from resident state only. fn depth(&self) -> u8 { match self { - ColumnChunk::Resident(_, depth) => *depth, - ColumnChunk::Spilled(body) => body.depth, + ColumnChunk::Resident(_, depth) | ColumnChunk::Spilled(_, depth) => *depth, } } @@ -287,7 +372,7 @@ impl ColumnChunk { let data = col.borrow().0; (data.get(0), data.get(data.len() - 1)) } - ColumnChunk::Spilled(body) => { + ColumnChunk::Spilled(body, _) => { let fences = body.fences.borrow(); (fences.get(0), fences.get(1)) } @@ -309,20 +394,70 @@ impl ColumnChunk { /// Spill a non-empty column into `pool` unconditionally, capturing the /// resident fence metadata. + /// + /// Generations below the compression depth floor store under the + /// identity codec: rewritten too soon for compression to amortize, they + /// stay budgeted and swap-backed while encode and decode reduce to + /// copies. fn spill_body(column: Column<(D, T, R)>, pool: &Pool, depth: u8) -> Self { + let (codec, compressed) = codec_for_depth(depth); let len_bytes = column.length_in_bytes(); let view = column.borrow(); let records = view.len(); let mut fences = D::Container::default(); fences.push(view.0.get(0)); fences.push(view.0.get(records - 1)); - let handle = spill_column(column, pool, len_bytes, ChunkHints { depth }); - ColumnChunk::Spilled(Rc::new(SpilledBody { - records, - fences, + let handle = spill_column(column, pool, len_bytes, ChunkHints { depth }, codec); + ColumnChunk::Spilled( + Rc::new(SpilledBody { + records, + fences, + compressed, + handle, + }), depth, - handle, - })) + ) + } + + /// Age a chunk that a merge carried forward untouched by one generation. + /// + /// Depth counts merge cadences lived through, not rewrites, so a + /// pass-through survivor ages like a merged chunk. The bump itself is + /// always free: depth rides on the chunk, so it does not care whether the + /// body is shared. + /// + /// An identity-coded body at or past the floor wants migrating, so a + /// sole owner re-spills it compressed: surviving a merge disproves the + /// imminent-rewrite premise that exempted it, and without the re-spill + /// key-disjoint input would keep its whole spilled backlog + /// identity-coded for as long as it lived. The test is the body's stored + /// codec against the floor, not a depth transition, so a migration that + /// cannot happen now is retried at the next survival rather than + /// consumed: skipping it while the body is shared or while no pool is + /// installed, or lowering the floor long after a body spilled, all + /// converge on a compressed body instead of stranding one. + /// + /// A shared body is skipped because re-spilling this reference cannot + /// change what the other holder stores, and the compaction merger that + /// shares bodies rewrites its clones immediately. + fn survive_merge(self) -> Self { + let depth = self.depth().saturating_add(1); + match self { + ColumnChunk::Resident(col, _) => ColumnChunk::Resident(col, depth), + ColumnChunk::Spilled(body, was) => { + let migrate = !body.compressed && depth >= compress_min_depth(); + if !migrate || Rc::strong_count(&body) > 1 { + return ColumnChunk::Spilled(body, depth); + } + match spill_pool() { + Some(pool) => { + let column = ColumnChunk::Spilled(body, was).into_column(); + Self::spill_body(column, &pool, depth) + } + None => ColumnChunk::Spilled(body, depth), + } + } + } } } @@ -379,13 +514,14 @@ fn spill_column( pool: &Pool, len_bytes: usize, hints: ChunkHints, + codec: &'static dyn ExtentCodec, ) -> ChunkHandle { mz_ore::soft_assert_eq_no_log!(len_bytes % 8, 0); match column { - Column::Align(words) => pool.insert_with(words.len(), hints, &LZ4_CODEC, |dst| { - dst.copy_from_slice(&words) - }), - other => pool.insert_with(len_bytes / 8, hints, &LZ4_CODEC, |dst| { + Column::Align(words) => { + pool.insert_with(words.len(), hints, codec, |dst| dst.copy_from_slice(&words)) + } + other => pool.insert_with(len_bytes / 8, hints, codec, |dst| { let bytes: &mut [u8] = bytemuck::cast_slice_mut(dst); let mut cursor = std::io::Cursor::new(bytes); other.into_bytes(&mut cursor); @@ -431,12 +567,14 @@ where /// [`Column::merge_from`] does the work: gallop bulk-copies for disjoint /// runs, semigroup consolidation on equal `(data, time)`, output cut at - /// the ship threshold. A survivor pushed back untouched keeps its - /// original form, in particular a spilled body is neither rebuilt nor - /// re-spilled. + /// the ship threshold. /// /// Fronts whose data ranges are disjoint never load at all: the resident - /// fence entries decide, and the lower front moves to the output verbatim. + /// fence entries decide, and the lower front moves to the output whole. + /// Chunks the merge carries forward untouched (that fast path, and a + /// survivor `merge_from` never consumed) age one generation through + /// `survive_merge`, which re-spills a body only at the compression-floor + /// crossing and otherwise leaves it untouched. fn merge(in1: &mut VecDeque, in2: &mut VecDeque, out: &mut VecDeque) { // Disjoint fast path: when one front lies strictly below the other's // first data item (equal boundary data could still interleave on @@ -453,11 +591,13 @@ where let a_low = rr::(a_last) < rr::(b_first); let b_low = rr::(b_last) < rr::(a_first); if a_low { - out.push_back(in1.pop_front().expect("front observed above")); + let chunk = in1.pop_front().expect("front observed above"); + out.push_back(chunk.survive_merge()); return; } if b_low { - out.push_back(in2.pop_front().expect("front observed above")); + let chunk = in2.pop_front().expect("front observed above"); + out.push_back(chunk.survive_merge()); return; } @@ -468,11 +608,11 @@ where let depths = [a.depth(), b.depth()]; let out_depth = depths[0].max(depths[1]).saturating_add(1); let mut spill_a = match &a { - ColumnChunk::Spilled(body) => Some(Rc::clone(body)), + ColumnChunk::Spilled(body, _) => Some(Rc::clone(body)), ColumnChunk::Resident(_, _) => None, }; let mut spill_b = match &b { - ColumnChunk::Spilled(body) => Some(Rc::clone(body)), + ColumnChunk::Spilled(body, _) => Some(Rc::clone(body)), ColumnChunk::Resident(_, _) => None, }; let mut cols = [a.into_column(), b.into_column()]; @@ -497,13 +637,13 @@ where ] { let len = col.borrow().len(); if pos == 0 && len > 0 { - // Untouched survivor: restore it as it was, spilled bodies - // included (the loaded copy is dropped). + // Untouched survivor: restore it as it was (the loaded copy + // is dropped), aged one generation by its survival. let chunk = match spilled.take() { - Some(body) => ColumnChunk::Spilled(body), + Some(body) => ColumnChunk::Spilled(body, depth), None => ColumnChunk::Resident(Rc::new(std::mem::take(col)), depth), }; - queue.push_front(chunk); + queue.push_front(chunk.survive_merge()); } else if pos < len { let view = col.borrow(); let mut rest = <(D, T, R) as Columnar>::Container::default(); @@ -697,7 +837,7 @@ where let mut carry: Option<(Column<(D, T, R)>, u8)> = None; while let Some(chunk) = input.pop_front() { let (rc, depth) = match chunk { - spilled @ ColumnChunk::Spilled(_) => { + spilled @ ColumnChunk::Spilled(_, _) => { if let Some((col, depth)) = carry.take() { out.push_back(ColumnChunk::commit(col, depth)); } @@ -835,7 +975,7 @@ where ColumnChunk::Resident(col, _) => { extract_view_into::(col.borrow(), probes, probe_index, staging); } - ColumnChunk::Spilled(body) => with_scratch(|scratch| { + ColumnChunk::Spilled(body, _) => with_scratch(|scratch| { // NOTE: deliberately the non-admitting read. One probe set // touching a chunk is weak evidence it will be touched again, // and probing a spilled trace must not accrete it back into @@ -854,7 +994,7 @@ where let view = col.borrow(); staging.extend_from_self(view, 0..view.len()); } - ColumnChunk::Spilled(body) => with_scratch(|scratch| { + ColumnChunk::Spilled(body, _) => with_scratch(|scratch| { body.handle.read_into(scratch); let view = borrow_words::<((K, V), T, R)>(scratch); staging.extend_from_self(view, 0..view.len()); @@ -1118,6 +1258,16 @@ mod tests { .collect() } + /// Whether a spilled chunk's body is stored compressed. Deliberately + /// does not retain the body: an `Rc` held across a `survive_merge` would + /// itself make the body shared and suppress the migration under test. + fn body_compressed(chunk: &TestChunk) -> bool { + match chunk { + ColumnChunk::Spilled(body, _) => body.compressed, + ColumnChunk::Resident(_, _) => panic!("chunk must be spilled"), + } + } + /// Spill one chunk through `pool`, bypassing the size threshold and /// keeping the chunk's depth. fn force_spill(chunk: TestChunk, pool: &Pool) -> TestChunk { @@ -1550,7 +1700,7 @@ mod tests { /// Merge output is one generation past its deepest input, a survivor /// rewritten from its remainder keeps its own depth, and a chunk passed - /// through the disjoint fast path keeps its depth. + /// through the disjoint fast path ages by its survival. #[mz_ore::test] fn merge_derives_generational_depth() { let low: Vec = (0..100u64).map(|i| ((i, 0), 0, 1i64)).collect(); @@ -1570,14 +1720,16 @@ mod tests { assert_eq!(in2.len(), 1); assert_eq!(in2[0].depth(), 0, "rewritten survivor keeps its depth"); - // A disjoint merge moves the lower front to the output unchanged. + // A disjoint merge moves the lower front to the output with its data + // unchanged, one generation older for having outlived the merge. let mut in1 = VecDeque::from([ColumnChunk::Resident(Rc::new(build_column(&low)), 3)]); let far: Vec = (1000..1100u64).map(|i| ((i, 0), 0, 1i64)).collect(); let mut in2 = VecDeque::from([ColumnChunk::from_column(build_column(&far))]); let mut out = VecDeque::new(); TestChunk::merge(&mut in1, &mut in2, &mut out); assert_eq!(out.len(), 1); - assert_eq!(out[0].depth(), 3, "pass-through keeps its depth"); + assert_eq!(out[0].depth(), 4, "pass-through ages a generation"); + assert_eq!(collect_chunks(out), low); } /// Advance output and carry keep the deepest input depth, since @@ -1701,6 +1853,226 @@ mod tests { set_spill_override(None); } + /// The compression depth floor picks the codec, not whether a body + /// spills: shallow generations store at identity, the floor and deeper + /// at lz4, and every depth spills and round-trips. + #[mz_ore::test] + fn spill_codec_depth_floor() { + set_spill_override(Some(test_pool())); + set_compress_min_depth_override(Some(2)); + // Codec identity via Debug: ZST statics and dyn vtables make + // pointer comparison unreliable. The flag must agree with the codec, + // since it is what decides whether a body wants migrating. + let codec_name = |depth: u8| { + let (codec, compressed) = codec_for_depth(depth); + let name = format!("{:?}", codec); + assert_eq!(compressed, name == "Lz4Codec", "flag tracks the codec"); + name + }; + assert_eq!(codec_name(0), "IdentityCodec"); + assert_eq!(codec_name(1), "IdentityCodec"); + assert_eq!(codec_name(2), "Lz4Codec"); + assert_eq!(codec_name(u8::MAX), "Lz4Codec"); + + let data: Vec = (0..20_000u64).map(|i| ((i, 0), 0, 1i64)).collect(); + let data = consolidate(data); + let column = build_column(&data); + for depth in [0u8, 1, 2, 3] { + let chunk = TestChunk::commit(column.clone(), depth); + assert!(chunk.is_spilled(), "depth {depth} must spill"); + assert_eq!(collect_column(&chunk.into_column()), data); + } + set_spill_override(None); + set_compress_min_depth_override(None); + + // The default floor stores only fresh (depth 0) bodies at identity. + set_compress_min_depth_override(Some(DEFAULT_COMPRESS_MIN_DEPTH)); + assert_eq!(codec_name(0), "IdentityCodec"); + assert_eq!(codec_name(1), "Lz4Codec"); + set_compress_min_depth_override(None); + } + + /// A chunk a merge carries forward untouched ages a generation, and a + /// spilled body crossing the compression floor by doing so is re-spilled + /// under the compressing codec. Key-disjoint input takes that path on + /// every merge, so without the crossing its backlog would stay + /// identity-coded for as long as it lived. + #[mz_ore::test] + #[cfg_attr(miri, ignore)] // too slow + fn merge_survivor_crosses_compression_floor() { + set_spill_override(Some(test_pool())); + set_compress_min_depth_override(Some(1)); + + let low = consolidate((0..20_000u64).map(|i| ((i, 0), 0, 1i64)).collect()); + let far = consolidate((100_000..120_000u64).map(|i| ((i, 0), 0, 1i64)).collect()); + let fresh_far = || VecDeque::from([TestChunk::commit(build_column(&far), 0)]); + + // Fresh spilled chunks sit below the floor, so both store identity + // coded, and their data ranges are disjoint. + let mut in1 = VecDeque::from([TestChunk::commit(build_column(&low), 0)]); + let mut in2 = fresh_far(); + assert!(in1[0].is_spilled() && in2[0].is_spilled()); + assert!( + !body_compressed(&in1[0]), + "a fresh body below the floor is identity coded" + ); + + let mut out = VecDeque::new(); + TestChunk::merge(&mut in1, &mut in2, &mut out); + assert_eq!(out.len(), 1); + let survived = out.pop_front().expect("the lower front passes through"); + assert_eq!(survived.depth(), 1, "survival ages across the floor"); + assert!( + survived.is_spilled(), + "the crossing re-spills, it does not evict" + ); + assert!( + body_compressed(&survived), + "the survivor is re-spilled under the compressing codec" + ); + + // Past the floor the next survival is a metadata-only bump: the body + // is already compressed and stays where it is. + let mut in1 = VecDeque::from([survived]); + let mut in2 = fresh_far(); + let mut out = VecDeque::new(); + TestChunk::merge(&mut in1, &mut in2, &mut out); + assert_eq!(out.len(), 1); + assert_eq!(out[0].depth(), 2, "an aged survivor keeps aging"); + assert!(out[0].is_spilled()); + assert_eq!( + collect_chunks(out), + low, + "the body reads back intact across both survivals" + ); + + set_spill_override(None); + set_compress_min_depth_override(None); + } + + /// Aging does not depend on holding the only reference to a body. The + /// trace's compaction merger feeds `merge` clones of a source batch's + /// chunks and keeps the batch alive throughout, so a shared body must + /// still age. It must not be re-spilled: the other holder goes on + /// storing the original whatever this reference does, and the merger + /// rewrites its clone immediately. + #[mz_ore::test] + #[cfg_attr(miri, ignore)] // too slow + fn merge_survivor_ages_while_shared() { + set_spill_override(Some(test_pool())); + set_compress_min_depth_override(Some(1)); + + let low = consolidate((0..20_000u64).map(|i| ((i, 0), 0, 1i64)).collect()); + let far = consolidate((100_000..120_000u64).map(|i| ((i, 0), 0, 1i64)).collect()); + + // The source batch's chunk, held for the whole merge as the spine + // holds it. + let source = TestChunk::commit(build_column(&low), 0); + let ColumnChunk::Spilled(source_body, 0) = &source else { + panic!("a fresh commit above the spill floor is spilled at depth 0"); + }; + let source_body = Rc::clone(source_body); + + let mut in1 = VecDeque::from([source.clone()]); + let mut in2 = VecDeque::from([TestChunk::commit(build_column(&far), 0)]); + let mut out = VecDeque::new(); + TestChunk::merge(&mut in1, &mut in2, &mut out); + + assert_eq!(out.len(), 1); + assert_eq!(out[0].depth(), 1, "a shared body ages all the same"); + let ColumnChunk::Spilled(survived_body, _) = &out[0] else { + panic!("the survivor stays spilled"); + }; + assert!( + Rc::ptr_eq(&source_body, survived_body), + "a shared body is aged in place, not re-spilled" + ); + assert_eq!(source.depth(), 0, "the other holder is left as it was"); + + // Past the floor, where no re-spill is in question, a shared body + // goes on aging rather than pinning at the crossing depth. + let mut in1 = VecDeque::from([out.pop_front().expect("survivor observed above")]); + let mut in2 = VecDeque::from([TestChunk::commit(build_column(&far), 0)]); + let mut out = VecDeque::new(); + TestChunk::merge(&mut in1, &mut in2, &mut out); + assert_eq!(out.len(), 1); + assert_eq!(out[0].depth(), 2, "aging past the floor is not pinned"); + assert_eq!(collect_chunks(out), low); + + set_spill_override(None); + set_compress_min_depth_override(None); + } + + /// A migration that cannot happen when a body first qualifies is retried + /// at the next survival, never consumed. Each case leaves an + /// identity-coded body at or past the floor, which would be stranded + /// uncompressed for the rest of its life if the test were a depth + /// transition rather than the body's stored codec. + #[mz_ore::test] + #[cfg_attr(miri, ignore)] // too slow + fn survive_merge_retries_missed_migrations() { + let low = consolidate((0..20_000u64).map(|i| ((i, 0), 0, 1i64)).collect()); + let far = consolidate((100_000..120_000u64).map(|i| ((i, 0), 0, 1i64)).collect()); + + // Age a chunk one generation through a disjoint merge, which passes + // the lower front through `survive_merge`. + let survive = |chunk: TestChunk| { + let mut in1 = VecDeque::from([chunk]); + let mut in2 = VecDeque::from([TestChunk::commit(build_column(&far), 0)]); + let mut out = VecDeque::new(); + TestChunk::merge(&mut in1, &mut in2, &mut out); + out.pop_front().expect("the lower front passes through") + }; + + // No pool installed when the body qualifies: spilling can be toggled + // off at runtime while existing handles stay valid. + set_spill_override(Some(test_pool())); + set_compress_min_depth_override(Some(1)); + let chunk = TestChunk::commit(build_column(&low), 0); + assert!(!body_compressed(&chunk)); + set_spill_override(None); + let chunk = survive(chunk); + assert_eq!(chunk.depth(), 1, "aging does not need a pool"); + assert!(!body_compressed(&chunk), "no pool, no migration"); + set_spill_override(Some(test_pool())); + let chunk = survive(chunk); + assert!( + body_compressed(&chunk), + "the migration retries once a pool is back" + ); + + // Shared when the body qualifies: the compaction merger holds the + // source batch while merging clones of its chunks. + let chunk = TestChunk::commit(build_column(&low), 0); + let held = chunk.clone(); + let chunk = survive(chunk); + assert!(!body_compressed(&chunk), "shared, so not migrated"); + drop(held); + let chunk = survive(chunk); + assert!( + body_compressed(&chunk), + "the migration retries once the body is unshared" + ); + + // The floor lowered long after the body spilled, which is what an + // operator reaches for under pool pressure. Nothing here is a + // transition: the body is already several generations past the new + // floor when it moves. + set_compress_min_depth_override(Some(8)); + let chunk = TestChunk::commit(build_column(&low), 3); + assert!(!body_compressed(&chunk)); + set_compress_min_depth_override(Some(1)); + let chunk = survive(chunk); + assert_eq!(chunk.depth(), 4); + assert!( + body_compressed(&chunk), + "lowering the floor migrates bodies already past it" + ); + + set_spill_override(None); + set_compress_min_depth_override(None); + } + /// The compute and storage spill gates compose as an OR: either gate /// routes commits to the installed pool, and each setter writes only its /// own gate. @@ -1732,6 +2104,7 @@ mod tests { assert!(commit(&col), "the compute gate alone spills"); set_compute_spill_enabled(false); assert!(!commit(&col), "both gates off again"); + set_compress_min_depth_override(None); } /// Re-spilling an already-serialized body exercises the `Column::Align` From bd97ff324ee32f538e702dbdea3faf7200fa105f Mon Sep 17 00:00:00 2001 From: Dov Alperin Date: Mon, 24 Aug 2026 11:25:52 -0400 Subject: [PATCH 8/8] timely-util: whole-chunk extract pass-through via resident time bounds (#38254) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### Motivation A seal walks every stashed chunk to partition updates against the frontier, and for a spilled chunk that walk previously loaded and decoded the whole body even when no update in it could possibly be extracted (or when all of them must be). During sustained ingest the stash backlog is mostly frontier-disjoint chunks, so per seal this decoded the entire backlog to move one boundary chunk. The August benchmark campaign measured the two stash patches together as a 35% mean and ~2x p90 reduction in co-tenant probe latency at 50cc. Spilled bodies now carry their time bounds as resident metadata (an antichain lower bound and the maximal time elements). `extract_into` uses them for two whole-chunk fast paths: ship the chunk untouched when the frontier is past all of its times, keep it untouched (folding its lower bound into the residual frontier) when none of its times are ready. Only chunks the frontier genuinely straddles are loaded, decoded, and split. ### Tips for reviewer * The residual-frontier fold is the subtle part for partial orders: a kept chunk contributes its lower bound, not its individual times, which is sound because extraction only needs an upper bound on what remains. * Both fast paths have dedicated tests, including a frontier-disjoint pass-through case. ### Checklist - [ ] This PR has adequate test coverage / QA involvement has been duly considered. ([trigger-ci for additional test/nightly runs](https://trigger-ci.dev.materialize.com/)) - [ ] This PR has an associated up-to-date [design doc](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/design/README.md), is a design doc ([template](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/design/00000000_template.md)), or is sufficiently small to not require a design. - [ ] If this PR evolves [an existing `$T ⇔ Proto$T` mapping](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/command-and-response-binary-encoding.md) (possibly in a backwards-incompatible way), then it is tagged with a `T-proto` label. - [ ] If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label ([example](https://github.com/MaterializeInc/cloud/pull/5021)). - [ ] If this PR includes major [user-facing behavior changes](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/guide-changes.md#what-changes-require-a-release-note), I have pinged the relevant PM to schedule a changelog post. --- Cargo.lock | 1 + src/timely-util/Cargo.toml | 1 + src/timely-util/src/columnar/chunk.rs | 151 ++++++++++++++++++++++++-- 3 files changed, 145 insertions(+), 8 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index bb087a9b6c6e5..ce9398cd42d20 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -9021,6 +9021,7 @@ dependencies = [ "proptest", "rand 0.9.4", "serde", + "smallvec", "tempfile", "timely", "tokio", diff --git a/src/timely-util/Cargo.toml b/src/timely-util/Cargo.toml index 6231cc0c42e2b..b4c4a0bc57e74 100644 --- a/src/timely-util/Cargo.toml +++ b/src/timely-util/Cargo.toml @@ -40,6 +40,7 @@ lz4_flex.workspace = true mz-ore = { path = "../ore", default-features = false, features = ["async", "process", "tracing", "test", "num-traits", "region", "differential-dataflow", "overflowing", "pager", "pool"] } num-traits.workspace = true serde.workspace = true +smallvec.workspace = true timely.workspace = true tokio.workspace = true tracing.workspace = true diff --git a/src/timely-util/src/columnar/chunk.rs b/src/timely-util/src/columnar/chunk.rs index 1048f46d9852a..629e823245baa 100644 --- a/src/timely-util/src/columnar/chunk.rs +++ b/src/timely-util/src/columnar/chunk.rs @@ -44,6 +44,7 @@ //! resident fence metadata so a probe set faults only the chunk bodies it //! actually touches. +use std::borrow::Cow; #[cfg(test)] use std::cell::Cell; use std::cell::RefCell; @@ -58,11 +59,13 @@ use differential_dataflow::lattice::Lattice; use differential_dataflow::trace::chunk::Chunk; use mz_ore::cast::CastFrom; use mz_ore::pool::{ChunkHandle, ChunkHints, ExtentCodec, IDENTITY_CODEC, Pool}; +use smallvec::SmallVec; use timely::Accountable; +use timely::PartialOrder; use timely::container::{ContainerBuilder, PushInto}; use timely::dataflow::channels::ContainerBytes; use timely::progress::Timestamp; -use timely::progress::frontier::AntichainRef; +use timely::progress::frontier::{Antichain, AntichainRef}; use crate::columnar::batcher::{ColumnChunker, gallop}; use crate::columnar::unload::UnloadChunk; @@ -258,15 +261,25 @@ fn rr<'b, 'a: 'b, C: Columnar>(item: columnar::Ref<'a, C>) -> columnar::Ref<'b, /// A spilled chunk body: the serialized column in the pool, plus the resident /// metadata every [`Chunk`] must answer without fetching. That metadata is -/// the record count and the first and last data items (the fence entries -/// [`UnloadChunk::locate`] consults). -pub struct SpilledBody { +/// the record count, the first and last data items (the fence entries +/// [`UnloadChunk::locate`] consults), and the time bounds `extract` consults +/// to pass frontier-disjoint chunks through without loading them. +pub struct SpilledBody { /// Number of updates in the body. records: usize, /// The first and last data items, as a two-element container. One /// container rather than two singletons, so the leaf allocations are not /// duplicated per fence. fences: D::Container, + /// The minimal times in the body: a lower bound antichain every + /// contained time is greater-or-equal to. Folded into `extract`'s + /// residual frontier when the chunk is kept whole. + time_lower: Antichain, + /// The maximal times in the body. Some contained time is + /// greater-or-equal to a frontier exactly when some maximal time is, + /// which is `extract`'s ship-whole test. A single element for totally + /// ordered times, hence the inline capacity. + time_upper: SmallVec<[T; 1]>, /// Whether the body was inserted under the compressing codec. The pool /// stores the codec itself and reads decode through it, so this is the /// only handle chunk code has on what a body is stored as, and it is @@ -298,7 +311,7 @@ pub enum ColumnChunk { /// Body on the heap, shared via `Rc`, with its generational depth. Resident(Rc>, u8), /// Body in the pool, with its generational depth. See [`SpilledBody`]. - Spilled(Rc>, u8), + Spilled(Rc>, u8), } impl Clone for ColumnChunk { @@ -382,7 +395,10 @@ impl ColumnChunk { /// Commit a non-empty column at the given generational depth: spill it to /// the pool when spilling is on and the body is worth a slot, else keep it /// resident. - fn commit(column: Column<(D, T, R)>, depth: u8) -> Self { + fn commit(column: Column<(D, T, R)>, depth: u8) -> Self + where + T: Timestamp, + { mz_ore::soft_assert_no_log!(!column.is_empty(), "chunks must be non-empty"); if let Some(pool) = spill_pool() { if column.length_in_bytes() >= SPILL_MIN_BYTES { @@ -399,9 +415,13 @@ impl ColumnChunk { /// identity codec: rewritten too soon for compression to amortize, they /// stay budgeted and swap-backed while encode and decode reduce to /// copies. - fn spill_body(column: Column<(D, T, R)>, pool: &Pool, depth: u8) -> Self { + fn spill_body(column: Column<(D, T, R)>, pool: &Pool, depth: u8) -> Self + where + T: Timestamp, + { let (codec, compressed) = codec_for_depth(depth); let len_bytes = column.length_in_bytes(); + let (time_lower, time_upper) = Self::time_bounds(&column); let view = column.borrow(); let records = view.len(); let mut fences = D::Container::default(); @@ -412,6 +432,8 @@ impl ColumnChunk { Rc::new(SpilledBody { records, fences, + time_lower, + time_upper: time_upper.into(), compressed, handle, }), @@ -440,7 +462,10 @@ impl ColumnChunk { /// A shared body is skipped because re-spilling this reference cannot /// change what the other holder stores, and the compaction merger that /// shares bodies rewrites its clones immediately. - fn survive_merge(self) -> Self { + fn survive_merge(self) -> Self + where + T: Timestamp, + { let depth = self.depth().saturating_add(1); match self { ColumnChunk::Resident(col, _) => ColumnChunk::Resident(col, depth), @@ -459,6 +484,52 @@ impl ColumnChunk { } } } + + /// The chunk's time bounds: borrowed from the stored metadata for + /// spilled bodies, computed by a time-column scan for resident ones. The + /// scan costs less than the copy it lets `extract` avoid when the chunk + /// passes through whole. + fn chunk_time_bounds(&self) -> (Cow<'_, Antichain>, Cow<'_, [T]>) + where + T: Timestamp, + { + match self { + ColumnChunk::Resident(col, _) => { + let (lower, upper) = Self::time_bounds(col); + (Cow::Owned(lower), Cow::Owned(upper)) + } + ColumnChunk::Spilled(body, _) => ( + Cow::Borrowed(&body.time_lower), + Cow::Borrowed(&body.time_upper[..]), + ), + } + } + + /// The time bounds of a non-empty column: the antichain of minimal times + /// (every contained time is greater-or-equal to some element) and the + /// set of maximal times (some contained time is greater-or-equal to a + /// frontier exactly when some maximal one is). + fn time_bounds(column: &Column<(D, T, R)>) -> (Antichain, Vec) + where + T: Timestamp, + { + let (_, times, _) = column.borrow(); + let mut lower = Antichain::new(); + let mut upper: Vec = Vec::new(); + // One owned time reused across the scan, so times with owned + // allocations do not allocate per element; the bound sets clone only + // the elements they retain. + let mut time = T::minimum(); + for i in 0..times.len() { + time.copy_from(rr::(times.get(i))); + if !upper.iter().any(|u| PartialOrder::less_equal(&time, u)) { + upper.retain(|u| !PartialOrder::less_equal(u, &time)); + upper.push(time.clone()); + } + lower.insert_ref(&time); + } + (lower, upper) + } } /// Copy a column into a fresh `Typed` column via bulk per-leaf extension. @@ -666,6 +737,25 @@ where let Some(chunk) = input.pop_front() else { return; }; + // Whole-chunk pass-through from the resident time bounds: a chunk + // the frontier is entirely past ships unchanged, one entirely at or + // past the frontier keeps unchanged. Spilled bodies pass through + // without a load, a re-commit, or any codec work; only chunks the + // frontier actually splits are loaded below. + let (time_lower, time_upper) = chunk.chunk_time_bounds(); + if time_upper.iter().all(|t| !frontier.less_equal(t)) { + ship.push_back(chunk); + return; + } + if time_lower.elements().iter().all(|m| frontier.less_equal(m)) { + // The residual must lower-bound every kept time, which is the + // chunk's lower bound antichain by construction. + for m in time_lower.elements() { + residual.insert_ref(m); + } + keep.push_back(chunk); + return; + } // Partitioning rewrites within a generation, so both sides keep the // input chunk's depth. let depth = chunk.depth(); @@ -1553,6 +1643,51 @@ mod tests { assert_eq!(collect_chunks(out), consolidate(advanced)); } + /// Chunks the extract frontier does not split pass through whole from + /// their resident time bounds: spilled bodies land on their side still + /// spilled, with no load or re-commit, and a kept chunk's minimal times + /// feed the residual frontier. + #[mz_ore::test] + #[cfg_attr(miri, ignore)] + fn extract_passes_frontier_disjoint_chunks_through() { + set_spill_override(Some(test_pool())); + let low: Vec = (0..20_000u64).map(|i| ((i, 0), i % 4, 1)).collect(); + let high: Vec = (0..20_000u64).map(|i| ((i, 0), 6 + i % 4, 1)).collect(); + let spilled_chunk = |data: &[Tuple]| { + let chunk = TestChunk::commit(build_column(&consolidate(data.to_vec())), 1); + assert!(chunk.is_spilled()); + chunk + }; + + // A frontier between the two chunks' time ranges: the low chunk + // ships whole and the high chunk keeps whole, both still spilled + // (no load, no re-commit), and the residual is the kept chunk's + // minimal time. + let mut input = VecDeque::from([spilled_chunk(&low), spilled_chunk(&high)]); + let frontier = Antichain::from_elem(5u64); + let mut residual = Antichain::new(); + let (mut keep, mut ship) = (VecDeque::new(), VecDeque::new()); + while !input.is_empty() { + TestChunk::extract( + &mut input, + frontier.borrow(), + &mut residual, + &mut keep, + &mut ship, + ); + } + assert_eq!(ship.len(), 1); + assert!(ship[0].is_spilled(), "shipped whole: body untouched"); + assert_eq!(keep.len(), 1); + assert!(keep[0].is_spilled(), "kept whole: body untouched"); + assert_eq!(residual, Antichain::from_elem(6)); + let shipped = ship.pop_front().unwrap().into_column(); + assert_eq!(collect_column(&shipped), consolidate(low)); + let kept = keep.pop_front().unwrap().into_column(); + assert_eq!(collect_column(&kept), consolidate(high)); + set_spill_override(None); + } + /// Extracting a large chunk at an intermediate frontier cuts both sides /// into several chunks and partitions exactly by time. #[mz_ore::test]