Skip to content

fix: the upgrade fixtures must be what their tags shipped (#901) - #1103

Merged
jdatcmd merged 2 commits into
mainfrom
fix/901-fixtures-from-the-tag
Sep 17, 2026
Merged

jdatcmd merged 2 commits into
mainfrom
fix/901-fixtures-from-the-tag

Conversation

@jdatcmd

@jdatcmd jdatcmd commented Sep 17, 2026

Copy link
Copy Markdown
Collaborator

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

🤖 Generated with Claude Code

https://claude.ai/code/session_01NhwXKAgSmYDUjteWkfajHK

`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 [34a359d] want [ef3f381]

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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NhwXKAgSmYDUjteWkfajHK
…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<version>

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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NhwXKAgSmYDUjteWkfajHK

@OffgridwithJD OffgridwithJD left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving 36e0309. The fix is right, the check runs where the comment says it runs, and I tested that claim rather than reading it. One defect that fails closed, and one note about how the exclusion is written.

What I verified against the tree

All three fixtures now agree with their tags. Blob ids, not a diff:

version tag blob fixture blob
1.0-alpha2 ef3f381c6805 ef3f381c6805 AGREE
1.0-alpha3 c7269cf1e242 c7269cf1e242 AGREE
1.0-alpha4 fd5ae29fec7d fd5ae29fec7d AGREE

The 50 lines the diff removes from pgcolumnar--1.0-alpha2.sql are the two post-release set_options guard fixes, which is the drift #901 named.

Excluding 1.0-alpha is correct, and for a stronger reason than "synthetic". That path exists at no tag at all:

=== every *.sql at the ROOT of tag v1.0-alpha ===
  pgcolumnar--1.0-dev.sql
  pgcolumnar.control
default_version at v1.0-alpha: '1.0-dev'

pgcolumnar--1.0-alpha.sql is absent from all four v1.0-alpha* tags. There is no shipped artifact to compare it against, so it cannot be in the list.

The derived list reproduces the hardcoded one exactly, evaluated through your own code path:

  derived : [1.0-alpha2 1.0-alpha3 1.0-alpha4]
  declared: [1.0-alpha2 1.0-alpha3 1.0-alpha4]

[ "$_fx_v" = "1.0-alpha" ] && continue is safe here, and I checked both halves rather than reasoning about set -e: the suite sets set -uo pipefail with no -e, and the idiom completes the loop even when -e is on (loop completed, kept 2 of 3).

The claim I came to disprove, and could not

Your comment says the arm "runs locally and in the five-major release gate". I went looking for the opposite, because run_all_versions.sh:690 copies with cp -a "$SRCDIR/." "$builddir/" and the sibling check added in #1099 says git tag fails in a tree copied without .git. Those cannot both be true.

cp -a copies .git, and in the gate's own build directory the tags still resolve — including from a linked worktree, where .git is a gitfile whose absolute path still points somewhere real:

  .git in the copy          : file -> gitdir: /root/pgcolumnar/.git/worktrees/t1103
  git tag -l 'v1.0-alpha*'  : v1.0-alpha v1.0-alpha2 v1.0-alpha3 v1.0-alpha4
  rev-parse v1.0-alpha2:... : ef3f381c68051c54
  hash-object the fixture   : ef3f381c68051c54
  => they RUN

Your comment is right and #1099's sibling is the one describing a different environment. Worth reconciling those two comments at some point, but not here.

One defect: || echo cannot produce a clean sentinel after git rev-parse

test/native_upgrade_converge.sh:159:

_fx_tag="$(_fx_git rev-parse "v$v:pgcolumnar--$v.sql" || echo "no such path at v$v")"

git rev-parse echoes its argument to stdout before failing, so both land in the variable:

_fx_tag = $'v1.0-alpha:pgcolumnar--1.0-alpha.sql\nno such path at v1.0-alpha'

The next line is the contrast, and it is already correct — git hash-object writes nothing on failure:

_fx_fix = 'fixture missing'

--verify -q gives the clean sentinel:

-	_fx_tag="$(_fx_git rev-parse "v$v:pgcolumnar--$v.sql" || echo "no such path at v$v")"
+	_fx_tag="$(_fx_git rev-parse --verify -q "v$v:pgcolumnar--$v.sql" || echo "no such path at v$v")"

It fails closed either way — a two-line value never equals a hash — so this is a diagnostic bug, not a correctness one, which is why it is not blocking. The same shape is live in #1102 (grep -c ... || echo 0 yields $'0\n0' and turns the comparison into a shell error); I have raised it there with the measurement.

One note on the exclusion

The sweep derives the list from disk and then removes one entry by name:

[ "$_fx_v" = "1.0-alpha" ] && continue

The property that justifies the exclusion is "no tag shipped this path", which is exactly what the loop below tests. A fixture added later for another untagged version — a 1.0-dev one, say — gets swept in and fails with the two-line sentinel above rather than being recognised as the same case. Deriving the skip (rev-parse --verify -q misses, so there is nothing to compare) would close both at once. Not blocking, and arguably a name is clearer about intent than a silent skip; your call.

Sequencing

Agreed on landing #1098 first — I have rebased it onto ba56ec2 and applied your three findings, including a fourth copy of the version list you did not name: the convergence loop at what is now line 203 wrote it again. All four now read one _NUC_FIXTURES.

Nothing in this PR contradicts it. Yours checks fixture-against-tag, mine checks installed-against-fixture, and the run where alpha3's fixture-vs-tag passes while its convergence arm fails is the clearest statement that they are two defects.

@jdatcmd
jdatcmd merged commit 99fd9bc into main Sep 17, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants