Skip to content

storage: split upsert_continual_feedback_v2 into submodules - #40190

Merged
DAlperin merged 1 commit into
mainfrom
dov/upsert-v2-split-module
Oct 27, 2026
Merged

DAlperin merged 1 commit into
mainfrom
dov/upsert-v2-split-module

Conversation

@DAlperin

@DAlperin DAlperin commented Oct 26, 2026 •

Copy link
Copy Markdown
Member

Stacked on #40189.

upsert_continual_feedback_v2.rs had 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 the UpsertValue to Row codec.
  • stash: UpsertDiff, the stash batcher types, seal_upper, 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.

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_row and the fuzzing-gated datum_seq_to_upsert_value are re-exported from the parent.

Release notes

None.

Generated with Claude Code

@DAlperin
DAlperin requested a review from a team as a code owner October 26, 2026 14:45
@DAlperin
DAlperin added this pull request to stack #40191 October 26, 2026 14:52
@DAlperin
DAlperin marked this pull request as draft October 26, 2026 14:54
@DAlperin
DAlperin force-pushed the dov/upsert-v2-split-module branch from 9a40b7c to b074256 Compare October 26, 2026 15:06
@DAlperin
DAlperin force-pushed the dov/upsert-v2-stash-buckets branch from 4eb7b3e to 5e71a9e Compare October 26, 2026 15:06
@DAlperin
DAlperin force-pushed the dov/upsert-v2-split-module branch from b074256 to 2f99f92 Compare October 26, 2026 15:23
@DAlperin
DAlperin force-pushed the dov/upsert-v2-stash-buckets branch from 5e71a9e to c5b9002 Compare October 26, 2026 15:23
@DAlperin
DAlperin marked this pull request as ready for review October 26, 2026 15:23
@DAlperin
DAlperin requested a review from antiguru October 26, 2026 15:23

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Devin Review

@DAlperin
DAlperin force-pushed the dov/upsert-v2-stash-buckets branch from c5b9002 to dca974f Compare October 26, 2026 16:13
@DAlperin
DAlperin force-pushed the dov/upsert-v2-split-module branch 2 times, most recently from 7bf4dfd to a8a885c Compare October 26, 2026 16:28
@DAlperin
DAlperin force-pushed the dov/upsert-v2-stash-buckets branch 2 times, most recently from 7c14dec to fcbf650 Compare October 26, 2026 16:59
@DAlperin
DAlperin force-pushed the dov/upsert-v2-split-module branch 2 times, most recently from 3860cb0 to 8365a39 Compare October 26, 2026 18:52
@DAlperin
DAlperin force-pushed the dov/upsert-v2-stash-buckets branch from fcbf650 to af4127b Compare October 26, 2026 18:52
@DAlperin
DAlperin force-pushed the dov/upsert-v2-split-module branch from 8365a39 to e5e79d9 Compare October 26, 2026 20:28
@DAlperin
DAlperin force-pushed the dov/upsert-v2-stash-buckets branch from 20ba367 to b3c5127 Compare October 26, 2026 20:53
@DAlperin
DAlperin force-pushed the dov/upsert-v2-split-module branch from e5e79d9 to 9f63102 Compare October 26, 2026 20:53
Base automatically changed from dov/upsert-v2-stash-buckets to main October 26, 2026 21:27
@buildkite-limited-access
buildkite-limited-access Bot requested a review from a team as a code owner October 26, 2026 21:27
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
DAlperin force-pushed the dov/upsert-v2-split-module branch from 9f63102 to ee49fce Compare October 26, 2026 21:27
@DAlperin
DAlperin removed this pull request from stack #40191 October 27, 2026 05:10
@DAlperin
DAlperin enabled auto-merge (squash) October 27, 2026 05:14

@antiguru antiguru left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛺

@DAlperin
DAlperin merged commit fc090b2 into main Oct 27, 2026
83 checks passed
@DAlperin
DAlperin deleted the dov/upsert-v2-split-module branch October 27, 2026 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants