From 1f480e8f52b9a64d90a4cc149f80aa06eedafadd Mon Sep 17 00:00:00 2001 From: "Joshua (D) Drake" Date: Tue, 4 Aug 2026 16:00:30 -0600 Subject: [PATCH 1/3] Actually run test/extension_upgrade.sh (#396) The suite landed in #389 with pg_upgrade's exemption from the registration check and without pg_upgrade's invocation, so nothing ran it. Not the matrix, not CI, not the Makefile. It ran only when a human typed its name. That is the gap #257 existed to close, and it is worse here than there. The break this suite catches is invisible to a build and to every suite that creates the extension from scratch, so a guard nobody runs leaves exactly the failure it was written for undetected. It is invoked now from run_all_versions.sh under PGC_RUN_UPGRADE=1, the same switch and the same terms as pg_upgrade. One major is enough, so it runs once against the first config rather than per pair. Asking for the gate and getting nothing is a failure rather than a quiet pass, matching the pair check above it. Second problem from the same report: it could not run in the documented container loop at all, because that loop copies the tree without .git and the ref form needs tags. The second argument now takes either a git ref or a path to an already-checked-out old source tree. A tree that is not a git checkout now says so, and says what to pass instead, rather than failing on a missing tag. docs/testing.md gains a section for it, since the cross-major upgrade had one and this is a different upgrade with a different failure. --- docs/testing.md | 33 ++++++++++++++++++++++++ test/extension_upgrade.sh | 54 ++++++++++++++++++++++++++++----------- test/run_all_versions.sh | 31 ++++++++++++++++++++++ 3 files changed, 103 insertions(+), 15 deletions(-) diff --git a/docs/testing.md b/docs/testing.md index 2606645..7ed37a2 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -167,6 +167,39 @@ rather than per-PR because of its cost, and not because it is optional. A claim in the documentation with only a suite that nobody runs behind it is how coverage becomes stale without notice. +## Extension upgrade + +`PGC_RUN_UPGRADE=1` also runs `test/extension_upgrade.sh`, once, against the first +major in the list. It covers a different upgrade from the one above: not a new +PostgreSQL major, but a new build of pgColumnar on the same one. + +Every function the extension installs records the name of a C symbol. When a build +changes those names, the recorded names stop resolving, and the extension is inert +until `ALTER EXTENSION pgcolumnar UPDATE` runs. A build cannot see that, and neither can a suite that +creates the extension from scratch. That is why it needs its own gate. + +The suite installs the previous release and creates a columnar table with rows in it. +It then installs the tree under test over the top and runs the update. Reads, writes, +table creation and a maintenance call must all still work. + +It also asserts that every C function owned by the extension has a link name inside the +`pgcolumnar` namespace. That catches the next rename as well as the last one. + +It builds the old version, so it needs to be told where to find it. Given a git ref +it builds from a throwaway clone, which needs a checkout with tags: + +```sh +git fetch --tags +test/extension_upgrade.sh /usr/local/pg18/bin/pg_config v1.0-alpha +``` + +The container loop copies the tree without `.git`, so the ref form cannot work there. +Pass a directory holding the old source instead: + +```sh +test/extension_upgrade.sh /usr/local/pg18/bin/pg_config /root/pgcolumnar-1.0-alpha +``` + ## make installcheck The conventional entry point for a PostgreSQL extension: diff --git a/test/extension_upgrade.sh b/test/extension_upgrade.sh index efb2c58..3f9ab20 100755 --- a/test/extension_upgrade.sh +++ b/test/extension_upgrade.sh @@ -17,15 +17,24 @@ # second gate beside the matrix, run explicitly. # # Usage: -# test/extension_upgrade.sh [PG_CONFIG] [OLD_REF] +# test/extension_upgrade.sh [PG_CONFIG] [OLD_REF_OR_DIR] # -# OLD_REF defaults to the previous release, and may be any ref that still has the old -# link names. The old build is made in a throwaway clone, so the working tree is never -# checked out from under the caller. +# The second argument is either a git ref or a path to an already-checked-out source +# tree. A ref defaults to the previous release and is built in a throwaway clone, so the +# working tree is never checked out from under the caller. +# +# The directory form exists because the container dev loop copies the tree WITHOUT .git +# (see docs/testing.md), so the ref form cannot work there. Point it at a second copy of +# the old source instead: +# +# test/extension_upgrade.sh /usr/local/pg18a/bin/pg_config /root/pgcolumnar-1.0-alpha +# +# Requires a real checkout with tags when the ref form is used. That is a precondition, +# not a bug, and it is reported as one. set -uo pipefail PG_CONFIG=${1:-pg_config} -OLD_REF=${2:-v1.0-alpha} +OLD_SRC=${2:-v1.0-alpha} SRCDIR=$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd) # The cluster binds a port, so it must come from the band portlib.sh carves BELOW the @@ -54,19 +63,34 @@ trap cleanup EXIT runpg () { runuser -u postgres -- "$@"; } q () { runpg "$BINDIR/psql" -h /tmp -p "$PORT" -d extupg -X -Atc "$1" 2>&1; } -echo "== extension_upgrade: PG$PGMAJ, old ref $OLD_REF" +echo "== extension_upgrade: PG$PGMAJ, old $OLD_SRC" # ---- 1. build and install the old extension, from a throwaway clone ------------------ -# A missing ref must fail, not skip. This gate is invoked deliberately, and a skip that -# exits 0 would let it go inert the moment someone clones without tags. That is the same -# shape as the bug it exists to catch: everything green, nothing checked. -if ! git -C "$SRCDIR" rev-parse --verify -q "$OLD_REF^{commit}" >/dev/null 2>&1; then - echo " FAIL $OLD_REF is not present. Fetch tags, or pass an explicit ref:" - echo " git fetch --tags && test/extension_upgrade.sh $PG_CONFIG " - exit 1 +# Directory form first: a path to an already-checked-out old tree needs no git at all. +if [ -d "$OLD_SRC" ]; then + [ -f "$OLD_SRC/Makefile" ] || { echo " FAIL $OLD_SRC has no Makefile"; exit 1; } + cp -a "$OLD_SRC" "$TMP/old" || { echo "FATAL: could not copy $OLD_SRC"; exit 1; } + echo " old source: directory $OLD_SRC" +else + # A missing ref must fail, not skip. This gate is invoked deliberately, and a skip + # that exits 0 would let it go inert the moment someone clones without tags. That is + # the same shape as the bug it exists to catch: everything green, nothing checked. + if [ ! -d "$SRCDIR/.git" ]; then + echo " FAIL $SRCDIR is not a git checkout, so the ref form cannot work here." + echo " The container loop copies the tree without .git. Pass a directory:" + echo " test/extension_upgrade.sh $PG_CONFIG /path/to/old/source" + exit 1 + fi + if ! git -C "$SRCDIR" rev-parse --verify -q "$OLD_SRC^{commit}" >/dev/null 2>&1; then + echo " FAIL $OLD_SRC is not present. Fetch tags, or pass an explicit ref or dir:" + echo " git fetch --tags && test/extension_upgrade.sh $PG_CONFIG " + exit 1 + fi + git clone -q --shared "$SRCDIR" "$TMP/old" || { echo "FATAL: clone failed"; exit 1; } + git -C "$TMP/old" checkout -q --detach "$OLD_SRC" \ + || { echo "FATAL: checkout $OLD_SRC failed"; exit 1; } + echo " old source: ref $OLD_SRC" fi -git clone -q --shared "$SRCDIR" "$TMP/old" || { echo "FATAL: clone failed"; exit 1; } -git -C "$TMP/old" checkout -q --detach "$OLD_REF" || { echo "FATAL: checkout $OLD_REF failed"; exit 1; } make -C "$TMP/old" PG_CONFIG="$PG_CONFIG" clean >/dev/null 2>&1 make -C "$TMP/old" PG_CONFIG="$PG_CONFIG" -j"$(nproc)" >"$TMP/build_old.log" 2>&1 \ || { echo "FAIL old build"; tail -20 "$TMP/build_old.log"; exit 1; } diff --git a/test/run_all_versions.sh b/test/run_all_versions.sh index a4fea03..b52fb6c 100755 --- a/test/run_all_versions.sh +++ b/test/run_all_versions.sh @@ -528,6 +528,37 @@ if [ "${PGC_RUN_UPGRADE:-0}" = 1 ]; then SUMMARY+=("FAIL upgrade (no runnable pair)") overall=1 fi + + # Extension upgrade, on the same opt-in switch and for the same reason. + # test/extension_upgrade.sh had pg_upgrade's exemption from the registration + # check without pg_upgrade's invocation, so nothing ran it (#396). A guard + # nobody runs is the failure mode #257 existed to close, and it matters more + # here: the break it catches is invisible until a user upgrades. + # + # One major is enough, so this runs once against the first config rather than + # per pair. It builds the previous release from a throwaway clone, so it needs + # a checkout with tags. + _ex="${CONFIGS[0]}" + if [ -x "$_ex" ]; then + _exmaj="$("$_ex" --version | sed -E 's/^[^0-9]*([0-9]+).*/\1/')" + _exlog="$(mktemp "/tmp/pgcolumnar-extupgrade-${_exmaj}.XXXXXX.log")" + if bash "$SRCDIR/test/extension_upgrade.sh" "$_ex" >"$_exlog" 2>&1; then + echo " PASS extension_upgrade PG$_exmaj" + SUMMARY+=("PASS extension_upgrade PG$_exmaj") + rm -f "$_exlog" + else + echo " FAIL extension_upgrade PG$_exmaj" + grep -E '^\s*FAIL' "$_exlog" | sed 's/^/ >> /' + tail -30 "$_exlog" | sed 's/^/ /' + echo " full log: $_exlog" + SUMMARY+=("FAIL extension_upgrade PG$_exmaj") + overall=1 + fi + else + echo " FAIL PGC_RUN_UPGRADE=1 but no runnable pg_config for extension_upgrade" + SUMMARY+=("FAIL extension_upgrade (no runnable pg_config)") + overall=1 + fi fi echo From 8e2469ec8ff3709d5e936ba9390c4424eb42e5a3 Mon Sep 17 00:00:00 2001 From: "Joshua (D) Drake" Date: Tue, 4 Aug 2026 16:58:21 -0600 Subject: [PATCH 2/3] Make the directory form reachable from the runner, and skip rather than lie jdatcmd ran the previous commit in the container, which is the environment #396 was about, and the gate went red there. The directory form was reachable by a person and not by the runner: line 545 passed no second argument, so the runner always took the ref form, and the documented container loop has no .git. So it created a new source of false red in the loop the docs tell people to use. This runner's own comment argues against exactly that, saying a gate that goes red for reasons unrelated to the change teaches its readers to discount red. That is #396 wearing different clothes. The first time the guard did not run. This time it ran and lied. Two changes, both suggested in the review. PGC_UPGRADE_OLD_SRC names a directory holding the old source, and the runner appends it when set. That makes the directory form reachable from the gate. Absent any named old source, a tree with no .git now reports SKIP and exits 2 rather than failing. The runner reports SKIP and records SKIP in the summary, following PGC_SKIP_TIMING above it. Skip and fail are different answers and the difference is who asked for what. A caller who names an old source that cannot be honoured gets a failure. A caller who names nothing, in a tree that cannot supply one, gets a skip. The skip is loud, prints its reason, and is never reported as a pass, because a gate reporting green having run nothing is the defect this suite was written for. --- docs/testing.md | 12 ++++++++++++ test/extension_upgrade.sh | 32 ++++++++++++++++++++++++++------ test/run_all_versions.sh | 17 ++++++++++++++++- 3 files changed, 54 insertions(+), 7 deletions(-) diff --git a/docs/testing.md b/docs/testing.md index 7ed37a2..a42c03d 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -200,6 +200,18 @@ Pass a directory holding the old source instead: test/extension_upgrade.sh /usr/local/pg18/bin/pg_config /root/pgcolumnar-1.0-alpha ``` +Through the runner, give it the same directory by environment: + +```sh +PGC_RUN_UPGRADE=1 PGC_UPGRADE_OLD_SRC=/root/pgcolumnar-1.0-alpha \ + test/run_all_versions.sh +``` + +Without it, in a tree that has no `.git`, the suite reports **SKIP** and the matrix +records `SKIP`. It does not report a pass, because a gate that goes green having run +nothing is the defect this suite exists to catch. It does not report a failure either. Being unable to +obtain an old build is a property of the environment, not of the code under test. + ## make installcheck The conventional entry point for a PostgreSQL extension: diff --git a/test/extension_upgrade.sh b/test/extension_upgrade.sh index 3f9ab20..63aed84 100755 --- a/test/extension_upgrade.sh +++ b/test/extension_upgrade.sh @@ -19,6 +19,9 @@ # Usage: # test/extension_upgrade.sh [PG_CONFIG] [OLD_REF_OR_DIR] # +# PGC_UPGRADE_OLD_SRC is the same thing by environment, which is how run_all_versions.sh +# supplies it in an environment with no .git. An explicit second argument wins over it. +# # The second argument is either a git ref or a path to an already-checked-out source # tree. A ref defaults to the previous release and is built in a throwaway clone, so the # working tree is never checked out from under the caller. @@ -34,7 +37,7 @@ set -uo pipefail PG_CONFIG=${1:-pg_config} -OLD_SRC=${2:-v1.0-alpha} +OLD_SRC=${2:-${PGC_UPGRADE_OLD_SRC:-v1.0-alpha}} SRCDIR=$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd) # The cluster binds a port, so it must come from the band portlib.sh carves BELOW the @@ -66,17 +69,34 @@ q () { runpg "$BINDIR/psql" -h /tmp -p "$PORT" -d extupg -X -Atc "$1" 2>&1; } echo "== extension_upgrade: PG$PGMAJ, old $OLD_SRC" # ---- 1. build and install the old extension, from a throwaway clone ------------------ -# Directory form first: a path to an already-checked-out old tree needs no git at all. +# Skip and fail are different answers, and the difference is who asked for what. +# +# named an old source that cannot be honoured -> FAIL, the caller asked for a thing +# named nothing, and the tree cannot supply one -> SKIP, the environment cannot +# +# A silent skip that exits 0 is the bug this suite exists to catch, so the skip is loud, +# reports SKIP rather than PASS, and exits 2 so the runner can tell the two apart. +EXPLICIT=0 +{ [ "$#" -ge 2 ] || [ -n "${PGC_UPGRADE_OLD_SRC:-}" ]; } && EXPLICIT=1 + +if [ "$EXPLICIT" = 0 ] && [ ! -d "$SRCDIR/.git" ]; then + echo " SKIP $SRCDIR is not a git checkout, so the default ref $OLD_SRC cannot be built." + echo " The container loop copies the tree without .git. Supply the old source:" + echo " PGC_UPGRADE_OLD_SRC=/path/to/old/source, or pass it as the second argument." + echo "== extension_upgrade: SKIP" + exit 2 +fi + +# Directory form: a path to an already-checked-out old tree needs no git at all. if [ -d "$OLD_SRC" ]; then [ -f "$OLD_SRC/Makefile" ] || { echo " FAIL $OLD_SRC has no Makefile"; exit 1; } cp -a "$OLD_SRC" "$TMP/old" || { echo "FATAL: could not copy $OLD_SRC"; exit 1; } echo " old source: directory $OLD_SRC" else - # A missing ref must fail, not skip. This gate is invoked deliberately, and a skip - # that exits 0 would let it go inert the moment someone clones without tags. That is - # the same shape as the bug it exists to catch: everything green, nothing checked. + # Only reachable when an old source was named explicitly, so this is a failure and + # not a skip: the caller asked for something this tree cannot provide. if [ ! -d "$SRCDIR/.git" ]; then - echo " FAIL $SRCDIR is not a git checkout, so the ref form cannot work here." + echo " FAIL $SRCDIR is not a git checkout, so the ref $OLD_SRC cannot be built." echo " The container loop copies the tree without .git. Pass a directory:" echo " test/extension_upgrade.sh $PG_CONFIG /path/to/old/source" exit 1 diff --git a/test/run_all_versions.sh b/test/run_all_versions.sh index b52fb6c..5a5365b 100755 --- a/test/run_all_versions.sh +++ b/test/run_all_versions.sh @@ -542,10 +542,25 @@ if [ "${PGC_RUN_UPGRADE:-0}" = 1 ]; then if [ -x "$_ex" ]; then _exmaj="$("$_ex" --version | sed -E 's/^[^0-9]*([0-9]+).*/\1/')" _exlog="$(mktemp "/tmp/pgcolumnar-extupgrade-${_exmaj}.XXXXXX.log")" - if bash "$SRCDIR/test/extension_upgrade.sh" "$_ex" >"$_exlog" 2>&1; then + # The suite builds a previous release, so it has to be told where to find one. + # PGC_UPGRADE_OLD_SRC carries a directory through, which is the only form that + # works where the tree has no .git, and the documented container loop is exactly + # that. Without it the suite falls back to a ref and cannot build one there. + bash "$SRCDIR/test/extension_upgrade.sh" "$_ex" \ + ${PGC_UPGRADE_OLD_SRC:+"$PGC_UPGRADE_OLD_SRC"} >"$_exlog" 2>&1 + _exrc=$? + # Exit 2 is "the environment could not supply an old source", which is not a + # product failure. It is reported as SKIP and not as PASS, because a gate that + # reports green having run nothing is the defect this suite was written for. + if [ "$_exrc" = 0 ]; then echo " PASS extension_upgrade PG$_exmaj" SUMMARY+=("PASS extension_upgrade PG$_exmaj") rm -f "$_exlog" + elif [ "$_exrc" = 2 ]; then + echo " SKIP extension_upgrade PG$_exmaj" + grep -E '^\s*(SKIP| )' "$_exlog" | sed 's/^/ /' + SUMMARY+=("SKIP extension_upgrade PG$_exmaj") + rm -f "$_exlog" else echo " FAIL extension_upgrade PG$_exmaj" grep -E '^\s*FAIL' "$_exlog" | sed 's/^/ >> /' From 6a095c1e317115d82b156c6ac673a062735451aa Mon Sep 17 00:00:00 2001 From: "Joshua (D) Drake" Date: Tue, 4 Aug 2026 17:04:41 -0600 Subject: [PATCH 3/3] Say so when a named directory is not there Verifying the exit codes turned up a misleading message. A PGC_UPGRADE_OLD_SRC that names a path which does not exist fell through to the ref branch, so the user got an error about git checkouts and refs in answer to a question about a directory. The exit code was right and the message pointed at the wrong thing. A value containing a slash is now treated as a path, and a missing one says that, rather than being reinterpreted as a ref. Co-Authored-By: Claude Opus 5 (1M context) --- test/extension_upgrade.sh | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/test/extension_upgrade.sh b/test/extension_upgrade.sh index 63aed84..28d15e3 100755 --- a/test/extension_upgrade.sh +++ b/test/extension_upgrade.sh @@ -87,6 +87,19 @@ if [ "$EXPLICIT" = 0 ] && [ ! -d "$SRCDIR/.git" ]; then exit 2 fi +# A value that looks like a path but is not there is a wrong path, not a ref. Falling +# through to the ref branch would answer a question about directories with an error about +# git checkouts, which is the kind of message that costs someone an afternoon. +case "$OLD_SRC" in + */*) + if [ ! -d "$OLD_SRC" ]; then + echo " FAIL $OLD_SRC looks like a path and is not a directory." + echo " Give a checked-out source tree of the previous release." + exit 1 + fi + ;; +esac + # Directory form: a path to an already-checked-out old tree needs no git at all. if [ -d "$OLD_SRC" ]; then [ -f "$OLD_SRC/Makefile" ] || { echo " FAIL $OLD_SRC has no Makefile"; exit 1; }