Restore sql/test_factory--0.5.0.sql to its true historical content - #41
Restore sql/test_factory--0.5.0.sql to its true historical content#41jnasbyupgrade wants to merge 5 commits into
Conversation
|
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 |
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
test_factory 0.5.0 has been the released, real-world-installed version since 2017 (confirmed: no tag for it in this repo, but the "Bump version to 0.5.0" commit is nine years old, and issue Postgres-Extensions#14 itself describes a live production failure against it). Every edit to sql/test_factory.sql on fix/issue-14-clean got silently baked into this file too, via pgxntool's own auto-regeneration (it copies the base file to sql/{ext}--{version}.sql whenever default_version matches) -- retroactively rewriting what "version 0.5.0" contains without ever bumping the version number, which is exactly the anti-pattern pgxntool's own docs warn a released version's file should never be subject to. Restored to match upstream/master (verified byte-for-byte identical), i.e. what 0.5.0 has always actually contained. The issue Postgres-Extensions#14 fix itself stays intact in the base sql/test_factory.sql file, ready for whichever new version number eventually ships it -- that decision is deliberately not made here. release_status in META.in.json/META.json is already "stable" in both files; no change needed there. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The real problem wasn't just that sql/test_factory--0.5.0.sql had drifted -- it's that test_factory.control's default_version was pinned at '0.5.0' (a real, already-released version) the whole time. Per ../ai/RELEASE.md's `stable` pseudo-version convention: between releases, default_version should sit at the literal string 'stable', so ordinary source edits regenerate sql/<ext>--stable.sql (a disposable scratch file) and never touch a frozen, already-shipped version's file. Left at a real version number, every edit to sql/test_factory.sql kept silently corrupting sql/test_factory--0.5.0.sql -- exactly what happened all session, and what would happen again on the next source edit if left as-is. Set default_version = 'stable' in both test_factory.control and test_factory_pgtap.control (test_factory_pgtap.control's own `superuser = false` addition has the identical problem, one version number earlier). Regenerated sql/test_factory--stable.sql and sql/test_factory_pgtap--stable.sql from the current source (confirmed sql/test_factory--0.5.0.sql and sql/test_factory_pgtap--0.1.0.sql are untouched by this regeneration). Added sql/test_factory--0.5.0--stable.sql, the update script from the last real release to `stable`, containing the same GRANT-if-missing logic as the fresh-install fix -- required for `ALTER EXTENSION test_factory UPDATE` to have anywhere to go at all. test/install/load.sql's update-mode branch needed a real restructuring, not just a version bump: CREATE EXTENSION test_factory VERSION '0.5.0' (the FROM version, genuinely unfixed) cannot succeed as a non-superuser -- that's the original bug, unrelated to versioning -- and extension ownership can't be transferred (`ALTER EXTENSION ... OWNER TO` isn't valid syntax, confirmed), so only the role that ran the original install can ever run ALTER EXTENSION UPDATE on it. There's no reachable "different non-superuser applies this later" scenario to test. Update mode now runs entirely as the ambient role; only fresh mode switches to test_factory_installer (which already fully covers the non-superuser install path) -- moved the issue Postgres-Extensions#14 regression check into the fresh branch specifically, since it was checking the wrong thing under update mode's ambient (typically superuser) role. Verified locally on PG12 and PG17: fresh and update modes both pass, and default_version = 'stable' means `make` no longer touches sql/test_factory--0.5.0.sql or sql/test_factory_pgtap--0.1.0.sql on any future source edit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
c0ea0e8 to
3f3fa4a
Compare
assert_version() compared installed extversion against `make -s print-PGXNVERSION`, which pulls META.json's *distribution* version (still the real semver, e.g. 0.5.0) -- unaffected by test_factory.control's default_version, which this PR pins to the literal 'stable' pseudo-version between releases. A bare CREATE EXTENSION now installs at 'stable', so the two were guaranteed to mismatch: confirmed via the pg-upgrade-test CI job failing with "test_factory in '...' is 'stable', expected '0.5.0'". Switched to `make -s print-EXTENSION_test_factory_VERSION` (from pgxntool's generated control.mk, tracks default_version directly), verified locally end-to-end: a fresh CREATE EXTENSION installs at 'stable', and bin/test_existing run-suite's own version check now reports installed='stable' expected='stable' and passes. Also picked up the ERRCODE fix in sql/test_factory--stable.sql (regenerated after rebasing onto the latest fix/issue-14-clean). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
sql/test_factory--stable.sql and sql/test_factory_pgtap--stable.sql are permanently current, not a frozen release -- per ../ai/CLAUDE.md's "Version-specific SQL files" section (referenced directly from ../ai/RELEASE.md), they'd be regenerated and re-diffed on every single source edit for zero test-coverage value if tracked. Removed from git, added sql/*--stable.sql to .gitignore. sql/test_factory--0.5.0--stable.sql (the real update script) is unaffected and stays tracked -- confirmed via git check-ignore. Verified: rm'd both files, ran `make clean && make test` (fresh and update modes) from that state -- both regenerate automatically and the full suite still passes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ong context Opened with "CREATE ROLE never granted..." -- language carried over from sql/test_factory.sql's own comment, where a CREATE ROLE statement exists right above it. This file has no CREATE ROLE at all (test_factory__owner already exists on any real 0.5.0 install), so that framing described an event that never happens here, confusing about why role creation was even being discussed. Rewrote it to stand on its own: the actual issue Postgres-Extensions#14 bug (never getting SET/plain membership automatically) applies to an existing install just as much as a fresh one, which is why this update script exists at all. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| # and re-diffed on every source edit for zero test-coverage value if | ||
| # tracked. The update script *to* stable (sql/*--<last-release>--stable.sql) | ||
| # is not affected by this and must stay committed. | ||
| sql/*--stable.sql |
There was a problem hiding this comment.
Bug: this pattern also matches the update scripts the comment above it says are exempt.
The added comment states:
The update script to stable (
sql/*--<last-release>--stable.sql) is not affected by this and must stay committed.
But in gitignore glob syntax * matches any run of characters except /, so sql/*--stable.sql also matches sql/test_factory--0.5.0--stable.sql (with * = test_factory--0.5.0) — the very file this PR adds. It's harmless right now only because that file is already tracked (git doesn't apply .gitignore to tracked paths), but the next release's analogous update script (e.g. sql/test_factory--0.6.0--stable.sql, or a future test_factory_pgtap one) would be silently skipped by git add, shipping a distribution with no upgrade path and no error anywhere.
A negation line after it fixes this — sql/*--*--stable.sql requires two separate ---delimited segments before stable.sql, so it matches only <ext>--<version>--stable.sql update scripts, not the single-segment <ext>--stable.sql files this rule is meant to ignore:
| sql/*--stable.sql | |
| sql/*--stable.sql | |
| !sql/*--*--stable.sql |
| @@ -1,5 +1,5 @@ | |||
| comment = 'A framework for managing test data' | |||
| default_version = '0.1.0' | |||
| default_version = 'stable' | |||
There was a problem hiding this comment.
Bug: default_version moves to 'stable' here, mirroring test_factory.control, but unlike test_factory.control (which gets sql/test_factory--0.5.0--stable.sql in this same PR), there's no corresponding sql/test_factory_pgtap--0.1.0--stable.sql anywhere in this diff.
PostgreSQL's ALTER EXTENSION ... UPDATE requires a literal sql/<extension>--<from>--<to>.sql script to move between two specific versions — there's no fuzzy/partial matching. Any existing test_factory_pgtap 0.1.0 install (the version that has shipped since this extension's only release) would fail ALTER EXTENSION test_factory_pgtap UPDATE with "no update path from version "0.1.0" to version "stable"".
This isn't caught by the test suite: test/install/load.sql's update-mode branch installs test_factory_pgtap fresh (at stable directly) rather than updating it from 0.1.0, so the missing path is never exercised.
Fix: add a sql/test_factory_pgtap--0.1.0--stable.sql script (can be a no-op, since no pgtap-side objects changed) mirroring what this PR already did for test_factory.
1 participant
Summary
test_factory0.5.0 has been the released, real-world-installed version since 2017 -- issue CREATE EXTENSION fails on PG16+ without a superuser (RDS/Aurora): must be able to SET ROLE "test_factory__owner" #14 itself describes a live production failure against it.sql/test_factory.sqlonfix/issue-14-cleangot silently baked intosql/test_factory--0.5.0.sqltoo, via pgxntool's own auto-regeneration -- retroactively rewriting what "version 0.5.0" contains without ever bumping the version number.sql/test_factory--0.5.0.sqlto matchupstream/masterbyte-for-byte (verified). The issue CREATE EXTENSION fails on PG16+ without a superuser (RDS/Aurora): must be able to SET ROLE "test_factory__owner" #14 fix itself stays intact in the basesql/test_factory.sqlfile, ready for whichever version eventually ships it -- that decision isn't made here.release_statusinMETA.in.json/META.jsonis already"stable"in both -- no change needed.Stacked on #18 (
issue-14-clean, pushed to this repo for that purpose) rather than a fork-to-fork PR.Test plan
diffconfirms the restored file is byte-for-byte identical toupstream/master's version.make lintpasses.