[integration check] new_functions + pgxntool 2.1.0 (do not merge)#31
Closed
jnasbyupgrade wants to merge 97 commits into
Closed
[integration check] new_functions + pgxntool 2.1.0 (do not merge)#31jnasbyupgrade wants to merge 97 commits into
jnasbyupgrade wants to merge 97 commits into
Conversation
Biggest change is that starting with Postgres 12 OID columns in catalog tables were no longer hidden, which required adjusting a few views and tests. Also update pgxntool.
Also, refactor common code between it and function__arg_types()
- Eliminate duplicate NULL array creation logic by restructuring CASE statement - Use pronargs instead of parsing proargtypes text for array size - Move data type definitions and mapping functions to beginning of file - Improve code organization and readability
- Add enum types equivalent to relation_relkind for pg_proc fields: * routine_prokind and routine_type (function/procedure/aggregate/window) * routine_proargmode and routine_argument_mode (in/out/inout/variadic/table) * routine_provolatile and routine_volatility (immutable/stable/volatile) * routine_proparallel and routine_parallel_safety (safe/restricted/unsafe) - Remove explicit type grants, rely on default privileges - Clean up redundant comments in mapping functions - Add comprehensive test suite for new enum types and permissions
54793a3 Merge branch 'master' into upstream/stable ab7f6e2 Stamp 1.0.0 3a571ba Add pg_tle support and modernize test infrastructure (#11) b96ea6d Add support for Claude code; build and doc improvements (#9) e9c24de Fix pg_regress on versions > 12 (#5) git-subtree-dir: pgxntool git-subtree-split: 54793a39251290657767816d23b45d6297f3a671
Versioned files should now be kept in git. Also, fix missing .gitignore
3b8cb2a Stamp 1.1.0 550a901 Remove commit.md (maintained in pgxntool-test) d73ca93 Add unique test database names to prevent conflicts (Postgres-Extensions#13) 9b344be Add update-setup-files.sh for 3-way merging after pgxntool-sync (#12) REVERT: 54793a3 Merge branch 'master' into upstream/stable REVERT: bed3604 Fix pg_regress on versions > 12 (#5) (#6) git-subtree-dir: pgxntool git-subtree-split: 3b8cb2a96c2611bb44b1d69fd533fd0f23fa8995
639756c Stamp 1.1.1 6ba3176 Fix pg_tle exception handler and empty upgrade files (Postgres-Extensions#15) git-subtree-dir: pgxntool git-subtree-split: 639756c43a64717347b82b46acfec5be478a7bbf
PostgreSQL 15 (commit 07eee5a0) moved "char" from TYPCATEGORY_STRING to
TYPCATEGORY_INTERNAL, which removed the implicit I/O coercion path that
allowed "char"::some_enum to work. Replace the "char" overloads for
routine__type, routine__argument_mode, routine__volatility, and
routine__parallel_safety with explicit CREATE CAST ... WITH INOUT AS
IMPLICIT entries.
Also fix two bugs in trigger__parse exposed by PG17:
- current_setting('server_version')::real fails when the version string
includes a distro suffix; switch to server_version_num::int
- pg_get_triggerdef renders temp functions as "pg_temp.f" while ::regproc
gives "pg_temp_N.f", breaking the regexp split on the EXECUTE clause;
replace the name-based pattern with a generic EXECUTE (FUNCTION|PROCEDURE)
regex
Fix PG15+ permission denied on public schema in object_type tests by
granting CREATE ON SCHEMA public to the test role (PG15 revoked this
from PUBLIC by default).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add all new types, casts, and functions to the upgrade file that were
previously only in the versioned install file:
- New routine enum types: routine_prokind, routine_type, routine_proargmode,
routine_argument_mode, routine_provolatile, routine_volatility,
routine_proparallel, routine_parallel_safety, and composite routine_argument
- CREATE CAST ("char" AS ...) WITH INOUT AS IMPLICIT for the four prokind/
proargmode/provolatile/proparallel enums (PG15+ compatibility)
- Functions: routine__type, routine__argument_mode, routine__volatility,
routine__parallel_safety, and trigger__parse(oid) with PG17 fixes
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Resolve conflicts keeping new_functions branch changes: - CI: updated PG version matrix (18..10) and test command - pgxntool: version history, unique REGRESS_DBNAME - trigger__parse: generic EXECUTE regex pattern - Test expected output and setup for new functions
121f0b3 Stamp 2.0.0 ad3ca7e Remove .source support; add test/install, test/build, and verify-results (Postgres-Extensions#18) c010cf8 Fix bash 3.2 compatibility (Postgres-Extensions#23) abeb9d3 Remove .source file support from pg_regress integration (Postgres-Extensions#22) 08c1879 Stamp 1.1.2 6e0dad2 Fix double --dbname bug that defeated unique test database names git-subtree-dir: pgxntool git-subtree-split: 121f0b38fabd1d0ebd2c975254e59421553e2830
…ests - Use leading comma format in throws_ok call (trailing comma was wrong style) - Move CREATE FUNCTION pg_temp.test_function/named_function to just before the routine__arg_types tests where they are first used - Add \set named_args and use :'named_args' consistently throughout Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…irst use Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Fix backwards relkind c/f/m mapping in relation__kind/relation__relkind (was c->materialized view, f->composite type, m->foreign table; correct per pg_class.relkind is c=composite type, f=foreign table, m=materialized view). Fixed in both the fresh-install and 0.2.2->0.3.0 update scripts. - De-tautologize test/sql/relation__.sql: assert the mapping against hard-coded canonical (relkind, type) pairs instead of a positional zip of the two enums, which previously passed for any internally-consistent mapping (including the backwards one). - Revoke EXECUTE from PUBLIC on the _cat_tools helper functions in the update path so updated databases match fresh-install ACLs. - README typos; real 0.3.0 HISTORY bullets; bump PGXN PostgreSQL prereq to 12.0 to match the 0.3.0 support policy. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
These fixtures still described the routine__type(prokind "char") et al. function overloads that were replaced with implicit CREATE CAST ... WITH INOUT in 7473aa7; they were last regenerated against a dirty regression database, so they diverged from a clean fresh install. Regenerated from an actual clean test run (fresh install now creates a single routine__type). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Replace `grep -qF` version comparisons with an empty-value guard plus exact string equality; a partial or empty match previously passed. - Gate every `make test` step on `make verify-results`. pgxntool marks installcheck `.IGNORE`, so `make test` always exits 0 even when pg_regress reports failures; without this gate, regressions pass CI silently (the permissions/routine__ failures were green for months). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The 0.2.2→0.3.0 update was only tested on PG12. PG12 is the floor (ALTER TYPE ... ADD VALUE cannot run in an update script on PG11 and below), but there is no upper bound: 0.2.2 installs cleanly on PG12+, so the update runs on every version 0.3.0 supports. Verified locally on PG17. Expand the matrix to 12–18 and update the CLAUDE.md note accordingly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…E + manual GRANT
function__arg_to_regprocedure and function__drop_temp were created with a bare
CREATE FUNCTION followed by hand-written REVOKE-from-PUBLIC + GRANT statements.
That hand-written REVOKE is exactly what drifted between the fresh-install and
update scripts (the update path had dropped it). Route both helpers through
__cat_tools.create_function, which applies REVOKE ALL FROM public + GRANT to
cat_tools__usage from its templates, so the ACL is defined once and cannot
diverge between the two scripts.
In the fresh-install script create_function runs while routine__parse_arg_types_text
is still the pass-through stub, so the generated REVOKE/GRANT carry argument
names rather than bare types; PostgreSQL ignores argument names when matching a
function signature, so the resulting ACL is identical (verified
{root=X/root,cat_tools__usage=X/root} on both fresh install and the 0.2.2→0.3.0
update path). Regenerate test/build/expected/build.out for the two added void
SELECT outputs.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The relation__ test asserts the mapping against a hard-coded relkind data set (`kinds`). Add a drift check that compares that hard-coded set against the relkinds the PostgreSQL being built against actually defines: test/gen-relkinds.sh extracts every RELKIND_* value from the server's catalog/pg_class.h into a temp view, and the test fails if pg_class.h defines a relkind absent from `kinds` (i.e. PostgreSQL added or renamed one and the enum/mapping/test data need updating). A relkind in `kinds` that an older PostgreSQL lacks is ignored. Output is stable: the assertion is string_agg of the unknown relkinds, which is NULL (pass) on every supported version since the relkind set is identical across PG12–18. When postgresql-server-dev-NN is not installed the generated view is empty, giving the same NULL/pass, so `make test` produces identical output with or without the headers. The CI test job now installs the -server-dev package so the check runs for real on all matrix versions; jobs without it skip vacuously. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Broaden the GitHub CI instruction to cover every push, including a branch without a PR yet and pushes to master (use `gh run watch` for the commit when there is no PR). Note that docs-only pushes are excluded from CI via paths-ignore, so there is nothing to watch for those. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…lds Postgres-Extensions#26) Drop `SET LOCAL client_min_messages = WARNING;` from the top of the install script -- setting the message level is the caller's responsibility, not the install script's (deps.sql already does it for the main suite). The build sanity test (test/build/build.sql) is the one path that \i's the install script directly instead of going through deps.sql, so move the suppression there; without it build.out would capture verbose, version-specific NOTICEs (e.g. "%TYPE converted to regclass" with a source-file LOCATION line) and break across the PG matrix. build.out is unchanged. Folds in the change from PR Postgres-Extensions#26 (fix/client-min-messages) so it does not need its own PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the hand-rolled `is(string_agg(... WHERE relkind NOT IN ...), NULL)`
construction with `set_has('SELECT relkind FROM kinds', 'SELECT relkind FROM
pg_class_relkind_source')`. set_has is a subset assertion, which is exactly the
intended (asymmetric) semantics: every relkind pg_class.h defines must be in the
hard-coded `kinds` set, while `kinds` may list relkinds an older PostgreSQL
lacks, and an empty source (no server headers) is trivially contained. Equality
checks like bag_eq/set_eq would wrongly fail those two cases.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…olumn_names tests Those three sections each created their own throwaway temp table (is_temp_test, is_catalog_test, temp_test_table, plus a redundant test_table). They only need "a temp relation with known columns," so use a single shared rel_test for all of them. Drop the redundant test_table check (column_names on a fresh 2-column table is already covered by the multi-column and post-DROP COLUMN assertions on rel_test). Plan drops from 41 to 38; all assertions except that redundant one are preserved. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The relkind drift check degrades to an empty source view when the server headers are absent, which keeps `make test` working locally but would let the check pass in CI without actually running. Add a `make check-relkind-source` target that fails when the generated source has no relkinds, and run it before `make test` on every PostgreSQL version in both the `test` and `extension-update-test` jobs (which now also install postgresql-server-dev-NN). This guarantees the drift check genuinely runs on all versions in CI and can never silently pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
gen-relkinds was .PHONY, so the relkind drift source was regenerated on every `make`. Make it a real file target that rebuilds only when the generator script, the pg_class.h header, or the header *path* change. The path is tracked via a stamp file (test/generated/.relkind-header-path) that is rewritten only when the path differs, so switching PostgreSQL versions (pg_config -> a different header path) forces a rebuild even though the previous header file is untouched. Also add warn-relkind-source, run at the end of `make test`: if the source is empty because postgresql-server-dev-NN is missing, print a warning so a local run makes clear the drift check did not actually run. CI still hard-fails via check-relkind-source; local stays lenient. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ostgres-Extensions#24) Re-add the ability to run the FULL pgTAP suite against a database loaded via the extension upgrade path (CREATE EXTENSION VERSION '0.2.2' + ALTER EXTENSION UPDATE), reusing the same expected output as a fresh install. A correct upgrade must produce a database equivalent to a fresh install, so identical expected output passing in both modes is the equivalence check. Mechanism: pgxntool's native test/install feature, not a revived custom Makefile. The upgrade MUST be committed before the suite runs, because the 0.2.2->0.3.0 update uses ALTER TYPE ... ADD VALUE and PostgreSQL forbids using a newly added enum value in the transaction that added it -- so doing the upgrade inside deps.sql's per-test (rolled-back) transaction fails with 55P04. test/ install/load_upgrade.sql runs the CREATE+UPDATE committed, before the suite, in the same pg_regress session; test/deps.sql then skips its fresh CREATE in upgrade mode. This mirrors a real production upgrade. - test/deps.sql: branch on the cat_tools.test_load_mode placeholder GUC; fresh mode creates the extension per-test as before, upgrade mode leaves the committed, already-upgraded extension in place. - test/install/load_upgrade.sql: committed 0.2.2 install + UPDATE, deprecation NOTICEs suppressed. Only runs when the Makefile enables test/install. - Makefile: TEST_LOAD_SOURCE=upgrade sets the GUC via PGOPTIONS (inherited by the psql processes pg_regress spawns) and enables test/install; otherwise test/install is forced off so the default `make test` path is unchanged. - test/load_upgrade.sql: removed (orphaned since 1763a93, pinned to 0.2.0 which no longer installs on PG12+); replaced by test/install/load_upgrade.sql. - ci.yml: extension-update-test now runs its suite step with TEST_LOAD_SOURCE=upgrade, so it exercises the upgraded database instead of a fresh one. Version-number assertion, check-relkind-source and verify-results gates preserved. Verified: fresh mode unchanged (build + all 13) and upgrade mode green (build + 14, i.e. the 13 tests + the install step) on PG17 and PG12. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Defensive shell quoting: the target paths are fixed and space-free today, so this is a no-op in practice, but unquoted $@ would break if a path ever contained spaces or glob characters. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Document why the generated-source targets list test/generated after `|`: the directory must exist before writing into it, but its mtime (which bumps whenever any file is added/removed) must not be treated as a rebuild trigger. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… the package The check-relkind-source error and warn-relkind-source warning named the Debian package "postgresql-server-dev-NN", which is too environment-specific. The real point is that the catalog header was not found at the expected location, so the relkind drift check did not run — so report $(RELKIND_HEADER) and mention server development headers generically. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Move the per-test CREATE EXTENSION + role/grant setup out of test/deps.sql into a single committed-once installer, test/install/load.sql, run before the suite by pgxntool's test/install feature. It has two modes (fresh default, upgrade via TEST_LOAD_SOURCE) and drops the extension + roles first so re-runs always install the newest build. deps.sql is reduced to just the psql variables the suite needs. Add a make test-update wrapper and simplify the extension-update-test CI job. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…cipe before its callers Hide the generated-artifacts dir (dotfile) and move the `test/.generated` mkdir recipe above the stamp/source recipes that reference it via the order-only `| test/.generated` prerequisite, so a top-to-bottom reader sees the directory target defined before its uses. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…g test roles PART A (require the GUC): the Makefile now always exports cat_tools.test_load_mode (defaulting to fresh) and errors at parse time if TEST_LOAD_SOURCE is not exactly fresh/upgrade. load.sql reads the GUC without missing_ok, so a broken make->PGOPTIONS->env->psql chain fails loudly instead of silently running fresh; an invalid value is rejected by a RAISE. PART B (test roles): new test/roles.sql is the single source of truth for the two test-role names, \i'd by both load.sql and deps.sql. The roles are renamed to mixed-case identifiers (cat_tools_testing__USE / __NO_USE) that require double-quoting, so any unquoted use site fails loudly. Every :use_role / :no_use_role site that interpolates an identifier now uses :"..."; the has_*_privilege() sites keep :'...' (name as string literal). load.sql drops the roles via a pg_temp.drop_role() helper (pg_roles guard + DROP OWNED BY + format(%I)). Co-Authored-By: Claude Opus 4.8 <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:
✨ 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 |
eb84bc6 Stamp 2.1.0 9fbe6f4 Fix results ordering, control file whitespace, ENABLE_* override, debug levels (Postgres-Extensions#31) 88bb4f2 Add Claude Code GitHub Actions workflows (#41) af5bbbb ci: pass repo owners to run-tests.yml for fork-account matching (#40) c7928af Fix repo-root guard to work inside a git worktree (#39) b062fca ci: point reusable test workflow at @master b6cdbfd Add CI workflows and multi-session PR guard (Postgres-Extensions#33) 1ba0987 Stamp 2.0.3 1931cbe Fix pgxntool-sync remote and make it runnable without make (Postgres-Extensions#37) 8176304 Stamp 2.0.2 3e142ab Fix parse_control_file: remove comments before stripping quotes (Postgres-Extensions#27) cacc301 Stamp 2.0.1 bf1db6b Fix bash 3.2 / Linux compatibility issues (Postgres-Extensions#26) 62d0fcb Fix broken ifeq for --load-language=plpgsql on PG < 13 (Postgres-Extensions#24) 121f0b3 Stamp 2.0.0 ad3ca7e Remove .source support; add test/install, test/build, and verify-results (Postgres-Extensions#18) c010cf8 Fix bash 3.2 compatibility (Postgres-Extensions#23) abeb9d3 Remove .source file support from pg_regress integration (Postgres-Extensions#22) 08c1879 Stamp 1.1.2 6e0dad2 Fix double --dbname bug that defeated unique test database names 639756c Stamp 1.1.1 6ba3176 Fix pg_tle exception handler and empty upgrade files (Postgres-Extensions#15) 3b8cb2a Stamp 1.1.0 550a901 Remove commit.md (maintained in pgxntool-test) d73ca93 Add unique test database names to prevent conflicts (Postgres-Extensions#13) 9b344be Add update-setup-files.sh for 3-way merging after pgxntool-sync (#12) ab7f6e2 Stamp 1.0.0 3a571ba Add pg_tle support and modernize test infrastructure (#11) b96ea6d Add support for Claude code; build and doc improvements (#9) e9c24de Fix pg_regress on versions > 12 (#5) git-subtree-dir: pgxntool git-subtree-split: eb84bc6e87e21f2ced11ff1b8ddb4028b7c67c8f
…2.1.0 # Conflicts: # pgxntool/.gitattributes # pgxntool/CLAUDE.md # pgxntool/HISTORY.asc # pgxntool/README.asc # pgxntool/README.html # pgxntool/base.mk # pgxntool/lib.sh # pgxntool/pgtle.sh # pgxntool/pgxntool-sync.sh # pgxntool/setup.sh # pgxntool/update-setup-files.sh
update-setup-files.sh removes pgxntool's own CI and Claude config from the embedded copy; these are dev-only files not meant for embedding projects.
jnasbyupgrade
force-pushed
the
new_functions-pgxntool-2.1.0
branch
from
aed8291 to
5e6624a
Compare
Contributor
Author
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.
DO NOT MERGE — integration/CI check only
Validates that the PR #16 (
new_functions) work still passes on top of pgxntool 2.1.0, since #16 will eventually sit on a master bumped to 2.1.0 (see #30). Not intended to be merged.What's in it
master(currently pgxntool 2.0.3 + the Claude workflows from #27) + the pgxntool 2.1.0 bump (#30) +origin/new_functionsmerged in.The merge of
new_functionswas clean — no conflicts. The new_functions changes live in the repo-rootMakefile,test/, andsql/; the pgxntool bump only touchespgxntool/, so the two are disjoint. The vendoredpgxntool/tree is byte-for-byte identical to upstream 2.1.0 (minus the pruned dev-only dirs).Result: pgxntool 2.1.0 is compatible with our test harness
Verified locally on PG17, and compared against a plain
origin/new_functionsbaseline (pgxntool 2.0.2) — behavior is identical:make test-update(upgrade mode): fully green — test-build (build+upgrade) and all 14 main tests pass.make test(fresh mode): all 14 main tests pass;make verify-resultsexits 0.PGXNTOOL_ENABLE_TEST_INSTALLcommitted-once mechanism, the relkind drift check, and upgrade mode all work under 2.1.0.One pre-existing quirk: in fresh mode only, the
test-buildbuild upgradeschedule fails withno update path from version "0.2.2" to version "0.3.0"(the generated0.2.2--0.3.0upgrade script isn't installed into the extension dir when built fresh). This reproduces identically on a cleanorigin/new_functionscheckout with pgxntool 2.0.2 — it is not introduced by the 2.1.0 bump, and does not affectverify-results.