adapter: move arrangement size snapshotting off the coordinator loop#37455
adapter: move arrangement size snapshotting off the coordinator loop#37455leedqin wants to merge 3 commits into
Conversation
The mz_object_arrangement_size_history snapshot did its persist reads,
consolidation, and row building inline on the coordinator main loop.
This was blocking the main coordinator work, so the
arrangement_size_history_collection_interval dyncfg is currently set to
disabled in staging and production.
Restructure the collection to mirror the storage_usage_fetch /
storage_usage_update split:
* ArrangementSizesSnapshot now only resolves catalog ids on the main
loop, then spawns a task that takes a read timestamp from a shared
oracle handle, snapshots mz_object_arrangement_sizes and
mz_compute_hydration_times, consolidates, and prepares records.
* A new ArrangementSizesWrite message carries the prepared records
back to the coordinator, which stamps the collection timestamp,
packs rows, and hands the append to an async task.
The read timestamp is taken inside the spawned task immediately before
the reads, so the window for the collections' since to overtake it is
smaller than before. A failed snapshot skips the cycle and reschedules,
as before.
Measured locally at a 10s collection interval: the snapshot handler's
main-loop time drops from ~163ms (the full collection time, by
construction) to ~61µs over 31 cycles, while the collection work runs
on the spawned task.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The collections backing the arrangement size history snapshots, mz_object_arrangement_sizes and mz_compute_hydration_times, retain rows from before an environmentd restart until the new introspection subscribes deliver their first data and the shards are reconciled. Track on each introspection subscribe when it first appended data in this process, and only record history rows for replicas whose sizes and hydration subscribes first delivered at least 10s ago. The margin covers the collection manager's batched flush and the oracle read timestamp trailing the wall clock. Without it, a snapshot racing the first append could still read the pre-restart shard contents. Stale hydration data is likewise no longer trusted: rows only get hydration_complete = true if the replica's hydration subscribe is fresh. Adds a restart workflow that kills and restarts environmentd five times at a 500ms collection interval and asserts that no post-restart history row carries a pre-restart size. Fails without this fix with 299 stale rows in the first round. Fixes SQL-218. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The subscribe feeding mz_object_arrangement_sizes filtered out objects whose arrangements totaled less than 10 MiB, so small objects never appeared in the collection at all and consumers could not distinguish "has small arrangements" from "has no arrangements". The column comment meanwhile promised exact sizes below 10 MiB, which the quantization never delivered. Drop the HAVING floor and keep the round-to-nearest-10-MiB quantization: arrangements below 5 MiB now report a size of 0 and larger ones their quantized size, with churn behavior unchanged. The column comment now describes the actual behavior. The history snapshot skips size-0 rows. The floor had been doubling as history volume control, and recording every small object on every cycle would bloat mz_object_arrangement_size_history without adding signal. Fixes SQL-271. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
d59660e to
52f8f8f
Compare
MEDIUM - replica restarts can still poison arrangement-size history before the subscribe error is processedThe PR adds That leaves a reachable stale-history window on normal replica failure:
The resulting rows are the same kind of poison rows SQL-218 is trying to avoid: A robust fix would invalidate freshness for all introspection subscribes on the |
ggevay
left a comment
There was a problem hiding this comment.
Looks good overall! Claude and I have some minor comments.
And how should we handle the flag flips in LD, i.e. turning arrangement_size_history_collection_interval back on? Someone will need to:
- turn it on in staging and prod canaries after the fix is rolled there
- turn it on in prod after we verify in staging and prod canaries that this indeed fixed the slow coordinator messages issue and the fix is rolled out to prod.
You could write about this in the release-discuss channel, either asking the release manager to do this, or just noting that you'll do this yourself.
| // transitions out of read-only. The transition is one-way, so | ||
| // `arrangement_sizes_write` needs no check of its own. | ||
| if self.controller.read_only() { | ||
| self.schedule_arrangement_sizes_collection().await; |
There was a problem hiding this comment.
I'm not sure if this schedule_arrangement_sizes_collection() call is needed here. When envd comes out of read-only mode, it restarts.
There was a problem hiding this comment.
And a nit:
The catalog server is not writable in read-only mode.
I'd say "builtin collections are not writable in read-only mode".
| collection.", | ||
| "The total arrangement heap and batcher size in bytes for this object on this replica, \ | ||
| rounded to the nearest 10 MiB boundary to reduce per-byte churn in the differential \ | ||
| collection. Objects with less than 5 MiB of arrangements report a size of 0.", |
There was a problem hiding this comment.
I think the same comment change is needed on mz_object_arrangement_size_history as well.
| and int(sz) != post_sizes[(rid, n)] | ||
| ] | ||
|
|
||
| assert not mismatches, ( |
There was a problem hiding this comment.
This assertion can false-positive, and the CI failure on this build is an instance of it, rather than the fix being broken. For a row to be recorded at all, the replica's size subscribe must be fresh, and freshness is only set once its first batch is appended, which is also when the deferred delete clears the pre-restart shard rows. So the flagged 120 MiB rows are genuine post-restart measurements: the indexes hydrate at their pre-insert state (the dataflow as-of predates the post-restart INSERT), correctly report hydration_complete = true, and a snapshot catches them before the insert has propagated into the arrangements. That state is indistinguishable by value from a stale row.
Suggestion: drop a couple of the indexes right after each restart and assert those get zero post-restart history rows. Without the fix their lingering shard rows would get recorded; with the fix nothing can be, since the new subscribes never report a dropped object. No value ambiguity.
That would also remove the need for the cumulative inserts: by the last round they add up to ~7 GB raw (likely 10+ GB actual) of arrangements on the 16 GB agents, and that memory pressure widens exactly this timing window (the CI failure hit in round 3 as the machine slowed down). If you'd rather keep the size-mismatch approach, deleting the previous round's insert before adding the new one would bound the peak instead.
| // Taking `read_ts` inside the task keeps the window between | ||
| // choosing the timestamp and reading minimal. If the collection's | ||
| // since still overtakes it, the cycle is skipped and the | ||
| // reschedule retries at the next interval. |
There was a problem hiding this comment.
Nothing pins the since across this gap, so the reads rely on both collections being retained-metrics objects, whose since lags the upper by metrics_retention (default 30 days) rather than tracking it closely. Could you record that assumption here, so it's visible if someone later repoints this at a normally-compacted collection? E.g.:
// No read hold is taken, so the reads rely on both collections being
// retained-metrics objects, whose since lags the upper by
// `metrics_retention` rather than tracking it closely. If the since
// still overtakes `read_ts`, the snapshot fails, and the cycle is
// skipped and retried at the next interval.
| }; | ||
| // It is not an error for this task to outlive `internal_cmd_rx`. | ||
| if let Err(e) = internal_cmd_tx.send(msg) { | ||
| warn!("internal_cmd_rx dropped before we could send: {e:?}"); |
There was a problem hiding this comment.
nit: The two error paths above use let _ = internal_cmd_tx.send(...) while this one warns, and the comment says outliving the receiver is not an error. Consider one treatment for all three (the silent form, going by the comment).
| ]) | ||
| } | ||
|
|
||
| fn hydration_row(replica_id: &str, object_id: &str, hydrated: bool) -> Row { |
There was a problem hiding this comment.
nit: The real mz_compute_hydration_times.time_ns column is uint8, so Datum::UInt64 would keep the fixture faithful to the schema. Works today because the code under test only checks is_null.
| &fresh_size_replicas, | ||
| &fresh_hydration_replicas, | ||
| ); | ||
| collection_metric_timer.observe_duration(); |
There was a problem hiding this comment.
nit: The help text of mz_arrangement_sizes_collection_time_seconds ("…and prepare history-table updates…") now slightly overstates the timer's scope, since row packing moved to arrangement_sizes_write outside it. Fine to fold in only if you touch metrics.rs anyway.
| let live_snapshot = match storage_collections.snapshot(live_global_id, read_ts).await { | ||
| Ok(s) => s, | ||
| Err(e) => { | ||
| tracing::warn!("arrangement sizes snapshot failed: {e:?}"); |
There was a problem hiding this comment.
All reachable failure modes of snapshot indicate invariant violations for these two collections: ReadBeforeSince cannot happen short of a read-policy bug, since both collections are retained-metrics objects whose since lags the upper by metrics_retention (default 30 days), IdentifierMissing cannot happen for a builtin collection, and a decode error would mean error rows in an introspection collection. Consider soft_panic_or_log instead of warn! here and in the arm below, so a violation is loud in CI and reaches Sentry in production. Since soft_panic_or_log returns in production, keep the ArrangementSizesSchedule send after it, so production degrades to a skipped cycle rather than the collection stopping.
Remove these sections if your commit already has a good description!
Motivation
mz_object_arrangement_size_historycollection is currently disabled instaging and production via the
arrangement_size_history_collection_intervalLD flag: the snapshot ran its persist reads and row building inline on the
coordinator main loop and dominated the "Slow Coordinator Messages"
dashboards during canary verification
(https://materializeinc.slack.com/archives/CTESPM7FU/p1778508034797449).
This PR fixes CNS-42, plus two known issues with the same feature.
Fixes SQL-218
Fixes SQL-271
Description
Three commits, best reviewed individually:
Move snapshotting off the coordinator loop. Mirrors the
storage_usage_fetch/storage_usage_updatesplit: the snapshot handleronly resolves catalog ids on the main loop, a spawned task does the
persist snapshots and record prep, and a new
ArrangementSizesWritemessage carries the prepared records back for the cheap
stamp-pack-append phase. The read timestamp is taken inside the task
immediately before the reads; a failed snapshot skips the cycle and
reschedules, as before. No change to the data produced.
Don't record stale sizes after a restart (SQL-218). The backing
collections retain pre-restart rows until the new introspection
subscribes deliver and the shards are reconciled, so post-restart
snapshots recorded outdated sizes tagged
hydration_complete = true.Each introspection subscribe now tracks when it first appended data in
this process, and the snapshot only records rows for replicas whose
sizes and hydration subscribes first delivered at least 10s ago. The
margin covers the collection manager's batched flush and the oracle
read timestamp trailing the wall clock. Observable change: a short
history gap after restarts and replica reconnects, instead of poison
rows that lived until the 7-day pruner.
Report sub-10 MiB arrangements (SQL-271). The subscribe's
HAVINGfloor silently dropped small objects, contradicting the column docs.
The floor is removed and the round-to-nearest-10-MiB quantization
stays, so arrangements below 5 MiB report a size of 0 and every
arranged object is present, with churn behavior unchanged. The history
snapshot skips size-0 rows, since the floor had been doubling as
history volume control. Observable change:
mz_object_arrangement_sizesgains rows for small objects; the
sizecolumn comment now describesthe actual behavior.
Verification
covering the hydration flag, freshness filtering, zero-size skip, and
malformed input.
arrangement_sizes_stale_snapshot_after_restart(
test/restart/mzcompose.py): five kill/restart rounds at a 500mscollection interval asserting no post-restart history row carries a
pre-restart size. Without commit 2 it fails with 299 stale rows in the
first round.
test/testdrive/arrangement-sizes.td: a sub-10 MiB indexappears in the live collection with size 0 on both replicas and never
lands in the history.
time (
mz_slow_message_handling) drops from ~163ms, the full collectiontime by construction, to ~61µs over 31 cycles, while the collection work
runs on the spawned task.
Rollout: after this ships in a release, re-enable
arrangement_size_history_collection_intervalfor a canary environment inLaunchDarkly and confirm the "Slow Coordinator Messages" panel stays flat
before restoring the default.