Docs: versioned SQL file tracking tradeoffs and update-testing pattern#60
Merged
jnasbyupgrade merged 8 commits intoJul 26, 2026
Merged
Conversation
Postgres-Extensions#51, Postgres-Extensions#42, Postgres-Extensions#52) Adds a coherent doc section (README.asc) covering three related conventions around committing versioned install scripts: - When to track a version's install script vs. when it's acceptable to skip it (minor, low-risk bumps in large repos), and why: update testing across PostgreSQL major-version boundaries is the primary value (Postgres-Extensions#51) - If you skip tracking a version's file, clean up with a one-time `rm` after the version bump rather than a permanent `.gitignore` entry, since `make` only ever generates the current version's file (Postgres-Extensions#51 follow-up) - The `test/install` fresh-vs-upgrade testing pattern: load the extension once via `ALTER EXTENSION ... UPDATE` in a committed `test/install` file (not per-test in `test/deps.sql`, since pgTAP's per-test ROLLBACK can't safely exercise a newly-added enum value in the same transaction that added it), then run the same test suite against both fresh and upgraded installs (Postgres-Extensions#42) - Never hand-edit a versioned install script once it's no longer the current `default_version` — it's a frozen historical record, not a regenerate-on-build file; bump the version and add an upgrade script instead. Added a matching guardrail note to CLAUDE.md's "Generated files" list, since AI coding agents in particular won't infer this convention from the generic "DO NOT EDIT" header alone (Postgres-Extensions#52) Changes only in pgxntool. No related changes in pgxntool-test. Co-authored-by: Claude Sonnet 5 <[email protected]>
|
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:
📝 WalkthroughWalkthroughThe documentation adds guidance for testing extension update and upgrade workflows across fresh, update, and existing installation modes. It also explains when to commit version-specific SQL files, how current files are regenerated, how stale files are removed, and why historical versioned SQL files must not be manually edited. Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
jnasbyupgrade
added
the
commit-with-no-tests
…e, don't claim Postgres-Extensions#42 fixed The enum ADD VALUE restriction was presented as the reason the install step must be committed, which read as if extensions without custom enum types don't need this pattern. Generalize the reasoning: running a committed update is what faithfully models production regardless of enum usage; the enum restriction is just the case that turns the mismatch into a hard error. Broaden the section to cover pg_upgrade testing alongside ALTER EXTENSION UPDATE testing (both are "U&U testing"), via the existing mode used for post-pg_upgrade assertions. Note that first-class support is planned (issue Postgres-Extensions#42) and link worked examples in cat_tools and pg_count_nulls at specific files/commits rather than whole PRs. Issue Postgres-Extensions#42 asks for both documentation and (optionally) first-class pgxntool support; only the documentation half is done here, so it stays open rather than being closed by this PR.
- The per-test BEGIN/ROLLBACK wrapping is pgxntool's own convention
(test/pgxntool/setup.sql), not something pgTAP does.
- Trim the enum ADD VALUE detail down to a brief example rather than
a full mechanism explanation, and drop the redundant hedge ('can
hide' -> 'hides').
- 'Worked examples' -> 'Working examples'.
The BEGIN/ROLLBACK-per-test-file wrapping is purely a pgxntool convention (test/pgxntool/setup.sql) -- no need to reference pgTAP at all, not even to deny it does this. Also drop the ALTER TYPE / error code detail down to a bare 'modifying an enum' example.
Contributor
Author
|
@CodeRabbit pls review |
|
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.asc`:
- Around line 366-380: Update README.asc lines 366-380 and the earlier “What
Controls the Version Number” section to state that the current generated version
filename comes from the extension control file’s default_version, not META.json;
preserve the guidance to remove only the stale old-version file after a bump.
Update CLAUDE.md line 272 to remove the META.json implication and reference the
control file’s default_version consistently with control.mk.sh.
- Line 177: Update the README section describing `myext.test_load_mode` to
identify it as a backend GUC set through `PGOPTIONS`, not a psql variable;
document retrieving it with `current_setting('myext.test_load_mode')`, or revise
the example to use psql `-v`/`\set` if a psql variable is intended.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7e7bc092-0fd3-4873-af9f-f3664ecd8933
📒 Files selected for processing (2)
CLAUDE.mdREADME.asc
Comment on lines
+366
to
+380
| `make` only ever generates the *current* version's file, stamped from the base source according to `META.json`'s `provides.{extension}.version`. The moment you bump the version, `make` starts generating the *new* current version's file and stops touching the old one — the old file left behind in your working tree is a stale, one-off artifact, not something `make` keeps recreating on every build. | ||
|
|
||
| Because it's only ever transiently present, the correct cleanup is a single `rm sql/{extension}--{old-version}.sql` right after the version bump — not a permanent `.gitignore` rule. A per-version `.gitignore` line would be perpetual clutter added on every release, for a file that's only ever present for one build cycle. A broad glob (e.g. `sql/*--*.sql`) is wrong for the same reason it's wrong below in <<_alternative_ignoring_all_version_files>>: it would also hide the prior-version install scripts and upgrade scripts (`sql/{extension}--{a}--{b}.sql`) that you *do* want tracked and visible in `git status`. | ||
|
|
||
| ==== Never Hand-Edit a Version File That's No Longer Current | ||
|
|
||
| `control.mk.sh` generates a Make rule along these lines for the current version's file: | ||
|
|
||
| ---- | ||
| $(EXTENSION_ext_VERSION_FILE): sql/ext.sql extension.control | ||
| @echo '/* DO NOT EDIT - AUTO-GENERATED FILE */' > $(EXTENSION_ext_VERSION_FILE) | ||
| @cat sql/ext.sql >> $(EXTENSION_ext_VERSION_FILE) | ||
| ---- | ||
|
|
||
| That rule only ever targets the file matching the *current* `default_version` / `META.json` version, so editing the base `sql/{extension}.sql` and running `make` is the correct, safe way to change that one file. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use .control’s default_version as the generation source.
The generated filename is derived from the extension control file’s default_version; META.json can differ. PostgreSQL also resolves an unspecified ALTER EXTENSION ... UPDATE target from the control file. (postgresql.org)
README.asc#L366-L380: replace theMETA.json-based generation claims with.controldefault_version, and correct the earlier “What Controls the Version Number” section consistently.CLAUDE.md#L272-L272: remove the implication thatMETA.jsondetermines the current generated version file; refer to.controldefault_version.
📍 Affects 2 files
README.asc#L366-L380(this comment)CLAUDE.md#L272-L272
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.asc` around lines 366 - 380, Update README.asc lines 366-380 and the
earlier “What Controls the Version Number” section to state that the current
generated version filename comes from the extension control file’s
default_version, not META.json; preserve the guidance to remove only the stale
old-version file after a bump. Update CLAUDE.md line 272 to remove the META.json
implication and reference the control file’s default_version consistently with
control.mk.sh.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…ibution vs extension
CodeRabbit flagged that the version-files docs had the source of truth
backwards: control.mk.sh's own comments say it deliberately reads each
extension's .control file default_version, NOT META.json's
provides.{extension}.version, precisely because PostgreSQL uses
default_version to pick which SQL file to load and META.json can differ
(meta.mk.sh's header comment says the same for the distribution-level
PGXNVERSION vs per-extension EXTENSION_*_VERSION split).
- How Version Files Are Generated / What Controls the Version Number:
corrected to say the .control file's default_version drives generation,
META.json does not.
- Fixed two other spots repeating the same inversion (the rm-once
cleanup rationale, the never-hand-edit rule's 'current file' wording,
and Multiple Versions' regeneration note).
- Added a 'PGXN Distributions vs. Extensions' section grounding the
terminology in the PGXN meta spec (pgxn.org/spec/) and noting a single
distribution can provide more than one extension, each independently
versioned via its own .control file.
- CLAUDE.md gotcha Postgres-Extensions#7: removed the '/META.json' implication, pointing at
the new README section instead.
Verified against the actual PGXN meta spec (pgxn.org/spec/): provides. <ext>.version is defined as 'a Version for the extension' in its own right, NOT the distribution's version as previously stated here. It's meant to track the same thing as the .control file's default_version, but build_meta.sh only strips empty fields from META.in.json -- it never reads .control files, so nothing keeps provides.<ext>.version and default_version in sync. That's a real, pre-existing gap (not hypothetical); issue Postgres-Extensions#47 already tracks it, so linked that instead of filing a duplicate. Also added a 'edit META.in.json, never META.json directly' mention to each distinct topic that references META.json (intro, tag, dist, distclean, version-files), per the earlier feedback that this fact is easy to miss and worth surfacing once per topic rather than at every mention.
# Conflicts: # README.asc
jnasbyupgrade
merged commit Jul 26, 2026
9c2f75f
into
Postgres-Extensions:master
8 of 9 checks passed
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
1 participant
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.
Fixes #51, #52. Partially addresses #42 — see note below.
Adds a coherent doc section (README.asc) covering three related conventions around committing versioned install scripts, plus a matching guardrail note in CLAUDE.md:
rmafter the version bump, not a permanent.gitignoreentry, sincemakeonly ever generates the current version's file.test/installfile to test bothALTER EXTENSION ... UPDATE(fresh vs. update equivalence) andpg_upgrade(via anexistingmode that only asserts, never touches) against the sametest/sql/suite. Points to specific files in cat_tools and pg_count_nulls as worked examples. Document (and maybe support) testing ALTER EXTENSION UPDATE via test/install #42 is intentionally left open: this is the documentation half only. Document (and maybe support) testing ALTER EXTENSION UPDATE via test/install #42 also asks for first-class pgxntool support (a standard mode-switching toggle, built-in pg_upgrade scaffolding), which is still wanted and not implemented here.Doc-only change, no code changes.