Conversation
Chunks are sized in rows and cut from the live table; the first is open below and the last open above so every key belongs to exactly one chunk (CO-4 coverage) and a watermark at the largest int64 means copy complete. Boundary parameters are declared bigint so bounds outside a smallint or integer key's range can be sent while the key index still serves the query.
The landed watermark lags the chunks already read under concurrent workers, so the applier's CO-4 discard rule needs Chunker.Cut, not the watermark. Feedback scales the measured chunk's own size so concurrent reports agree instead of compounding; INV markers, ST-6 ids, and one-bound defaults fitting address the remaining review findings.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
🤖 1/2: adversarial correctness. I reviewed
0 blocking, 3 non-blocking. All 17 mutations are killed. They cover:
The tests reach every branch they claim to cover. Splitting the cut frontier from the landed watermark is the right call. The CO-4 text now says which position governs the discard rule. Before, it said "the copier's watermark", which becomes wrong the moment a second worker exists. Invariants:
Non-blocking1. Resuming from the landed watermark leaves A concrete run:
The checksum would catch it before cutover, but The discard rule's premise is "the copier will read the current row", and that needs "and nothing is in the shadow for that key yet".
Neither is this PR's code to write, but a sentence there keeps Phase 8 from inheriting the gap. Test case (not run here)This one crosses components that do not exist yet: resume, the applier, and the copy step. The shape it would take:
2.
Test case: a boundary query that does not returntype slowBoundary struct{ started, release chan struct{} }
type slowRow struct{ q *slowBoundary }
func (q *slowBoundary) QueryRow(context.Context, string, ...any) pgx.Row {
close(q.started)
return slowRow{q}
}
func (r slowRow) Scan(dest ...any) error {
<-r.q.release
v := int64(999)
*(dest[0].(**int64)) = &v
return nil
}
func TestChunkerCutDoesNotWaitOnBoundaryQuery(t *testing.T) {
c := &Chunker{opts: ChunkerOptions{}.withDefaults(), next: math.MinInt64, rows: 1000}
q := &slowBoundary{started: make(chan struct{}), release: make(chan struct{})}
nextDone := make(chan struct{})
go func() {
defer close(nextDone)
_, _, _ = c.Next(context.Background(), q)
}()
<-q.started
cutDone := make(chan struct{})
go func() { defer close(cutDone); c.Cut() }()
select {
case <-cutDone:
case <-time.After(2 * time.Second):
close(q.release)
<-nextDone
require.Fail(t, "Cut blocked for the duration of an in-flight boundary query")
}
close(q.release)
<-nextDone
upper, ok := c.Cut()
assert.True(t, ok)
assert.Equal(t, int64(999), upper)
}On With the two-mutex split described above, it passes, and so does the rest of 3. This review was generated by Claude Code (claude-opus-5-5). |
|
🤖 2/2: the two lenses. OSS adoption, and integration ease for 0 blocking, 2 non-blocking. Lens 1: OSS adoption
The package synopsis has not caught up. Lens 2: integration ease for importersThe exported surface grows by additions only, and nothing outside
This is the same split I raised for This review was generated by Claude Code (claude-opus-5-5). |
aparajon
left a comment
aparajon
left a comment
There was a problem hiding this comment.
🤖 Stamped: 0 blocking, 5 non-blocking. See the review comments above.
This stamp was left by Claude Code (claude-opus-5-5).
morgo
left a comment
morgo
left a comment
There was a problem hiding this comment.
🤖 Automated adversarial review, posted on Morgan Tocker's behalf.
Approving. The cut-frontier-vs-landed-watermark distinction is the right call and the description makes the case for it better than the code alone could. The tiling is what earns CO-4 rather than merely claiming it.
Things I checked rather than assumed, because each would have been a coverage hole:
c.next == math.MinInt64really does mean "nothing cut yet".Cutuses it as the sentinel forok=false, which would be wrong if a resume could land there. It cannot:startAfterreturnsfrom.Value()+1for a valid watermark, so reachingMinInt64would need a watermark ofMinInt64-1, which is not representable. The sentinel is sound.- The discard rule is safe in both directions. A key at or below
Cutis in a chunk already cut, so it is applied or deferred — that is the case the landed watermark got wrong. A key aboveCutis in a region not yet cut, and since cutting precedes copying, the eventual read is strictly later in real time than theCutcall that authorised the discard, so the copier sees the committed change. No window either way. - Even division does not lose the open-above chunk. When exactly
rowskeys remain, the boundary query returns the last one,nextadvances past it, and the following call takes theErrNoRowspath to produce[L+1, MaxInt64]. The trailing chunk is empty but it exists, which is what coverage requires. upper + 1cannot overflow — theMaxInt64case takes thedonebranch instead.- Error paths leave the cursor where it was. Neither a failed boundary query nor a failed
NewChunkadvancesnext, so a retry re-cuts the same chunk rather than skipping a range. For a chunker whose whole job is gap-free coverage, that is the property that matters most on the failure path, and it holds by construction rather than by comment. - The
::bigintrationale is right, and pinning it with anEXPLAINassertion onsmallint/integerkeys is the correct way to keep a future edit from silently turning the boundary query into a sequential scan.
One finding worth acting on before the applier is written, plus three smaller notes.
Next holds the mutex across the database round trip, so Cut blocks on it
func (c *Chunker) Next(ctx context.Context, db dbconn.RowQuerier) (Chunk, bool, error) {
c.mu.Lock()
defer c.mu.Unlock()
...
upper, err := c.boundary(ctx, db, c.next, c.rows) // network round trip, under the lockCut takes the same mutex and reads two fields. Per the invariants change, the applier consults the cut frontier per captured change — "the applier therefore discards only for keys above the cut frontier". So every discard decision on the apply path can block behind a live query against the source table.
Failure scenario. Four copy workers, defaults, a large table with a sparse key space. A worker calls Next; the boundary query is ORDER BY pk OFFSET rows-1 LIMIT 1, which walks rows-1 index entries before returning one — with feedback having grown the size toward MaxRows that is up to 100,000 index tuples, and on a table under concurrent write load it can take hundreds of milliseconds. For that whole window every Cut call blocks, and so does every Feedback from the other three workers. At DefaultTargetChunkTime of 500ms with four workers, cuts are frequent enough that the applier spends a meaningful fraction of its time parked on a mutex it only ever wanted to read two int64s from.
Nothing about the design requires this. Serialising Next against Next is deliberate and correct — chunks must stay consecutive — but Cut and Feedback are dragged into that serialisation incidentally. Splitting the two concerns fixes it: one mutex held across the query purely to serialise cutting, and a second short-lived one (or atomics) guarding next/rows/done, taken only to read the cursor before the query and to publish the result after it. Cut then never waits on the database.
I am raising this now rather than as a follow-up because the applier does not exist yet — the invariants entry lists the registry and flush scheduling as "planned enforcement". Changing the locking discipline is free today and awkward once there is a hot path built on top of it. It is a liveness property, not a correctness one: nothing here returns a wrong answer.
Feedback does not check what it says it checks
A chunk this chunker did not cut is refused.
The code checks chunk.rows <= 0, which refuses a chunk built by the exported NewChunk (rows zero by construction) but accepts any chunk cut by any chunker. Two chunkers over two tables, and feeding one's chunk to the other resizes the wrong one silently — no error, just a size derived from work on a different table.
Whether that is worth a real identity check is a judgement call; the cheap version is to weaken the comment to what the guard actually establishes ("a chunk that no chunker sized"). As written the comment promises an ownership check that a reader will assume is there.
Setting InitialRows alone fails, while setting either bound alone succeeds
withDefaults fits the defaults around MinRows and MaxRows — that is what makes {MaxRows: 7} yield a floor of 7 rather than a contradiction. InitialRows gets no such treatment: it is only defaulted when zero, never fitted.
So ChunkerOptions{InitialRows: 50} is refused with initial rows 50 is outside [100, 100000], naming a floor and ceiling the caller never chose. A caller wanting small chunks — most obviously in a test — writes exactly that, and the error reads as though they set a bound they did not set.
The type comment says "a floor or ceiling given on its own pulls the other defaults inside it, so setting one bound never makes the defaults contradict it", which is accurate but leaves the reader to notice that InitialRows is not a bound. Either fit the default bounds around an explicit InitialRows the same way, or say in the comment that InitialRows is validated against the bounds and so may need them set alongside it.
Nit: a legal MaxRows makes fast chunks shrink instead of grow
ChunkerOptions{MaxRows: math.MaxInt64} passes validate today. Once feedback has grown rows into that range, a fast chunk takes ratio = 2.0, and int64(math.Round(float64(rows) * 2.0)) overflows. The conversion is not defined to saturate, and in practice it lands at MinInt64, so the clamp min(MaxRows, max(MinRows, scaled)) returns MinRows.
A chunk that completed far under target therefore collapses the next chunk to the floor — the exact opposite of the intended response. Clamping the scaled value in float64 before converting, or rejecting a MaxRows above some sane ceiling in validate, closes it. Nobody sets a row count to nine quintillion on purpose, which is why this is a nit and not a finding, but the current bound check is what makes it reachable at all.
3 participants
Add the keyset
Chunkertopkg/copier: it cuts the proven primary-key space into consecutive row-sized chunks that tile the whole int64 range, reports the cut frontier the applier's CO-4 discard rule reads, and sizes chunks toward the D12 time target from each measured chunk.Why
The copy-and-swap copier needs chunks that give equal work per chunk on sparse and dense key spaces, and that cover every key a row can carry so that "not yet read by the copier" always names a chunk still to come (CO-4). Cutting by key width fails the first; closing the first and last chunks at the table's current min/max fails the second.
Under concurrent copy workers the landed watermark (the contiguous prefix of landed chunks, what is checkpointed) lags behind the chunks the copier has already read, so an applier that discarded changes "above the watermark" would lose a change to a key inside an already-read chunk. The chunker therefore exposes the cut frontier (
Cut) as the discard boundary; the watermark stays the checkpoint/resume position. Likewise, feedback from concurrent workers must not compound: each report sizes from the chunk it measured, not from whatever the current size has become.The
bigintparameter cast is needed because PostgreSQL infers the bound's type from the key column, so a bound outside asmallint/integerkey's range could not otherwise be sent.What
Chunker(NewChunker(target, from, opts),Next,Cut,Feedback(chunk, elapsed),Rows): keyset boundary query (ORDER BY pk OFFSET rows-1 LIMIT 1) against the live table; first chunk open below (MinInt64), last open above (MaxInt64); resume from aWatermark; a watermark atMaxInt64means copy complete.Cutderives the frontier from existing state (next-1,MaxInt64once done,ok=falsebefore the first cut). Requires a non-zeroCopySwapTargetproof; fail-closedErrInvariantViolationnaming the invariant (ST-6,CO-4) otherwise.// INV: CO-4at the open-below start, the open-above close, and the no-rows boundary.Chunk.Rows()records the size a chunk was cut to;Feedbackscales that size toward the target by at most ×0.5..×2 per step within min/max bounds, and refuses a chunk no chunker cut.ChunkerOptions: zero values take defaults fitted to whatever bounds the caller set ({MaxRows: 500}yields initial 500;{MaxRows: 7}yields floor 7), so setting one bound alone always validates; explicitly set values are validated as given.startAfter,Cutbefore any query, concurrent-feedback settling (four reports on one 1000-row chunk settle at 2000, not 16000),nextRows; integration tests for coverage (disjoint, gap-free, sums to row count),Cutequal to each returned chunk's upper bound, even-division trailing chunk, empty table, resume, feedback resizing incl. a late report sizing from its own chunk,smallint/integerkeys with anEXPLAIN-based proof that the PK index still serves the query, exact-string freeze of the boundary SQL, and error propagation. Green on PG 14/16/18.SAFETY.mdcopier row, D12 and package map indocs/copy-and-swap-design.md, CO-4 cut-frontier vs landed-watermark paragraph and "enforced today" indocs/invariants.md.Before / after
References
docs/copy-and-swap-design.md§ D4, § D12;docs/invariants.md§ CO-4;docs/low-level-design.md§ Copy and apply ordering.CopySwapTargetproof); next: the chunk copy step and progress reporting.🤖 Drafted with Amp (Claude Opus 4.6); reviewed and edited by the author.