Skip to content

Prove the row-capped drain actually drains, against real PostgreSQL - #2397

Merged
erikdarlingdata merged 1 commit into
devfrom
test/2388-live-multi-batch-drain
Aug 21, 2026
Merged

erikdarlingdata merged 1 commit into
devfrom
test/2388-live-multi-batch-drain

Conversation

@erikdarlingdata

Copy link
Copy Markdown
Owner

#2388 landed with the loop covered and the statement uncovered.

DrainBatches_* inject a fake executor and prove the batch arithmetic. RowCappedDeleteSql was asserted as a string. Nothing ran the two together against PostgreSQL, so the statement's shape — cutoff bound, ORDER BY, and LIMIT inside a ctid IN (...) subquery — had never met a real planner.

Plants 110 expired rows and 7 fresh, drains with cap 25, asserts both the total (110) and the batch count (5).

The batch count is the load-bearing assertion. One statement deleting all 110 would mean the LIMIT never bound — that is the unbounded delete #2388 replaced, the one that hits 300s on a real backlog and rolls back to zero rows. And a drain that deletes something is precisely what looked like success on the dogfood store for a week while query_plan_dim grew: the 08-20 purge logged 0 failed having peeled one thin slice. Deleting a slice and draining a backlog are different outcomes, and neither the log nor the existing tests could tell them apart.

Runs under DARLING_TEST_PG in the Darling PostgreSQL job. Test-only.

…2388)

#2388 shipped with the loop covered and the statement uncovered. The drain
tests inject a fake executor and prove the batch arithmetic; the SQL those
batches run was only ever asserted as a string. Both halves can be right
in isolation and still delete nothing, because RowCappedDeleteSql puts the
cutoff, the ORDER BY and the LIMIT inside a ctid IN (...) subquery and that
shape has to survive a real planner.

Plants 110 expired rows and 7 fresh ones, drains with a cap of 25, and
asserts the total AND the batch count. The count is the load-bearing half:
one statement deleting all 110 would mean the LIMIT never bound, which is
the unbounded delete #2388 replaced -- the one that times out at 300s on a
real backlog and rolls back to zero rows.

Asserting the batch count is also what distinguishes this from the failure
#2386 measured, where a purge deleting SOMETHING looked like success for a
week while the table grew. Deleting a slice and draining a backlog are
different outcomes and the log could not tell them apart.

Refs #2388, #2386

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@claude

claude Bot commented Aug 21, 2026

Copy link
Copy Markdown

Reviewed. This is a well-scoped, test-only addition to PayloadDimensionLiveTests.cs — no production code changes, so no Lite/Darling parity concerns (Lite has no PostgreSQL-specific drain path to mirror) and no T-SQL style conventions apply.

Checked the substance:

  • Batch math is correct: cap=25 against 110 expired rows yields batches of 25/25/25/25/10, and DrainBatchesAsync terminates on the first batch returning fewer than `batchSize" rows — so 5 batches and 110 total deleted is the right expectation.
  • Isolation: the fixed 2001-era timestamps only collide with rows from this same test, and the class runs in the serialized live-postgres xUnit collection, so no races with other live tests touching query_plan_dim.
  • Cleanup/helper usage (ScalarAsync, DeleteDimRowAsync, LiveStoreCleanup.RunAsync) matches existing conventions elsewhere in the file, including safe double-delete of already-purged expired rows in the finally block.

No correctness, security, or performance issues found.

@erikdarlingdata
erikdarlingdata merged commit 219bec5 into dev Aug 21, 2026
6 checks passed
@erikdarlingdata
erikdarlingdata deleted the test/2388-live-multi-batch-drain branch August 21, 2026 10:09
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.

1 participant