Skip to content

Commit f058f2b

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 1d2c5fb commit f058f2b

4 files changed

Lines changed: 216 additions & 17 deletions

File tree

.github/workflows/ci.yml

Lines changed: 49 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,8 @@
2020
# job here, installing two full PostgreSQL
2121
# majors and running the real pg_upgrade
2222
# binary per leg).
23-
# pg-tle-test -- pg_tle DEPLOYMENT: fresh install registered
23+
# pg-tle-test -- pg_tle DEPLOYMENT: fresh install AND the
24+
# 0.9.6 -> current update path, both registered
2425
# through AWS pg_tle's database-backed catalog
2526
# instead of a filesystem .control file.
2627
#
@@ -352,15 +353,18 @@ jobs:
352353
bin/compare_fresh_vs_update "" 0.9.6 count_nulls_upgrade_none
353354
bin/compare_fresh_vs_update Quoted 0.9.6 count_nulls_upgrade_quoted
354355
355-
# Fresh-install smoke test only, deliberately - NOT extended to the
356-
# update path. pgxntool 2.3.0's fix for installcheck's ordering bug
356+
# Covers both a fresh install AND the 0.9.6 -> current update path, both
357+
# purely via pg_tle. pgxntool 2.3.0's fix for installcheck's ordering bug
357358
# (Postgres-Extensions/pgxntool#83) made `installcheck` (and so `make
358359
# test`) unconditionally depend on `install`, which writes a real
359360
# .control file to disk - defeating the entire point of proving a pg_tle
360-
# deployment never touches the filesystem. There's currently no way to
361-
# invoke the real pgTAP suite without that happening first; filed as
362-
# Postgres-Extensions/pgxntool#90. Revisit extending this job to the
363-
# update path once that's resolved.
361+
# deployment never touches the filesystem. There's still no upstream fix
362+
# for that (Postgres-Extensions/pgxntool#90, open) that would let
363+
# bin/test_existing's real pgTAP suite run without it, so the update-path
364+
# steps below use TEST_EXISTING_DEPLOY=pgtle (see bin/test_existing),
365+
# which instead sandboxes `make test`'s install step behind a scratch
366+
# DESTDIR - harmless here since a pg_tle-deployed database never needs
367+
# those files.
364368
pg-tle-test:
365369
needs: [changes]
366370
if: needs.changes.outputs.docs_only != 'true'
@@ -481,6 +485,44 @@ jobs:
481485
fi
482486
- name: Verify no stray extension control files after the fresh-install smoke test
483487
run: bin/assert_fs_clean verify ${{ matrix.pg }} /tmp/control_baseline.txt pg_tle.control
488+
- name: Install count_nulls at 0.9.6, purely via pg_tle (update-path prep)
489+
# A SECOND, separate scratch database, created after the template1
490+
# registration above so it inherits both registrations too (same
491+
# reasoning as count_nulls_smoke). prepare-old creates the database,
492+
# CREATE EXTENSIONs at 0.9.6 (pure SQL - resolves through pg_tle's
493+
# catalog, no `make install` call, which would defeat the whole
494+
# point), then plants + proves the dependency guard so a stray
495+
# CASCADE drop anywhere below can't silently turn the eventual
496+
# existing-mode run into a fresh install instead.
497+
run: |
498+
test ! -e /usr/share/postgresql/${{ matrix.pg }}/extension/count_nulls.control
499+
bin/test_existing prepare-old count_nulls_pgtle_update "" 0.9.6
500+
- name: Verify no stray extension control files after installing 0.9.6 via pg_tle
501+
run: bin/assert_fs_clean verify ${{ matrix.pg }} /tmp/control_baseline.txt pg_tle.control
502+
- name: Update 0.9.6 -> current, purely via pg_tle
503+
# Pure SQL (ALTER EXTENSION ... UPDATE), no filesystem write either.
504+
run: bin/test_existing update count_nulls_pgtle_update
505+
- name: Verify no stray extension control files after the pg_tle update
506+
run: bin/assert_fs_clean verify ${{ matrix.pg }} /tmp/control_baseline.txt pg_tle.control
507+
- name: Run the real pgTAP suite against the pg_tle-updated database (existing mode)
508+
# run-suite re-proves the dependency guard, drops it, then runs the
509+
# FULL suite via --use-existing against the real pg_tle-deployed +
510+
# updated database - the same suite/expected-output as every other
511+
# leg (see test/README.md). TEST_EXISTING_DEPLOY=pgtle makes
512+
# run-suite sandbox `make test`'s otherwise-unavoidable `install`
513+
# step behind a scratch DESTDIR instead of writing to the real
514+
# extension directory (see bin/test_existing's TEST_EXISTING_DEPLOY
515+
# comment), and makes test/install/load.sql's existing-mode
516+
# assertion cross-check pgtle.available_extensions() instead of
517+
# pg_available_extensions (which never sees pg_tle registrations -
518+
# see the Makefile's TEST_EXISTING_DEPLOY comment).
519+
run: TEST_EXISTING_DEPLOY=pgtle bin/test_existing run-suite count_nulls_pgtle_update ""
520+
- name: Verify no stray extension control files after the pgTAP suite
521+
# THE step that actually proves the DESTDIR sandboxing worked: the
522+
# real extension directory must still be clean after `make test`
523+
# ran (with its otherwise-unavoidable `install` step) sandboxed
524+
# behind a scratch DESTDIR.
525+
run: bin/assert_fs_clean verify ${{ matrix.pg }} /tmp/control_baseline.txt pg_tle.control
484526

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

Makefile

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

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

bin/test_existing

Lines changed: 93 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,14 @@
1515
# prepare-old (install + plant guard) -> [real pg_upgrade binary, in CI] ->
1616
# update (ALTER EXTENSION UPDATE) -> run-suite (assert + run existing-mode)
1717
#
18-
# so it lives here once instead of being duplicated as inline YAML. Not
19-
# CI-only: a developer can run any subcommand locally against a scratch
20-
# database. Modeled on Postgres-Extensions/cat_tools's bin/test_existing.
18+
# so it lives here once instead of being duplicated as inline YAML. The
19+
# pg-tle-test CI job's update-path leg reuses the exact same three
20+
# subcommands unmodified (a pg_tle-registered database needs no real
21+
# pg_upgrade or filesystem install between prepare-old and update - both are
22+
# pure SQL) - see TEST_EXISTING_DEPLOY below for the one piece of plumbing
23+
# that job needed. Not CI-only: a developer can run any subcommand locally
24+
# against a scratch database. Modeled on Postgres-Extensions/cat_tools's
25+
# bin/test_existing.
2126
# Two differences from that script: count_nulls ships no
2227
# SELECT-*-over-catalog views, so it has no known pg_upgrade-unsafe old
2328
# version to bridge past before running pg_upgrade; and its own suite has a
@@ -47,6 +52,37 @@
4752
# actively PROVE that (see bin/test_existing.sql/assert_guard.sql): if the
4853
# drop unexpectedly succeeds, this script fails CI rather than silently
4954
# passing.
55+
#
56+
# TEST_EXISTING_DEPLOY (optional environment variable, read by run_suite):
57+
# unset/filesystem (default): count_nulls was deployed the normal way (a
58+
# real `make install`). run_suite calls `make test` exactly as before.
59+
# pgtle: count_nulls was deployed purely through pg_tle's database-backed
60+
# catalog (see the pg-tle-test CI job), never `make install`ed onto the
61+
# filesystem. pgxntool's `test` target unconditionally depends on
62+
# `install` (base.mk: `TEST_DEPS += install installcheck`, and
63+
# `installcheck: install` - the pgxntool#83/2.3.0 ordering fix), so
64+
# naively calling `make test` here would silently write a real
65+
# count_nulls.control to disk - which would then shadow the pg_tle
66+
# registration on any FUTURE version-less CREATE EXTENSION in the same
67+
# cluster, defeating the entire point of proving a pg_tle-only
68+
# deployment, without ever raising an error.
69+
#
70+
# In this mode run_suite instead passes DESTDIR=<scratch dir> to `make
71+
# test`: PGXS's own `install` target prefixes every path with
72+
# $(DESTDIR), so it still "runs" (satisfying Make's dependency graph)
73+
# but writes harmlessly to a throwaway directory instead of the real
74+
# extension directory - fine here specifically because the pg_tle-
75+
# deployed database under test never needs those files (it's already
76+
# live via pg_tle's own catalog). One more prerequisite needs help:
77+
# `installcheck: pgtap` has a DESTDIR-aware prerequisite check
78+
# ($(DESTDIR)$(datadir)/extension/pgtap.control) but a DESTDIR-blind
79+
# recipe (`pgxn install pgtap --sudo`) - against an empty scratch
80+
# DESTDIR, Make would consider pgtap "missing" and re-run that recipe
81+
# for real (network + sudo), even though pgtap is already genuinely
82+
# installed from an earlier CI step. seed_pgtap_stub() pre-seeds a
83+
# zero-byte stub at that exact scratch-prefixed path so Make considers
84+
# the prerequisite already satisfied and never invokes the real recipe.
85+
# See Postgres-Extensions/pgxntool#90 (open, no upstream fix yet).
5086
set -euo pipefail
5187

5288
# Run from the repository root (where `make` works and test paths resolve),
@@ -149,6 +185,30 @@ create_extension_in_schema() {
149185
psql_do "$db" -c "${sql}CREATE EXTENSION count_nulls VERSION '$version'"
150186
}
151187

188+
# ---------------------------------------------------------------------------
189+
# pg_tle DESTDIR sandbox helpers (see TEST_EXISTING_DEPLOY in the file header)
190+
# ---------------------------------------------------------------------------
191+
192+
# Seed a stub pgtap.control at the exact DESTDIR-prefixed path pgxntool's
193+
# `pgtap` target's prerequisite check looks for
194+
# ($(DESTDIR)$(datadir)/extension/pgtap.control), so Make considers that
195+
# prerequisite already satisfied and never invokes the real recipe (`pgxn
196+
# install pgtap --sudo`) against a scratch DESTDIR that can never contain a
197+
# real install. Existence is all Make checks here - pgtap has no other
198+
# prerequisites to compare timestamps against - so a zero-byte file is
199+
# enough.
200+
seed_pgtap_stub() {
201+
local destdir=$1 datadir control_path
202+
datadir=$(make -s print-datadir 2>/dev/null | sed -n 's/.*set to "\(.*\)"$/\1/p')
203+
if [ -z "$datadir" ]; then
204+
echo "FAIL: could not resolve datadir (make -s print-datadir)" >&2
205+
exit 1
206+
fi
207+
control_path="${destdir}${datadir}/extension/pgtap.control"
208+
mkdir -p "$(dirname "$control_path")"
209+
: > "$control_path"
210+
}
211+
152212
# ---------------------------------------------------------------------------
153213
# Subcommand implementations
154214
# ---------------------------------------------------------------------------
@@ -173,16 +233,44 @@ prepare_old() {
173233
# then runs the suite via --use-existing so pg_regress does NOT drop/recreate
174234
# the database.
175235
run_suite() {
176-
local db=$1 schema=$2
236+
local db=$1 schema=$2 deploy=${TEST_EXISTING_DEPLOY:-filesystem}
237+
case "$deploy" in
238+
filesystem|pgtle) ;;
239+
*)
240+
echo "FAIL: TEST_EXISTING_DEPLOY must be 'filesystem' or 'pgtle', got '$deploy'" >&2
241+
exit 1
242+
;;
243+
esac
244+
177245
assert_version "$db" current
178246
assert_drop_blocked "$db"
179247
drop_guard "$db"
248+
249+
# See TEST_EXISTING_DEPLOY in the file header. Only 'pgtle' changes
250+
# anything here; a scratch DESTDIR is created and baked into a trap so it
251+
# is cleaned up regardless of how this function/script exits (a RETURN
252+
# trap would NOT fire under `set -e` if `make test` below reports a
253+
# regression - EXIT does, for any exit reason).
254+
local destdir_opt=()
255+
if [ "$deploy" = pgtle ]; then
256+
local scratch
257+
scratch=$(mktemp -d)
258+
trap "rm -rf '$scratch'" EXIT
259+
seed_pgtap_stub "$scratch"
260+
destdir_opt=(DESTDIR="$scratch")
261+
fi
262+
180263
# In existing mode pg_regress runs against $db via --use-existing and must
181264
# NOT create/drop its own database. `make test` (not just `make
182265
# verify-results`) is a real gate as of pgxntool 2.3.0 - it now exits
183266
# non-zero on regression failures instead of always exiting 0 regardless
184267
# of pg_regress's result (see this repo's pgxntool 2.3.0 bump).
185-
make test TEST_LOAD_SOURCE=existing TEST_SCHEMA="$schema" CONTRIB_TESTDB="$db" EXTRA_REGRESS_OPTS=--use-existing
268+
# TEST_EXISTING_DEPLOY is passed through explicitly (not left to
269+
# environment inheritance) so test/install/load.sql's existing-mode
270+
# assertion (via the count_nulls.test_existing_deploy GUC - see the
271+
# Makefile) picks the right source regardless of how this function is
272+
# invoked.
273+
make test TEST_LOAD_SOURCE=existing TEST_SCHEMA="$schema" CONTRIB_TESTDB="$db" EXTRA_REGRESS_OPTS=--use-existing TEST_EXISTING_DEPLOY="$deploy" "${destdir_opt[@]}"
186274
}
187275

188276
usage() {

test/install/load.sql

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

0 commit comments

Comments
 (0)