recluster_extent fails intermittently: extent mark claims concurrently inserted rows (sorted 1520000 > base 1500000)

[ChronicallyJD]

Summary

test/recluster_extent.sh fails intermittently. The same commit passed and failed the same PG18 CI job in two different runs. The failing assertion is the one the suite exists to enforce, so this is either a real defect in the online-recluster extent mark or a race in the test itself. Which one is not yet established. This issue records the evidence; the conclusion will be added when it is proven, not before.

Evidence

CI, PG18, run 30772737889 (job 91562747530):

base 1500000 rows, 50000 inserted during the rewrite; sorted 1520000, appended 30000
FAIL  the run does not claim rows written during the rewrite: got [no] want [yes]

The identical commit passed the identical job in run 30772271903. Both runs are of the tree merged as #339 / under review as #340. Non-determinism is therefore established as fact, independent of the cause.

Locally on PG18 the suite passes 8/8, with sorted ranging 400000-480000 across runs -- nowhere near the 1500000 threshold. The same suite also failed once on PG19 in an earlier local full-matrix run and passed on the rerun. So it has now been seen failing on two different majors and passing on both.

What the numbers rule out

The concurrent INSERT is a single transaction of 50000 rows, so any single snapshot sees all of them or none. A partial split therefore needs explaining.

Forcing the insert to commit entirely before the rewrite starts was tested directly. It produces:

base 1500000 rows, 50000 inserted during the rewrite; sorted 1550000, appended 0

That is all-or-nothing, and it is not what CI showed. So "the insert simply landed before the rewrite snapshotted" does not by itself account for the observed 1520000 / 30000.

The fixture uses stripe_row_limit => 20000, so a 50000-row insert produces groups of roughly 20000 / 20000 / 10000. A 20000 / 30000 split is consistent with exactly one of those groups falling at or below the mark. That is the specific failure mode record_online_sorted_extent was written to prevent, which is why this is not being written off as flakiness.

What is already proven about the test

The suite's premise check is:

check "the insert committed while the rewrite was still running" \
	"$( [ "$INS_DONE" -lt "$RECL_DONE" ] && echo yes || echo no )" "yes"

This only establishes that the insert finished before the rewrite finished. It does not establish that the insert landed after the rewrite snapshotted its work set. Demonstrated by construction: with the insert forced to commit entirely before the rewrite starts, this premise check still PASSES while the property check fails. So the premise is too weak in at least one direction, regardless of what else is wrong.

That is a defect in the test independent of the outcome of the investigation, and it should be fixed either way.

The code under suspicion

src/columnar_vacuum.c :: record_online_sorted_extent. Its contract is that sort_status reads the mark as "every group numbered at or below this one is ordered". Its guard is that the rewrite's own reserved stripe ids must be consecutive from its lowest and the lowest live group must equal that lowest id; otherwise it marks less, or nothing. The stated justification is that a foreign reservation always consumes an id and therefore always leaves a gap in the rewrite's own ids.

The open question is whether that argument is airtight, specifically:

• whether ids are always drawn from one monotonic sequence, and whether a reserved-then-aborted id can leave a gap that is not foreign;
• whether sort_status counts rows in all live groups at or below the mark regardless of who wrote them, or only the rewrite's own;
• whether a group can be invisible to the lowestLive check yet later become visible at an id below the mark.

Not caused by column projection (#338)

The suite and the extent marking both predate #338, and the projection change does not touch the recluster path (columnar_vacuum.c passes a NULL projection, so it keeps the whole-group read). The failure surfaced on a #338 review PR only because that PR triggered a fresh CI run of an already-merged tree. This should still be confirmed rather than assumed.

Why this matters even if the product is correct

A gate that reds intermittently teaches its readers to ignore reds. This one has now produced a false-looking red on two majors. Whatever the cause, the test must end up either deterministic or honestly reporting an unmet premise instead of a failed property.

[ChronicallyJD]

Confirmed: real defect, not a test artifact

Reproduced the exact CI numbers (sorted 1520000 / appended 30000, sorted_through=151) repeatedly, on PG18, with the byte-identical fixture workload. Three independent verifications concur at high confidence, including one whose assignment was specifically to refute the defect conclusion and one arithmetic referee whose only job was to reject explanations that cannot produce the 20000/30000 split.

Ground truth

Group layout of a reproducing run, with each row's ctid mapped back to its row number and joined to pgcolumnar.row_group, so group membership is read from stored data rather than inferred from stripe ids. Base rows have id <= 1500000; concurrently inserted rows have id > 1500000.

76 :20000: id 1500001-1520000 : SORTED    <-- FOREIGN, at or below the mark
77..151 : 75 groups, 1,500,000 rows, max id over all of them = 1500000
152:20000: id 1520001-1540000 : appended
153:10000: id 1540001-1550000 : appended
sorted_through = 151

Group 76 holds 20,000 concurrently-inserted rows and is counted as sorted. No rewrite output group contains any id > 1500000, so the rewrite provably did not order those rows. recluster returned 75, its retire count taken from its own work-set snapshot, which independently confirms the insert was never in its work set. Totals sum to 1,550,000, so nothing is duplicated or lost.

This refutes the "the rewrite legitimately ordered them" explanation by data, not by argument.

Root cause: two holes, both required

1. The gap argument does not cover ids drawn below the rewrite's first.

record_online_sorted_extent reasons that "a foreign reservation always consumes an id and therefore leaves a gap in the rewrite's own ids" (src/columnar_vacuum.c:308-310). That holds only for a foreign id drawn between two of the rewrite's draws. A foreign id drawn before the rewrite's first leaves the rewrite's own ids perfectly consecutive, so the run scan at :361-368 walks 77 to 151 unbroken and ColumnarSetSortedThrough(151) sweeps group 76 underneath the mark.

Stripe ids come from one monotonic metapage counter under BUFFER_LOCK_EXCLUSIVE, drawn when a writer buffers a stripe's first row (src/columnar_storage.c:181-184, src/columnar_write_state.c:661-666). So id order is reservation order, and 76 < 77 proves the inserter drew first.

2. The lowestLive != ours[0] backstop is blind by construction.

The group list at :340-341 is read under ColumnarCatalogSnapshot(GetActiveSnapshot()), and the active snapshot there is the rewrite's own, taken at :450 before it read a single row. ColumnarCatalogSnapshot (src/columnar_metadata.c:192-208) only advances curcid; it does not refresh xmin/xmax. A concurrent inserter's group is therefore invisible to that check whenever it commits, so lowestLive always equals ours[0].

Demonstrated independently: in one run the insert committed 4.1 s before record_online_sorted_extent ran, and the mark was still set to the maximum. Since :355 returns early when lowestLive != ours[0], the mark being set at all proves the walk computed lowestLive = ours[0] while the foreign groups were live and committed.

Why the split is 20000/30000 and nothing else

Nothing is split by a snapshot. The inserter draws its three stripe ids at three separate moments and gets parked between the first and the second, on the transaction-scoped advisory ExclusiveLock on the storage id taken in ColumnarInsertNativeStorageRow (src/columnar_metadata.c:1526-1530), which every stripe flush calls. Traced in pg_locks: rewrite granted at +1217 ms, inserter blocked at +1531 ms, granted at +4893 ms.

Because that lock is held to commit, exactly two over-claiming shapes exist: the inserter finishes all three stripes before the rewrite's first reservation (1550000 / 0), or it draws exactly one id first and is then parked (1520000 / 30000). A 1540000 / 10000 split is unreachable. So the CI numbers are the signature of precisely one concurrent stripe slipping in ahead.

Impact

sort_status over-reports sorted rows and under-reports decay. A table with concurrently-inserted, unordered rows can look more ordered than it is, which is the direction the function's own comment says it must never fail in: it "leaves a decayed table looking ordered and costs every query against it." No data loss, no wrong query results; this is reporting and re-sort scheduling.

The fix

Make the mark a range rather than a single upper bound, and stop depending on any visibility test. Add sorted_from alongside sorted_through, have record_online_sorted_extent store [ours[0], runEnd], and change sort_status to test group_number BETWEEN sorted_from AND sorted_through.

This is airtight from the single serialized counter alone, with no dependence on snapshots: a foreign id strictly inside the range must have been drawn between two of the rewrite's draws, so it already breaks consecutiveness and is already truncated at :362-368; an id below ours[0] falls outside the range by construction, which is exactly what a bare upper bound cannot express. The lowestLive check can then be deleted along with its false claim.

record_sorted_extent (the full-rewrite path, :855-878) needs sorted_from set too.

The test is also unsound, separately

test/recluster_extent.sh:64-65 checks INS_DONE < RECL_DONE, which establishes only that the insert's client returned before the rewrite's did. It never establishes that the insert was excluded from the rewrite's work set. Forcing the insert to commit entirely before the rewrite makes the premise pass while the property fails honestly, at 1550000 / 0.

That is a genuine false-red mode and will be fixed too, but it is not what produced this red: it yields 1550000 / 0, and 1520000 / 30000 is arithmetically unreachable from it.