Add new functions, enums#16
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
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>
1ca582e to
af30da3
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
CLAUDE.md (1)
26-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename extension update terminology in Makefile
The Makefile comment andupgrade_scripts_outstill use “upgrade” for extension version changes; rename them to “update scripts” /update_scripts_outto match the terminology rule.🤖 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 `@CLAUDE.md` around lines 26 - 38, Update the Makefile terminology from “upgrade” to “update” for extension version-change scripts: rename the upgrade_scripts_out variable to update_scripts_out and revise its related comment to say “update scripts,” preserving all existing generation behavior and references.Source: Learnings
sql/cat_tools--0.2.2--0.3.0.sql.in (1)
12-21: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStale function reference reintroduces its own "deprecated" warnings during the upgrade.
c_simple_argsis computed viacat_tools.function__arg_types_text(args). Later in this same script (line 572-586) that exact function isCREATE OR REPLACEd into a deprecated wrapper that doesRAISE WARNING 'function__arg_types_text() is deprecated...'and delegates toroutine__parse_arg_types_text. Every__cat_tools.create_function(...)call after that point (roughly a dozen+ remaining statements:cat_tools.regprocedure,object__catalog,_pg_sv_column_array,relation__is_temp/is_catalog/column_names,trigger__parse, etc.) will now silently route through the deprecated wrapper and spam a "deprecated" warning during the extension's ownALTER EXTENSION ... UPDATE, confusing users who never called the deprecated function themselves.The fresh-install script (
cat_tools.sql.in/cat_tools--0.3.0.sql.in) avoids this because itscreate_functionalready usescat_tools.routine__parse_arg_types_text(args)directly (bootstrapped via the temporary stub). This update script should do the same.🐛 Proposed fix
DECLARE - c_simple_args CONSTANT text := cat_tools.function__arg_types_text(args); + c_simple_args CONSTANT text := cat_tools.routine__parse_arg_types_text(args);Note: at line 21's point in the script,
cat_tools.routine__parse_arg_types_textdoesn't exist yet either — it's only created later via__cat_tools.create_functionitself (line 527+) — so this fix needs the same bootstrap-stub trick used in the fresh-install script (create a temporary pass-throughcat_tools.routine__parse_arg_types_text(text)stub before__cat_tools.create_functionis defined, matchingcat_tools.sql.inlines 74-81).🤖 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 `@sql/cat_tools--0.2.2--0.3.0.sql.in` around lines 12 - 21, Update __cat_tools.create_function to use cat_tools.routine__parse_arg_types_text(args) instead of the deprecated cat_tools.function__arg_types_text(args). Before defining __cat_tools.create_function, add the temporary pass-through routine__parse_arg_types_text(text) bootstrap stub used by the fresh-install script, then preserve the existing later replacement with the full implementation.
🤖 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.
Outside diff comments:
In `@CLAUDE.md`:
- Around line 26-38: Update the Makefile terminology from “upgrade” to “update”
for extension version-change scripts: rename the upgrade_scripts_out variable to
update_scripts_out and revise its related comment to say “update scripts,”
preserving all existing generation behavior and references.
In `@sql/cat_tools--0.2.2--0.3.0.sql.in`:
- Around line 12-21: Update __cat_tools.create_function to use
cat_tools.routine__parse_arg_types_text(args) instead of the deprecated
cat_tools.function__arg_types_text(args). Before defining
__cat_tools.create_function, add the temporary pass-through
routine__parse_arg_types_text(text) bootstrap stub used by the fresh-install
script, then preserve the existing later replacement with the full
implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fd44ba13-5ca3-459c-be71-1192a00be8a0
⛔ Files ignored due to path filters (2)
test/build/expected/build.outis excluded by!**/*.outtest/expected/relation__.outis excluded by!**/*.out
📒 Files selected for processing (9)
.github/workflows/ci.ymlCLAUDE.mdMakefilesql/cat_tools--0.2.2--0.3.0.sql.insql/cat_tools--0.3.0.sql.insql/cat_tools.sql.intest/.gitignoretest/gen-relkinds.shtest/sql/relation__.sql
|
@claude please review this PR. |
|
Claude finished @jnasbyupgrade's task in 3m 11s —— View job Reviewing PR #16
CI is currently green on the latest commit (af30da3). Below is a review focused on what's still outstanding at HEAD — CodeRabbit already caught and the author already fixed several rounds of issues (the Confirmed, not yet fixed1. Spurious deprecation warnings during every 0.2.2→0.3.0 update — c_simple_args CONSTANT text := cat_tools.function__arg_types_text(args);
RAISE WARNING 'function__arg_types_text() is deprecated, use routine__parse_arg_types_text instead';
RETURN cat_tools.routine__parse_arg_types_text(arguments);There are 7 more 2. This PR requires PG12+ (per 3. "X_comment": "Requires named argument support in SQL functions, which started in 9.2",This no longer matches the 4. No Confirmed, minor/edge-case, low priority5. v_on_clause := ' ON ' || r_trigger.tgrelid::pg_catalog.regclass || ' ';
v_array := regexp_split_to_array( v_work, v_on_clause );
Verified as already resolved (no action needed)
Not verified (no local Postgres available in this environment)I didn't have a way to actually run |
…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>
pg_procfields:routine_type,routine_argument_mode,routine_volatility,routine_parallel_safetyrelation__is_temp,relation__is_catalog,relation__column_namesrun-test-build.sh,verify-results-pgtap.sh, and more)attmissingvaland PG12+oidvisibility breakage in pre-0.2.2 scripts0.1.0,0.2.1,0.2.2) now tracked in git