Skip to content

Commit 409741b

Browse files
jnasbyupgradeclaude
andcommitted
pg-tle-test: extend to the 0.9.6 -> current update path via DESTDIR sandboxing
pgxntool 2.3.0's installcheck-ordering fix (pgxntool#83) made `installcheck` (and so `make test`) unconditionally depend on `install`, which writes a real .control file to disk - defeating the whole point of a pg_tle-only-deployment proof, and with no upstream fix yet (pgxntool#90). bin/test_existing's run_suite gains a TEST_EXISTING_DEPLOY=pgtle mode that instead redirects `make test`'s install step through a scratch DESTDIR (harmless here since a pg_tle-deployed database never needs those files), pre-seeding a stub pgtap.control at the DESTDIR-prefixed path pgxntool's `pgtap` prerequisite checks for - otherwise its DESTDIR-blind recipe (`pgxn install pgtap --sudo`) would trigger a real, unwanted reinstall against the scratch dir. Also fixes test/install/load.sql's existing-mode assertion, which cross-checks the installed version against pg_available_extensions.default_version - that view never sees pg_tle registrations (it only reads .control files off disk), so it would return NULL under a pg_tle deployment even though CREATE EXTENSION resolves correctly through pg_tle's own catalog. A new count_nulls.test_existing_deploy GUC (driven by the same TEST_EXISTING_DEPLOY var, Makefile-exported like TEST_SCHEMA/TEST_LOAD_SOURCE) selects pgtle.available_extensions() instead when deploy=pgtle. The pg-tle-test CI job now installs count_nulls at 0.9.6 into a second scratch database (inheriting the job's template1 pg_tle registration), plants + proves the dependency guard, ALTER EXTENSION UPDATEs to current, then runs the real pgTAP suite against it via the new mode - with bin/assert_fs_clean verify checked after every one of those steps, not just at the end, since that's what actually proves the DESTDIR sandboxing worked. Verified locally (no real pg_tle in this container): confirmed via mtime comparison that the real count_nulls.control and pgtap.control are untouched by a full prepare-old/update/run-suite(pgtle) cycle, that the scratch DESTDIR is cleaned up on both success and a forced regression failure (EXIT trap surviving `set -e`), and exercised the count_nulls.test_existing_deploy branch in load.sql against a stubbed pgtle.available_extensions() function. The genuine pg_tle-registered version needs a real CI run to confirm end to end. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent 89db338 commit 409741b

4 files changed

Lines changed: 216 additions & 14 deletions

File tree

.github/workflows/ci.yml

Lines changed: 49 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,8 @@
2929
# - by far the most expensive job here,
3030
# installing two full PostgreSQL majors and
3131
# running the real pg_upgrade binary per leg).
32-
# pg-tle-test -- pg_tle DEPLOYMENT: fresh install registered
32+
# pg-tle-test -- pg_tle DEPLOYMENT: fresh install AND the
33+
# 0.9.6 -> current update path, both registered
3334
# through AWS pg_tle's database-backed catalog
3435
# instead of a filesystem .control file.
3536
#
@@ -376,15 +377,18 @@ jobs:
376377
bin/compare_fresh_vs_update "" 0.9.6 count_nulls_upgrade_none
377378
bin/compare_fresh_vs_update Quoted 0.9.6 count_nulls_upgrade_quoted
378379
379-
# Fresh-install smoke test only, deliberately - NOT extended to the
380-
# update path. pgxntool 2.3.0's fix for installcheck's ordering bug
380+
# Covers both a fresh install AND the 0.9.6 -> current update path, both
381+
# purely via pg_tle. pgxntool 2.3.0's fix for installcheck's ordering bug
381382
# (Postgres-Extensions/pgxntool#83) made `installcheck` (and so `make
382383
# test`) unconditionally depend on `install`, which writes a real
383384
# .control file to disk - defeating the entire point of proving a pg_tle
384-
# deployment never touches the filesystem. There's currently no way to
385-
# invoke the real pgTAP suite without that happening first; filed as
386-
# Postgres-Extensions/pgxntool#90. Revisit extending this job to the
387-
# update path once that's resolved.
385+
# deployment never touches the filesystem. There's still no upstream fix
386+
# for that (Postgres-Extensions/pgxntool#90, open) that would let
387+
# bin/test_existing's real pgTAP suite run without it, so the update-path
388+
# steps below use TEST_EXISTING_DEPLOY=pgtle (see bin/test_existing),
389+
# which instead sandboxes `make test`'s install step behind a scratch
390+
# DESTDIR - harmless here since a pg_tle-deployed database never needs
391+
# those files.
388392
pg-tle-test:
389393
needs: [changes]
390394
if: needs.changes.outputs.docs_only != 'true'
@@ -506,6 +510,44 @@ jobs:
506510
fi
507511
- name: Verify no stray extension control files after the fresh-install smoke test
508512
run: bin/assert_fs_clean verify ${{ matrix.pg }} /tmp/control_baseline.txt pg_tle.control
513+
- name: Install count_nulls at 0.9.6, purely via pg_tle (update-path prep)
514+
# A SECOND, separate scratch database, created after the template1
515+
# registration above so it inherits both registrations too (same
516+
# reasoning as count_nulls_smoke). prepare-old creates the database,
517+
# CREATE EXTENSIONs at 0.9.6 (pure SQL - resolves through pg_tle's
518+
# catalog, no `make install` call, which would defeat the whole
519+
# point), then plants + proves the dependency guard so a stray
520+
# CASCADE drop anywhere below can't silently turn the eventual
521+
# existing-mode run into a fresh install instead.
522+
run: |
523+
test ! -e /usr/share/postgresql/${{ matrix.pg }}/extension/count_nulls.control
524+
bin/test_existing prepare-old count_nulls_pgtle_update "" 0.9.6
525+
- name: Verify no stray extension control files after installing 0.9.6 via pg_tle
526+
run: bin/assert_fs_clean verify ${{ matrix.pg }} /tmp/control_baseline.txt pg_tle.control
527+
- name: Update 0.9.6 -> current, purely via pg_tle
528+
# Pure SQL (ALTER EXTENSION ... UPDATE), no filesystem write either.
529+
run: bin/test_existing update count_nulls_pgtle_update
530+
- name: Verify no stray extension control files after the pg_tle update
531+
run: bin/assert_fs_clean verify ${{ matrix.pg }} /tmp/control_baseline.txt pg_tle.control
532+
- name: Run the real pgTAP suite against the pg_tle-updated database (existing mode)
533+
# run-suite re-proves the dependency guard, drops it, then runs the
534+
# FULL suite via --use-existing against the real pg_tle-deployed +
535+
# updated database - the same suite/expected-output as every other
536+
# leg (see test/README.md). TEST_EXISTING_DEPLOY=pgtle makes
537+
# run-suite sandbox `make test`'s otherwise-unavoidable `install`
538+
# step behind a scratch DESTDIR instead of writing to the real
539+
# extension directory (see bin/test_existing's TEST_EXISTING_DEPLOY
540+
# comment), and makes test/install/load.sql's existing-mode
541+
# assertion cross-check pgtle.available_extensions() instead of
542+
# pg_available_extensions (which never sees pg_tle registrations -
543+
# see the Makefile's TEST_EXISTING_DEPLOY comment).
544+
run: TEST_EXISTING_DEPLOY=pgtle bin/test_existing run-suite count_nulls_pgtle_update ""
545+
- name: Verify no stray extension control files after the pgTAP suite
546+
# THE step that actually proves the DESTDIR sandboxing worked: the
547+
# real extension directory must still be clean after `make test`
548+
# ran (with its otherwise-unavoidable `install` step) sandboxed
549+
# behind a scratch DESTDIR.
550+
run: bin/assert_fs_clean verify ${{ matrix.pg }} /tmp/control_baseline.txt pg_tle.control
509551

510552
# A single stable check name for use as a required status check in branch
511553
# protection rules. Matrix jobs produce check names like

Makefile

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,43 @@ $(error TEST_LOAD_SOURCE must be 'fresh', 'update' or 'existing', got '$(TEST_LO
8383
endif
8484
export PGOPTIONS := $(PGOPTIONS) -c count_nulls.test_load_mode=$(TEST_LOAD_SOURCE)
8585

86+
# TEST_EXISTING_DEPLOY: in 'existing' mode (TEST_LOAD_SOURCE=existing), how
87+
# was the extension actually deployed onto the cluster before this run
88+
# started? Unlike TEST_LOAD_SOURCE/TEST_SCHEMA above, this does not select or
89+
# change any install behavior - it only tells test/install/load.sql's
90+
# existing-mode assertion where to cross-check "what does the cluster
91+
# consider count_nulls's current version" against the actually-installed
92+
# extversion:
93+
# - filesystem (default): a real .control file is on disk (a real `make
94+
# install`, or a pg_upgrade'd cluster carrying one over) - cross-check
95+
# against pg_available_extensions.default_version, which reads .control
96+
# files directly off disk.
97+
# - pgtle: count_nulls was registered purely through pg_tle's
98+
# database-backed catalog (see the pg-tle-test CI job), never touching
99+
# the filesystem. pg_available_extensions does NOT see pg_tle
100+
# registrations at all - it only ever reads .control files off disk - so
101+
# its default_version comes back NULL for a pg_tle-only extension even
102+
# though a version-less CREATE EXTENSION resolves correctly through
103+
# pg_tle. pg_tle ships its own separate, non-integrated analog instead:
104+
# pgtle.available_extensions(), a C function whose own doc comment in
105+
# pg_tle's tleextension.c says "The system view pg_available_extensions
106+
# provides a user interface to this SRF" - i.e. pg_tle's SRF is modeled
107+
# on pg_available_extensions, but pg_tle never hooks or populates the
108+
# real view itself. In this mode, cross-check against pg_tle's SRF
109+
# instead.
110+
#
111+
# Deliberately the SAME env var name bin/test_existing already reads (to
112+
# decide whether to sandbox `make test`'s install step via a scratch
113+
# DESTDIR) - it's already exported to any `make` invocation bin/test_existing
114+
# spawns as a child process (and explicitly passed through on run_suite's
115+
# `make test` command line too), so no extra plumbing is needed to get it
116+
# here.
117+
TEST_EXISTING_DEPLOY ?= filesystem
118+
ifeq ($(filter $(TEST_EXISTING_DEPLOY),filesystem pgtle),)
119+
$(error TEST_EXISTING_DEPLOY must be 'filesystem' or 'pgtle', got '$(TEST_EXISTING_DEPLOY)')
120+
endif
121+
export PGOPTIONS := $(PGOPTIONS) -c count_nulls.test_existing_deploy=$(TEST_EXISTING_DEPLOY)
122+
86123
# Convenience wrapper: `make test-update` == `make test TEST_LOAD_SOURCE=update`.
87124
# Must recurse (a fresh $(MAKE)) rather than depend on `test`, so the
88125
# parse-time TEST_LOAD_SOURCE conditional above re-evaluates with update set.

bin/test_existing

Lines changed: 93 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,14 @@
2222
# code creates, so pg_upgrade needs to run against already-current objects,
2323
# not ones still frozen at the old INSTALL_VERSION.
2424
#
25+
# The pg-tle-test CI job's update-path leg reuses the exact same three
26+
# subcommands unmodified, in the same prepare-old -> update -> run-suite
27+
# order (a pg_tle-registered database needs no real pg_upgrade or
28+
# filesystem install between prepare-old and update - both are pure SQL,
29+
# so there's no binary-upgrade step to reorder around there) - see
30+
# TEST_EXISTING_DEPLOY below for the one piece of plumbing that job
31+
# needed.
32+
#
2533
# Not CI-only: a developer can run any subcommand locally against a scratch
2634
# database. Modeled on Postgres-Extensions/cat_tools's bin/test_existing.
2735
# Two differences from that script: count_nulls ships no
@@ -53,6 +61,37 @@
5361
# actively PROVE that (see bin/test_existing.sql/assert_guard.sql): if the
5462
# drop unexpectedly succeeds, this script fails CI rather than silently
5563
# passing.
64+
#
65+
# TEST_EXISTING_DEPLOY (optional environment variable, read by run_suite):
66+
# unset/filesystem (default): count_nulls was deployed the normal way (a
67+
# real `make install`). run_suite calls `make test` exactly as before.
68+
# pgtle: count_nulls was deployed purely through pg_tle's database-backed
69+
# catalog (see the pg-tle-test CI job), never `make install`ed onto the
70+
# filesystem. pgxntool's `test` target unconditionally depends on
71+
# `install` (base.mk: `TEST_DEPS += install installcheck`, and
72+
# `installcheck: install` - the pgxntool#83/2.3.0 ordering fix), so
73+
# naively calling `make test` here would silently write a real
74+
# count_nulls.control to disk - which would then shadow the pg_tle
75+
# registration on any FUTURE version-less CREATE EXTENSION in the same
76+
# cluster, defeating the entire point of proving a pg_tle-only
77+
# deployment, without ever raising an error.
78+
#
79+
# In this mode run_suite instead passes DESTDIR=<scratch dir> to `make
80+
# test`: PGXS's own `install` target prefixes every path with
81+
# $(DESTDIR), so it still "runs" (satisfying Make's dependency graph)
82+
# but writes harmlessly to a throwaway directory instead of the real
83+
# extension directory - fine here specifically because the pg_tle-
84+
# deployed database under test never needs those files (it's already
85+
# live via pg_tle's own catalog). One more prerequisite needs help:
86+
# `installcheck: pgtap` has a DESTDIR-aware prerequisite check
87+
# ($(DESTDIR)$(datadir)/extension/pgtap.control) but a DESTDIR-blind
88+
# recipe (`pgxn install pgtap --sudo`) - against an empty scratch
89+
# DESTDIR, Make would consider pgtap "missing" and re-run that recipe
90+
# for real (network + sudo), even though pgtap is already genuinely
91+
# installed from an earlier CI step. seed_pgtap_stub() pre-seeds a
92+
# zero-byte stub at that exact scratch-prefixed path so Make considers
93+
# the prerequisite already satisfied and never invokes the real recipe.
94+
# See Postgres-Extensions/pgxntool#90 (open, no upstream fix yet).
5695
set -euo pipefail
5796

5897
# Run from the repository root (where `make` works and test paths resolve),
@@ -155,6 +194,30 @@ create_extension_in_schema() {
155194
psql_do "$db" -c "${sql}CREATE EXTENSION count_nulls VERSION '$version'"
156195
}
157196

197+
# ---------------------------------------------------------------------------
198+
# pg_tle DESTDIR sandbox helpers (see TEST_EXISTING_DEPLOY in the file header)
199+
# ---------------------------------------------------------------------------
200+
201+
# Seed a stub pgtap.control at the exact DESTDIR-prefixed path pgxntool's
202+
# `pgtap` target's prerequisite check looks for
203+
# ($(DESTDIR)$(datadir)/extension/pgtap.control), so Make considers that
204+
# prerequisite already satisfied and never invokes the real recipe (`pgxn
205+
# install pgtap --sudo`) against a scratch DESTDIR that can never contain a
206+
# real install. Existence is all Make checks here - pgtap has no other
207+
# prerequisites to compare timestamps against - so a zero-byte file is
208+
# enough.
209+
seed_pgtap_stub() {
210+
local destdir=$1 datadir control_path
211+
datadir=$(make -s print-datadir 2>/dev/null | sed -n 's/.*set to "\(.*\)"$/\1/p')
212+
if [ -z "$datadir" ]; then
213+
echo "FAIL: could not resolve datadir (make -s print-datadir)" >&2
214+
exit 1
215+
fi
216+
control_path="${destdir}${datadir}/extension/pgtap.control"
217+
mkdir -p "$(dirname "$control_path")"
218+
: > "$control_path"
219+
}
220+
158221
# ---------------------------------------------------------------------------
159222
# Subcommand implementations
160223
# ---------------------------------------------------------------------------
@@ -179,16 +242,44 @@ prepare_old() {
179242
# then runs the suite via --use-existing so pg_regress does NOT drop/recreate
180243
# the database.
181244
run_suite() {
182-
local db=$1 schema=$2
245+
local db=$1 schema=$2 deploy=${TEST_EXISTING_DEPLOY:-filesystem}
246+
case "$deploy" in
247+
filesystem|pgtle) ;;
248+
*)
249+
echo "FAIL: TEST_EXISTING_DEPLOY must be 'filesystem' or 'pgtle', got '$deploy'" >&2
250+
exit 1
251+
;;
252+
esac
253+
183254
assert_version "$db" current
184255
assert_drop_blocked "$db"
185256
drop_guard "$db"
257+
258+
# See TEST_EXISTING_DEPLOY in the file header. Only 'pgtle' changes
259+
# anything here; a scratch DESTDIR is created and baked into a trap so it
260+
# is cleaned up regardless of how this function/script exits (a RETURN
261+
# trap would NOT fire under `set -e` if `make test` below reports a
262+
# regression - EXIT does, for any exit reason).
263+
local destdir_opt=()
264+
if [ "$deploy" = pgtle ]; then
265+
local scratch
266+
scratch=$(mktemp -d)
267+
trap "rm -rf '$scratch'" EXIT
268+
seed_pgtap_stub "$scratch"
269+
destdir_opt=(DESTDIR="$scratch")
270+
fi
271+
186272
# In existing mode pg_regress runs against $db via --use-existing and must
187273
# NOT create/drop its own database. `make test` (not just `make
188274
# verify-results`) is a real gate as of pgxntool 2.3.0 - it now exits
189275
# non-zero on regression failures instead of always exiting 0 regardless
190276
# of pg_regress's result (see this repo's pgxntool 2.3.0 bump).
191-
make test TEST_LOAD_SOURCE=existing TEST_SCHEMA="$schema" CONTRIB_TESTDB="$db" EXTRA_REGRESS_OPTS=--use-existing
277+
# TEST_EXISTING_DEPLOY is passed through explicitly (not left to
278+
# environment inheritance) so test/install/load.sql's existing-mode
279+
# assertion (via the count_nulls.test_existing_deploy GUC - see the
280+
# Makefile) picks the right source regardless of how this function is
281+
# invoked.
282+
make test TEST_LOAD_SOURCE=existing TEST_SCHEMA="$schema" CONTRIB_TESTDB="$db" EXTRA_REGRESS_OPTS=--use-existing TEST_EXISTING_DEPLOY="$deploy" "${destdir_opt[@]}"
192283
}
193284

194285
usage() {

test/install/load.sql

Lines changed: 37 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -67,20 +67,52 @@ SELECT :'count_nulls_test_load_mode' = 'existing' AS count_nulls_existing_mode
6767
\if :count_nulls_existing_mode
6868
/*
6969
* Already installed by something external to this pg_regress invocation
70-
* (a real pg_upgrade run - see the pg-upgrade-test CI job). Only assert
71-
* it's present and at the current version; do NOT drop/create/update it -
72-
* the whole point of this mode is testing the REAL migrated objects.
70+
* (a real pg_upgrade run, or a pg_tle registration - see the
71+
* pg-upgrade-test / pg-tle-test CI jobs). Only assert it's present and at
72+
* the current version; do NOT drop/create/update it - the whole point of
73+
* this mode is testing the REAL migrated/deployed objects.
74+
*
75+
* The "current version" half of that assertion (v_default below) has two
76+
* sources depending on count_nulls.test_existing_deploy (see the
77+
* TEST_EXISTING_DEPLOY comment in the Makefile):
78+
* - filesystem (default): pg_available_extensions.default_version, read
79+
* straight from a real .control file on disk.
80+
* - pgtle: count_nulls was registered purely through pg_tle's
81+
* database-backed catalog, never touching the filesystem.
82+
* pg_available_extensions does NOT see pg_tle registrations at all - it
83+
* only ever reads .control files off disk - so it comes back NULL here
84+
* even though CREATE EXTENSION correctly resolves the default version
85+
* through pg_tle. pg_tle ships its own separate, non-integrated analog
86+
* for this: pgtle.available_extensions() (see pg_tle's tleextension.c,
87+
* which documents pg_available_extensions as merely modeled on this
88+
* SRF, not backed by it). Use that instead when running under pg_tle,
89+
* rather than weakening the check for the filesystem case.
7390
*/
7491
DO $$
7592
DECLARE
7693
v_installed text := (SELECT extversion FROM pg_extension WHERE extname = 'count_nulls');
77-
v_default text := (SELECT default_version FROM pg_available_extensions WHERE name = 'count_nulls');
94+
v_deploy text := current_setting('count_nulls.test_existing_deploy');
95+
v_default text;
7896
BEGIN
7997
IF v_installed IS NULL THEN
8098
RAISE EXCEPTION 'count_nulls.test_load_mode=existing but count_nulls is not installed';
8199
END IF;
100+
101+
IF v_deploy = 'pgtle' THEN
102+
SELECT default_version INTO v_default
103+
FROM pgtle.available_extensions() WHERE name = 'count_nulls';
104+
ELSIF v_deploy = 'filesystem' THEN
105+
SELECT default_version INTO v_default
106+
FROM pg_available_extensions WHERE name = 'count_nulls';
107+
ELSE
108+
RAISE EXCEPTION
109+
'count_nulls.test_existing_deploy must be ''filesystem'' or ''pgtle'', got ''%'''
110+
, v_deploy
111+
;
112+
END IF;
113+
82114
IF v_installed IS DISTINCT FROM v_default THEN
83-
RAISE EXCEPTION 'count_nulls installed at % but default_version is %', v_installed, v_default;
115+
RAISE EXCEPTION 'count_nulls installed at % but default_version (deploy=%) is %', v_installed, v_deploy, v_default;
84116
END IF;
85117
END
86118
$$;

0 commit comments

Comments
 (0)