Add an update path from 0.1.1 to stable - #22
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 |
cc69f19 to
2e36592
Compare
jnasbyupgrade
changed the title
2e36592 to
3add6cc
Compare
cc387bd to
d0c0fe2
Compare
ea5ed24 to
57e2920
Compare
d0c0fe2 to
7c306dd
Compare
57e2920 to
ab272b1
Compare
ab272b1 to
7d753ae
Compare
2925fca to
03b0355
Compare
7d753ae to
f5629e5
Compare
|
Rebased onto #40 (`relandstable-rename`) instead of `master`: this branch needs the `default_version = 'stable'` rename to exist first (its own tree assumes `sql/extension_drop--stable.sql` and the `0.1.1` -> `stable` update path), and that rename hasn't actually reached `master` yet — see #40's description for why. Two of this branch's own commits (a local "Rename default_version from 1.0.0 to stable" + a follow-up "Strip invented commentary..." fixup) turned out to be an independent, slightly different reimplementation of the same rename that #26/#40 already covers, so they were dropped as duplicates during the rebase rather than replayed on top. Base is set to `relandstable-rename` for now, not `master`. Once #40 merges, this should be rebased again onto `master` and the base switched back. Verified post-rebase with `make test`, `make test-update`, `bin/test_update_path.sh`, and `make lint` — all green. |
…, quoting-requiring schema test Builds the U&U (update & upgrade) test infrastructure that doesn't require a real second extension_drop version or pg_upgrade CI to already exist: - PGXNTOOL_ENABLE_TEST_INSTALL = yes, with test/install/load.sql as the committed-once installer for the extension (no test roles exist for this extension, so unlike cat_tools there's nothing role-related to add). - TEST_LOAD_SOURCE (fresh/update/existing) GUC/make-var switch, parse-time validated, exported unconditionally, read in load.sql without missing_ok. `existing` mode is fully exercised locally (verified against a real, already-installed database, including the failure path when the extension is genuinely absent). `update` mode is wired up and structurally verified end-to-end, but extension_drop has no real prior released version to update FROM yet -- the Makefile refuses to run it without TEST_UPDATE_FROM set explicitly, and no CI leg exercises it in this repo today. - Dependency guard (test/sql/dependency_guard.sql): a view depending on extension_drop__commands' row type blocks a non-CASCADE DROP EXTENSION; proven by actually attempting the drop and asserting failure, not assumed. - test/sql/schema.sql's custom-schema test names renamed to mixed case (requires identifier quoting), reusing its existing coverage rather than adding a new schema-testing dimension. - ci.yml: run `make test && make verify-results` instead of pg-build-test, so a real regression actually fails the build (pgxntool's .IGNORE: installcheck otherwise reports green regardless of test results, per RELEASE.md's existing note about PRs #6/#7). Moving the extension's own installation into test/install/load.sql required adapting every test file that used to install it per-test in a rolled-back transaction (test/deps.sql, test/sql/simple.sql, test/sql/schema.sql, test/sql/zzz_build.sql) to work against the new committed-once install instead, since an extension name is a database-wide singleton. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…redate this branch CI on this branch showed the switch to `make test && make verify-results` surfacing real pgTAP failures on PostgreSQL 9.3/9.6 (cat_tools/extension_drop never actually install there). Checked PR #10's own baseline CI (#10, run 30665031257): PG 9.3 and 9.6 already report "3 of 3 tests failed" in the raw job log there too, just silently reported as a passing check because pg-build-test's underlying `make test` hits pgxntool's `.IGNORE: installcheck` the same way. So this isn't a regression from this PR's own changes -- it's the exact masking problem RELEASE.md already documents, just now applying to a different, older part of the PG matrix than the PRs (#6/#7) it originally cites. Reverting the ci.yml step back to pg-build-test here keeps this PR scoped to test/install infrastructure; fixing cat_tools's install path on pre-PG10 belongs to whoever owns that dependency setup (PR #10 or a follow-up), not this PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
extension_drop's real 0.1.1 install script (2017) was never committed to this repo -- only ever generated as a PGXN dist build artifact and uploaded directly -- so no update-diff script has ever existed either, despite a real SQL change between 0.1.1 and the current `stable` source (removing redundant client_min_messages handling; see #24, which renamed default_version from the never-released "1.0.0" to `stable` and is a prerequisite for this branch). That left anyone who actually installed 0.1.1 from PGXN with no path forward at all. - Recover the real 0.1.1 sql/extension_drop.sql from PGXN's dist archive (https://api.pgxn.org/dist/extension_drop/0.1.1/extension_drop-0.1.1.zip) and commit it as sql/extension_drop--0.1.1.sql. Add it to DATA explicitly (pgxntool's DATA wildcard only picks up the current version file and two-dash update scripts, not other historical single-version files -- Postgres-Extensions/pgxntool#48). - Author sql/extension_drop--0.1.1--stable.sql: the actual delta is just extension_drop__event_trigger() gaining one RAISE DEBUG line (found by diffing recovered 0.1.1 against current source). The client_min_messages removal and a cat_tools function rename (function__arg_types_text -> routine__parse_arg_types_text) both turned out to be install-script-only behavior with nothing persisted to replay. - Default TEST_UPDATE_FROM to 0.1.1 and wire up a real extension-update-test CI job: installs 0.1.1, plants the dependency guard, runs ALTER EXTENSION UPDATE, re-proves the guard survived, dynamically asserts the version landed at stable, then runs the pgTAP suite in update mode. Factored into bin/test_update_path.sh so it's runnable locally, not just inline CI YAML. Verified locally against PG17: 0.1.1 install -> guard blocks non-CASCADE drop -> ALTER EXTENSION UPDATE -> guard still blocks drop -> version lands at 'stable' -> full pgTAP suite passes in both fresh and update modes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
A separate extension-update-test job meant a whole extra container boot + PostgreSQL startup for a leg that needs no isolation of its own -- unlike pg_tle's job, which genuinely does. Runs as extra steps in each matrix leg's already-running container instead.
CREATE EXTENSION ... CASCADE only exists from PG10 -- the script crashed with a syntax error on the 9.3-9.6 matrix legs. Same branch test/install/load.sql already uses for the same reason.
cat_tools.routine__parse_arg_types_text() -- called unconditionally by extension_drop's install script, in both 0.1.1 and current -- hits cat_tools's hardcoded DROP ROUTINE internally, which doesn't exist before PG11. Not something to work around here: this is a real, pre-existing incompatibility affecting a fresh install of current extension_drop on PG9.3-10 too, just never surfaced by this repo's non-gating CI until this leg's ON_ERROR_STOP actually failed loudly.
pgxntool 2.3.0 (which this repo now vendors) already includes
sql/extension_drop--0.1.1.sql in its own generated DATA list, since
pgxntool#48 (the gap this DATA += line worked around) was fixed
upstream. Keeping the manual line duplicated the file in DATA and
broke `make install` outright ("will not overwrite just-created ...
with ..."). Left a comment instead of deleting the context entirely,
in case a future pgxntool downgrade reintroduces the gap.
f5629e5 to
66fa8ba
Compare
pgxntool's test-build feature needs rsync (pgxntool/run-test-build.sh), but pg-build-test never actually exercises test-build -- it goes through a lower-level PGXS path that bypasses this repo's own `test:` Makefile target. A plain `make test`/`make install` does exercise it, which is exactly what the new update-path steps (and pg-upgrade-test, which already installs rsync) do. First real CI exercise of this path failed with "rsync: command not found" -- confirmed locally the pgTAP suite itself passes; only the test-build sub-step was missing this dependency.
jnasbyupgrade
changed the title
|
Claude finished @jnasbyupgrade's task in 4m 12s —— View job Review: real update path (0.1.1 → stable)
SummaryVerified the core claim of the PR directly: diffed the recovered Nothing here looks incorrect or unsafe to merge. One documentation-consistency issue worth fixing while in the area:
Minor/optional nit: |
Several comments described HOW the 0.1.1 install script got into this
repo ("recovered from PGXN's dist archive", "never previously in this
repo's git history") instead of just stating what it is now (a
byte-for-byte match of PGXN's originally published dist archive).
Also: bin/test_update_path.sh referenced advanced-extension-testing.md,
a container-local doc with no path anyone outside that session can
resolve -- reworded to explain the dependency-guard/dynamic-version
rationale inline instead. Fixed one accuracy issue found along the
way: test/install/load.sql still named a separate "extension-update-test
job" that no longer exists (folded into the main test job's matrix).
|
Claude finished @jnasbyupgrade's task in 2m 53s —— View job Code review
What changed since the last reviewThe only commit since the prior review ( Independent verificationRe-diffed No CLAUDE.md violations and no new bugs found in this push. No issues found. Checked for bugs and CLAUDE.md compliance. For the record, the pre-existing, non-blocking follow-up noted in the prior review still stands (untouched by this PR, so not re-flagged inline): |
…rom comments
0.1.1 is the only version of extension_drop ever published to PGXN, not
"the last real" one in a series -- and describing it as an "actual real
compat floor" implies a deliberate compatibility boundary that isn't
there. Also drops HISTORY.asc's archaeology narration ("recovered...
never committed... uploaded directly") and ci.yml's "has never surfaced
before now" in favor of stating the current facts directly.
|
Claude finished @jnasbyupgrade's task in 1m 29s —— View job Code review
What changed since the last reviewThe only commit since the prior review ( Independent verificationConfirmed via No CLAUDE.md violations and no new bugs in this push. No issues found in the new commit. The one pre-existing, non-blocking item flagged in earlier reviews still stands, untouched by this push: |
An update script from 0.1.1 to stable is ordinary infrastructure and doesn't need repeated defending -- the same "matches PGXN's dist archive byte-for-byte" justification was copy-pasted across six files. State it plainly once (in the file it actually concerns) and just reference the version/files elsewhere.
|
Claude finished @jnasbyupgrade's task in 54s —— View job Code review
What changed since the last reviewThe only commit since the prior review ( Independent verificationTotal diff stat vs No CLAUDE.md violations and no new bugs in this push. No issues found in the new commit. The one pre-existing, non-blocking item flagged in earlier reviews still stands, untouched by this push: |
…comments - ci.yml: point at #44 instead of re-explaining the incompatibility inline and promising to track it "separately." - bin/test_update_path.sh: drop the "PROVEN, not assumed" framing -- that's just what a test does. - HISTORY.asc: a new version's update script isn't user-facing news, drop the entry. - Makefile: drop the paragraph claiming base.mk's DATA wildcard won't ship sql/extension_drop--0.1.1.sql on its own -- false since pgxntool 2.3.0, which already ships it (per the NOTE immediately below, which stays).
|
Claude finished @jnasbyupgrade's task in 1m 31s —— View job Code review
What changed since the last reviewThe only commit since the prior review (
Independent verificationConfirmed via No CLAUDE.md violations and no new bugs in this push. No issues found in the new commit. The one pre-existing, non-blocking item flagged in earlier reviews still stands, untouched by this push: |
|
Claude finished @jnasbyupgrade's task in 1m 35s —— View job Code review
What changed since the last reviewThree commits since the prior review, all comment/doc-only (confirmed via
Independent verification
Unlike the prior rounds of comment-trimming on this PR — which each removed text that was redundant with something still documented elsewhere — these three deletions remove content that isn't restated anywhere else in the tree:
Neither is a functional bug, and it's consistent with this PR's overall direction of paring comments down to just the facts — but flagging in case the loss of that rationale (particularly the update-script-scope explanation, which a future contributor extending this update path would want) wasn't intentional. No CLAUDE.md violations and no new bugs in this push. The one pre-existing, non-blocking item flagged in every prior review still stands, untouched by this push: |
Dropped claims contradicted elsewhere in the repo: extension_drop's declared version is no longer "1.0.0" and 0.1.1 is exactly the real, still-relevant release this PR adds an update path from -- not something that "predates the current SQL entirely". The old_pg-floor-vs-9.3 tangent is moot too: META.in.json's own floor is already 12, matching this job's, so there's no discrepancy left to explain. Also drops a container-local doc path and a reference to a specific commit message, neither resolvable outside this session/repo's git history. The CAT_TOOLS_GIT_REF mechanism this paragraph re-explained is already fully covered a few lines below (the env: block's own comment).
|
Claude finished @jnasbyupgrade's task in 1m 6s —— View job Code review
What changed since the last reviewThe only commit since the prior review (
Independent verification
This directly resolves the one outstanding item every prior review on this PR flagged (the No issues found in the new commit. One pre-existing, non-blocking nit noted in earlier reviews still stands, untouched by this or any prior push: |
…ARGETS comment No such file exists -- state the general policy (frozen version files aren't linted) instead of naming one, so it doesn't go stale again as new versions release.
|
Claude finished @jnasbyupgrade's task in 48s —— View job Code review
What changed since the last reviewThe only commit since the prior review ( Independent verificationConfirmed via No CLAUDE.md violations and no new bugs in this push. No issues found. Both non-blocking follow-ups noted across earlier reviews on this PR are now resolved. Nothing else outstanding — this PR looks ready to merge from a review standpoint. |
…dcoding it pgxntool's control.mk.sh already generates EXTENSION_SQL_FILES (one sql/<ext>.sql per .control file at the repo root) -- exactly the hand-maintained-source scoping this needed, and it keeps working unchanged if this distribution ever gains a second extension.
|
Claude finished @jnasbyupgrade's task in 1m 56s —— View job Code review
What changed since the last reviewThe only commit since the prior review ( Independent verification
No CLAUDE.md violations and no bugs found in this push. No issues found. Both non-blocking follow-ups noted in earlier rounds of review on this PR were already resolved in prior commits ( |
jnasbyupgrade
changed the title
|
Claude finished @jnasbyupgrade's task in 1m 42s —— View job Code review
What changed since the last reviewThe only new commit since the prior review ( Independent verification
This is consistent with every fact already independently verified in prior reviews on this PR: the 0.1.1 SQL matches current source byte-for-byte modulo the three documented deltas, the update script replays exactly the right one, and No CLAUDE.md violations and no bugs found in this push. No issues found. This was a routine, conflict-free sync merge with master — no new logic, SQL, or test behavior to review. All previously-flagged non-blocking follow-ups were already resolved in earlier commits ( |
1 participant
Adds an
ALTER EXTENSION UPDATEpath for extension_drop, from 0.1.1 -- the only version ever published to PGXN (2017) -- tostable:sql/extension_drop--0.1.1.sqlandsql/extension_drop--0.1.1--stable.sql, withTEST_UPDATE_FROMdefaulting to0.1.1.bin/test_update_path.shinstalls 0.1.1, plants the dependency guard, runsALTER EXTENSION UPDATE, re-proves the guard survives, and asserts the landed version dynamically againstextension_drop.control. It runs as extra steps inside the existingtestjob's matrix legs rather than a separate job, since this leg needs no isolation of its own. Those steps are gated to PG11+:cat_tools.routine__parse_arg_types_text()(called unconditionally by extension_drop's install script, in both 0.1.1 and current) transitively hits a hardcodedDROP ROUTINEthat doesn't exist before PG11 -- tracked at #44, since it also affects a fresh install on PG9.3-10.Also switches
LINT_TARGETSto derive from pgxntool'sEXTENSION_SQL_FILESinstead of hardcodingsql/extension_drop.sql, so it keeps working if this distribution ever gains a second extension.