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

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
 	)

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);

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);

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);
 }
 
 /*

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