From 280d5afcead5e07c8e36525cf20c5c550fe69e5a Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Thu, 17 Sep 2026 18:44:55 +0000 Subject: [PATCH] test: stage the upgrade fixtures unconditionally, so a leftover cannot win (#1090) native_upgrade_converge creates an old-version extension to upgrade from, which needs the old base install script in the extension directory. It staged each fixture only `if [ -f "$src" ] && [ ! -f "$dst" ]`. This repository shipped pgcolumnar--1.0-alpha2.sql and pgcolumnar--1.0-alpha3.sql from the tree until the cycle-open rename moved them under test/fixtures, so any prefix installed before that still carries them and nothing prunes them. Because the leftover was not staged, the EXIT trap did not remove it either: it persisted and won again on every later run. IT FAILED IN BOTH DIRECTIONS. Measured on one machine, same commit, two prefixes: leftover DIFFERS PG15 holds the v1.0-alpha2 TAG content (fead351f84ca) against the fixture's cbb4f36e4308. The suite reported 11 passed + 0 failed -- CONVERGED, having tested a file nobody committed, with nothing in its output naming which file it read. leftover BROKEN a one-line invalid file gave 8 passed + 3 failed: a red for a defect that is not in the tree. The second is how this was noticed. The first is what it costs, and it is why the fix is not simply "delete the leftovers". That the suite reads the leftover rather than the fixture was established with a discriminator that cannot be ambiguous: a syntactically invalid leftover must fail if it is read and pass if it is not. It failed. Staging is now unconditional. A pre-existing file is preserved and restored byte for byte rather than deleted -- this suite did not create it and must not change the state of a prefix it does not own. Verified: the planted leftover is still 8e3f1a0f2317 after the run, and PG15's is still fead351f84ca. Two arms say the property out loud, so a future change that reintroduces a conditional cannot pass silently. The count is pinned separately from the content, because without it a fixture that vanished would leave the content arm comparing nothing and reporting clean. Removal proof -- restore the conditional, keep the arms: every staged install script is the committed fixture, not a leftover: got [1.0-alpha2] want [] Verified: 13/13 on pg18a (no leftovers) and pg19a (leftovers present), 967 + 0 in harness_selftest, shellcheck clean. The ledger is untouched: this suite is registered but uncovered, so no rows move and the gate is unaffected. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012RSw4qMHS7ByE7PY8Ns4cs --- CHANGELOG.md | 57 +++++++++++++++++ test/native_upgrade_converge.sh | 109 ++++++++++++++++++++++++++++++-- 2 files changed, 159 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fa90354a..062752aa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,63 @@ installed, `1.0-alpha`, `1.0-alpha2`, and `1.0-alpha3`), so a single notes in this file describe `default_version` as pinned at an earlier version, each true until the next version shipped. +## [Unreleased] + +### Fixed + +- `native_upgrade_converge` staged its fixtures only when nothing was already + installed, so a leftover install script won over the committed fixture (#1090). + + The suite creates an old-version extension to upgrade from, which needs the old + base install script present in the extension directory. It staged each fixture + `if [ -f "$src" ] && [ ! -f "$dst" ]`. This repository shipped + `pgcolumnar--1.0-alpha2.sql` and `pgcolumnar--1.0-alpha3.sql` from the tree + until the cycle-open rename moved them under `test/fixtures`, so every prefix + installed before that still carries them and nothing prunes them. And because + the leftover was not staged, the EXIT trap did not remove it either: it + persisted and won again on every later run. + + IT FAILED IN BOTH DIRECTIONS, and the quiet one is the one that matters. + Measured on one machine, same commit, two prefixes: + + leftover DIFFERS PG15 holds the v1.0-alpha2 TAG content (fead351f84ca) + against the fixture's cbb4f36e4308. The suite reported + 11 passed + 0 failed -- CONVERGED, having tested a file + nobody committed, with nothing in the output naming + which file it read. + leftover BROKEN a one-line invalid file gave 8 passed + 3 failed: a red + for a defect that is not in the tree at all. + + That the suite reads the leftover rather than the fixture was established with + a discriminator that cannot be ambiguous -- a syntactically invalid leftover + must fail if it is read and pass if it is not. + + Staging is now unconditional, and a pre-existing file is preserved and restored + rather than deleted: the suite did not create it and must not change the state of + a prefix it does not own. Its CONTENT is restored, not its every attribute -- + `cp -p` cannot give back an owner the suite does not have, and the leftovers on a + developer's box are root-owned while the suite runs as `postgres`. + + The backup is taken only when there is not one already. `cp -p "$dst" + "$dst.pgcbak"` run unconditionally destroys the original across a crashed run: + the first run leaves the fixture installed and the original in `.pgcbak`, and the + second overwrites the backup with the fixture. The pre-existing file is then gone + for good, silently, by the route the preserve-rather-than-delete design exists to + avoid. Those leftovers are the evidence for #901, so this is not hypothetical. + A pre-existing `.pgcbak` also now fails a named check rather than passing + unremarked, because it means an earlier run died mid-staging. Reported by + jdatcmd. + + Two arms say so out loud, so a future change that reintroduces a conditional + cannot pass silently: one pins that all three fixtures were staged, the other + that each installed script IS the committed fixture. The count is pinned + separately because without it a fixture that vanished would leave the content + arm comparing nothing and reporting clean. + + Removal proof: restoring the conditional while keeping the arms gives + `every staged install script is the committed fixture, not a leftover: + got [1.0-alpha2] want []`, naming the script that was wrong. + ## [1.0-alpha4] - 2026-09-17 ### Fixed diff --git a/test/native_upgrade_converge.sh b/test/native_upgrade_converge.sh index 8535595a..7838910e 100755 --- a/test/native_upgrade_converge.sh +++ b/test/native_upgrade_converge.sh @@ -65,16 +65,88 @@ TARGET="$(sed -n "s/^default_version *= *'\\(.*\\)'.*/\\1/p" "$HERE/../pgcolumna check "control default_version is 1.0-alpha4" "$TARGET" "1.0-alpha4" # Stage the frozen old base install scripts so an old-version extension can be -# created. These are fixtures, not shipped; remove them at the end. +# created. These are fixtures, not shipped; whatever was there is put back at the +# end. +# +# STAGED UNCONDITIONALLY (#1090). The old form staged only `if [ ! -f "$dst" ]`, +# so an install script left in the extension directory by an older `make install` +# WON over the committed fixture. This repository shipped +# pgcolumnar--1.0-alpha2.sql and pgcolumnar--1.0-alpha3.sql from the tree until +# the cycle-open rename moved them under test/fixtures, so any prefix installed +# before that still carries them and nothing prunes them. And because the leftover +# was not staged, the EXIT trap did not remove it either: it persisted and won +# again on every later run. +# +# IT FAILS IN BOTH DIRECTIONS, and the quiet one is the dangerous one. Measured on +# this tree, same commit, two prefixes on one machine: +# +# leftover DIFFERS from the fixture PG15's alpha2 is the v1.0-alpha2 TAG +# content (fead351f84ca) against the +# fixture's cbb4f36e4308. The suite reported +# 11 passed + 0 failed -- CONVERGED, having +# tested a file nobody committed, with +# nothing in the output naming which file it +# read. +# leftover is BROKEN planting a one-line invalid file gave +# 8 passed + 3 failed, a red for a defect +# that is not in the tree at all. +# +# The second is how this was noticed; the first is what it costs. +# +# A pre-existing file is preserved and restored rather than deleted: this suite +# did not create it and removing it would change the state of a prefix it does not +# own. Its CONTENT is restored, not its every attribute -- `cp -p` cannot give back +# an owner the suite does not have, and these leftovers are root-owned on a +# developer's box while the suite runs as `postgres`. Content is what the next run +# reads, which is what this is protecting. +# +# ONE LIST. The versions were written out FOUR times -- here, in the staging arm, +# as a literal `3` in its count, and again in the convergence loop that upgrades +# from each of them -- so adding a fixture meant editing four places, and editing +# three of them left an arm passing while testing less than its name claims. +# Reported by jdatcmd, who counted three; the convergence loop is the fourth. +_NUC_FIXTURES=(1.0-alpha 1.0-alpha2 1.0-alpha3) + +# A `.pgcbak` ALREADY HERE MEANS AN EARLIER RUN DIED before its EXIT trap, so the +# original is in the backup and the install script holds the fixture. Taking the +# backup again would overwrite the original WITH the fixture and lose it for good: +# +# start: ext=[ORIGINAL] bak=- +# run 1 crashes: ext=[FIXTURE] bak=[ORIGINAL] +# run 2 unguarded:ext=[FIXTURE] bak=[FIXTURE] <- original gone +# +# That is the outcome the preserve-rather-than-delete design exists to avoid, +# reached by another route, and those leftovers are the evidence for #901. So the +# backup is taken only when there is not one already, and the state is NAMED rather +# than silently worked around: the prefix is mid-surgery and a reader should know. +# To clear it, restore by hand from the `.pgcbak` and remove it. Reported by +# jdatcmd, who reproduced the loss. +_nuc_mid="" +for v in "${_NUC_FIXTURES[@]}"; do + [ -f "$EXTDIR/pgcolumnar--$v.sql.pgcbak" ] && _nuc_mid="$_nuc_mid $v" +done +check "premise: no earlier run of this suite died with a backup still staged" \ + "${_nuc_mid# }" "" + STAGED=() -for v in 1.0-alpha 1.0-alpha2 1.0-alpha3; do +for v in "${_NUC_FIXTURES[@]}"; do src="$HERE/fixtures/pgcolumnar--$v.sql" dst="$EXTDIR/pgcolumnar--$v.sql" - if [ -f "$src" ] && [ ! -f "$dst" ]; then - cp "$src" "$dst"; STAGED+=("$dst") - fi + [ -f "$src" ] || continue + [ -f "$dst" ] && [ ! -f "$dst.pgcbak" ] && cp -p "$dst" "$dst.pgcbak" + cp "$src" "$dst" + STAGED+=("$dst") done -cleanup() { for f in "${STAGED[@]:-}"; do [ -n "$f" ] && rm -f "$f"; done; } +cleanup() { + for f in "${STAGED[@]:-}"; do + [ -n "$f" ] || continue + if [ -f "$f.pgcbak" ]; then + mv -f "$f.pgcbak" "$f" + else + rm -f "$f" + fi + done +} trap cleanup EXIT # ---- each fixture must be what its tag actually shipped (#901) -------------- @@ -161,6 +233,29 @@ for v in $_FX_TAGGED; do check "$_fx_name" "$_fx_fix" "$_fx_tag" done +# THE ARM THAT CATCHES THE QUIET HALF. Staging unconditionally fixes it; this +# says so out loud, so a future change that reintroduces a conditional cannot +# pass silently. Comparing the installed file against the fixture is the only +# thing that separates "tested the committed fixture" from "tested whatever was +# lying in the extension directory". +_nuc_staged=0 +_nuc_wrong="" +for v in "${_NUC_FIXTURES[@]}"; do + src="$HERE/fixtures/pgcolumnar--$v.sql" + dst="$EXTDIR/pgcolumnar--$v.sql" + [ -f "$src" ] || continue + _nuc_staged=$((_nuc_staged + 1)) + cmp -s "$src" "$dst" || _nuc_wrong="$_nuc_wrong $v" +done + +# The count is pinned separately: without it a fixture that vanished would leave +# the content arm comparing nothing and reporting "none". +check "premise: every fixture named for staging was staged" \ + "$_nuc_staged" "${#_NUC_FIXTURES[@]}" + +check "every staged install script is the committed fixture, not a leftover" \ + "${_nuc_wrong# }" "" + 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. @@ -190,7 +285,7 @@ check "fresh $TARGET install has objects to compare" \ "$([ "$(wc -l <"$REF")" -gt 100 ] && echo yes || echo no)" "yes" # Each released starting point must upgrade to an identical catalog. -for from in 1.0-alpha 1.0-alpha2 1.0-alpha3; do +for from in "${_NUC_FIXTURES[@]}"; do [ -f "$EXTDIR/pgcolumnar--$from.sql" ] || { check "fixture for $from present" "missing" "present"; continue; } db="conv_from_$(echo "$from" | tr '.-' '__')" P -d postgres -c "DROP DATABASE IF EXISTS $db;" >/dev/null