From c71e7f7280723078e95fd779d1a68f3522889525 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Mon, 12 Oct 2026 18:42:52 +0000 Subject: [PATCH] fix: the format stamp is a range, and the first write raises it (#1137) #1130 stopped writing a validity bitmap for a chunk with no nulls, and an alpha4 binary misreads such a chunk rather than refusing it. The owner chose option 2: stamp new tables 2, and turn the equality check into a range so this build still reads 1. COLUMNAR_NATIVE_FORMAT_VERSION 2 written COLUMNAR_NATIVE_FORMAT_MIN_READABLE 1 oldest read Measured with the real v1.0-alpha4 .so on PG 17, one seq scan and 40 single-row index fetches per table (custom scan off): written by alpha4 seq alpha4 fetches main before this (stamp 1) reads 0 right, 9 NULL, 32 errors this change (stamp 2) refused 0A000 41 refused main, appended to by this refused 0A000 41 refused THE THIRD ROW IS A HOLE OPTION 2 AS WRITTEN LEFT OPEN. Existing tables were to keep stamp 1, but this build writes bitmap-less chunks when it APPENDS to one. So PgColumnarInsertNativeStorageRow, which every flush calls, raises an older stamp on the first write. A read never moves it. The bulk loader's unlocked path does not raise: it falls through to the locked path, because two unserialized loaders would race on the UPDATE. The fetch path's liveness-only exemption from the check is now an Assert at the decode point. With that path's check disabled, a correctly stamped table trips it; without the Assert that mutation passed every arm. native_format.sh gains eight arms over four decode entry points. Each of four mutations is caught by the arm aimed at it: equality restored, raise removed, build still writing 1, lower bound dropped. The fetch arm turns the custom scan off; with it on, that path's check could be deleted and every arm stayed green (measured). test_native_format.py ports the new arms and is declared INCOMPLETE for the rest. native_format 28 passed, PG 15..19 pg17 catalog_scan_index native_writer parallel_copy concurrent_diff extension_upgrade docs_style: pass pytest guard half 404; cluster twins 50 harness_selftest 1213 passed Spec section 13, docs/limitations.md and docs/installation.md describe the range. Neither suite has ledger rows, so the ledger is untouched. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 49 +++++++++ design/NATIVE_FORMAT_AND_INTERFACE_SPEC.md | 20 +++- docs/installation.md | 6 +- docs/limitations.md | 20 +++- src/columnar.h | 26 +++-- src/columnar_metadata.c | 94 ++++++++++++++-- src/columnar_reader.c | 16 ++- src/columnar_write_state.c | 4 +- test/native_format.sh | 65 ++++++++++- test/native_writer.sh | 2 +- test/pytest/TESTS.md | 23 ++++ test/pytest/expected_tests.txt | 2 +- test/pytest/test_compare_to_bash.py | 6 ++ test/pytest/test_native_format.py | 119 +++++++++++++++++++++ 14 files changed, 423 insertions(+), 29 deletions(-) create mode 100644 test/pytest/test_native_format.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 908557c8..2e19d4a6 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/design/NATIVE_FORMAT_AND_INTERFACE_SPEC.md b/design/NATIVE_FORMAT_AND_INTERFACE_SPEC.md index db8c33f6..50edd3be 100644 --- a/design/NATIVE_FORMAT_AND_INTERFACE_SPEC.md +++ b/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. diff --git a/docs/installation.md b/docs/installation.md index 8561b373..e25b6988 100644 --- a/docs/installation.md +++ b/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 diff --git a/docs/limitations.md b/docs/limitations.md index 8c021a81..73c68ed1 100644 --- a/docs/limitations.md +++ b/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 diff --git a/src/columnar.h b/src/columnar.h index 1b84591c..71021f78 100644 --- a/src/columnar.h +++ b/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 diff --git a/src/columnar_metadata.c b/src/columnar_metadata.c index 83a855fe..e25692cc 100644 --- a/src/columnar_metadata.c +++ b/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 diff --git a/src/columnar_reader.c b/src/columnar_reader.c index 15ed35ae..fac0e306 100644 --- a/src/columnar_reader.c +++ b/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 diff --git a/src/columnar_write_state.c b/src/columnar_write_state.c index 5b6a5314..94845de9 100644 --- a/src/columnar_write_state.c +++ b/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); diff --git a/test/native_format.sh b/test/native_format.sh index ea5bdc34..6af2bb9f 100755 --- a/test/native_format.sh +++ b/test/native_format.sh @@ -56,8 +56,8 @@ load_pair "SELECT s, FROM generate_series(1, 5000) s" sid="$(storage_id_of t_col)" -check "native format_version is 1" \ - "$(q "SELECT format_version FROM pgcolumnar.storage WHERE storage_id = $sid;")" "1" +check "native format_version is 2" \ + "$(q "SELECT format_version FROM pgcolumnar.storage WHERE storage_id = $sid;")" "2" check "native vector_length is 1024" \ "$(q "SELECT vector_length FROM pgcolumnar.storage WHERE storage_id = $sid;")" "1024" @@ -104,7 +104,7 @@ check "backend survives the rejection" "$(q 'SELECT 1;')" "1" psql_run "CREATE TABLE fv (id int, v text) USING pgcolumnar;" psql_run "INSERT INTO fv SELECT s, md5(s::text) FROM generate_series(1, 2000) s;" psql_run "CREATE INDEX fv_id ON fv (id);" -check "table reads at native format_version 1" "$(q 'SELECT count(*) FROM fv;')" "2000" +check "table reads at the native format_version this build writes" "$(q 'SELECT count(*) FROM fv;')" "2000" fvsid="$(storage_id_of fv)" psql_run "UPDATE pgcolumnar.storage SET format_version = 99 WHERE storage_id = $fvsid;" # Three decode shapes must all reject. A seq scan opens a read state @@ -128,4 +128,63 @@ check "the native-format error names the offending version" \ "$(contains "$fverr_idx" '99')" "yes" check "backend survives the native-format rejection" "$(q 'SELECT 1;')" "1" +# --- 5: the stamp is a RANGE, and an append raises it (#1137) --------------- +# 1.0-alpha5 writes chunks with no validity bitmap (#1130), which an alpha4 +# binary misreads: some rows that hold a value come back NULL. So this build +# stamps 2, and alpha4 -- which refuses any stamp but 1 -- refuses the table at +# every decode entry point. This build still reads 1 (MIN_READABLE). Each arm +# below drives all four entry points that check the stamp: +# +# seq scan PgColumnarBeginReadWithStorage SELECT v ... LIMIT +# aggregate scan PgColumnarBeginAggScan count(*) +# group agg scan PgColumnarBeginGroupAggScan GROUP BY +# row fetch pgcolumnar_fetch_row index scan of a non-key column, +# custom scan OFF +# +# THE CUSTOM SCAN HAS TO BE OFF FOR THE FOURTH, and that was measured rather +# than assumed. With it on, the index query is served by pgcolumnar's own scan +# node and never reaches pgcolumnar_fetch_row's decode: with that path's stamp +# check deleted outright, every arm here still passed. +# +# A catalog UPDATE stands in for the other build's stamp; the value is read from +# pgcolumnar.storage, so no bytes need forging. +sqlstate_of() { # the SQLSTATE a statement fails with, or "ok" + local out + out="$(env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres \ + -d "$PGC_DB" -v ON_ERROR_STOP=1 -v VERBOSITY=sqlstate -Atq -c "$1" 2>&1 >/dev/null)" + if [ -z "$out" ]; then echo ok; else echo "$out" | sed -n 's/^ERROR: *\([0-9A-Z]\{5\}\).*/\1/p' | head -1; fi +} +four_paths() { # TABLE -> "scan agg group fetch", each a SQLSTATE or ok + echo "$(sqlstate_of "SELECT v FROM $1 LIMIT 1;") \ +$(sqlstate_of "SELECT count(*) FROM $1;") \ +$(sqlstate_of "SELECT id % 3, count(*) FROM $1 GROUP BY 1;") \ +$(sqlstate_of "SET pgcolumnar.enable_custom_scan=off; SET enable_seqscan=off; SET enable_bitmapscan=off; SELECT v FROM $1 WHERE id = 42;")" +} +stamp_of() { q "SELECT format_version FROM pgcolumnar.storage WHERE storage_id = $(storage_id_of "$1");"; } + +psql_run "CREATE TABLE fr (id int, v text) USING pgcolumnar;" +psql_run "INSERT INTO fr SELECT s, md5(s::text) FROM generate_series(1, 2000) s;" +psql_run "CREATE INDEX fr_id ON fr (id);" +frsid="$(storage_id_of fr)" +check "premise: the fixture was stamped by this build" "$(stamp_of fr)" "2" + +# What an alpha4 table looks like to this build. +psql_run "UPDATE pgcolumnar.storage SET format_version = 1 WHERE storage_id = $frsid;" +check "a table stamped 1 is read on all four paths" "$(four_paths fr)" "ok ok ok ok" +check "and returns its rows" "$(q 'SELECT count(*), sum(id) FROM fr;')" "2000|2001000" +check "reading a table stamped 1 does not raise its stamp" "$(stamp_of fr)" "1" + +# Appending is what puts bitmap-less chunks in it, so appending moves the stamp. +psql_run "INSERT INTO fr SELECT s, md5(s::text) FROM generate_series(2001, 2100) s;" +check "appending to a table stamped 1 raises its stamp" "$(stamp_of fr)" "2" +check "and the table still reads whole" "$(q 'SELECT count(*), sum(id) FROM fr;')" "2100|2206050" + +# Either side of the range is refused, everywhere, by SQLSTATE rather than by +# the absence of a wrong answer. 0A000 is feature_not_supported. +psql_run "UPDATE pgcolumnar.storage SET format_version = 3 WHERE storage_id = $frsid;" +check "a stamp above this build's is refused on all four paths" "$(four_paths fr)" "0A000 0A000 0A000 0A000" +psql_run "UPDATE pgcolumnar.storage SET format_version = 0 WHERE storage_id = $frsid;" +check "a stamp below MIN_READABLE is refused on all four paths" "$(four_paths fr)" "0A000 0A000 0A000 0A000" +check "backend survives the range rejections" "$(q 'SELECT 1;')" "1" + pgc_summary diff --git a/test/native_writer.sh b/test/native_writer.sh index 463df79d..dac7a0c4 100755 --- a/test/native_writer.sh +++ b/test/native_writer.sh @@ -21,7 +21,7 @@ SID="$(q "SELECT pgcolumnar.get_storage_id('nw');")" check "native storage row written" \ "$(q "SELECT count(*) FROM pgcolumnar.storage WHERE storage_id = $SID;")" "1" check "native format version recorded" \ - "$(q "SELECT format_version FROM pgcolumnar.storage WHERE storage_id = $SID;")" "1" + "$(q "SELECT format_version FROM pgcolumnar.storage WHERE storage_id = $SID;")" "2" check "native vector length recorded" \ "$(q "SELECT vector_length FROM pgcolumnar.storage WHERE storage_id = $SID;")" "1024" check "native row groups (5000 rows / 1000 limit)" \ diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index 0b92e951..5f814d99 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -129,6 +129,7 @@ behaviour, the source of that number is named. - [81. test_catalog_delete_index.py: retiring a row group costs no more for a bigger database](#81-test_catalog_delete_indexpy-retiring-a-row-group-costs-no-more-for-a-bigger-database) - [82. test_rewrite_storage_oid.py: a type rewrite keeps the storage row on the live table](#82-test_rewrite_storage_oidpy-a-type-rewrite-keeps-the-storage-row-on-the-live-table) - [83. test_catalog_scan_index.py: twelve metadata scans do not read their catalog whole](#83-test_catalog_scan_indexpy-twelve-metadata-scans-do-not-read-their-catalog-whole) +- [84. test_native_format.py: the format stamp is a range, and an append raises it](#84-test_native_formatpy-the-format-stamp-is-a-range-and-an-append-raises-it) ## 1. How to read a test in here @@ -6320,3 +6321,25 @@ The two halves share no code and no fixture. The shell suite adds 400 tables wit | test | what it holds | | --- | --- | | `test_catalog_scans_do_not_read_their_catalog_whole` | each of the four catalogs at or above `index_min_blocks`; then, for eleven operations (an insert into an existing table, into a table created in the same transaction, a `parallel_copy` load, a one-row delete, `compact_rewrite`, `truncate()`, `vacuum_sorted`, renaming a projected column, `TRUNCATE`, `drop_projection`, `DROP TABLE`), that the target held two free ranges, that the operation took effect, that it reached its catalog, and that it made no sequential scan of it | + +## 84. test_native_format.py: the format stamp is a range, and an append raises it + +A partial port of `native_format.sh`: the arms #1137 added and the format-version pin. #1130 stopped writing a validity bitmap for a chunk with no nulls, and an alpha4 binary misreads such a chunk without refusing it. Measured with the real alpha4 `.so`, 40 single-row fetches gave 9 NULL values for rows that hold one. So this build stamps `format_version` 2, and alpha4, which refuses any stamp but 1, refuses the table. The check became a range, `1 <= v <= 2`, so this build still reads what alpha4 wrote. + +THE FIRST WRITE RAISES THE STAMP, AND A READ DOES NOT. A table stamped 1 that this build appends to holds chunks alpha4 misreads, so the stamp has to move on that write. The arms set a stamp of 1 by catalog `UPDATE`, which is what an alpha4 table looks like, and check that reading leaves it at 1 and appending raises it to 2. + +FOUR ENTRY POINTS, AND THE FOURTH NEEDS THE CUSTOM SCAN OFF. The stamp is checked in the sequential scan, the aggregate scan, the group-aggregate scan and the by-row fetch. With the custom scan on, an index query is served by pgcolumnar's own node and never reaches `pgcolumnar_fetch_row`. That path's check was deleted outright on the shell twin and every arm still passed. + +REFUSED BY SQLSTATE, `0A000`, not by the absence of a wrong answer. Stamps 3 and 0 are both refused on all four paths. + +`RESET ALL` IS NOT USED after the fetch path's settings, because it also drops the private `search_path` that `pgc_conn` sets. The first draft did this, and every later query failed `42P01` instead of measuring anything. + +Removal proof, on this twin: making the append path skip the raise reddens `appending to a table stamped 1 raises its stamp` (got 1, want 2). + +Not ported, and declared INCOMPLETE in `test_compare_to_bash.py`: the metapage-version arms, which need a C hook bound at test time, and the diverse-type round trip against a heap mirror. + +### Every test + +| test | what it holds | +| --- | --- | +| `test_the_format_stamp_is_a_range_and_an_append_raises_it` | this build stamps 2; a table stamped 1 is read on all four entry points and returns its rows; reading leaves its stamp at 1 and appending raises it to 2; stamps 3 and 0 are refused with `0A000` on all four; the backend survives | diff --git a/test/pytest/expected_tests.txt b/test/pytest/expected_tests.txt index 19d65eaf..c8f32e01 100644 --- a/test/pytest/expected_tests.txt +++ b/test/pytest/expected_tests.txt @@ -543,4 +543,4 @@ guard_tests 404 # own test is not in it. Git conflicted again -- loudly, because the key moved # on both sides -- and the answer is neither branch's number. Collected on the # rebased tree, not added up. -cluster_tests 489 +cluster_tests 490 diff --git a/test/pytest/test_compare_to_bash.py b/test/pytest/test_compare_to_bash.py index 98cab219..3203c391 100644 --- a/test/pytest/test_compare_to_bash.py +++ b/test/pytest/test_compare_to_bash.py @@ -143,6 +143,12 @@ "range_pruning": "counters, heap oracle, scattered zero and the declared " "collation are ported; the three-state and fixture-geometry " "premises of the shell suite are not", + # #1137 ported the arms it added and the format-version pin. The rest of the + # shell suite needs a C hook bound at test time (the metapage version) and + # lib.sh's heap-mirror pair, and porting those is #432's work, not this one. + "native_format": "the format-stamp range arms and the version pin are " + "ported; the metapage-version hook arms and the diverse-type " + "round trip are not", } diff --git a/test/pytest/test_native_format.py b/test/pytest/test_native_format.py new file mode 100644 index 00000000..758fe653 --- /dev/null +++ b/test/pytest/test_native_format.py @@ -0,0 +1,119 @@ +"""The native format stamp is a RANGE, and the first write by a newer build raises it (#1137). + +1.0-alpha5 writes chunks with no validity bitmap (#1130), which an alpha4 binary +misreads: some rows that hold a value come back NULL. So this build stamps +`pgcolumnar.storage.format_version` 2, and alpha4, which refuses any stamp but 1, +refuses the table at every decode entry point. This build still reads 1. + +Each arm drives all four entry points that check the stamp: + + seq scan PgColumnarBeginReadWithStorage a non-aggregate SELECT + aggregate scan PgColumnarBeginAggScan count(*) + group agg scan PgColumnarBeginGroupAggScan GROUP BY + row fetch pgcolumnar_fetch_row index scan, custom scan OFF + +The custom scan has to be off for the fourth. With it on, the index query is +served by pgcolumnar's own node and never reaches `pgcolumnar_fetch_row`; that +path's check was deleted outright and every arm still passed (measured on the +shell twin). + +A partial port of `test/native_format.sh`: the stamp arms and the version pin. +The metapage-version hook and the diverse-type round trip are not ported, and +`test_compare_to_bash.py` declares the pair INCOMPLETE with that reason. +Independent of the shell twin: its own table, row count and key. +""" + +import psycopg + +ROWS = 1500 +# sum(1..1500) +FULL_SUM = ROWS * (ROWS + 1) // 2 + + +def _one(cur, sql): + cur.execute(sql) + return cur.fetchone()[0] + + +def _sqlstate(conn, statements): + """The SQLSTATE the last statement fails with, or "ok".""" + try: + with conn.cursor() as cur: + for s in statements: + cur.execute(s) + if cur.description: + cur.fetchall() + return "ok" + except psycopg.Error as e: + return e.sqlstate or "?" + finally: + # Only what the fetch path SET: RESET ALL would also drop the private + # search_path pgc_conn gives this test, and every later query would + # fail 42P01 instead of measuring anything. + with conn.cursor() as cur: + for g in ("pgcolumnar.enable_custom_scan", "enable_seqscan", + "enable_bitmapscan"): + cur.execute(f"RESET {g}") + + +def _four_paths(conn, t): + return " ".join([ + _sqlstate(conn, [f"SELECT v FROM {t} LIMIT 1"]), + _sqlstate(conn, [f"SELECT count(*) FROM {t}"]), + _sqlstate(conn, [f"SELECT k % 5, count(*) FROM {t} GROUP BY 1"]), + _sqlstate(conn, ["SET pgcolumnar.enable_custom_scan = off", + "SET enable_seqscan = off", + "SET enable_bitmapscan = off", + f"SELECT v FROM {t} WHERE k = 77"]), + ]) + + +def test_the_format_stamp_is_a_range_and_an_append_raises_it(pgc_conn, expect): + with pgc_conn.cursor() as cur: + cur.execute("CREATE TABLE stamp_t (k int, v text) USING pgcolumnar") + cur.execute(f"INSERT INTO stamp_t SELECT g, 'w' || g FROM generate_series(1,{ROWS}) g") + cur.execute("CREATE INDEX ON stamp_t (k)") + sid = _one(cur, "SELECT pgcolumnar.get_storage_id('stamp_t')") + + def stamp(): + with pgc_conn.cursor() as cur: + return _one(cur, f"SELECT format_version FROM pgcolumnar.storage WHERE storage_id = {sid}") + + def set_stamp(v): + with pgc_conn.cursor() as cur: + cur.execute(f"UPDATE pgcolumnar.storage SET format_version = {v} WHERE storage_id = {sid}") + + def rows(): + with pgc_conn.cursor() as cur: + cur.execute("SELECT count(*), sum(k) FROM stamp_t") + c, s = cur.fetchone() + return f"{c}|{s}" + + expect.num(stamp(), 2, "native format_version is 2") + expect.num(stamp(), 2, "premise: the fixture was stamped by this build") + + # What an alpha4 table looks like to this build. + set_stamp(1) + expect.text(_four_paths(pgc_conn, "stamp_t"), "ok ok ok ok", + "a table stamped 1 is read on all four paths") + expect.text(rows(), f"{ROWS}|{FULL_SUM}", "and returns its rows") + expect.num(stamp(), 1, "reading a table stamped 1 does not raise its stamp") + + # Appending is what puts bitmap-less chunks in it, so appending moves the stamp. + with pgc_conn.cursor() as cur: + cur.execute(f"INSERT INTO stamp_t SELECT g, 'w' || g FROM generate_series({ROWS + 1},{ROWS + 40}) g") + expect.num(stamp(), 2, "appending to a table stamped 1 raises its stamp") + total = ROWS + 40 + expect.text(rows(), f"{total}|{total * (total + 1) // 2}", + "and the table still reads whole") + + # Either side of the range is refused, everywhere, by SQLSTATE. 0A000 is + # feature_not_supported. + set_stamp(3) + expect.text(_four_paths(pgc_conn, "stamp_t"), "0A000 0A000 0A000 0A000", + "a stamp above this build's is refused on all four paths") + set_stamp(0) + expect.text(_four_paths(pgc_conn, "stamp_t"), "0A000 0A000 0A000 0A000", + "a stamp below MIN_READABLE is refused on all four paths") + with pgc_conn.cursor() as cur: + expect.num(_one(cur, "SELECT 1"), 1, "backend survives the range rejections")