Repository navigation
storage: split upsert_continual_feedback_v2 into submodules - #40190
Merged
Merged
Conversation
DAlperin
marked this pull request as draft
October 26, 2026 14:54
DAlperin
force-pushed
the
dov/upsert-v2-split-module
branch
from
October 26, 2026 15:06
9a40b7c to
b074256
Compare
DAlperin
force-pushed
the
dov/upsert-v2-stash-buckets
branch
from
October 26, 2026 15:06
4eb7b3e to
5e71a9e
Compare
DAlperin
force-pushed
the
dov/upsert-v2-split-module
branch
from
October 26, 2026 15:23
b074256 to
2f99f92
Compare
DAlperin
force-pushed
the
dov/upsert-v2-stash-buckets
branch
from
October 26, 2026 15:23
5e71a9e to
c5b9002
Compare
DAlperin
marked this pull request as ready for review
October 26, 2026 15:23
DAlperin
force-pushed
the
dov/upsert-v2-stash-buckets
branch
from
October 26, 2026 16:13
c5b9002 to
dca974f
Compare
DAlperin
force-pushed
the
dov/upsert-v2-split-module
branch
2 times, most recently
from
October 26, 2026 16:28
7bf4dfd to
a8a885c
Compare
DAlperin
force-pushed
the
dov/upsert-v2-stash-buckets
branch
2 times, most recently
from
October 26, 2026 16:59
7c14dec to
fcbf650
Compare
DAlperin
force-pushed
the
dov/upsert-v2-split-module
branch
2 times, most recently
from
October 26, 2026 18:52
3860cb0 to
8365a39
Compare
DAlperin
force-pushed
the
dov/upsert-v2-stash-buckets
branch
from
October 26, 2026 18:52
fcbf650 to
af4127b
Compare
DAlperin
force-pushed
the
dov/upsert-v2-split-module
branch
from
October 26, 2026 20:28
8365a39 to
e5e79d9
Compare
DAlperin
force-pushed
the
dov/upsert-v2-stash-buckets
branch
from
October 26, 2026 20:53
20ba367 to
b3c5127
Compare
DAlperin
force-pushed
the
dov/upsert-v2-split-module
branch
from
October 26, 2026 20:53
e5e79d9 to
9f63102
Compare
DAlperin
added a commit
that referenced
this pull request
Oct 26, 2026
The upsert v2 operator drains one source timestamp per persist round trip, but its stash was a single key-sorted batcher sealed at the input frontier. When a source falls behind, every round merged and extracted the whole backlog, processed the one timestamp at the persist frontier, and pushed everything else back through a resident `Vec` outside the buffer pool budget. Per-round cost grew with the backlog, so catch-up was quadratic in it. In production this showed up as a paged-flavor source that could not catch up on any replica size after a producer rate increase. CPU went to pushing ineligible data back (re-paging and re-compressing it) and to sealing. Doubling workers doubled CPU without raising the drain rate. See [CPU-320](https://linear.app/materializeinc/issue/CPU-320). #40190, stacked on this one, splits the module into submodules as a pure move. ### Changes * **Seal just past the persist frontier** (`seal_upper`: the start of the next `mz_repr::Timestamp`, capped at the input frontier), in both flavors, so a round extracts only what it can process. * **Make the chunked stash time-major.** It stores `((time, key), time, diff)`, so a seal cuts a prefix of the batcher's chains. `ColumnChunk`'s merge passes chunks with disjoint data ranges through, and its extract passes chunks wholly on one side of the upper through by their time bounds, so a seal rewrites only the boundary chunk. Within one time, entries stay sorted by key, which the drain's monotone probes rely on. * **The paged flavor keeps its key-major batcher.** Its seal (`extract_chain`) reads every chunk whatever the upper, so it stays quadratic, but the narrower seal halves its catch-up time. * `UpsertStashArm` gains a per-arm chunker and `flush`. * **Count pushed-back updates.** Adds `mz_storage_upsert_restashed_total`, the number of updates the v2 operator extracted before the output reached their time and pushed back. On a source stuck the way [CPU-320](https://linear.app/materializeinc/issue/CPU-320) describes, it climbs every round. An earlier revision of this PR bucketed the stash by time with a `BucketChain` of key-major batchers instead. Review pointed out that time-major order removes the need for it. The time-major version needs no changes to `mz_timely_util::temporal`, is about 200 lines smaller, and drains faster when the stash spills. ### Performance Optimized build, draining a tail of N timestamps × 10k updates over 1M keys, one timestamp per persist advance. Drain seconds, paged / chunked: | | 100 ts | 200 ts | 400 ts | | -- | -- | -- | -- | | `main` | 6.5 / 4.0 | 26.9 / 16.9 | | | `seal_upper` alone, key-major | 3.3 / 3.6 | 12.8 / 14.2 | | | `BucketChain` (earlier revision) | 0.50 / 0.47 | 1.11 / 1.19 | 2.67 / 2.63 | | this PR, chunked stash resident | n/a / 0.48 | n/a / 1.48 | n/a / 5.35 | | this PR, chunked stash spilled | n/a / 0.15 | n/a / 0.32 | n/a / 0.67 | `seal_upper` alone stays quadratic. With the chunked stash spilling to the buffer pool (`enable_upsert_paged_spill`), the time-major stash drains linearly and about 4x faster than the bucketed one, and writes a quarter as many chunks to the pool (647 against 2636 at 400 timestamps). With spilling off, chunks stay resident and recompute their time bounds by scanning on every seal, so the drain grows faster than linear. Caching the time bounds on resident `ColumnChunk`s would remove that, as a follow-up in `mz-timely-util`. ### Tests * `tail_catch_up_restashes_nothing`: drains a 20-timestamp tail one persist advance at a time and asserts nothing is pushed back. On `main` the same scenario pushes back 1710 updates. * `later_subtime_is_pushed_back_and_drained`: an update at a later subtime of the persist frontier's timestamp is pushed back exactly once, then drained against the earlier value. * `persist_jump_into_tail`: a persist frontier that jumps into a stashed tail drops the times it passed, drains the one it lands on, and keeps the rest without extracting it. * `spilled_tail_drains_across_split_chunks`: with spilling forced, a 12-timestamp tail with more than a chunk per timestamp drains correctly, and the pool sees at most 5 inserts per timestamp (34 in practice). A seal that rewrote every chunk past its upper would add about 100. * `refined_input_frontier_drains_finalized_subtimes`, `far_ahead_data_drains`, `source_tail_ahead_of_persist`, `source_tail_across_rounds`: draining multi-timestamp tails. * `open_time_across_rounds_consolidates`, `time_ahead_of_frontier_across_rounds_consolidates`: two updates for one key at one time, arriving in separate operator rounds, consolidate to the later one. * `paged_stash_matches_model`, `chunked_stash_matches_model`: each arm's batcher against a model through random pushes, seals and push-backs. The chunked check also asserts sealed output is time-major. * `seal_upper_stops_past_persist_upper`: the seal upper computation, including the maximum timestamp. All v2 operator tests run against both stash flavors and assert equal output. ### Release notes None. Upsert v2 is gated behind `enable_upsert_v2`. Generated with [Claude Code](https://claude.com/claude-code)
The module had grown to 2.4k lines. Move code without changing it into submodules along the existing seams: * feedback: the persist-feedback arrangement types, encode_feedback, and the UpsertValue to Row codec. * stash: UpsertDiff, the stash buckets, and the per-flavor UpsertStashArm impls. * drain: classifying sealed stash entries against the persist frontier and emitting eligible output, one function per flavor. * tests: the unit tests. The parent keeps the module docs, UpsertStashFlavor, upsert_inner, and the operator loop. Moved items are pub(super), and the crate-visible surface is unchanged.
DAlperin
force-pushed
the
dov/upsert-v2-split-module
branch
from
October 26, 2026 21:27
9f63102 to
ee49fce
Compare
DAlperin
enabled auto-merge (squash)
October 27, 2026 05:14
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #40189.
upsert_continual_feedback_v2.rshad grown to 2.8k lines. This moves its code into submodules along the seams it already had:feedback: the persist-feedback arrangement types,encode_feedback, and theUpsertValuetoRowcodec.stash:UpsertDiff, the stash batcher types,seal_upper, and the per-flavorUpsertStashArmimpls.drain: classifying sealed stash entries against the persist frontier and emitting eligible output, one function per flavor.tests: the unit tests.The parent keeps the module docs,
UpsertStashFlavor,upsert_inner, and the operator loop.This is a pure move. Apart from rustfmt reflows, the only differences are module headers, explicit imports per file,
pub(super)on the moved items, and two intra-doc links that now need paths. The crate-visible surface is unchanged:upsert_value_to_rowand the fuzzing-gateddatum_seq_to_upsert_valueare re-exported from the parent.Release notes
None.
Generated with Claude Code