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
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>
…ensions#28 DATA gap Extract the versioned-SQL generation (the $B/%.sql pattern rule, EXTENSION_VERSION_FILES override, SED-version machinery, versioned/upgrade name lists), the DATA handling, and the relkind drift-source generation out of Makefile into a new sql.mk that Makefile includes after base.mk. Makefile keeps a short pointer comment; the detail (and the critical two-phase-make explanation) lives in sql.mk. sql.mk documents GNU Make's two-phase model (parse/expand + directory-listing caching vs. recipe execution) and why a parse-time $(wildcard) over generated .sql misses them, framing Postgres-Extensions#28 as the canonical symptom of a general hazard. Fix Postgres-Extensions#28: the generated update scripts are now added to DATA by name via $(upgrade_scripts_out) (parse-stable, derived from .sql.in sources) with $(sort) to dedup against base.mk's wildcard, so a clean-tree build installs them and `ALTER EXTENSION UPDATE` no longer fails with "no update path". Replace the hardcoded historical-install list with a derived, parse-stable $(filter-out ...) over committed .sql (globbing committed files at parse time is safe; generated files are name-derived instead). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… params Rename the test-load mode value `upgrade` -> `update` (it is an extension UPDATE, not a PostgreSQL cluster upgrade) across the Makefile, load.sql, ci.yml and CLAUDE.md. The GUC name stays cat_tools.test_load_mode; only the value changes. Parameterize update mode: TEST_UPDATE_FROM (default 0.2.2) and TEST_UPDATE_TO (default empty -> current default_version) make vars, exported as cat_tools.test_update_from / cat_tools.test_update_to GUCs. load.sql update mode now CREATEs at :from and ALTER EXTENSION UPDATE [TO :to]. Add existing mode: the extension is already installed (by binary pg_upgrade or an external ALTER EXTENSION UPDATE). load.sql does not drop/create/update it; it asserts presence + current default_version (failing loudly otherwise) and creates the test roles idempotently. CI wiring: - pg-upgrade-test now installs 0.2.2, binary pg_upgrades it, ALTER EXTENSION UPDATEs to current, and runs the suite against the REAL pg_upgraded database in existing mode (CONTRIB_TESTDB=test EXTRA_REGRESS_OPTS=--use-existing) -- the old `make test` silently tested a fresh install. 0.2.0 cannot be used: pre-0.2.2 views reference catalog columns removed in newer PG and do not survive pg_upgrade. - extension-update-test runs 0.2.2 -> current in update mode on PG12+, plus a PG10 leg exercising the pre-0.2.2 update scripts (0.2.0->0.2.2, 0.2.1->0.2.2). - Version assertions now source EXPECTED from the build (print-PGXNVERSION / META.in.json) with an empty-value guard so they cannot false-pass (converges with master Postgres-Extensions#37). CLAUDE.md: converge Bug Fixes + Terminology conventions with master; document the test-load modes; add the full-URL rule for PR/issue references in committed files. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…o a script Real-database "existing" mode must run the suite against the ACTUAL upgraded/updated objects; a silent drop+reinstall would test a fresh install and hide a regression. Add a defense-in-depth dependency guard and prove it works: - test/ci/existing_mode.sh: shared, parameterized helper for the repeated install -> plant-guard -> update/upgrade -> assert-version -> run-suite flow. plant-guard creates a view whose column has a cat_tools enum type (a hard pg_depend edge; the enum is only ever ALTER TYPE ... ADD VALUE'd, so it survives updates and pg_upgrade) and then actively PROVES a non-CASCADE DROP EXTENSION fails -- if that drop unexpectedly succeeds the guard is ineffective and the script fails CI. run-suite re-proves the guard before the suite (it survived the update/upgrade) and confirms it is still present after (a CASCADE reinstall would have removed it). - ci.yml: pg-upgrade-test and extension-update-test now call the script instead of duplicating inline YAML. The primary safeguard remains load.sql, which never drops the extension in existing mode; the DB guard is belt-and-suspenders. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
87e6501 to
01262b6
Compare
…2.2 → pg_upgrade → current) The pre-0.2.2 update scripts exist for a real migration: a user on old PostgreSQL + old cat_tools must bridge to the pg_upgrade-safe 0.2.2 BEFORE pg_upgrading (the 0.2.0->0.2.2 script recreates the views with the omit_column fix; raw 0.2.0 views reference catalog columns removed in newer PG and break binary pg_upgrade). - test/ci/existing_mode.sh: add `prepare-old DB INSTALL [BRIDGE_TO]`. Installs the extension at INSTALL and, if BRIDGE_TO is given, ALTER EXTENSION UPDATE TO it before planting the dependency guard. (Uses `if`, not `&&`, so an empty bridge doesn't trip `set -e`.) - ci.yml pg-upgrade-test: add an old_pg=10 -> new_pg=18 leg that installs 0.2.0, bridges to 0.2.2 on PG10, plants the guard, binary pg_upgrades, updates to current, and runs the suite in existing mode. old_pg>=11 legs keep the direct 0.2.2 flow (0.2.0/0.2.1 don't install on PG11+). Matrix carries per-leg old_install/bridge_to. Drop-guard self-check and build-sourced version assertions retained. 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