Add update+upgrade (U&U) test infrastructure (stack 2/3) - #18
Open
jnasbyupgrade wants to merge 2 commits into
Open
Add update+upgrade (U&U) test infrastructure (stack 2/3)#18jnasbyupgrade wants to merge 2 commits into
jnasbyupgrade wants to merge 2 commits into
Conversation
…ript, test/install/load.sql three-mode loader, dependency-guard anchor, and a permanent schema-qualification pgTAP assertion Implements advanced-extension-testing.md checklist items 1-6 on top of PR #17 (reconcile-object-functions): - PGXNTOOL_ENABLE_TEST_INSTALL / PGXNTOOL_ENABLE_VERIFY_RESULTS set explicitly; TEST_LOAD_SOURCE (fresh/update/existing) + TEST_UPDATE_FROM/TO make vars, parse-time validated, propagated as placeholder GUCs via PGOPTIONS; `make test-update` wrapper. - test/install/load.sql: single committed-once installer for the extension, covering all three load modes, including a drop-first reset (with pg_temp.drop_role() for the extension's own global roles) and an existing-mode presence/version assertion. - sql/object_reference--0.1.0--stable.sql: hand-authored update script (there was previously no update path at all from the only real historical release to current). Recreates every function/view that changed via the same private-helper-schema bootstrap/teardown convention the fresh install uses, so the update path is verified byte-for-byte structurally identical to a fresh install (function bodies, comments, ACLs, table/view columns). - Makefile: DATA += sql/object_reference--0.1.0.sql (pgxntool#48 workaround, needed for CREATE EXTENSION ... VERSION '0.1.0' to work at all); a conditional count_nulls install step for the update-mode floor only (0.1.0's own install script still needs it, even though current object_reference no longer requires it). - test/finish.sql: one permanent pgTAP assertion (modeled on pg_count_nulls'/extension_tools' own schema-qualification checks) proving object_reference/_object_reference are never resolved via search_path. - Moved the pre-existing raw-source-load sanity check (test/sql/zzz_build.sql) to test/build/, pgxntool's own test-build feature: it needs a schema-free database to create `object_reference` manually in, which the committed-once installer above no longer provides in the shared main-suite database. Dependency-guard anchor for a future existing-mode CI job: a view typed on _object_reference.object's row type (object_reference-owned, never dropped or redefined by the update script) blocks a non-CASCADE DROP EXTENSION; manually proven to block the drop (and to keep blocking it after the update path) as part of this PR's own verification, not committed as CI machinery yet. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
test/build/zzz_build.sql (moved there in the previous commit) enables pgxntool's test-build feature, whose run-test-build.sh syncs test/build/*.sql into test/build/sql/ via rsync -- not present in the pgxn/pgxn-tools image, causing every PG-matrix leg to fail with "rsync: command not found". Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Second link of a 3-PR stack. Stacked on #17 (
reconcile-object-functions) —review this diff on top of that PR's changes, not against
master.Implements
advanced-extension-testing.mdchecklist items 1-6 (the localtest/Makefile machinery for fresh/update/existing-mode testing). CI wiring
(a dedicated job that actually invokes
TEST_LOAD_SOURCE=update/=existing) is deliberately not part of this PR — that's link 3 of thestack. CI here just runs the normal
testjob in fresh mode and stays green.PGXNTOOL_ENABLE_TEST_INSTALL/PGXNTOOL_ENABLE_VERIFY_RESULTSsetexplicitly;
TEST_LOAD_SOURCE(fresh/update/existing) +TEST_UPDATE_FROM/TEST_UPDATE_TOmake vars, parse-time validated, propagated as placeholderGUCs via
PGOPTIONS;make test-updatewrapper.test/install/load.sql: single, committed-once installer for theextension, covering all three load modes — drop-first reset (with a
pg_temp.drop_role()helper forobject_reference__usage/object_reference__dependency, the extension's own global roles) forfresh/update, and a presence+version assertion for existing.
sql/object_reference--0.1.0--stable.sql: hand-authored update script.There was previously no update path at all from 0.1.0 (the only real
historical PGXN release) to current. Diffed the two files in full to find
every delta (see the file's own header comment for the itemized list —
reg* pseudotype column removal, the count_nulls-backed trigger, new
object-info functions, temp-schema rejection, etc.). Every function/view
that changed is recreated via the same private-helper-schema
bootstrap/teardown convention the fresh install itself uses (not hand-typed
DROP FUNCTION/CREATE FUNCTION/REVOKE/GRANT/COMMENT), so theupdate path can't silently diverge from a fresh install's template.
Makefile:DATA += sql/object_reference--0.1.0.sql— pgxntool'sDATAwildcard only picks up the current version file and two-dash update
scripts, never a one-dash historical full-install file, so without this
CREATE EXTENSION object_reference VERSION '0.1.0'fails outright (samegap as
Postgres-Extensions/pgxntool#48). Also a conditionalcount_nullsinstall step, gated onTEST_LOAD_SOURCE=update: 0.1.0's owninstall script still creates a trigger that calls count_nulls'
not_null_count_trigger(), andobject_reference.control'srequires(cat_tools only now) no longer CASCADEs it in — current object_reference
itself has no runtime dependency on count_nulls at all, so this is scoped
to the update-mode floor only, not folded into the unconditional
install: cat_tools.test/finish.sql: the one permanent pgTAP assertion recommended instead ofa
TEST_SCHEMAdimension (object_reference is genuinely schema-pinned —schema=set,relocatable=false, zero@extschema@usage) — modeled onpg_count_nulls'/extension_tools' own schema-qualification checks.Excludes
object_reference/_object_referencefromsearch_path(alreadytrue via
test/load.sql→ pgxntool'stap_setup.sql) and asserts viacurrent_schemas(), checked at the end of everytest/sql/*.sqlfile (soa test that mutates
search_pathmid-file and never restores it getscaught). Every existing test file's
plan()count bumped by 1 accordingly.(
test/sql/zzz_build.sql→test/build/zzz_build.sql, pgxntool's owntest-buildfeature) — it manually doesCREATE SCHEMA object_reference;and loads
sql/object_reference.sqldirectly, which now collides with thereal extension
test/install/load.sqlalready installed in the sharedmain-suite database.
test-buildruns its own separate, freshlydrop+recreated database each time, matching what this check always
assumed.
both empirically hit and fixed (not theoretical):
_object_v/_object_v__for_updateviews must be dropped beforethe
_object_oidtable's reg* columns (theySELECTthose columnsdirectly) — got the order backwards on the first pass; Postgres's own
"cannot drop column ... other objects depend on it" caught it.
object_reference's own event triggers (already installed and activefrom the 0.1.0 base) fire on every
sql_drop/ddl_command_endin thesession — including the update script's own
DROP VIEW/ALTER TABLEstatements.
zzz__object_reference_drop's body queries_object_reference._object_v, so it errored the instant the scriptdropped that view. Fixed by disabling all three event triggers for the
structural portion of the update script and re-enabling them once
everything is back in its final shape.
Convergence/divergence vs.
cat_tools(the reference implementation)Per
advanced-extension-testing.md's stated goal (find out what's genericenough to belong in pgxntool itself), cloned
Postgres-Extensions/cat_toolsfresh (
ca12802) and readtest/install/load.sql,test/roles.sql,test/deps.sql,test/finish.sql,Makefile'sTEST_LOAD_SOURCEblock,and
bin/test_existingbefore writing anything.Took verbatim (structure/pattern, names adapted):
TEST_LOAD_SOURCE/TEST_UPDATE_FROM/TEST_UPDATE_TOGUC-via-PGOPTIONSpropagation mechanism, including the parse-timefresh|update|existingvalidation and thetest-updaterecursive-$(MAKE)wrapper.
test/install/load.sql's three-mode\ifstructure, thepg_temp.drop_role()helper (DROP OWNED BYthenDROP ROLE IF EXISTS),and the existing-mode presence/version
DOblock — all copied nearlyline-for-line, only object_reference's own names substituted in.
test/install/.gitignore(load.out/install.out.diff) plus theEXTRA_CLEANwiring in the Makefile — cat_tools deliberately never commitsmeaningful content there because pgxntool's
../install/<name>scheduletrick resolves the expected-output path and the results path to the exact
same file, making it self-comparing. Only realized this after initially
trying to treat it like a normal expected-output file and getting confused
by spurious "ok" results; cat_tools's own
Makefilecomment ("theself-comparing result .out and its diff") is what clarified it.
test/finish.sql's exactcurrent_schemas(false)assertion shape (adaptedfrom
pg_count_nulls/extension_tools, which cat_tools's own equivalentalso draws from) per test-fixes.md's item 9 recommendation.
Adapted:
TEST_UPDATE_FROM-floor dependency gap (its own oldestsupported version installs cleanly with just its own
requires).object_reference's 0.1.0 floor needs count_nulls, which the current
control file no longer declares — required inventing the conditional
install: count_nullsMakefile step and the explicitCREATE EXTENSION IF NOT EXISTS count_nulls;in update mode's branch.Nothing in cat_tools's
load.sqlneeded an equivalent, since it has noextra-dependency floor gap of its own.
ALTER TYPE ... ADD VALUE(additive);object_reference's has real
DROP COLUMN/DROP VIEW/signature changes,so the update script itself is structured very differently (the
private-helper-schema bootstrap/teardown convention, the explicit
constraint/index/trigger drop ordering, the event-trigger disable/enable
bracket) — none of that has a cat_tools analog to copy from.
Skipped (explicitly out of scope per the containing doc/task):
bin/test_existing-equivalent script and any CI wiring(
extension-update-test,pg-upgrade-testjobs, dependency-guard plantingin CI) — next link in the stack.
TEST_SCHEMAdimension — object_reference is schema-pinned; test-fixes.mditem 9 explicitly says not to build this, use the one permanent assertion
instead (done above).
object_reference has exactly one real historical version (0.1.0), so there
is only one update-from floor to test, no shortest-path duplicity concern.
cleanly on every currently-supported PostgreSQL major (verified by hand on
12 and 17), so there's no known unsafe old version needing a bridge.
structural_diff.sql/bin/structural_diffas committed tooling — I didthe structural comparison (function bodies via
pg_get_functiondef,table/view columns, comments, ACLs) by hand with one-off SQL for this PR's
own verification and confirmed a byte-for-byte match, but didn't commit it
as reusable tooling; flagging as a candidate for a future PR/pgxntool
feature rather than building it now.
Invented (not in cat_tools):
count_nullsversion-floor handling above.\set ON_ERROR_STOP 1at the top oftest/install/load.sql. cat_tools'sown
load.sqldoesn't set it either (same latent gap) — without it, agenuine script error doesn't abort; it just prints and keeps going
statement-by-statement, which is exactly what made the two bugs above (view
drop ordering, event-trigger self-interference) hard to see clearly at
first — errors were present in the output but buried, and the ../install/
self-comparing
.outfile made everything downstream of them lookspuriously "ok". Worth a follow-up issue against cat_tools itself.
structural changes. A generic pattern for any extension whose update path
needs to modify objects that its own already-active event triggers
reference — cat_tools has no event triggers of its own so this never came
up there, but it's a first-instance discovery of a bug class advanced-
extension-testing.md's doc doesn't currently mention (an update script's
own DDL becoming an unwitting trigger of the previous version's event
handlers). Worth surfacing back into the doc/pgxntool guidance.
Dependency-guard anchor
Picked
_object_reference.object's row type (the composite type Postgresgives every table):
SELECT NULL::_object_reference.object AS guarded_memberin a guard view. Reasons:cat_tools.object_type, which thisextension only consumes — using an external type wouldn't demonstrate
object_reference's own extension membership blocking the drop).
0.1.0--stable.sql(only_object_oidchanges structurally), so theguard is stable across the one real update path that exists today.
Manually verified end-to-end (not yet wired into CI — that's the next
stack link):
CASCADEDROP EXTENSIONfailswith
_object_reference.object's type listed as the blocker,CASCADEcorrectly drops both the extension and the guard view together.
ALTER EXTENSION UPDATEunchanged, still blocks a non-CASCADEdropafterward.
Verification performed by hand
make lint— clean, both before and after every change in this PR.make test(fresh) — all 7 pgTAP files +zzz_build(now undertest/build/) pass, on PostgreSQL 12 and 17.make test TEST_LOAD_SOURCE=update/make test-update— same suite,same expected output, passes identically to fresh, on PostgreSQL 12 and 17.
make test TEST_LOAD_SOURCE=existing CONTRIB_TESTDB=<db> EXTRA_REGRESS_OPTS=--use-existing PGXNTOOL_ENABLE_TEST_BUILD=noagainst acorrectly-installed database (passes) and a deliberately-broken one (stuck
at 0.1.0) — fails loudly at the
install/loadstep with a clear "installedat version 0.1.0 but the current default_version is stable" error, not
silently.
pg_get_functiondef, table/view columns, comments, ACLs) — byte-for-byteidentical.
CASCADEdrop and letCASCADEthrough, both on a fresh install and after the update path, as described
above.
Not fully verified
pg_upgradeacross PostgreSQL majors — out of scope for this PR(no
pg-upgrade-test/bin/test_existingCI infrastructure exists yet;that's part of the next stack link).
TEST_LOAD_SOURCE=update/existing—by design, per the task this PR implements; confirmed CI still runs the
plain
testjob in fresh mode and stays green.Supersedes
Supersedes
jnasbyupgrade/object_reference#3("u-and-u-foundation"), whichwas built against the wrong version model (
TEST_UPDATE_TO=0.2.0, no longercurrent — everything is
stablenow) and included aTEST_SCHEMAdimensionthat test-fixes.md's item 9 identifies as a mistake for this schema-pinned
extension. Please close #3 once this merges (leaving that to the repo
owner rather than closing it myself).
🤖 Generated with Claude Code