From 19e34f29a21bb502085fea67b00854665b950f74 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Sun, 2 Aug 2026 20:01:50 -0600 Subject: [PATCH] fix: record the ordered run as a range, not an upper bound (#342) pgcolumnar.sort_status counted concurrently written rows as ordered. record_online_sorted_extent recorded a single upper bound and relied on two things to make it honest: the rewrite's own stripe ids being consecutive from its lowest, and the lowest live group being that lowest id. The first argument only covers a foreign id drawn between two of the rewrite's own draws. An id drawn below the rewrite's first leaves its ids perfectly consecutive, so the run scan walked to the end unbroken and swept the foreign group underneath the mark. The second check could not catch that case at all. 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. A concurrent inserter's group was therefore invisible to it whenever that transaction committed, so lowestLive always equalled ours[0]. Demonstrated: a run where the insert committed 4.1 s before the check ran still set the mark to the maximum. The run is now recorded as a range, [ours[0], runEnd], and sort_status tests group_number BETWEEN sorted_from AND sorted_through. That 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 the range by construction. The lowestLive check is deleted along with the comment claiming it closed this. sorted_from is NULL only for a mark written before the column existed, where the old everything-below reading is kept. Found by CI, not by me: the suite failed on PG18 in one run and passed the same commit in another. Reproduced with ground truth by mapping each row's ctid back to its row number and joining to row_group, which showed group 76 holding ids 1500001-1520000 counted as sorted while no rewrite output group contained any id above 1500000. So the rewrite provably never read those rows. Impact is reporting, not data: a decayed table could look more ordered than it is, which is the direction record_online_sorted_extent's own comment says it must never fail in. No data loss and no wrong query results. test/recluster_extent.sh gains a deterministic case for this. The existing concurrent section only hits it 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 staying under stripe_row_limit so it never flushes and never takes the per-storage advisory lock that would block the rewrite. Removal proof: with sort_status reverted to a bare upper bound the new case fails at sorted 105000 / appended 0 against 100000 / 5000 expected, naming the 5000 rows the rewrite never read. That suite's premise was also unsound, independently of this defect, and is fixed too. It compared client wall clocks (INS_DONE < RECL_DONE), which 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. 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 rather than looking like a product defect. Full 15-19 matrix, all suites, on an otherwise idle machine. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_011miCFRSatixeNRw3w5yNq8 --- pgcolumnar--1.0-dev.sql | 28 ++++++++++++---- src/columnar.h | 3 +- src/columnar_metadata.c | 10 ++++-- src/columnar_vacuum.c | 72 ++++++++++++++++------------------------ test/recluster_extent.sh | 60 +++++++++++++++++++++++++++++++-- 5 files changed, 118 insertions(+), 55 deletions(-) diff --git a/pgcolumnar--1.0-dev.sql b/pgcolumnar--1.0-dev.sql index df02642..39da81c 100644 --- a/pgcolumnar--1.0-dev.sql +++ b/pgcolumnar--1.0-dev.sql @@ -189,7 +189,12 @@ CREATE TABLE pgcolumnar.storage ( -- unsorted vacuum leaves this NULL and correctly reports the table as -- unsorted, with no invalidation step. A value in an options row, which is -- keyed by relation, would outlive the layout it describes. - sorted_through bigint + sorted_through bigint, + -- Lower end of the ordered run (#342). The run is [sorted_from, + -- sorted_through]; a bare upper bound cannot exclude a concurrently written + -- group whose id was drawn below the rewrite's own first id, which is how a + -- foreign group came to be counted as ordered. + sorted_from bigint ); CREATE UNIQUE INDEX storage_pkey ON pgcolumnar.storage USING btree (storage_id); @@ -623,9 +628,11 @@ COMMENT ON FUNCTION pgcolumnar.stats(regclass) * reports the size of each part, so a DBA can decide when a re-sort is worth its * cost instead of guessing. * - * The ordered run is every row group numbered at or below the mark that the - * rewrite left in pgcolumnar.storage.sorted_through. Everything above it was - * written later. + * The ordered run is every row group numbered within the range the rewrite left + * in pgcolumnar.storage: from sorted_from to sorted_through inclusive. Groups + * above it were written later. Groups below it belong to a writer that started + * before the rewrite did and so were never ordered by it (#342); recording only + * an upper bound counted those as ordered. * * The row counts are stored rows. Rows deleted but not yet reclaimed are still * stored, so they are still counted. pgcolumnar.stats reports the deleted count @@ -664,16 +671,25 @@ CREATE FUNCTION pgcolumnar.sort_status( LANGUAGE sql STABLE AS $sort_status$ WITH s AS ( - SELECT st.storage_id, st.sorted_through + SELECT st.storage_id, st.sorted_through, st.sorted_from FROM pgcolumnar.storage st WHERE st.storage_id = pgcolumnar.get_storage_id(rel) ), g AS ( -- A NULL mark means the storage was never ordered, so no group is in the -- run. Comparing against NULL would make every count NULL instead. + -- + -- The run is a range, not everything below a boundary (#342). A group + -- numbered below sorted_from was not written by the rewrite that set the + -- mark: its stripe id was drawn before the rewrite's first, so it is a + -- concurrent writer's group and is not ordered. sorted_from is NULL only + -- for a mark written before this column existed, where the old + -- everything-below reading is kept. SELECT rg.row_count, (s.sorted_through IS NOT NULL - AND rg.group_number <= s.sorted_through) AS in_run + AND rg.group_number <= s.sorted_through + AND (s.sorted_from IS NULL + OR rg.group_number >= s.sorted_from)) AS in_run FROM pgcolumnar.row_group rg JOIN s ON rg.storage_id = s.storage_id ) diff --git a/src/columnar.h b/src/columnar.h index 09ac4ca..e5756d5 100644 --- a/src/columnar.h +++ b/src/columnar.h @@ -442,7 +442,8 @@ extern void ColumnarCheckFreeSpaceNoOverlap(uint64 storageId); * ------------------------------------------------------------------------- */ extern uint64 ColumnarNextStorageId(void); extern void ColumnarInsertNativeStorageRow(const NativeStorageMetadata *s); -extern void ColumnarSetSortedThrough(uint64 storageId, int64 groupNumber); +extern void ColumnarSetSortedExtent(uint64 storageId, int64 firstGroup, + int64 lastGroup); extern void ColumnarCheckNativeFormatVersion(uint64 storageId, const char *relName); extern void ColumnarInsertRowGroupRow(const NativeRowGroupMetadata *rg); extern void ColumnarInsertColumnChunkRow(const NativeColumnChunkMetadata *cc); diff --git a/src/columnar_metadata.c b/src/columnar_metadata.c index 9e77359..f23c73a 100644 --- a/src/columnar_metadata.c +++ b/src/columnar_metadata.c @@ -65,7 +65,8 @@ #define Anum_native_storage_vector_length 4 #define Anum_native_storage_row_group_limit 5 #define Anum_native_storage_sorted_through 6 -#define Natts_native_storage 6 +#define Anum_native_storage_sorted_from 7 +#define Natts_native_storage 7 #define Anum_row_group_storage_id 1 #define Anum_row_group_group_number 2 @@ -1565,6 +1566,7 @@ ColumnarInsertNativeStorageRow(const NativeStorageMetadata *s) * makes a rewrite reset the sort state with no invalidation step. */ nulls[Anum_native_storage_sorted_through - 1] = true; + nulls[Anum_native_storage_sorted_from - 1] = true; tuple = heap_form_tuple(tupdesc, values, nulls); CatalogTupleInsert(rel, tuple); @@ -1592,7 +1594,7 @@ ColumnarInsertNativeStorageRow(const NativeStorageMetadata *s) * groups and reports no decay. */ void -ColumnarSetSortedThrough(uint64 storageId, int64 groupNumber) +ColumnarSetSortedExtent(uint64 storageId, int64 firstGroup, int64 lastGroup) { Relation rel; TupleDesc tupdesc; @@ -1629,8 +1631,10 @@ ColumnarSetSortedThrough(uint64 storageId, int64 groupNumber) memset(values, 0, sizeof(values)); memset(nulls, false, sizeof(nulls)); memset(replace, false, sizeof(replace)); - values[Anum_native_storage_sorted_through - 1] = Int64GetDatum(groupNumber); + values[Anum_native_storage_sorted_through - 1] = Int64GetDatum(lastGroup); replace[Anum_native_storage_sorted_through - 1] = true; + values[Anum_native_storage_sorted_from - 1] = Int64GetDatum(firstGroup); + replace[Anum_native_storage_sorted_from - 1] = true; newTuple = heap_modify_tuple(tuple, tupdesc, values, nulls, replace); CatalogTupleUpdate(rel, &newTuple->t_self, newTuple); diff --git a/src/columnar_vacuum.c b/src/columnar_vacuum.c index 0c3f328..fb4e97b 100644 --- a/src/columnar_vacuum.c +++ b/src/columnar_vacuum.c @@ -296,25 +296,31 @@ columnar_rewrite_partial_groups(Relation rel, double minDeletedFraction, * "every group numbered at or below this one is ordered", and a single * boundary can only say that if no foreign group is numbered below it. * - * Walking the live groups and stopping at the first one the rewrite did not - * write is not enough, and the concurrent fixture in - * test/recluster_extent.sh is what showed it. A concurrent inserter - * reserves its stripe id when it buffers its first row and commits some - * time later. If it commits after this runs, its group is not in the list - * the walk sees, yet its id was drawn before some of the rewrite's own, so - * it lands underneath the mark and is claimed as ordered. The visible - * catalog cannot rule that out; the reservation sequence can. + * A concurrent inserter reserves its stripe id when it buffers its first + * row and commits some time later, so its group can be absent from any list + * this reads while still owning an id among the rewrite's own. * - * A foreign reservation always consumes an id, so it always leaves a gap in - * the rewrite's own ids, whenever it commits. So: the rewrite's ids must be - * consecutive from its lowest, and the lowest live group must be that - * lowest id. Then every group at or below the run's end is one the rewrite - * wrote, and the boundary means what sort_status reads it to mean. + * The run is therefore recorded as a range, [ours[0], runEnd], not as a + * single upper bound. Ids come from one serialized counter, so a foreign id + * strictly inside that range must have been drawn between two of the + * rewrite's own draws, which breaks the consecutive run below and truncates + * it before reaching that id. An id drawn below the rewrite's first falls + * outside the range by construction. Both cases hold whenever the foreign + * transaction commits, because neither depends on seeing it. * - * When it cannot prove that, it marks the part it can and leaves the rest - * out. That reports more decay than there is, which is the direction to - * fail in: it can prompt a re-sort that was not needed, where the opposite - * leaves a decayed table looking ordered and costs every query against it. + * An earlier version marked only an upper bound and tried to exclude the + * below case by requiring the lowest live group to equal ours[0]. That + * could not work (#342): the group list was read 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 the check whenever it committed. A + * foreign group written just before the rewrite's first reservation was + * then swept under the mark and counted as ordered. + * + * Truncating at the first gap still marks only the part it can prove. That + * reports more decay than there is, which is the direction to fail in: it + * can prompt a re-sort that was not needed, where the opposite leaves a + * decayed table looking ordered and costs every query against it. */ static void record_online_sorted_extent(Relation rel, uint64 storageId, @@ -324,8 +330,6 @@ record_online_sorted_extent(Relation rel, uint64 storageId, uint64 *all = ColumnarWriteStateStripeIds(writeState, &nAll); int nOurs = nAll - stripeMark; uint64 *ours; - List *groups; - uint64 lowestLive; uint64 runEnd; int i; @@ -337,27 +341,6 @@ record_online_sorted_extent(Relation rel, uint64 storageId, memcpy(ours, all + stripeMark, sizeof(uint64) * nOurs); qsort(ours, nOurs, sizeof(uint64), uint64_cmp); - groups = ColumnarReadRowGroupList(storageId, - ColumnarCatalogSnapshot(GetActiveSnapshot())); - if (groups == NIL) - { - pfree(ours); - return; - } - /* the list is ordered by group number */ - lowestLive = ((NativeRowGroupMetadata *) linitial(groups))->groupNumber; - list_free_deep(groups); - - /* - * A live group below our first reservation is one we did not write and did - * not retire, so no boundary above it can be honest. - */ - if (lowestLive != ours[0]) - { - pfree(ours); - return; - } - /* the consecutive run of our own ids, starting at the lowest */ runEnd = ours[0]; for (i = 1; i < nOurs; i++) @@ -366,9 +349,9 @@ record_online_sorted_extent(Relation rel, uint64 storageId, break; /* a foreign reservation took this id */ runEnd = ours[i]; } - pfree(ours); - ColumnarSetSortedThrough(storageId, (int64) runEnd); + ColumnarSetSortedExtent(storageId, (int64) ours[0], (int64) runEnd); + pfree(ours); } /* @@ -858,6 +841,7 @@ record_sorted_extent(Relation rel) uint64 storageId = ColumnarStorageId(rel); List *groups; uint64 lastGroup = 0; + uint64 firstGroup = 0; bool haveGroup = false; ListCell *lc; @@ -869,12 +853,14 @@ record_sorted_extent(Relation rel) if (!haveGroup || rg->groupNumber > lastGroup) lastGroup = rg->groupNumber; + if (!haveGroup || rg->groupNumber < firstGroup) + firstGroup = rg->groupNumber; haveGroup = true; } list_free_deep(groups); if (haveGroup) - ColumnarSetSortedThrough(storageId, (int64) lastGroup); + ColumnarSetSortedExtent(storageId, (int64) firstGroup, (int64) lastGroup); } /* diff --git a/test/recluster_extent.sh b/test/recluster_extent.sh index 587b3ef..ee41762 100755 --- a/test/recluster_extent.sh +++ b/test/recluster_extent.sh @@ -63,9 +63,19 @@ RECL_DONE=$(date +%s%N) # nothing concurrent happened and the rest of this suite proves nothing. check "the insert committed while the rewrite was still running" \ "$( [ "$INS_DONE" -lt "$RECL_DONE" ] && echo yes || echo no )" "yes" -check "the rewrite reported groups reclustered" \ - "$( [ "$(cat /tmp/pgc_recluster_out.$$)" -gt 0 ] 2>/dev/null && echo yes || echo no )" "yes" +# The real premise, and the one the wall-clock comparison above cannot establish +# (#342). recluster returns the number of groups it retired, which is exactly the +# set it read. The base load is BASE rows at stripe_row_limit, so a rewrite that +# excluded the concurrent insert retires exactly that many groups; if the insert +# landed in its work set it retires more, and it then legitimately ordered those +# rows. Without this, that case fails the property check below and looks like a +# product defect instead of an unmet precondition. +RETIRED="$(cat /tmp/pgc_recluster_out.$$)" rm -f /tmp/pgc_recluster_out.$$ +check "the rewrite reported groups reclustered" \ + "$( [ "$RETIRED" -gt 0 ] 2>/dev/null && echo yes || echo no )" "yes" +check "premise: the concurrent insert was not in the rewrite's work set" \ + "$RETIRED" "$((BASE / 20000))" check "every row is present afterwards" "$(raw 'SELECT count(*) FROM t;')" "$((BASE + 50000))" @@ -88,6 +98,52 @@ check "the counts still cover every stored row" \ check "the concurrent rows are reported as appended" \ "$( [ "$APPENDED" -gt 0 ] && echo yes || echo no )" "yes" +# -------------------------------- the id-drawn-below case, deterministically + +# The defect in #342: a concurrent writer draws its stripe id when it buffers its +# first row, so a writer that starts before the rewrite owns an id BELOW every id +# the rewrite draws. The rewrite's own ids stay consecutive, so a mark that is a +# bare upper bound sweeps that foreign group underneath it and counts unordered +# rows as ordered. +# +# The concurrent section above only hits this when the scheduler cooperates, which +# is why it failed on CI and not locally. This reproduces it with no timing at all: +# the writer holds its transaction open, so the ordering is forced rather than +# raced. +# +# The insert is deliberately smaller than stripe_row_limit. It draws its stripe id +# while buffering, but does not flush until commit -- so it does not hold the +# per-storage advisory lock that a flush takes to transaction end, and the rewrite +# is free to run to completion in the meantime. +raw "DROP TABLE IF EXISTS d;" >/dev/null +raw "CREATE TABLE d (id int, k int, v text) USING pgcolumnar;" >/dev/null +raw "SELECT pgcolumnar.set_options('d', stripe_row_limit => 20000, chunk_group_row_limit => 2048);" >/dev/null +raw "INSERT INTO d SELECT g, ((g::bigint * 7919) % 100000)::int, 'v' || g FROM generate_series(1, 100000) g;" >/dev/null + +# Session B: draw a stripe id, then hold the transaction open across the rewrite. +raw "BEGIN; + INSERT INTO d SELECT g, 1, 'late' || g FROM generate_series(900001, 905000) g; + SELECT pg_sleep(12); + COMMIT;" >/dev/null & +HOLD_PID=$! +sleep 3 # let B buffer its first row, which is when its id is drawn + +raw "SELECT pgcolumnar.recluster('d', 'id', 'k');" >/dev/null +wait $HOLD_PID # B commits now, writing its group at an id below the rewrite's + +D_SORTED="$(raw "SELECT sorted_rows FROM pgcolumnar.sort_status('d');")" +D_APPEND="$(raw "SELECT appended_rows FROM pgcolumnar.sort_status('d');")" +echo " id-drawn-below: sorted $D_SORTED, appended $D_APPEND (100000 ordered + 5000 concurrent)" + +# The rewrite ordered the 100000 rows that existed. The 5000 written by the held +# transaction were never read by it, so they must not be counted as ordered no +# matter where their group number falls. +check "a group numbered below the run is not counted as ordered" "$D_SORTED" "100000" +check "the rows written below the run are reported as appended" "$D_APPEND" "5000" +check "the id-drawn-below counts still cover every stored row" \ + "$(raw "SELECT (sorted_rows + appended_rows = (SELECT sum(rowcount) FROM pgcolumnar.stats('d')))::text + FROM pgcolumnar.sort_status('d');")" "true" + # ---------------------------------------------------- the uncontended case # With no concurrent writer the rewrite orders everything, so the run covers the