From cad920c70d1bc675b91b59b5a33a07eba10624e4 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Thu, 17 Sep 2026 13:55:18 -0600 Subject: [PATCH 1/2] fix: the upgrade fixtures must be what their tags shipped (#901) `native_upgrade_converge` claims every released starting point upgrades to the current catalog. It can only claim that if each fixture IS the released starting point, and `1.0-alpha2`'s was not. The fixtures are made by renaming the root base script at cycle-open. That equals the release only if nothing touched the file between the tag and the rename. For alpha2 two post-release `set_options` fixes did, so the fixture was the released script plus 50 lines it never shipped with: ERRCODE = 'wrong_object_type' 42809 rather than plpgsql's default P0001 a relation that is not columnar refused Taken from the tag now, and the corrected fixture CONVERGES, so the wrong one was not hiding a broken upgrade. It was hiding that nothing tested the real one. A CHECK, COMPARING BLOB IDS RATHER THAN DIFFING. A blob id is a lookup: no similarity heuristic and no pathspec can distort it, and a rename-detection argument once fabricated an `R098` against this very file. the 1.0-alpha2 fixture is byte-identical to what v1.0-alpha2 shipped PASS the 1.0-alpha3 fixture is byte-identical to what v1.0-alpha3 shipped PASS the 1.0-alpha4 fixture is byte-identical to what v1.0-alpha4 shipped PASS Removal proof, the pre-fix fixture against the tag: FAIL got [34a359d9ad6a] want [ef3f381c6805] It SKIPS where tags are absent, with the reason named. `actions/checkout` takes one ref at depth 1, so this cannot run in CI. It runs locally and in the five-major release gate, which is where a fixture is captured and therefore where it can be captured wrongly. `1.0-alpha` IS EXCLUDED, AND THE ISSUE'S FIX FOR IT DOES NOT WORK. #901 proposed replacing that fixture with the real `1.0-dev` script from `v1.0-alpha`. Tried it: ERROR: could not find function "columnar_handler" in file "pgcolumnar.so" `v1.0-alpha` shipped `pgcolumnar--1.0-dev.sql` with `default_version = 1.0-dev`, and no tag ships a `pgcolumnar--1.0-alpha.sql` at all. The real script names the pre-rename C symbol, and that rename is exactly why `ALTER EXTENSION UPDATE` is mandatory rather than cosmetic. So no single-library test can start from a genuine `1.0-dev` or `1.0-alpha` install. The arm is kept rather than dropped: it is the only cover for the `1.0-dev--1.0-alpha` and `1.0-alpha--1.0-alpha2` upgrade scripts, which ship. The header now says it tests catalog shape rather than a released artifact, and the exclusion is written down rather than left as a silent gap. NOT FIXED HERE: the suite still stages a fixture only when nothing is already installed, so a leftover in the extension directory wins over the committed file (#1090, @OffgridwithJD's #1098). The two are independent and this run shows both: alpha3's fixture-vs-tag check PASSES while its convergence arm FAILS, because the fixture is right and the suite is not reading it. 13 passed + 1 failed = 14, the failure being #1090 on this box Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NhwXKAgSmYDUjteWkfajHK --- test/fixtures/pgcolumnar--1.0-alpha2.sql | 50 --------------------- test/native_upgrade_converge.sh | 56 ++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 50 deletions(-) diff --git a/test/fixtures/pgcolumnar--1.0-alpha2.sql b/test/fixtures/pgcolumnar--1.0-alpha2.sql index 34a359d9..ef3f381c 100644 --- a/test/fixtures/pgcolumnar--1.0-alpha2.sql +++ b/test/fixtures/pgcolumnar--1.0-alpha2.sql @@ -374,56 +374,6 @@ CREATE FUNCTION pgcolumnar.set_options( DECLARE col name; BEGIN - /* - * The options are per-relation and are read by the columnar writer, so a row - * recorded for a relation that is not columnar can never be used. Storing one - * is not merely useless: the drop hook that clears pgcolumnar.options fires - * only for columnar relations, so the row outlives the table and is left - * keyed to a dangling oid that a later relation reusing that oid inherits. - * Measured before this guard, on the same cluster: set_options on a heap - * table stored a row, DROP TABLE left it behind, and regclass then rendered - * as the bare oid; the identical sequence on a columnar table cleaned up. - * - * Rejecting is safe for the one workflow that could want the other order: - * ALTER TABLE ... SET ACCESS METHOD pgcolumnar keeps the relation's oid - * (measured), so options set after the conversion apply to the same relation - * a caller would have been trying to name before it. - * - * The ERRCODE is explicit. plpgsql's RAISE EXCEPTION defaults to P0001, and - * the C paths raise this same sentence with ERRCODE_WRONG_OBJECT_TYPE - * (42809). Without it the identical message carried two different SQLSTATEs - * depending on which path refused the caller, in a tree whose own privilege - * suites deliberately assert SQLSTATE rather than message text. - * - * relkind is part of the test, and it is what makes the guard match the - * cleanup rather than merely look strict. The drop hook returns before it - * examines the access method for anything that is not an ordinary table - * (columnar_tableam.c: `if (get_rel_relkind(objectId) != RELKIND_RELATION) - * return;`), so 'r' is exactly the set of relations whose options row can - * ever be cleaned up. From PG17 a PARTITIONED table may carry an access - * method, so `relam = pgcolumnar` alone admits a parent that has no storage, - * that the writer never writes, and whose row the hook will never clear. - * Measured on 17.6 with the amname-only test: accepted, one row recorded, - * and the row still there after DROP TABLE keyed to the dropped oid, while - * an ordinary columnar table in the same run cleaned up. PG16 and earlier - * cannot reach it -- they refuse `PARTITION BY ... USING pgcolumnar` - * outright, checked on 16.14 -- so this is PG17, 18 and 19. - */ - IF NOT EXISTS (SELECT 1 FROM pg_class c - JOIN pg_am a ON a.oid = c.relam - WHERE c.oid = table_name - AND a.amname = 'pgcolumnar' - AND c.relkind = 'r') THEN - RAISE EXCEPTION 'relation "%" is not a columnar table', table_name - USING ERRCODE = 'wrong_object_type', - HINT = 'Per-table options are read by the columnar writer and ' - 'apply only to an ordinary table using the pgcolumnar access ' - 'method. A partitioned table has no storage of its own: set the ' - 'options on each partition. Otherwise convert the table first ' - 'with ALTER TABLE ... SET ACCESS METHOD pgcolumnar, then set ' - 'the options.'; - END IF; - IF encode_effort IS NOT NULL AND encode_effort NOT IN ('full', 'fast') THEN RAISE EXCEPTION 'unknown columnar encode_effort "%"', encode_effort diff --git a/test/native_upgrade_converge.sh b/test/native_upgrade_converge.sh index 6b5ab356..d8aa41b8 100755 --- a/test/native_upgrade_converge.sh +++ b/test/native_upgrade_converge.sh @@ -77,6 +77,62 @@ done cleanup() { for f in "${STAGED[@]:-}"; do [ -n "$f" ] && rm -f "$f"; done; } trap cleanup EXIT +# ---- each fixture must be what its tag actually shipped (#901) -------------- +# +# THE FIXTURES ARE THE PREMISE OF EVERY ARM BELOW. This suite claims that each +# released starting point upgrades to the current catalog, and it can only claim +# that if the fixture IS the released starting point. +# +# They were made by renaming the root base script when the next cycle opened. A +# rename at cycle-open equals the release only if nothing touched the file between +# the tag and the rename, and for `1.0-alpha2` something did: two post-release +# `set_options` fixes. The fixture was the released script plus those, so this +# suite spent two cycles upgrading from a state no user ever had. +# +# Comparing BLOB IDS rather than diffing, because a blob id is a lookup. No +# similarity heuristic and no pathspec can distort it, and a rename-detection +# argument once turned a content-preserving move into a fabricated `R098` here. +# +# `1.0-alpha` IS DELIBERATELY ABSENT FROM THIS LIST, and that is the finding this +# check exists to stop being invisible. `v1.0-alpha` shipped +# `pgcolumnar--1.0-dev.sql` with `default_version = 1.0-dev`; there is no +# `pgcolumnar--1.0-alpha.sql` at that tag or any other. Its fixture is constructed +# rather than released, and it cannot be replaced by the real `1.0-dev` script: +# that script names the pre-rename C symbol and the current library does not +# export it. +# +# ERROR: could not find function "columnar_handler" in file "pgcolumnar.so" +# +# So no single-library test can start from a genuine `1.0-dev` or `1.0-alpha` +# install. The arm is kept because it is the only cover for the +# `1.0-dev--1.0-alpha` and `1.0-alpha--1.0-alpha2` upgrade scripts, which ship. +# What it tests is catalog shape, not a released artifact, and it is named that +# way rather than counted with the others. +_FX_TAGGED="1.0-alpha2 1.0-alpha3 1.0-alpha4" + +_fx_git() { git -C "$HERE/.." "$@" 2>/dev/null; } +if ! _fx_git rev-parse --git-dir >/dev/null; then + _fx_why="no git repository in the tree under test" +elif [ -z "$(_fx_git tag -l 'v1.0-alpha*')" ]; then + # CI checks out at depth 1 with no tags, so this cannot run there. It runs + # locally and in the five-major release gate, which is where a fixture is + # captured and therefore where it can be captured wrongly. + _fx_why="no release tags in this checkout" +else + _fx_why="" +fi + +for v in $_FX_TAGGED; do + _fx_name="the $v fixture is byte-identical to what v$v shipped" + if [ -n "$_fx_why" ]; then + check_skip "$_fx_name" "SKIP $_fx_name ($_fx_why)" "$_fx_why" + continue + fi + _fx_tag="$(_fx_git rev-parse "v$v:pgcolumnar--$v.sql" || echo "no such path at v$v")" + _fx_fix="$(_fx_git hash-object "$HERE/fixtures/pgcolumnar--$v.sql" || echo "fixture missing")" + check "$_fx_name" "$_fx_fix" "$_fx_tag" +done + P() { env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres -tAq "$@"; } # Comprehensive catalog snapshot of the pgcolumnar schema, one line per object. From 36e030949d3ab5272aca10ada499defa276e0496 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Thu, 17 Sep 2026 14:50:31 -0600 Subject: [PATCH 2/2] fix: derive the fixture list, and say that the local tag is trusted (#901) Two defects in my own first version of this check, found reviewing it against the standard I had just applied to #1098. THE LIST WAS HARDCODED, AND COULD NOT SEE ITS OWN INCOMPLETENESS. `_FX_TAGGED` named three versions while the fixtures directory holds four. The one case this arm exists to catch is a NEW fixture captured wrongly at cycle-open, and a hardcoded list is exactly what omits a new fixture. It would have gone on passing while checking nothing about the thing it was added for. Derived from the fixtures on disk now, minus `1.0-alpha`, which is synthetic and documented as such. A fixture added next cycle is checked without anyone remembering this file exists. The sweep is a claim too, so it carries a premise. An empty derivation would make every arm below vanish and the suite would report clean having compared nothing. premise: the fixture sweep found fixtures to compare against their tags PASS THE LOCAL TAG IS TRUSTED AND THAT LIMIT WAS UNWRITTEN. `git fetch` never moves an existing local tag, so a stale one makes this arm compare against the wrong blob: it reports a drift that is not there, or misses one that is. This repository has already had a false release-integrity issue filed off a stale local ref. The limit is now stated where the arm is, with the command that settles it: git ls-remote origin refs/tags/v Checked today, and they agree: local v1.0-alpha2 and the server both at 7c317d3. That is why this is a documented limit rather than a bug. 14 passed + 1 failed = 15, the failure being #1090's leftover on this box Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NhwXKAgSmYDUjteWkfajHK --- test/native_upgrade_converge.sh | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/test/native_upgrade_converge.sh b/test/native_upgrade_converge.sh index d8aa41b8..8535595a 100755 --- a/test/native_upgrade_converge.sh +++ b/test/native_upgrade_converge.sh @@ -108,7 +108,35 @@ trap cleanup EXIT # `1.0-dev--1.0-alpha` and `1.0-alpha--1.0-alpha2` upgrade scripts, which ship. # What it tests is catalog shape, not a released artifact, and it is named that # way rather than counted with the others. -_FX_TAGGED="1.0-alpha2 1.0-alpha3 1.0-alpha4" +# DERIVED FROM THE FIXTURES ON DISK rather than listed, so a fixture added at the +# next cycle-open is checked without anyone remembering to add it here. A list +# that has to be edited alongside the thing it describes goes stale, and this +# suite already carries three copies of its version list for exactly that reason. +# A hardcoded list here could not see its own incompleteness: the one case it +# must catch is a NEW fixture, and a new fixture is precisely what it would omit. +# +# THE LOCAL TAG IS TRUSTED, AND THAT IS A REAL LIMIT. `git fetch` never moves an +# existing local tag, so a stale one compares the fixture against the wrong blob +# and this arm reports a drift that is not there, or misses one that is. A false +# release-integrity issue has already been filed off a stale local ref in this +# repository. If this arm fails and the fixture looks right, check the tag against +# the server before believing it: +# +# git ls-remote origin refs/tags/v +_FX_TAGGED="" +for _fx_f in "$HERE"/fixtures/pgcolumnar--*.sql; do + [ -f "$_fx_f" ] || continue + _fx_v="${_fx_f##*/pgcolumnar--}"; _fx_v="${_fx_v%.sql}" + # 1.0-alpha is synthetic and has no tag artifact: see above. + [ "$_fx_v" = "1.0-alpha" ] && continue + _FX_TAGGED="$_FX_TAGGED $_fx_v" +done +_FX_TAGGED="${_FX_TAGGED# }" + +# The sweep is a claim too. An empty one would make every arm below vanish and the +# suite would report clean having compared nothing. +check "premise: the fixture sweep found fixtures to compare against their tags" \ + "$([ -n "$_FX_TAGGED" ] && echo yes || echo no)" "yes" _fx_git() { git -C "$HERE/.." "$@" 2>/dev/null; } if ! _fx_git rev-parse --git-dir >/dev/null; then