fix: record the ordered run as a range, not an upper bound (#342)

[ChronicallyJD]

Fixes #342. Please review rather than expect me to merge this -- it changes the catalog, and I have self-merged too much today already.

The defect

pgcolumnar.sort_status counted concurrently written rows as ordered. A table with unordered rows appended during an online recluster reported as more ordered than it was -- the direction record_online_sorted_extent's own comment says it must never fail in, because it "leaves a decayed table looking ordered and costs every query against it."

Impact is reporting and re-sort scheduling. No data loss, no wrong query results.

Found by CI, not by me

The suite failed on PG18 in one run and passed the same commit in another. I had also seen it fail once on PG19 earlier and written that off as environmental, which was wrong.

Reproduced with ground truth rather than inference: each row's ctid mapped back to its row number and joined to pgcolumnar.row_group, so group membership comes from stored data.

76      :20000: id 1500001-1520000 : SORTED   <-- foreign, under the mark
77..151 : 75 groups, max id = 1500000         <-- rewrite output, zero concurrent rows
152,153 : the inserter's other two stripes    : appended
sorted_through = 151

No rewrite output group contains any id > 1500000, and recluster returned 75 -- its own retire count -- so the rewrite provably never read those 20,000 rows.

Cause: two holes, both required

1. The guard reasoned that "a foreign reservation always leaves a gap in the rewrite's own ids." That holds only for an id drawn between two of the rewrite's draws. An id drawn below the rewrite's first leaves its ids perfectly consecutive, so the run scan walks to the end unbroken and sweeps that group underneath the mark.

2. The lowestLive != ours[0] backstop could not catch it. It read the group list under the rewrite's own snapshot, taken before it read a row, and ColumnarCatalogSnapshot only advances curcid rather than refreshing xmin/xmax -- so a concurrent inserter's group was invisible to it whenever that transaction committed. Demonstrated by a run where the insert committed 4.1 s before the check ran and the mark was still set to the maximum.

Fix: a range, not a boundary

sorted_from is added alongside sorted_through; the run is recorded as [ours[0], runEnd] and sort_status tests BETWEEN.

This is sound from the single serialized stripe counter alone, with no dependence on visibility: a foreign id strictly inside the range must have been drawn between two of the rewrite's draws, so it breaks the consecutive run and is already truncated; an id below ours[0] falls outside by construction. That is exactly what a bare upper bound cannot express.

The lowestLive check is deleted, along with the comment claiming it closed this hole.

sorted_from is NULL only for a mark written before the column existed, where the old everything-below reading is kept.

Tests

A deterministic case is added. The existing concurrent section only hits this when the scheduler cooperates, which is why it failed on CI and not locally. The new one forces the ordering with transaction control instead of racing it: a writer holds its transaction open, drawing its stripe id while buffering, but stays under stripe_row_limit so it never flushes and never takes the per-storage advisory lock that would otherwise block the rewrite.

Removal proof -- revert sort_status to a bare upper bound and the new case fails naming the exact rows:

	sorted	appended
fix reverted	105000	0
fix in place	100000	5000

The suite's premise was also unsound, separately

INS_DONE < RECL_DONE compared client wall clocks. That establishes only that the insert's client returned first, and says nothing about the rewrite's work set -- with the insert forced entirely before the rewrite, the premise passed while the property failed honestly at 1550000 / 0. It now asserts on recluster's own retire count, which is exactly the set it read, so an insert that does land in the work set reports an unmet precondition instead of looking like a product defect.

Both defects were real and independent. Fixing only the test would have buried the bug.

Gate

Full 15-19 matrix, all suites, ALL VERSIONS PASSED, on an otherwise idle machine. recluster_extent, sort_status, cancel_decode and native_fetch_position all 5/5.

Worth flagging: an earlier matrix run showed cancel_decode and native_fetch_position failing on PG19. That was my own fault -- I was building and testing #343 across four majors concurrently, and cancel_decode's premise requires a scan to outlast 300 ms. Both pass 5/5 on a quiet box. I am noting it rather than omitting it because "I re-ran it and it passed" is exactly the reasoning that let this bug sit.

Catalog note

This adds a column to pgcolumnar.storage. There are no extension update scripts (single 1.0-dev file), so there is nothing to migrate, but that is a call you may want to weigh in on.

🤖 Generated with Claude Code

https://claude.ai/code/session_011miCFRSatixeNRw3w5yNq8

[jdatcmd]

Related finding while auditing this area, filed as #345: this PR does not fix it, and I want that on the record before you review.

sort_status reports ~90% decay on a table that was just fully reclustered, whenever the table has a projection. Reproduced with a control (same fixture, only difference is add_projection):

arm	sorted_rows	appended_rows
no projection	200000	0
with a projection	20000	180000

Cause: the projection fan-out reserves from the same base stripe counter but records its group under its own storage id, so the rewrite's own ids come back as 17, 19, 21, ... and the consecutive-run walk at src/columnar_vacuum.c breaks at the first step. The comment there says a gap means "a foreign reservation took this id", which is false when the rewrite is competing with its own projection writes.

Why it matters for reviewing #344: this PR changes the mark to a range [ours[0], runEnd], which closes the over-claim in #342, but runEnd still comes from that same consecutive-run walk. So the under-claim survives untouched. I verified #345 against a tree that has this fix in it.

The two are opposite errors in the same function -- #342 claimed groups it should not, #345 refuses groups it should -- and I would rather they be fixed as two changes with two removal proofs than folded together. If you would prefer them in one, say so and I will fold #345 in here.

Direction of error is safe in both senses: #345 over-reports decay, so at worst it prompts an unnecessary re-sort.

[ChronicallyJD]

Reviewed — mergeable; #345 noted as separate and pre-existing.

Correctly fixes #342. Verified:

• The range [ours[0], runEnd] is sound: a foreign id below the run is excluded (< sorted_from), one inside is impossible because the consecutive-run walk breaks at the first gap, and one above is excluded (> sorted_through).
• Empty-run guarded (if (nOurs <= 0) return;) before ours[0].
• No C consumer of sorted_through/sorted_from outside the writers — the sole reader is the SQL sort_status, so the range fix is complete.
• The new "id-drawn-below" test forces the race deterministically (held-open txn draws its id, doesn't flush) rather than relying on the scheduler, and asserts its premise so it can't pass vacuously.

Re #345 — this PR neither fixes nor worsens it. On a projected table the base and projection draws alternate in the shared base counter but ColumnarWriteStateStripeIds returns only the base reservations, so ours is non-consecutive and the walk breaks at the first step → this PR records [ours[0], ours[0]], the same single-group mark sorted_through alone would record on main. Same ~90% symptom, no regression. #345 wants its own fix — making ours include the projection write state's reservations so the walk breaks only on a genuinely foreign gap — best stacked on this, since it's the same function.

One migration note: sort_status now references st.sorted_from, so a dev install that reloads the function without recreating pgcolumnar.storage would error on the missing column. Fine if dev installs always recreate; worth confirming there's no in-place path.