adapter: hydrate migrated builtin MVs before 0dt cut-over - #38399
adapter: hydrate migrated builtin MVs before 0dt cut-over#38399mtabebe wants to merge 2 commits into
Conversation
89c7a98 to
29624d1
Compare
I didn't follow this one, about the "catalog shard frontier" being held. |
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. The behaviour is gated behind the `enable_0dt_hydrate_migrated_builtin_mvs` feature flag, on by default, a break-glass revert to the prior exclude-from-gate behaviour without a redeploy. 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.
29624d1 to
ed1200d
Compare
This refers to #35402. If the old version is before that, MVs might not be able to hydrate, because the catalog shard's upper was not guaranteed to be near the wall clock. |
QA LLM Review1. MEDIUM -- Neither new test can distinguish "hydrated before
|
ggevay
left a comment
There was a problem hiding this comment.
(Posting current thoughts, will continue on Monday)
| // read one (`mz_clusters` joins `mz_cluster_replica_size_internal`): | ||
| // `read_only_mode_table_worker` keeps advancing the uppers of migrated tables, so | ||
| // an MV over one still catches up. | ||
| let new_builtin_mvs = new_builtin_collections |
There was a problem hiding this comment.
Seconding finding 2 of the QA LLM review, which we found independently: the exclusion
closure is seeded from new builtin MVs only, so dependents of new builtin
tables and sources are not walked. Those inputs get no writer while read-only
(register_table retains only migrated tables for the read-only table worker), so a
migrated MV reading a new builtin table can produce nothing past the input's empty
frontier: its own upper stays at the minimum and it fails this gate, and with
hydrate_migrated_mvs it now stays in the caught-up check, blocking promotion until
someone runs skip-catchup (with_0dt_deployment_max_wait defaults to a year) or pulls
the break-glass and restarts the pod. No current conversion MV reads a new builtin
table, so this is latent, but it is one release away. Seeding the walk from all of
new_builtin_collections would close it.
There was a problem hiding this comment.
Good catch, walk starts from new_builtin_collections now.
| %collection_hydrated, | ||
| "collection {id} not in live frontiers" | ||
| ); | ||
| if write_frontier.less_equal(&Timestamp::minimum()) || !collection_hydrated { |
There was a problem hiding this comment.
Two notes on the strengthened condition:
- It is not gated on
hydrate_migrated_mvs(or the leader version) and applies to
every collection missing fromlive_frontiers, so disabling the break-glass flag
does not fully restore the old behavior, while the flag's description says it
"reverts to excluding migrated MVs". As far as we can tell the ungated reach is
narrow (with the flag off, migrated MVs are excluded, and the only newly blocked
class is collections with a frontier past the minimum that cannot hydrate), so
softening the flag description may be enough; gating this condition on the same
bool is the alternative if you want the revert to be exact. - Optional: unlike the strong path below, there is no lag check against
nowhere, so
an MV that hydrates once and then stalls keeps satisfying the gate. The pre-existing
condition had the same shape, so this may be fine, but it seems worth a conscious
decision.
| "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 \ |
There was a problem hiding this comment.
On top of finding 3 of the QA LLM review (boot-time capture, restart required, worth
stating in this description, seconded): there is no
LaunchDarkly flag (or KNOWN_MISSING_FROM_LD allowlist entry) for it yet, so in cloud
the break-glass could not actually be pulled today, and the
launchdarkly-flag-consistency check will likely flag it.
There was a problem hiding this comment.
I updated the description of the flag.
I think we should defer LD?
| #[mz_ore::test(tokio::test(flavor = "multi_thread"))] | ||
| #[cfg_attr(miri, ignore)] // too slow | ||
| #[allow(clippy::disallowed_methods)] | ||
| async fn test_0dt_migrated_builtin_mv_hydrates_before_promotion() { |
There was a problem hiding this comment.
Nice test, and it does fail without the fix. This overlaps finding 1 of the QA LLM
review, which we can partly confirm empirically: rerunning the test with logging shows
all 219 "not in live frontiers" hits land in the first caught-up tick (while the
replaced mz_cluster_replica_frontiers shard is still empty), and from the second tick
every collection, including the migrated MVs, passes on the with-live-frontier path
against the deployment's own frontiers. We also ran two mutants: with the
no-live-frontier condition rewritten to wave everything through, the test still passes;
with the flag defaulted off (MVs not write-enabled), it reaches ReadyToPromote and
only fails at the delayed peek, which supports the review's point that the peek cannot
pin the ordering either. The production shape, where the leader hosts the id as a table
so the MV must pass through the no-live-frontier branch's frontier > minimum && hydrated, is never in the deciding position in either test.
Related landmine worth a NOTE: a real Replacement step for
mz_cluster_replica_frontiers would make the production gate self-referential the same
way.
Concrete suggestion: a flag-off variant of this test is cheap and valuable (we ran one:
with enable_0dt_hydrate_migrated_builtin_mvs defaulted off, ReadyToPromote is still
reached and the pre-promote peek times out, so it pins both the fallback path and the
fix). Covering the decisive no-live-frontier path likely needs
--force-builtin-schema-migration to accept a subset of objects, so
mz_cluster_replica_frontiers keeps its real shard while the MVs are replaced.
There was a problem hiding this comment.
Ok I think I am going to defer the no live frontier change. Pushing the sub set down is a lot of churn for this bug fix IMO
| ) -> Result<(), CollectionUpdateError> { | ||
| // Every builtin eligible for the read-only bypass has a system id. A non-system id means | ||
| // the caller's `Replacement`-only invariant broke, so degrade to the read-only no-op (cold | ||
| // collection at cut-over) rather than risk writing a shard the leader still serves. Mirrors |
There was a problem hiding this comment.
Three small accuracy points on the docs here:
- "degrade to the read-only no-op (cold collection at cut-over)": with
hydrate_migrated_mvson, a collection that hits this tripwire stays in the
caught-up gate (exclusion only happens when the feature is off), and with its shard
unwritten its frontier never leaves the minimum, so the actual failure mode is
blocking promotion, not arriving cold at cut-over. - The storage-side check this claims to mirror is a hard
assert!, not a soft panic
with a fallback. - The exclusivity guarantee is per (version, deploy generation), not per process: two
read-only processes at the same version and generation share the shard and both
write it (savepoint opens do not fence). That is the same shape as multi-replica MV
sinks, so it is fine, but the comment could say why it is tolerable.
Also worth a sentence somewhere: the replica-side AllowWrites handler enables persist
compaction process-wide on the clusterd, which this path triggers inside a read-only
deployment for the first time.
| 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. |
There was a problem hiding this comment.
This paragraph states the force-write unconditionally. Worth mentioning the two
conditions the code has: the old leader must be at v26.17 or newer (it has to keep the
catalog shard's frontier advancing, see MIN_LEADER_VERSION_FOR_MIGRATED_MV_WRITES),
and the enable_0dt_hydrate_migrated_builtin_mvs break-glass, with exclusion from the
caught-up gate as the fallback in both cases.
| // 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 new builtin MVs, whose fresh shard has no writer until |
There was a problem hiding this comment.
Optional: the comment could say WHY new builtin MVs cannot get the same write-enable
treatment: their shard allocation lives only in the read-only savepoint, so the
promoted leader allocates a different shard and preflight writes would be discarded.
Replacement shards survive promotion via the durable migration-shard entry, which is
what makes force-writing them worthwhile.
| @@ -2773,7 +2787,22 @@ impl Coordinator { | |||
| // If this is a replacement MV, it must remain read-only until the replacement | |||
There was a problem hiding this comment.
Now that this branch also handles migration-Replacement MVs, the word "replacement"
means two unrelated things within three lines. Suggest disambiguating this comment,
for example: "If this MV is a pending REPLACEMENT FOR another MV, it must stay
read-only until ALTER ... APPLY REPLACEMENT swaps it in." The two sets are disjoint
(builtin MVs never come from user DDL), so this is wording only.
…caught-up path Two gaps in the 0dt caught-up gate, both found in review of MaterializeInc#38399. Seed the transitive-dependent walk from all of `new_builtin_collections`, not just the materialized views among them. A brand-new builtin table or source has no read-only writer either, since `register_table_collections` retains only migrated tables while read-only, so a `Replacement`-migrated MV reading one can never advance past that empty frontier. With migrated MVs now kept in the gate, such an MV would block promotion until an operator intervened. No builtin table or source has been added since v26.38.0, so this is latent today. Require the write frontier to be within the allowed lag of `now` on the no-live-frontier path, the same bound the live-frontier path applies. Hydration is a one-shot property, so a collection that hydrated and then stalled satisfied that branch forever. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review of MaterializeInc#38399 found several claims that no longer match the code: - The `allow_writes_in_read_only` tripwire falls back to a no-op, which now blocks promotion rather than leaving a cold collection at cut-over, and the storage-side check it points at is a hard assert, not a soft panic with a fallback. Say both, and say why compute is deliberately softer. - Exclusive ownership of a replacement shard holds per (build version, deploy generation), not per process. Two read-only processes of one generation both write the shard, the same shape as a multi-replica MV, which the self-correcting persist sink is built to tolerate. - The replica-side handler enables persist compaction process-wide, which this path is the first thing to trigger inside a read-only deployment. - "Replacement" meant two unrelated things three lines apart in `bootstrap`: user `REPLACEMENT FOR` DDL and the builtin-migration mechanism. The sets are disjoint, so this is wording only. - Record why a *new* builtin MV cannot be write-enabled the same way: its shard allocation lives only in the read-only savepoint, so the promoted leader allocates a different shard. - Cite PR MaterializeInc#35402 for the v26.17 leader floor, and note in `caught_up.rs` that a future `Replacement` of `mz_cluster_replica_frontiers` would make the gate read its own frontiers. The design doc stated the force-write unconditionally; give the two conditions and the exclude-from-gate fallback. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review of MaterializeInc#38399 found several claims that no longer match the code: - The `allow_writes_in_read_only` tripwire falls back to a no-op, which now blocks promotion rather than leaving a cold collection at cut-over, and the storage-side check it points at is a hard assert, not a soft panic with a fallback. Say both, and say why compute is deliberately softer. - Exclusive ownership of a replacement shard holds per (build version, deploy generation), not per process. Two read-only processes of one generation both write the shard, the same shape as a multi-replica MV, which the self-correcting persist sink is built to tolerate. - The replica-side handler enables persist compaction process-wide, which this path is the first thing to trigger inside a read-only deployment. - "Replacement" meant two unrelated things three lines apart in `bootstrap`: user `REPLACEMENT FOR` DDL and the builtin-migration mechanism. The sets are disjoint, so this is wording only. - Record why a *new* builtin MV cannot be write-enabled the same way: its shard allocation lives only in the read-only savepoint, so the promoted leader allocates a different shard. - Cite PR MaterializeInc#35402 for the v26.17 leader floor, and note in `caught_up.rs` that a future `Replacement` of `mz_cluster_replica_frontiers` would make the gate read its own frontiers. The design doc stated the force-write unconditionally; give the two conditions and the exclude-from-gate fallback. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review of MaterializeInc#38399 found several claims that no longer match the code: - The `allow_writes_in_read_only` tripwire falls back to a no-op, which now blocks promotion rather than leaving a cold collection at cut-over, and the storage-side check it points at is a hard assert, not a soft panic with a fallback. Say both, and say why compute is deliberately softer. - Exclusive ownership of a replacement shard holds per (build version, deploy generation), not per process. Two read-only processes of one generation both write the shard, the same shape as a multi-replica MV, which the self-correcting persist sink is built to tolerate. - The replica-side handler enables persist compaction process-wide, which this path is the first thing to trigger inside a read-only deployment. - "Replacement" meant two unrelated things three lines apart in `bootstrap`: user `REPLACEMENT FOR` DDL and the builtin-migration mechanism. The sets are disjoint, so this is wording only. - Record why a *new* builtin MV cannot be write-enabled the same way: its shard allocation lives only in the read-only savepoint, so the promoted leader allocates a different shard. - Cite PR MaterializeInc#35402 for the v26.17 leader floor, and note that forcing the replacement mechanism across all builtins, as the 0dt tests do, leaves the caught-up gate reading its own frontiers. The design doc stated the force-write unconditionally; give the two conditions and the exclude-from-gate fallback. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…caught-up path Two gaps in the 0dt caught-up gate, both found in review of MaterializeInc#38399. Seed the transitive-dependent walk from all of `new_builtin_collections`, not just the materialized views among them. A brand-new builtin table or source has no read-only writer either, since `register_table_collections` retains only migrated tables while read-only, so a `Replacement`-migrated MV reading one can never advance past that empty frontier. With migrated MVs now kept in the gate, such an MV would block promotion until an operator intervened. No builtin table or source has been added since v26.38.0, so this is latent today. Require the write frontier to be within the allowed lag of `now` on the no-live-frontier path, the same bound the live-frontier path applies. Hydration is a one-shot property, so a collection that hydrated and then stalled satisfied that branch forever. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review of MaterializeInc#38399 found several claims that no longer match the code: - The `allow_writes_in_read_only` tripwire falls back to a no-op, which now blocks promotion rather than leaving a cold collection at cut-over, and the storage-side check it points at is a hard assert, not a soft panic with a fallback. Say both, and say why compute is deliberately softer. - Exclusive ownership of a replacement shard holds per (build version, deploy generation), not per process. Two read-only processes of one generation both write the shard, the same shape as a multi-replica MV, which the self-correcting persist sink is built to tolerate. - The replica-side handler enables persist compaction process-wide, which this path is the first thing to trigger inside a read-only deployment. - "Replacement" meant two unrelated things three lines apart in `bootstrap`: user `REPLACEMENT FOR` DDL and the builtin-migration mechanism. The sets are disjoint, so this is wording only. - Record why a *new* builtin MV cannot be write-enabled the same way: its shard allocation lives only in the read-only savepoint, so the promoted leader allocates a different shard. - Cite PR MaterializeInc#35402 for the v26.17 leader floor, and note that forcing the replacement mechanism across all builtins, as the 0dt tests do, leaves the caught-up gate reading its own frontiers. The design doc stated the force-write unconditionally; give the two conditions and the exclude-from-gate fallback. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- Seed the caught-up gate's transitive-dependent walk from all of `new_builtin_collections`, not just the MVs, and require the write frontier to be within the allowed lag of `now` on the no-live-frontier path so a hydrated-then-stalled collection no longer passes the gate forever. - Document the `enable_0dt_hydrate_migrated_builtin_mvs` break-glass flag: it is read once at startup, so flipping it means changing the setting on the leader and restarting, and it is not an exact revert. - Correct the read-only write-enable comments to match the code: the `allow_writes_in_read_only` tripwire blocks promotion, exclusive shard ownership holds per (build version, deploy generation), a new builtin MV cannot be write-enabled, and cite PR MaterializeInc#35402 for the v26.17 leader floor.
Problem:
A builtin schema migration using the
Replacementmechanism hands the new deployment a fresh persist shard. Nothing writes that shard while the deployment is read-only, becauseComputeController::allow_writesno-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:
test_0dt_migrated_builtin_mv_hydrates_before_promotionboots a read-only generation with a forcedreplacementmigration and asserts the migrated builtin MVsmz_databasesandmz_clustersare readable before it reportsReadyToPromote, proving they hydrate before cut-over rather than at it.0dtmzcompose workflowbuiltin-schema-migrations-replacementto read those MVs from the read-only generation before promotion, guarding the same invariant through a real cross-version upgrade.