Fix parallel-build SQL corruption, remote hardcoding, and add safety checks - #61
Merged
jnasbyupgrade merged 5 commits intoJul 27, 2026
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:
📝 WalkthroughWalkthroughThe change adds an include guard and configurable test-output validation to Estimated code review effort: 3 (Moderate) | ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
This was referenced Jul 26, 2026
jnasbyupgrade
force-pushed
the
fix-buildsystem-batch
branch
from
July 26, 2026 19:54
0a406a5 to
6ccabae
Compare
This was referenced Jul 26, 2026
jnasbyupgrade
added a commit
to jnasbyupgrade/pgxntool
that referenced
this pull request
Jul 26, 2026
…ostgres-Extensions#28) The old name implied it listed every version file for an extension; it actually only ever holds the current/most-recent one per extension. Split out of Postgres-Extensions#61 to keep that PR's scope down. If your Makefile references EXTENSION_VERSION_FILES directly (e.g. to add extra dependencies to DATA), update it to EXTENSION__CURRENT_VERSION__FILES. Changes only in pgxntool. Related changes in pgxntool-test: - Update the one code comment referencing the old name in test/sequential/02-dist.bats Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
jnasbyupgrade
added a commit
to jnasbyupgrade/pgxntool
that referenced
this pull request
Jul 26, 2026
…ostgres-Extensions#28) The old name implied it listed every version file for an extension; it actually only ever holds the current/most-recent one per extension. Split out of Postgres-Extensions#61 to keep that PR's scope down. If your Makefile references EXTENSION_VERSION_FILES directly (e.g. to add extra dependencies to DATA), update it to EXTENSION__CURRENT_VERSION__FILES. Changes only in pgxntool. Related changes in pgxntool-test: - Update the one code comment referencing the old name in test/sequential/02-dist.bats Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
jnasbyupgrade
added a commit
to jnasbyupgrade/pgxntool
that referenced
this pull request
Jul 26, 2026
…ostgres-Extensions#28) The old name implied it listed every version file for an extension; it actually only ever holds the current/most-recent one per extension. Split out of Postgres-Extensions#61 to keep that PR's scope down. If your Makefile references EXTENSION_VERSION_FILES directly (e.g. to add extra dependencies to DATA), update it to EXTENSION__CURRENT_VERSION__FILES. Changes only in pgxntool. Related changes in pgxntool-test: - Update the one code comment referencing the old name in test/sequential/02-dist.bats Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…checks (Postgres-Extensions#7, Postgres-Extensions#14, Postgres-Extensions#19, Postgres-Extensions#28, Postgres-Extensions#50, Postgres-Extensions#53) - Fix a real data-corruption bug: versioned SQL file generation used `>` truncate followed by `>>` append across two recipe lines, so a rule that fires twice in one `make` run (e.g. reached via two dependency chains under parallel make) silently doubled the file's content. Now written atomically via a single subshell redirect (Postgres-Extensions#19) - `make test` never failed on a stale `test/expected/` file left behind after renaming/removing a test; new `check-stale-expected` target (run as part of `TEST_DEPS`) catches orphaned `.out` files in `test/sql`/`test/expected` and `test/build`/`test/build/expected` (Postgres-Extensions#14) - Tagging targets (`tag`/`rmtag`/`forcetag`/`dist`) hardcoded the `origin` remote, silently re-tagging the wrong repo for maintainers whose `origin` is a personal fork; now configurable via `PGXN_REMOTE ?= origin` (Postgres-Extensions#53) - `EXTENSION_VERSION_FILES` renamed to `EXTENSION_CURRENT_VERSION_FILES` — the old name implied all version files for an extension, but it only ever holds the current/most-recent file per extension (Postgres-Extensions#28) - `base.mk` had no include guard, so double-inclusion (e.g. an extension's own `.mk` module including it alongside the Makefile's own include) redefined every target with overriding-recipe warnings; now wrapped in an `ifndef`/`endif` guard (Postgres-Extensions#50) - `make clean`'s `EXTRA_CLEAN` targeted a nonexistent top-level `results/` instead of the real `$(TESTOUT)/results/` (Postgres-Extensions#7) Verified with a full test suite run (206/206 passing, 0 skips) against the dedicated pg_tle-preloaded cluster. Changes only in pgxntool. No related changes in pgxntool-test. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…ive, document all override variables, add HISTORY entries - check-stale-expected (Postgres-Extensions#14) false-flagged pg_regress's alternate expected-output files (test_0.out .. test_9.out -- a real, widely-used convention: pg_tle's own test suite ships pg_tle_perms_1.out/pg_tle_versions_1.out, as do pgtap/pglogical). Now strips a trailing _N before checking for a matching .sql. - Consolidate EXTRA_CLEAN's filter-out and += into a single assignment (no reason they needed to be separate lines) - Add a `Configuration Variables` section to README.asc documenting every override variable in base.mk (PGXN_REMOTE, ASCIIDOC, PG_CONFIG, TESTDIR, TESTOUT, PGXNTOOL_VERIFY_RESULTS_MODE, and the three PGXNTOOL_ENABLE_* flags, PGXNTOOL_NO_PGXS_INCLUDE) -- previously only scattered partially across the doc, with several undocumented entirely - Add HISTORY.asc entries for the user-visible changes in this PR, most importantly the EXTENSION_VERSION_FILES rename since it can break a Makefile that references the old name directly Changes only in pgxntool. Related changes in pgxntool-test: - New test/standard/*.bats coverage for Postgres-Extensions#7, Postgres-Extensions#14, Postgres-Extensions#19, Postgres-Extensions#50, Postgres-Extensions#53 (see paired pgxntool-test PR) Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
… it after pg_regress, rename to EXTENSION__CURRENT_VERSION__FILES - check-stale-expected's logic moved to check-stale-expected.sh (enough code to warrant a real script over an inline recipe). The base/alt filename computation no longer tracks a separate found/not-found flag -- $base is always resolved to the right value before the one existence check. - check-stale-expected now depends on `installcheck` directly, so it runs after pg_regress rather than before. TEST_DEPS list position doesn't guarantee ordering between unrelated prerequisites; an explicit dependency edge does. - EXTENSION_CURRENT_VERSION_FILES renamed to EXTENSION__CURRENT_VERSION__FILES for clearer segmentation. - README.asc: converted the Configuration Variables section from AsciiDoc labeled lists to `===` subsections per variable, and flagged the yes/no-only variables with a `*` marker plus an explanatory note, instead of repeating the constraint in every entry. - Tightened the two new HISTORY.asc entries that were more verbose than the rest of the changelog's style. Changes only in pgxntool. Related changes in pgxntool-test: - Rewrote the check-stale-expected ordering test to verify the new dependency-based ordering via `make -n test` dry-run, and to prove make test only fails on a stale file after pg_regress has actually produced results - Added check-stale-expected.sh to the dist file-count manifest Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…re TEST_DEPS at the target, validate expected/ file types, add enable toggles - Moved check-stale-expected.sh to test/bin/check-stale-expected.sh (fixed its lib.sh source path accordingly) - TEST_DEPS += check-stale-expected now lives directly under the target's own definition, not down in the TEST_DEPS assembly section -- the base `TEST_DEPS = testdeps` assignment moved up ahead of it so the += there is well-defined - check-stale-expected.sh: added comments explaining the nullglob guard and the pg_regress alternate-file (_N.out) convention; removed the redundant existence check that decided whether to strip a _N suffix -- recognizing the pattern is pure string matching and doesn't need a stat call, so there's now exactly one existence check per file (documented tradeoff: a real test literally named foo_1 with no foo.sql would be misflagged -- accepted as vanishingly rare) - expected/ is now also checked for non-*.out files (stray editor files, .orig files, etc.), a distinct failure class from an orphaned .out file with its own message and exit-code bit, independently disable-able via PGXNTOOL_CHECK_EXPECTED_FILE_TYPES - Added PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED to make the whole check a no-op if wanted, following the same pattern as the other PGXNTOOL_ENABLE_* variables; documented both new variables in README.asc, plus a note pointing at Postgres-Extensions#30 for the larger override-any-recipe question this doesn't attempt to solve - README.asc: split PGXNTOOL_VERIFY_RESULTS_MODE's two modes into their own subsections instead of one paragraph Changes only in pgxntool. Related changes in pgxntool-test: - Update dist-expected-files.txt for the new script path - Extend check-stale-expected.bats for the new file-type check and both enable/disable toggles; fix the ordering test to filter out test-build's own (unrelated) pg_regress invocation, which the TEST_DEPS reordering above exposed Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
jnasbyupgrade
force-pushed
the
fix-buildsystem-batch
branch
from
July 27, 2026 17:38
33dfec3 to
9d0669d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
… an arg check-stale-expected.sh now takes the file-type check toggle as a second positional argument (<testdir> [check-file-types], default yes) instead of reading PGXNTOOL_CHECK_EXPECTED_FILE_TYPES from its environment. base.mk's check-stale-expected recipe passes the make variable through as that arg. This makes the script easier to unit-test directly: a test loop can vary an argument per-invocation, where an env var has to be set/unset around each call and can leak between cases. Add PGXNTOOL_CHECK_STALE_EXPECTED_SCRIPT, a make variable naming exactly which script path the check-stale-expected recipe invokes (default $(PGXNTOOL_DIR)/test/bin/check-stale-expected.sh). This lets a test swap in a throwaway stub script to prove the target does or doesn't invoke it, without needing to fake out all of PGXNTOOL_DIR -- which many unrelated targets (META.json, control.mk, verify-results, pgtle.sh, etc.) also read. Related changes in pgxntool-test: - Add PGXNTOOL_ENABLE_CHECK_STALE_EXPECTED=no coverage: a stub script, substituted via PGXNTOOL_CHECK_STALE_EXPECTED_SCRIPT, proves check-stale-expected.sh is never invoked when disabled (not just that a failure from it is ignored). - Split check-stale-expected's pure script-logic tests (orphan detection, _N.out alternate-file handling, the build/expected pair) out of make-test.bats into a new check-stale-expected-script.bats that invokes the script directly against a scratch directory, no foundation/make/ Postgres required. - Update the remaining check-stale-expected tests in make-test.bats to use the script's new arg-passing form directly, and the make-variable form through `make`. - Document the "narrow the interface, then stub that seam" pattern in CLAUDE.md for future reuse. Co-Authored-By: Claude <noreply@anthropic.com>
jnasbyupgrade
force-pushed
the
fix-buildsystem-batch
branch
from
July 27, 2026 21:45
5cf9f8f to
0fe9dfc
Compare
jnasbyupgrade
merged commit Jul 27, 2026
493a0f8
into
Postgres-Extensions:master
5 of 6 checks passed
jnasbyupgrade
added a commit
to Postgres-Extensions/pgxntool-test
that referenced
this pull request
Jul 27, 2026
…, base.mk fixes (#31) Related pgxntool PR: Postgres-Extensions/pgxntool#61 New BATS coverage in `test/standard/`: - **`versioned-sql-race.bats`** (#19): races two genuinely concurrent `make all` invocations against the same versioned-SQL target across 5 iterations, asserting the resulting file is never doubled. Verified by hand first that the *old* two-line recipe reproduces doubling (~1/3 of trials); the fixed single-redirect recipe never doubled across 15+ manual trials. - **`check-stale-expected.bats`** (#14): orphaned-file failure (both the standalone target and via `make test` before PostgreSQL is needed), the pg_regress alternate-output-file (`_N.out`) false-positive exemption plus a case proving the exemption doesn't over-apply when the alternate's own base `.sql` is also missing, and the same coverage for the `test/build`/`test/build/expected` pair. - **`base-mk-fixes.bats`** (#7, #50, #53): `EXTRA_CLEAN` targets the real `test/results/` directory and `make clean` actually removes it; `PGXN_REMOTE` defaults to `origin` and is overridable for both `tag` and `rmtag`; a throwaway Makefile that includes `pgxntool/base.mk` twice (directly and via an intermediate module) parses with no overriding-recipe warnings. **#28 (variable rename) deliberately has no new test** — pure mechanical rename, no behavior change, already exercised by the existing suite's `DATA`/build-output coverage. Full suite passes 228/228, 0 skips. --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
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.
>truncate then>>append across two recipe lines; a second firing (e.g. two overlappingmakeprocesses) doubled the file's content. Now a single atomic redirect.expected/#14:make testnever caught a staletest/expected/*.outleft behind after a test was renamed/removed. Addedcheck-stale-expected, tolerant of pg_regress's_N.outalternate-output files, and made it depend oninstallcheckdirectly so it runs after pg_regress, not before.originremote. Now configurable viaPGXN_REMOTE ?= origin.EXTENSION_VERSION_FILESis a confusing name #28:EXTENSION_VERSION_FILESrenamed toEXTENSION__CURRENT_VERSION__FILES-- old name implied it held all version files; it only ever holds the current one per extension.base.mkhad no include guard; double-inclusion redefined every target with warnings. Wrapped inifndef/endif.make clean'sEXTRA_CLEANtargeted a nonexistent top-levelresults/instead of$(TESTOUT)/results/.Adds a
Configuration Variablessection to README.asc documenting all override variables, and HISTORY.asc entries for the user-visible changes above.Related pgxntool-test PR: Postgres-Extensions/pgxntool-test#31
Fixes #7, #14, #19, #28, #50, #53.