fix: the format stamp is a range, and the first write raises it (#1137)

CHANGELOG.md

@@ -39,6 +39,55 @@ true until the next version shipped.
   "workers share the covering projection scan", stayed green when no worker
   shared anything, because it only asks that both workers produced rows. It
   is recorded on #1236 rather than changed here.
+- The native format stamp, `pgcolumnar.storage.format_version`, is now 2, and a
+  build reads a RANGE of stamps instead of exactly one (#1137). The owner chose
+  option 2 on the issue for this release.
+
+  #1130 stopped writing a validity bitmap for a chunk with no nulls, and an
+  alpha4 binary misreads such a chunk without refusing it. Measured on PG 17
+  with the real alpha4 `.so` against tables written by three builds, one
+  sequential scan and 40 single-row index fetches each, custom scan off:
+
+      written by                       alpha4 seq scan   alpha4 fetches
+      main before this change (1)      reads             0 right, 9 NULL for a
+                                                         row holding a value,
+                                                         32 errors
+      this change (2)                  refused, 0A000    41 refused
+      main, then appended to by this   refused, 0A000    41 refused
+
+  The check was an equality, so raising the stamp would also have made this
+  build refuse every table alpha4 wrote. It is now
+  `COLUMNAR_NATIVE_FORMAT_MIN_READABLE (1) <= v <= COLUMNAR_NATIVE_FORMAT_VERSION
+  (2)`.
+
+  The third row is a hole the issue's option 2 did not cover. The issue had
+  existing tables keep stamp 1, but this build writes bitmap-less chunks when it
+  APPENDS to one of them. A table stamped 1 would then hold chunks alpha4
+  misreads, and alpha4 would accept the stamp. So the first write by this build
+  raises an older stamp to 2, in `PgColumnarInsertNativeStorageRow`, which every
+  flush already calls. A table that is only read keeps its stamp, so reading
+  alone never locks an older binary out. The bulk loader's unlocked fast path
+  skips the raise and falls through to the locked path, because two unserialized
+  loaders would race on the UPDATE.
+
+  The fetch path's one exemption from the stamp check, a liveness-only probe
+  that decodes nothing, is now held by an `Assert` at the decode point instead
+  of a comment. Disabling that path's check makes a correctly stamped table trip
+  it (`TRAP: failed Assert("columnarFetchFmtOk && ...")`) on the assert-enabled
+  builds every suite runs. Without the `Assert`, that mutation passed every arm.
+
+  `native_format.sh` gains eight arms, all on four decode entry points: a table
+  stamped 1 is read, an append raises the stamp, a read does not, and stamps 3
+  and 0 are refused by SQLSTATE. Each of four mutations is caught by the arm
+  aimed at it. Those mutations are the equality restored, the raise removed,
+  the build still writing 1, and the lower bound dropped. The fetch arm turns
+  the custom scan off. With it on, the index query never reaches
+  `pgcolumnar_fetch_row`, and that path's check could be deleted with every
+  arm still green, which was measured.
+
+  The specification's section 13, `docs/limitations.md` and
+  `docs/installation.md` now describe the range. Going back to an older binary
+  after a newer one has written to a table is refused rather than misread.
 
 - `test/pytest/README.md` now names all four registries a new pytest test has to
   be entered in, and which of them a local run can catch. Adding one test reddens

design/NATIVE_FORMAT_AND_INTERFACE_SPEC.md

@@ -281,10 +281,22 @@ configuration reference.
 ## 13. Format identity and versioning
 
 - Metapage magic: `PGCN`. Major version starts at 1.
-- The major version is the compatibility gate. A reader reads any minor within a
-  major it implements; a new minor is additive (a new optional catalog column, a
-  new encoding scheme a reader may not produce but must decode if the descriptor
-  requires it, subject to the reader knowing the scheme).
+- The metapage major version gates the physical block layout.
+- `pgcolumnar.storage.format_version` gates the data encoding, and it is a RANGE
+  (#1137). A build writes `COLUMNAR_NATIVE_FORMAT_VERSION` and reads every version
+  from `COLUMNAR_NATIVE_FORMAT_MIN_READABLE` up to it. It refuses anything else at
+  every decode entry point, before it reads a chunk byte. The stamp moves when a
+  build writes something that an older reader would misread rather than refuse.
+
+      1   through 1.0-alpha4
+      2   1.0-alpha5: a column chunk with no nulls stores no validity bitmap (#1130)
+
+  A table carries the stamp of the newest build that wrote to it. The first write
+  by a newer build raises an older stamp, because that write adds chunks the older
+  reader cannot read. Reading alone never changes the stamp.
+- An additive change (a new optional catalog column, or an encoding that a reader
+  must decode only if a descriptor asks for it and the reader knows the scheme)
+  does not move the stamp.
 - The native format has no requirement to read the 1.0-dev format (magic and
   version differ). A one-time migration path is `COPY` or Arrow/Parquet import
   from a 1.0-dev table into a native table.

docs/installation.md

@@ -170,8 +170,10 @@ names, settings and table syntax are all the same.
 
 The source records the on-disk format version. The specification also records it,
 in [../design/NATIVE_FORMAT_AND_INTERFACE_SPEC.md](https://github.com/commandprompt/pgcolumnar/blob/main/design/NATIVE_FORMAT_AND_INTERFACE_SPEC.md).
-A build that keeps the same format version reads the tables that earlier builds
-of that version wrote. A conversion is not necessary.
+A build reads the tables that earlier builds wrote, from format version 1 up to the
+version that it writes. A conversion is not necessary. The reverse is not true. A
+table that a newer build has written to is refused by an older build. See
+[limitations.md](limitations.md) for the details.
 
 ## Remove
 

docs/limitations.md

@@ -62,6 +62,22 @@ The metapage version guards the physical block layout. The native format version
 guards the data encoding. Thus the build also catches a future version that
 changes the encoding but keeps the metapage layout.
 
+The native format version is 2 from 1.0-alpha5. Version 2 does not store a
+validity bitmap for a column chunk that holds no null
+([issue #1130](https://github.com/commandprompt/pgcolumnar/issues/1130)). A build
+reads every version from 1 to the version that it writes. So 1.0-alpha5 reads the
+tables that 1.0-alpha4 wrote. When 1.0-alpha5 first writes to such a table, it
+raises the table's version to 2. The table then holds chunks that an older build
+cannot read
+([issue #1137](https://github.com/commandprompt/pgcolumnar/issues/1137)). A query
+that only reads the table does not change its version.
+
+Do not go back to an older build after a newer build has written to a table. The
+older build refuses the table, and it reports an error rather than a wrong answer.
+1.0-alpha4 refuses any version except 1. Before this change, 1.0-alpha4 could
+return NULL for a row that holds a value when it read a table that 1.0-alpha5 had
+written.
+
 The build refuses a version that it does not know. It reports `unsupported
 columnar format version` for the metapage, or `unsupported columnar native format
 version` for the data format. The read then fails. It does not read bytes that a
@@ -103,7 +119,9 @@ across a PostgreSQL major upgrade. It runs as an opt-in gate
 majors, in the copy transfer mode and in the link transfer mode.
 
 There is no in-place upgrade across an incompatible format version. The format has
-not changed during the pre-release, so no migration has been needed. The project has not
+changed once during the pre-release, to version 2 in 1.0-alpha5. That change is
+compatible in one direction. A newer build reads the older tables, so no
+migration has been needed. The project has not
 committed to an upgrade path or to a compatibility guarantee. Until it does, a
 change to an incompatible version needs a reload. This is the same position as
 the release-status note above. Keep the source that you loaded a columnar table

src/columnar.h

@@ -43,7 +43,19 @@
  * and per-chunk bloom filters for skipping.
  */
 #define COLUMNAR_NATIVE_MAGIC "PGCN"		/* native format magic tag */
-#define COLUMNAR_NATIVE_VERSION_MAJOR 1
+/*
+ * The native format stamp, pgcolumnar.storage.format_version (#1137). A build
+ * WRITES COLUMNAR_NATIVE_FORMAT_VERSION and READS every version from
+ * COLUMNAR_NATIVE_FORMAT_MIN_READABLE up to it; anything else is refused at
+ * every decode entry point before a byte is read.
+ *
+ *   1   through 1.0-alpha4
+ *   2   1.0-alpha5: a chunk with no nulls stores no validity bitmap (#1130).
+ *       A version-1 reader would take the chunk's first bytes for a bitmap and
+ *       return NULL for some rows that hold a value; stamped 2, it refuses.
+ */
+#define COLUMNAR_NATIVE_FORMAT_VERSION 2
+#define COLUMNAR_NATIVE_FORMAT_MIN_READABLE 1
 #define COLUMNAR_NATIVE_VECTOR_LENGTH 1024	/* values per vector (fixed) */
 
 /*
@@ -98,13 +110,11 @@
  *                        one bit; a zero bit answers "null" before any version
  *                        check is reached.
  *
- * So downgrading a binary below the one that wrote the table is NOT supported,
- * and this is the shape of the failure rather than a guarantee about it.
- * pgcolumnar.storage.format_version is the stamp that could refuse such a table
- * early on both paths, and it cannot express this change today: it is checked
- * `!= COLUMNAR_NATIVE_VERSION_MAJOR`, so bumping it would also make THIS build
- * refuse every table alpha4 wrote. Tracked separately; see the issue linked from
- * the #1130 changelog entry.
+ * That was the shape of the failure before #1137. A table this build writes, or
+ * appends to, is now stamped COLUMNAR_NATIVE_FORMAT_VERSION 2, and an alpha4
+ * binary refuses it at every decode entry point before reading a chunk byte.
+ * The table above is what remains for a binary older than the stamp itself,
+ * which no change here can reach.
  */
 #define COLUMNAR_NATIVE_ENCDESC_MIN_READABLE 2
 

src/columnar_metadata.c

@@ -2036,6 +2036,63 @@ columnar_relation_is_new_in_xact(Oid relid)
 	return isNew;
 }
 
+/*
+ * storage_row_probe
+ *		Read the storage row the scan positioned on, if any (#1137).
+ *
+ *		Returns STORAGE_ROW_ABSENT, STORAGE_ROW_CURRENT, or STORAGE_ROW_OLDER when
+ *		the row's format_version is below `formatVersion` -- a table an earlier
+ *		build wrote. With `raise`, an older row is first updated to
+ *		`formatVersion`.
+ *
+ *		THE STAMP MUST MOVE WHEN THIS BUILD APPENDS TO AN OLDER TABLE. The stamp
+ *		is what lets an older binary refuse bytes it cannot read, and this build
+ *		writes chunks with no validity bitmap (#1130). A table stamped 1 that this
+ *		build has appended to holds those chunks, and an alpha4 binary would accept
+ *		the stamp and read a row that holds a value as NULL. Raising the stamp on
+ *		the first flush closes that: the whole table becomes one this build can
+ *		read (every version from COLUMNAR_NATIVE_FORMAT_MIN_READABLE up) and an
+ *		older one refuses. A table this build only READS keeps its stamp, so
+ *		reading alone never locks an older binary out.
+ */
+#define STORAGE_ROW_ABSENT	0
+#define STORAGE_ROW_CURRENT	1
+#define STORAGE_ROW_OLDER	2
+
+static int
+storage_row_probe(Relation rel, SysScanDesc scan, int32 formatVersion, bool raise)
+{
+	HeapTuple	tuple = systable_getnext(scan);
+	TupleDesc	tupdesc = RelationGetDescr(rel);
+	bool		isnull;
+	Datum		d;
+
+	if (!HeapTupleIsValid(tuple))
+		return STORAGE_ROW_ABSENT;
+
+	d = heap_getattr(tuple, Anum_native_storage_format_version, tupdesc, &isnull);
+	if (isnull || DatumGetInt32(d) >= formatVersion)
+		return STORAGE_ROW_CURRENT;
+
+	if (raise)
+	{
+		Datum		values[Natts_native_storage];
+		bool		nulls[Natts_native_storage];
+		bool		replace[Natts_native_storage];
+		HeapTuple	newTuple;
+
+		memset(values, 0, sizeof(values));
+		memset(nulls, false, sizeof(nulls));
+		memset(replace, false, sizeof(replace));
+		values[Anum_native_storage_format_version - 1] = Int32GetDatum(formatVersion);
+		replace[Anum_native_storage_format_version - 1] = true;
+		newTuple = heap_modify_tuple(tuple, tupdesc, values, nulls, replace);
+		CatalogTupleUpdate(rel, &newTuple->t_self, newTuple);
+		heap_freetuple(newTuple);
+	}
+	return STORAGE_ROW_OLDER;
+}
+
 void
 PgColumnarInsertNativeStorageRow(const NativeStorageMetadata *s)
 {
@@ -2085,7 +2142,15 @@ PgColumnarInsertNativeStorageRow(const NativeStorageMetadata *s)
 					F_INT8EQ, Int64GetDatum((int64) s->storageId));
 		scanIdx = pgcolumnar_scan_index_oid(rel, "storage_pkey");
 		scan = systable_beginscan(rel, scanIdx, OidIsValid(scanIdx), snapshot, 1, key);
-		exists = HeapTupleIsValid(systable_getnext(scan));
+		/*
+		 * Only a row already at this build's stamp lets the loader skip the
+		 * lock. An older one has to be raised, and raising is an UPDATE that two
+		 * unserialized loaders would race on, so it falls through to the locked
+		 * path below. The coordinator's PgColumnarEnsureStorageRow takes that
+		 * path first, so in practice the loaders find it raised.
+		 */
+		exists = storage_row_probe(rel, scan, s->formatVersion, false) ==
+			STORAGE_ROW_CURRENT;
 		systable_endscan(scan);
 		PopActiveSnapshot();
 		if (exists)
@@ -2124,7 +2189,9 @@ PgColumnarInsertNativeStorageRow(const NativeStorageMetadata *s)
 					F_INT8EQ, Int64GetDatum((int64) s->storageId));
 		scanIdx = pgcolumnar_scan_index_oid(rel, "storage_pkey");
 		scan = systable_beginscan(rel, scanIdx, OidIsValid(scanIdx), snapshot, 1, key);
-		exists = HeapTupleIsValid(systable_getnext(scan));
+		/* Invisible to every other backend, so the raise cannot race. */
+		exists = storage_row_probe(rel, scan, s->formatVersion, true) !=
+			STORAGE_ROW_ABSENT;
 		systable_endscan(scan);
 		if (exists)
 		{
@@ -2157,7 +2224,13 @@ PgColumnarInsertNativeStorageRow(const NativeStorageMetadata *s)
 				F_INT8EQ, Int64GetDatum((int64) s->storageId));
 	scanIdx = pgcolumnar_scan_index_oid(rel, "storage_pkey");
 	scan = systable_beginscan(rel, scanIdx, OidIsValid(scanIdx), snapshot, 1, key);
-	exists = HeapTupleIsValid(systable_getnext(scan));
+	/*
+	 * Under the advisory lock, held to transaction end, so a second writer
+	 * re-checks only after this one's raise has committed and finds the row
+	 * current.
+	 */
+	exists = storage_row_probe(rel, scan, s->formatVersion, true) !=
+		STORAGE_ROW_ABSENT;
 	systable_endscan(scan);
 	PopActiveSnapshot();
 	if (exists)
@@ -2720,13 +2793,22 @@ PgColumnarCheckNativeFormatVersion(uint64 storageId, const char *relName)
 	systable_endscan(scan);
 	table_close(rel, AccessShareLock);
 
-	if (found && formatVersion != COLUMNAR_NATIVE_VERSION_MAJOR)
+	/*
+	 * A RANGE, NOT AN EQUALITY (#1137). An equality made the stamp unable to
+	 * move: raising it for a new layout would also have refused every table the
+	 * previous build wrote. This build reads every version from
+	 * COLUMNAR_NATIVE_FORMAT_MIN_READABLE to the one it writes.
+	 */
+	if (found && (formatVersion < COLUMNAR_NATIVE_FORMAT_MIN_READABLE ||
+				  formatVersion > COLUMNAR_NATIVE_FORMAT_VERSION))
 		ereport(ERROR,
 				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 				 errmsg("unsupported columnar native format version %d",
 						formatVersion),
-				 errdetail("Relation \"%s\" was written with native format version %d; this build supports version %d.",
-						   relName, formatVersion, COLUMNAR_NATIVE_VERSION_MAJOR)));
+				 errdetail("Relation \"%s\" was written with native format version %d; this build reads versions %d to %d.",
+						   relName, formatVersion,
+						   COLUMNAR_NATIVE_FORMAT_MIN_READABLE,
+						   COLUMNAR_NATIVE_FORMAT_VERSION)));
 }
 
 void

src/columnar_reader.c

@@ -3999,7 +3999,9 @@ static uint64 columnarFetchClock = 0;
  * Memoized native-format-version validation for the by-row-number fetch path
  * (#240). pgcolumnar_fetch_row runs once per row, so checking the storage's
  * format_version against a catalog row on every call would be a per-row systable
- * scan on a hot path. The value is immutable for a storage, so one check per
+ * scan on a hot path. The value only ever RISES, and only from one version this
+ * build reads to another (#1137: the first write by a newer build raises 1 to
+ * 2), so a check that passed stays true for the command and one per
  * (command, storageId) is enough; this single slot is cleared in
  * PgColumnarDiscardFetchCache, the same command/transaction boundary the fetch
  * cache resets on.
@@ -4801,6 +4803,18 @@ pgcolumnar_fetch_row(Relation rel, Snapshot snapshot, uint64 rowNumber,
 		return true;
 	}
 
+	/*
+	 * THE STAMP GATE'S ONE EXEMPTION, HELD BY CODE RATHER THAN A COMMENT
+	 * (#1137). The format-version check above runs only when wantValues is
+	 * set, on the reasoning that a liveness-only probe decodes nothing. That
+	 * is true exactly while every byte read below sits after the return just
+	 * above. Since the stamp moved to 2, this is the only thing between an
+	 * older or newer layout and a silent misread on this path, so a refactor
+	 * that reaches the decode without passing the check must fail here, on
+	 * every assert-enabled build the suites run, not in a user's query.
+	 */
+	Assert(columnarFetchFmtOk && columnarFetchFmtOkStorageId == storageId);
+
 	/*
 	 * The group's bytes and its decoded columns are the expensive part and the
 	 * part that repeats across fetches of the same group, so they come from the

src/columnar_write_state.c

@@ -754,7 +754,7 @@ PgColumnarEnsureStorageRow(Relation rel)
 
 	s.storageId = PgColumnarStorageId(rel);
 	s.relationOid = RelationGetRelid(rel);
-	s.formatVersion = COLUMNAR_NATIVE_VERSION_MAJOR;
+	s.formatVersion = COLUMNAR_NATIVE_FORMAT_VERSION;
 	s.vectorLength = COLUMNAR_NATIVE_VECTOR_LENGTH;
 	s.rowGroupLimit = stripeRowLimit;
 	PgColumnarInsertNativeStorageRow(&s);
@@ -3156,7 +3156,7 @@ pgcolumnar_flush_row_group(PgColumnarWriteState *writeState)
 
 		s.storageId = writeState->storageId;
 		s.relationOid = writeState->relid;
-		s.formatVersion = COLUMNAR_NATIVE_VERSION_MAJOR;
+		s.formatVersion = COLUMNAR_NATIVE_FORMAT_VERSION;
 		s.vectorLength = COLUMNAR_NATIVE_VECTOR_LENGTH;
 		s.rowGroupLimit = writeState->stripeRowLimit;
 		PgColumnarInsertNativeStorageRow(&s);