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