Add new functions, enums

[jnasbyupgrade]

• New enum types for pg_proc fields: routine_type, routine_argument_mode, routine_volatility, routine_parallel_safety
• New relation functions: relation__is_temp, relation__is_catalog, relation__column_names
• New relkind entries and permission tests
• Version bump to 0.3.0
• Updated pgxntool to 2.0.2 (adds pg_tle support, run-test-build.sh, verify-results-pgtap.sh, and more)
• CI expanded to PG 10–18; upgrade test matrix accounts for PG11+ attmissingval and PG12+ oid visibility breakage in pre-0.2.2 scripts
• Historical SQL install files (0.1.0, 0.2.1, 0.2.2) now tracked in git

[coderabbitai]

📝 Walkthrough

Walkthrough

This pull request delivers cat_tools v0.3.0, a significant extension release that introduces new routine and partitioned relation introspection APIs while narrowing PostgreSQL version support to 12+. The release updates metadata and version strings, refactors the CI/build infrastructure to remove PG10/11 testing, implements two new SQL scripts (upgrade and fresh install), adds helper utilities for routine argument parsing and trigger introspection, and provides comprehensive test coverage including security boundary validation and functional behavior assertions.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

🐰 From v0.2 to 0.3, routines now shine bright,
With triggers and partitions—oh what a sight!
PG12 and beyond, we've streamlined our way,
New tests ensure all APIs work each day! 🎉

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title 'Add new functions, enums' is vague and overly broad, using generic phrasing that fails to convey the main change effectively.	Use a more descriptive title that highlights the primary change, such as 'Release v0.3.0 with new routine and relation APIs' or 'Add routine enum types and relation helper functions'.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description check	✅ Passed	The description clearly relates to the changeset, providing specific details about the new enum types, relation functions, version bump, and infrastructure updates.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[jnasbyupgrade]

@coderabbitai full review and please use the github review functionality

[coderabbitai]

Actionable comments posted: 6

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

sql/cat_tools.sql.in (1)

1785-1787: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Don't feed an unescaped relation name into a regex split.

v_on_clause includes r_trigger.tgrelid::pg_catalog.regclass, then gets passed straight to regexp_split_to_array. Valid quoted identifiers can contain ., +, (, etc., so trigger__parse() will mis-parse or fail on those names. Prefer literal slicing with strpos/substr, or escape the pattern before calling the regex function. Please mirror the same fix into sql/cat_tools--0.2.2--0.3.0.sql.in, which embeds the same parser.

🤖 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.sql.in` around lines 1785 - 1787, The code builds v_on_clause
using r_trigger.tgrelid::pg_catalog.regclass and then passes it into
regexp_split_to_array, which can break on valid quoted relation names; update
trigger__parse() to avoid using an unescaped regex pattern by either escaping
v_on_clause before calling regexp_split_to_array or, preferably, replace the
regex split with literal slicing using strpos/substr on v_work and v_on_clause
to extract parts into v_array and events; apply the same change to the duplicate
parser in sql/cat_tools--0.2.2--0.3.0.sql.in so both copies use safe literal
splitting or properly escaped patterns.

🤖 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 @.github/workflows/ci.yml:
- Around line 63-65: The EXPECTED_VERSION extraction using sed should be
hardened: after running the sed extraction that sets EXPECTED_VERSION in the
environment, add an explicit check that EXPECTED_VERSION is non-empty and fail
fast (exit non-zero) if empty; similarly, wherever you compare VERSION to
EXPECTED_VERSION (currently using grep -qF), replace those checks with explicit
non-empty validation of VERSION and an exact string equality comparison [
"$VERSION" = "$EXPECTED_VERSION" ] (i.e., ensure VERSION is not empty, then
compare exact equality), and remove the use of grep -qF to avoid partial
matches; reference the variables EXPECTED_VERSION and VERSION and the sed
extraction that writes to GITHUB_ENV when applying these changes.

In `@HISTORY.asc`:
- Line 17: Replace the "TODO" placeholder under the 0.3.0 release heading in
HISTORY.asc with the actual change bullets for version 0.3.0: list concise,
user-facing entries (bug fixes, new features, breaking changes, and notable
internal changes) as individual dash-prefixed lines so the release section is
complete and readable; ensure the bullets reference the corresponding
commits/PRs or short descriptions and keep formatting consistent with other
release sections in the file.

In `@META.in.json`:
- Line 17: Update the advertised PostgreSQL prerequisite to match the package
version: change the prereqs.build.requires.PostgreSQL value in META.in.json
(look for the JSON key path prereqs.build.requires.PostgreSQL) from "9.2.0" to
"12.0" so it aligns with "version": "0.3.0", then regenerate META.json from
META.in.json to propagate the change (ensure the updated value appears in the
generated META.json).

In `@README.asc`:
- Line 33: Remove the stray trailing backtick in the `routine_volatility` bullet
(the text currently reads "* `routine_volatility` - Volatility levels for
functions/procedures`") and correct the possessive on line 87 from "it's full
set of arguments" to "its full set of arguments"; update the README.asc entries
accordingly, keeping the backticks around `routine_volatility` but deleting the
extra backtick at the end and fixing the contraction to the possessive form
"its".

In `@sql/cat_tools--0.2.2--0.3.0.sql.in`:
- Around line 167-169: Revoke the default PUBLIC privileges for the _cat_tools
helpers before granting to cat_tools__usage: add statements to REVOKE USAGE ON
SCHEMA _cat_tools FROM PUBLIC and REVOKE EXECUTE ON FUNCTION for each helper
function (_cat_tools.function__arg_to_regprocedure(text,text,text) and
_cat_tools.function__drop_temp(pg_catalog.regprocedure,text)) from PUBLIC,
placed immediately before the existing GRANT lines so updated databases don't
retain broader ACLs.

In `@sql/cat_tools.sql.in`:
- Around line 372-414: The relkind mappings are wrong in the SQL functions
cat_tools.relation__kind and cat_tools.relation__relkind: swap the labels so
that 'c' maps to 'composite type', 'f' maps to 'foreign table', and 'm' maps to
'materialized view' in relation__kind, and reverse those mappings in
relation__relkind (i.e. kind 'composite type' -> 'c', 'foreign table' -> 'f',
'materialized view' -> 'm') so fresh installs match the 0.2.2→0.3.0 update path.

---

Outside diff comments:
In `@sql/cat_tools.sql.in`:
- Around line 1785-1787: The code builds v_on_clause using
r_trigger.tgrelid::pg_catalog.regclass and then passes it into
regexp_split_to_array, which can break on valid quoted relation names; update
trigger__parse() to avoid using an unescaped regex pattern by either escaping
v_on_clause before calling regexp_split_to_array or, preferably, replace the
regex split with literal slicing using strpos/substr on v_work and v_on_clause
to extract parts into v_array and events; apply the same change to the duplicate
parser in sql/cat_tools--0.2.2--0.3.0.sql.in so both copies use safe literal
splitting or properly escaped patterns.

🪄 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: cb7a426f-c0df-4bf9-8378-d71916ce2b42

📥 Commits

Reviewing files that changed from the base of the PR and between 88c3893 and 434ef45.

⛔ Files ignored due to path filters (8)

• test/build/expected/build.out is excluded by !**/*.out
• test/build/expected/upgrade.out is excluded by !**/*.out
• test/expected/function.out is excluded by !**/*.out
• test/expected/object_type.out is excluded by !**/*.out
• test/expected/permissions.out is excluded by !**/*.out
• test/expected/relation__.out is excluded by !**/*.out
• test/expected/relation_type.out is excluded by !**/*.out
• test/expected/routine__.out is excluded by !**/*.out

📒 Files selected for processing (26)

• .github/workflows/ci.yml
• .gitignore
• CLAUDE.md
• HISTORY.asc
• LICENSE
• META.in.json
• META.json
• Makefile
• README.asc
• cat_tools.control
• control.mk
• sql/cat_tools--0.2.2--0.3.0.sql.in
• sql/cat_tools--0.3.0.sql.in
• sql/cat_tools.sql.in
• test/build/build.sql
• test/build/upgrade.sql
• test/deps.sql
• test/helpers/enum_mapping.sql
• test/load_new.sql
• test/setup.sql
• test/sql/function.sql
• test/sql/object_type.sql
• test/sql/permissions.sql
• test/sql/relation__.sql
• test/sql/relation_type.sql
• test/sql/routine__.sql

💤 Files with no reviewable changes (3)

• test/load_new.sql
• test/sql/relation_type.sql
• .gitignore

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Verify parser returns a non-empty expected version from repo control file.
EXPECTED_VERSION="$(sed -n "s/^default_version = '\(.*\)'$/\1/p" cat_tools.control)"
printf 'Parsed EXPECTED_VERSION=<%s>\n' "$EXPECTED_VERSION"
if [ -z "$EXPECTED_VERSION" ]; then
  echo "FAIL: EXPECTED_VERSION is empty (current grep check would false-pass)."
  exit 1
fi

# Demonstrate why grep is unsafe for equality assertions.
VERSION_PARTIAL="${EXPECTED_VERSION}-extra"
if echo "$VERSION_PARTIAL" | grep -qF "$EXPECTED_VERSION"; then
  echo "grep check passes on partial match: $VERSION_PARTIAL"
fi

# Exact check should fail for partials.
if [ "$VERSION_PARTIAL" = "$EXPECTED_VERSION" ]; then
  echo "Unexpected exact match"
  exit 1
else
  echo "Exact equality check correctly rejects partial match."
fi

Repository: Postgres-Extensions/cat_tools

Length of output: 207

---

🏁 Script executed:

cat -n .github/workflows/ci.yml | sed -n '50,150p'

Repository: Postgres-Extensions/cat_tools

Length of output: 6322

---

Harden EXPECTED_VERSION parsing and use exact version equality checks.

Current checks can false-pass: empty EXPECTED_VERSION from parsing always passes grep -qF, and partial matches pass too. Fail fast if parsing returns empty, then compare exact strings.

• Lines 65, 131: Add empty value validation after sed extracts default_version
• Lines 102, 141: Replace grep -qF with exact equality [ "$VERSION" = "$EXPECTED_VERSION" ] and validate VERSION is not empty

Suggested patch

       - name: Set expected extension version
         run: |
-          echo "EXPECTED_VERSION=$(sed -n "s/^default_version = '\(.*\)'$/\1/p" cat_tools.control)" >> $GITHUB_ENV
+          EXPECTED_VERSION="$(sed -n "s/^default_version = '\(.*\)'$/\1/p" cat_tools.control)"
+          test -n "$EXPECTED_VERSION" || { echo "Failed to parse default_version from cat_tools.control"; exit 1; }
+          echo "EXPECTED_VERSION=$EXPECTED_VERSION" >> "$GITHUB_ENV"
@@
       - name: Verify extension version after upgrade
         run: |
           VERSION=$(psql -tAc "SELECT extversion FROM pg_extension WHERE extname = 'cat_tools'")
           echo "Extension version: ${VERSION:-<not found>}"
-          echo "$VERSION" | grep -qF "$EXPECTED_VERSION"
+          test -n "$VERSION" || { echo "cat_tools extension version not found"; exit 1; }
+          [ "$VERSION" = "$EXPECTED_VERSION" ]
@@
       - name: Set expected extension version
         run: |
-          echo "EXPECTED_VERSION=$(sed -n "s/^default_version = '\(.*\)'$/\1/p" cat_tools.control)" >> $GITHUB_ENV
+          EXPECTED_VERSION="$(sed -n "s/^default_version = '\(.*\)'$/\1/p" cat_tools.control)"
+          test -n "$EXPECTED_VERSION" || { echo "Failed to parse default_version from cat_tools.control"; exit 1; }
+          echo "EXPECTED_VERSION=$EXPECTED_VERSION" >> "$GITHUB_ENV"
@@
       - name: Test update from 0.2.2 (0.2.2→current path)
         run: |
           createdb cat_tools_from_022
           psql -d cat_tools_from_022 -c "CREATE EXTENSION cat_tools VERSION '0.2.2'"
           psql -d cat_tools_from_022 -c "ALTER EXTENSION cat_tools UPDATE"
           VERSION=$(psql -d cat_tools_from_022 -tAc "SELECT extversion FROM pg_extension WHERE extname = 'cat_tools'")
           echo "Version after 0.2.2 update: ${VERSION:-<not found>}"
-          echo "$VERSION" | grep -qF "$EXPECTED_VERSION"
+          test -n "$VERSION" || { echo "cat_tools extension version not found after update"; exit 1; }
+          [ "$VERSION" = "$EXPECTED_VERSION" ]

🤖 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 @.github/workflows/ci.yml around lines 63 - 65, The EXPECTED_VERSION
extraction using sed should be hardened: after running the sed extraction that
sets EXPECTED_VERSION in the environment, add an explicit check that
EXPECTED_VERSION is non-empty and fail fast (exit non-zero) if empty; similarly,
wherever you compare VERSION to EXPECTED_VERSION (currently using grep -qF),
replace those checks with explicit non-empty validation of VERSION and an exact
string equality comparison [ "$VERSION" = "$EXPECTED_VERSION" ] (i.e., ensure
VERSION is not empty, then compare exact equality), and remove the use of grep
-qF to avoid partial matches; reference the variables EXPECTED_VERSION and
VERSION and the sed extraction that writes to GITHUB_ENV when applying these
changes.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Replace placeholder TODO with actual 0.3.0 change bullets.

Line 17 leaves the release section incomplete, which blocks usable release notes for this version.

🤖 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 `@HISTORY.asc` at line 17, Replace the "TODO" placeholder under the 0.3.0
release heading in HISTORY.asc with the actual change bullets for version 0.3.0:
list concise, user-facing entries (bug fixes, new features, breaking changes,
and notable internal changes) as individual dash-prefixed lines so the release
section is complete and readable; ensure the bullets reference the corresponding
commits/PRs or short descriptions and keep formatting consistent with other
release sections in the file.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Update PGXN prerequisite version to PostgreSQL 12+ to match 0.3.0 support policy.

With 0.3.0 declared on Lines 17 and 40, keeping prereqs.build.requires.PostgreSQL at 9.2.0 (Line 75) advertises unsupported versions and can trigger broken installs. Please set the minimum to 12.0 in META.in.json and regenerate META.json.

Suggested fix

--- a/META.in.json
+++ b/META.in.json
@@
-                "PostgreSQL": "9.2.0"
+                "PostgreSQL": "12.0"

Based on learnings: “PostgreSQL 10 and 11 are dropped as of version 0.3.0… cat_tools 0.3.0 supports PG12+.”

Also applies to: 40-40

🤖 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 `@META.in.json` at line 17, Update the advertised PostgreSQL prerequisite to
match the package version: change the prereqs.build.requires.PostgreSQL value in
META.in.json (look for the JSON key path prereqs.build.requires.PostgreSQL) from
"9.2.0" to "12.0" so it aligns with "version": "0.3.0", then regenerate
META.json from META.in.json to propagate the change (ensure the updated value
appears in the generated META.json).

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Fix two user-facing documentation typos.

• Line 33 has an extra trailing backtick after functions/procedures.
• Line 87 should be “its full set of arguments,” not “it's full set of arguments.”

Also applies to: 87-87

🤖 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` at line 33, Remove the stray trailing backtick in the
`routine_volatility` bullet (the text currently reads "* `routine_volatility` -
Volatility levels for functions/procedures`") and correct the possessive on line
87 from "it's full set of arguments" to "its full set of arguments"; update the
README.asc entries accordingly, keeping the backticks around
`routine_volatility` but deleting the extra backtick at the end and fixing the
contraction to the possessive form "its".

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Revoke the default PUBLIC execute grants here as well.

Fresh installs explicitly revoke PUBLIC on these helpers before granting cat_tools__usage, but the update path does not. That leaves updated databases with broader ACLs than new installs for the same release.

Suggested patch

 GRANT USAGE ON SCHEMA _cat_tools TO cat_tools__usage;
+REVOKE EXECUTE ON FUNCTION _cat_tools.function__arg_to_regprocedure(text, text, text) FROM PUBLIC;
 GRANT EXECUTE ON FUNCTION _cat_tools.function__arg_to_regprocedure(text, text, text) TO cat_tools__usage;
+REVOKE EXECUTE ON FUNCTION _cat_tools.function__drop_temp(pg_catalog.regprocedure, text) FROM PUBLIC;
 GRANT EXECUTE ON FUNCTION _cat_tools.function__drop_temp(pg_catalog.regprocedure, text) TO cat_tools__usage;

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	GRANT USAGE ON SCHEMA _cat_tools TO cat_tools__usage;
	GRANT EXECUTE ON FUNCTION _cat_tools.function__arg_to_regprocedure(text, text, text) TO cat_tools__usage;
	GRANT EXECUTE ON FUNCTION _cat_tools.function__drop_temp(pg_catalog.regprocedure, text) TO cat_tools__usage;
	GRANT USAGE ON SCHEMA _cat_tools TO cat_tools__usage;
	REVOKE EXECUTE ON FUNCTION _cat_tools.function__arg_to_regprocedure(text, text, text) FROM PUBLIC;
	GRANT EXECUTE ON FUNCTION _cat_tools.function__arg_to_regprocedure(text, text, text) TO cat_tools__usage;
	REVOKE EXECUTE ON FUNCTION _cat_tools.function__drop_temp(pg_catalog.regprocedure, text) FROM PUBLIC;
	GRANT EXECUTE ON FUNCTION _cat_tools.function__drop_temp(pg_catalog.regprocedure, text) TO cat_tools__usage;

🤖 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 167 - 169, Revoke the
default PUBLIC privileges for the _cat_tools helpers before granting to
cat_tools__usage: add statements to REVOKE USAGE ON SCHEMA _cat_tools FROM
PUBLIC and REVOKE EXECUTE ON FUNCTION for each helper function
(_cat_tools.function__arg_to_regprocedure(text,text,text) and
_cat_tools.function__drop_temp(pg_catalog.regprocedure,text)) from PUBLIC,
placed immediately before the existing GRANT lines so updated databases don't
retain broader ACLs.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Fix the swapped c / f / m relkind mappings.

These branches are backwards: c is composite type, f is foreign table, and m is materialized view. As written, fresh installs return different results from the 0.2.2→0.3.0 update path, which already maps them correctly.

Suggested patch

 SELECT CASE relkind
   WHEN 'r' THEN 'table'
   WHEN 'i' THEN 'index'
   WHEN 'S' THEN 'sequence'
   WHEN 't' THEN 'toast table'
   WHEN 'v' THEN 'view'
-  WHEN 'c' THEN 'materialized view' -- composite type (but mapped to materialized view)
-  WHEN 'f' THEN 'composite type' -- foreign table (but mapped to composite type)
-  WHEN 'm' THEN 'foreign table' -- materialized view (but mapped to foreign table)
+  WHEN 'c' THEN 'composite type'
+  WHEN 'f' THEN 'foreign table'
+  WHEN 'm' THEN 'materialized view'
   WHEN 'p' THEN 'partitioned table'
   WHEN 'I' THEN 'partitioned index'
 END::cat_tools.relation_type
@@
 SELECT CASE kind
   WHEN 'table' THEN 'r'
   WHEN 'index' THEN 'i'
   WHEN 'sequence' THEN 'S'
   WHEN 'toast table' THEN 't'
   WHEN 'view' THEN 'v'
-  WHEN 'materialized view' THEN 'c' -- materialized view (mapped from c)
-  WHEN 'composite type' THEN 'f' -- composite type (mapped from f)
-  WHEN 'foreign table' THEN 'm' -- foreign table (mapped from m)
+  WHEN 'composite type' THEN 'c'
+  WHEN 'foreign table' THEN 'f'
+  WHEN 'materialized view' THEN 'm'
   WHEN 'partitioned table' THEN 'p'
   WHEN 'partitioned index' THEN 'I'
 END::cat_tools.relation_relkind

🤖 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.sql.in` around lines 372 - 414, The relkind mappings are wrong
in the SQL functions cat_tools.relation__kind and cat_tools.relation__relkind:
swap the labels so that 'c' maps to 'composite type', 'f' maps to 'foreign
table', and 'm' maps to 'materialized view' in relation__kind, and reverse those
mappings in relation__relkind (i.e. kind 'composite type' -> 'c', 'foreign
table' -> 'f', 'materialized view' -> 'm') so fresh installs match the
0.2.2→0.3.0 update path.

[coderabbitai]

🧩 Analysis chain

@jnasbyupgrade I'll gather the full diff and file details now, then submit a proper GitHub review.

---

🏁 Script executed:

#!/bin/bash
# Get PR details and file list
gh pr view 16 --repo Postgres-Extensions/cat_tools --json title,body,files,additions,deletions,commits
echo "---FILES---"
gh pr diff 16 --repo Postgres-Extensions/cat_tools

Length of output: 270551

---

@jnasbyupgrade I've read through the full diff. Submitting the review now via GitHub's review API.

---

🏁 Script executed:

#!/bin/bash
set -e

# First, gather file contents for accurate line numbers
gh api repos/Postgres-Extensions/cat_tools/pulls/16/files --jq '.[].filename' | head -40

# Get the PR's head SHA for the review
gh api repos/Postgres-Extensions/cat_tools/pulls/16 --jq '.head.sha'

Length of output: 669

---

🏁 Script executed:

#!/bin/bash
set -e

# Get line numbers of key issues in files by looking at file content
# 1. routine__.out - error in expected output
echo "=== test/expected/routine__.out ==="
cat -n test/expected/routine__.out

echo ""
echo "=== README.asc supported versions ==="
grep -n "Supported\|9\.\|12\|Postgres" README.asc | head -20

echo ""
echo "=== HISTORY.asc TODO ==="
grep -n "TODO\|Changes\|0\.3\.0" HISTORY.asc | head -15

echo ""
echo "=== relation__kind trailing whitespace in cat_tools.sql.in ==="
grep -n "pg_catalog.pg_class $\|pg_class $" sql/cat_tools.sql.in | head -10

echo ""
echo "=== relation__kind mapping comments in cat_tools--0.3.0.sql.in ==="
grep -n "but mapped\|composite type\|materialized view\|foreign table" sql/cat_tools--0.3.0.sql.in | head -20

echo ""
echo "=== REVOKE in 0.2.2--0.3.0 upgrade script ==="
grep -n "REVOKE\|GRANT.*function__arg_to\|GRANT.*function__drop" sql/cat_tools--0.2.2--0.3.0.sql.in

echo ""
echo "=== enum_mapping.sql line 65 area ==="
sed -n '60,70p' test/helpers/enum_mapping.sql

echo ""
echo "=== routine__.sql - enum_mapping.sql call area ==="
grep -n "enum_mapping\|sample_char\|sample_text\|kind " test/sql/routine__.sql | head -30

Length of output: 3388

---

🏁 Script executed:

#!/bin/bash
set -e

SHA="434ef45ba69986a6cb4561302ea12a0067dca115"

# Verify specific line numbers in the upgrade script
echo "=== upgrade script lines 165-172 ==="
sed -n '165,172p' sql/cat_tools--0.2.2--0.3.0.sql.in

echo ""
echo "=== routine__.out full ==="
cat test/expected/routine__.out

echo ""
echo "=== enum_mapping.sql lines 60-70 with numbers ==="
sed -n '60,70p' test/helpers/enum_mapping.sql | cat -A

echo ""
echo "=== README.asc lines 20-26 ==="
sed -n '20,26p' README.asc

echo ""
echo "=== HISTORY.asc lines 14-20 ==="
sed -n '14,20p' HISTORY.asc

echo ""
echo "=== cat_tools.sql.in lines 1610-1632 (trailing space) ==="
sed -n '1610,1632p' sql/cat_tools.sql.in | cat -A

echo ""
echo "=== cat_tools--0.3.0.sql.in lines 380-410 (relkind mapping) ==="
sed -n '380,412p' sql/cat_tools--0.3.0.sql.in

echo ""
echo "=== test/build/upgrade.sql ==="
cat -n test/build/upgrade.sql

echo ""
echo "=== cat_tools--0.2.2--0.3.0.sql.in lines 255-175 - check REVOKE near GRANT ==="
grep -n "REVOKE\|GRANT" sql/cat_tools--0.2.2--0.3.0.sql.in

Length of output: 5113

---

🏁 Script executed:

#!/bin/bash
set -e

SHA="434ef45ba69986a6cb4561302ea12a0067dca115"
REPO="Postgres-Extensions/cat_tools"
PR=16

# Submit the full review with inline comments
gh api \
  repos/$REPO/pulls/$PR/reviews \
  --method POST \
  --field commit_id="$SHA" \
  --field event="REQUEST_CHANGES" \
  --field body="## Review Summary

This is a substantial and well-structured release — the new enum taxonomy for `pg_proc` fields, the relation helper functions, the cross-version CI matrix, and the dynamic permissions test suite are all strong improvements.

There is one **blocking** issue (a committed test failure in `routine__.out`) and several smaller bugs/doc gaps that should be addressed before merge.

### Blocking
- **`test/expected/routine__.out` contains a live psql ERROR** — the test suite is failing as committed (see inline comment).

### Bugs
- Missing \`REVOKE EXECUTE FROM PUBLIC\` in the 0.2.2→0.3.0 upgrade script before the GRANTs on \`_cat_tools\` helpers.
- \`test/build/upgrade.sql\` wraps the upgrade in \`BEGIN/ROLLBACK\`, so \`ALTER EXTENSION cat_tools UPDATE\` never actually commits; there is no post-update version assertion.

### Documentation
- \`HISTORY.asc\` 0.3.0 **Changes** section is still a \`// TODO\` placeholder.
- \`README.asc\` still states "Works on Postgres 9.3 and above" but PG10/11 support was dropped in this PR.

### Style/minor
- Trailing whitespace in the SQL body strings for \`relation__is_temp\` and \`relation__is_catalog\`.
- Confusing backward-compat comments on the relkind CASE mapping." \
  --field "comments[][path]=test/expected/routine__.out" \
  --field "comments[][line]=4" \
  --field "comments[][side]=RIGHT" \
  --field "comments[][body]=**Blocking: committed test failure.**

Lines 4–8 are a verbatim psql \`ERROR\`, not a passing TAP line. The test file produced this error, was captured, and committed as the expected output — which means \`pg_regress\` will accept it as \"passing\" only if the real run emits the identical error. Any fix to \`enum_mapping.sql\` will break CI; any PG version that renders a different error message will also break CI.

Root cause: \`enum_mapping.sql\` line 62 calls
\`\`\`sql
:s.routine__:kind(:'sample_char')
\`\`\`
When \`kind=type\` and \`sample_char='f'\`, \`:'sample_char'\` is an **untyped string literal**. PostgreSQL cannot choose between the \`routine_prokind\` overload and the implicit \`CREATE CAST (\"char\" AS cat_tools.routine_prokind) WITH INOUT AS IMPLICIT\` path, so it raises \`42725: function is not unique\`.

Fix: cast to \`\"char\"\` explicitly in enum_mapping.sql:
\`\`\`sql
:s.routine__:kind(:'sample_char'::\"char\")
\`\`\`
Then regenerate the expected output." \
  --field "comments[][path]=sql/cat_tools--0.2.2--0.3.0.sql.in" \
  --field "comments[][line]=168" \
  --field "comments[][side]=RIGHT" \
  --field "comments[][body]=The install script (\`cat_tools--0.3.0.sql.in\`) explicitly revokes PUBLIC execute on these two helpers before granting to \`cat_tools__usage\`, but the upgrade script omits the \`REVOKE\`. As a result, users upgrading from 0.2.2 to 0.3.0 will have \`PUBLIC\` execute access on \`_cat_tools.function__arg_to_regprocedure\` and \`_cat_tools.function__drop_temp\`, defeating the defense-in-depth that was added in the last commit of this PR.

Add before the two \`GRANT\` lines:
\`\`\`sql
REVOKE EXECUTE ON FUNCTION _cat_tools.function__arg_to_regprocedure(text, text, text) FROM PUBLIC;
REVOKE EXECUTE ON FUNCTION _cat_tools.function__drop_temp(pg_catalog.regprocedure, text) FROM PUBLIC;
\`\`\`" \
  --field "comments[][path]=test/build/upgrade.sql" \
  --field "comments[][line]=21" \
  --field "comments[][side]=RIGHT" \
  --field "comments[][body]=The entire update sequence is wrapped in a \`BEGIN … ROLLBACK\` block, so nothing is ever committed. While this is fine as a smoke-test for the update script's SQL validity, it means the test never verifies that the extension actually reaches the expected version after the update.

Consider adding a version assertion before the \`ROLLBACK\`:
\`\`\`sql
SELECT is(
  (SELECT extversion FROM pg_extension WHERE extname = 'cat_tools'),
  :'EXPECTED_VERSION',
  'Extension updated to expected version'
);
\`\`\`
(Requires passing \`EXPECTED_VERSION\` into the test, e.g. via \`\pgxntool/psql.sql\` or a \`\set\` before \`\if\`.)" \
  --field "comments[][path]=HISTORY.asc" \
  --field "comments[][line]=17" \
  --field "comments[][side]=RIGHT" \
  --field "comments[][body]=\`// TODO\` placeholder — the 0.3.0 Changes section is empty. This is a release changelog; filling it in before merge will help users understand what changed." \
  --field "comments[][path]=README.asc" \
  --field "comments[][line]=23" \
  --field "comments[][side]=RIGHT" \
  --field "comments[][body]=This line still says **"Works on Postgres 9.3 and above"** but this PR drops PG10 and PG11 support and requires **PG12+**. Should be updated to reflect the new minimum version." \
  --field "comments[][path]=sql/cat_tools.sql.in" \
  --field "comments[][line]=1614" \
  --field "comments[][side]=RIGHT" \
  --field "comments[][body]=Trailing whitespace after \`pg_class\` on this line (and on line 1627 for \`relation__is_catalog\`). These are inside a dollar-quoted SQL string so it's not harmful, but it's inconsistent with the rest of the file style and will appear in the deployed function source." \
  --field "comments[][path]=sql/cat_tools--0.3.0.sql.in" \
  --field "comments[][line]=383" \
  --field "comments[][side]=RIGHT" \
  --field "comments[][body]=These inline comments — \`-- composite type (but mapped to materialized view)\` — are confusing because they describe the mapping as wrong while it is in fact intentional (preserved for backward compatibility from the original cat_tools relkind enum definition). The comments suggest a bug when the behavior is by design.

Consider replacing with an explanation that makes the intent clear, e.g.:
\`\`\`sql
WHEN 'c' THEN 'materialized view'  -- historical: cat_tools.relation_type predates 'composite type'; kept for API compat
WHEN 'f' THEN 'composite type'     -- historical: ditto for 'foreign table'
WHEN 'm' THEN 'foreign table'      -- historical: ditto for 'materialized view'
\`\`\`
Or, if this is actually an unintentional inversion that should be fixed, this is the right time to do so with a version bump." \
  2>&1

echo "Done"

Length of output: 194

---

@jnasbyupgrade Posting a full GitHub review now (inline comments + summary).

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

REPO="Postgres-Extensions/cat_tools"
PR=16
SHA="434ef45ba69986a6cb4561302ea12a0067dca115"

tmpfile="$(mktemp)"
cat > "$tmpfile" <<'JSON'
{
  "event": "REQUEST_CHANGES",
  "commit_id": "434ef45ba69986a6cb4561302ea12a0067dca115",
  "body": "Review Summary\n\nThis is a substantial and well-structured release: the new enum taxonomy for pg_proc fields, the relation helper functions, the cross-version CI matrix, and the dynamic permissions test suite are all strong improvements.\n\nThere is one blocking issue (a committed test failure in test/expected/routine__.out) and several smaller bugs/doc gaps that should be addressed before merge.\n\nBlocking\n- test/expected/routine__.out contains a psql ERROR (see inline). Root cause: enum_mapping.sql calls :s.routine__type(:'sample_char') with an untyped literal; ambiguous between enum input and implicit 'char' cast. Fix by casting :'sample_char'::\"char\" and regenerate the expected file.\n\nBugs\n- Missing REVOKE EXECUTE FROM PUBLIC in the 0.2.2→0.3.0 upgrade script before the GRANTs on _cat_tools helpers.\n- test/build/upgrade.sql wraps the update in a transaction and rolls back; there is no post-update version assertion.\n\nDocumentation\n- HISTORY.asc 0.3.0 'Changes' section is still TODO.\n- README.asc still says 'Works on Postgres 9.3 and above' but minimum supported is now PG12+ per this PR.\n\nStyle/minor\n- Trailing whitespace in the SQL body strings for relation__is_temp and relation__is_catalog.\n- Relkind mapping comments are confusing; either clarify the historical compatibility or realign if unintended.\n",
  "comments": [
    {
      "path": "test/expected/routine__.out",
      "side": "RIGHT",
      "line": 4,
      "body": "Blocking: committed test failure. Lines 4–8 are a verbatim psql ERROR (42725: function cat_tools.routine__type(unknown) is not unique), not a TAP ok line. Root cause: enum_mapping.sql calls :s.routine__:kind(:'sample_char') with an untyped literal when kind=type and sample_char='f', which is ambiguous between the enum overload and the implicit ('char' → enum) cast. Fix by casting the variable in enum_mapping.sql: :s.routine__:kind(:'sample_char'::\"char\"). Then regenerate routine__.out."
    },
    {
      "path": "sql/cat_tools--0.2.2--0.3.0.sql.in",
      "side": "RIGHT",
      "line": 168,
      "body": "Upgrade script grants EXECUTE on _cat_tools.function__arg_to_regprocedure and _cat_tools.function__drop_temp but does not first REVOKE FROM PUBLIC. The install script adds the REVOKE for defense-in-depth. Please add:\\n\\nREVOKE EXECUTE ON FUNCTION _cat_tools.function__arg_to_regprocedure(text, text, text) FROM PUBLIC;\\nREVOKE EXECUTE ON FUNCTION _cat_tools.function__drop_temp(pg_catalog.regprocedure, text) FROM PUBLIC;\\n\\nthen keep the GRANT to cat_tools__usage."
    },
    {
      "path": "test/build/upgrade.sql",
      "side": "RIGHT",
      "line": 21,
      "body": "The update runs inside BEGIN … ROLLBACK, so nothing is committed; the test never asserts the resulting version. Consider asserting version before ROLLBACK, e.g.:\\n\\nSELECT is((SELECT extversion FROM pg_extension WHERE extname = 'cat_tools'), :'EXPECTED_VERSION', 'Extension updated to expected version');\\n\\n(You already set EXPECTED_VERSION in CI via cat_tools.control; piping it here would make this a stronger check.)"
    },
    {
      "path": "HISTORY.asc",
      "side": "RIGHT",
      "line": 17,
      "body": "TODO placeholder — please populate the 0.3.0 Changes section before merge so users have a proper changelog."
    },
    {
      "path": "README.asc",
      "side": "RIGHT",
      "line": 23,
      "body": "This still states 'Works on Postgres 9.3 and above' but this PR drops PG10 and PG11 (min is PG12+). Please update Supported Versions accordingly."
    },
    {
      "path": "sql/cat_tools.sql.in",
      "side": "RIGHT",
      "line": 1614,
      "body": "Trailing whitespace after 'FROM pg_catalog.pg_class' (and similarly at line 1627). It's inside a dollar-quoted string so harmless, but cleaning it avoids churn in diffed function definitions."
    },
    {
      "path": "sql/cat_tools--0.3.0.sql.in",
      "side": "RIGHT",
      "line": 383,
      "body": "These comments read like the mapping is wrong, but it looks intentional for backward compatibility with the historical cat_tools.relation_type values. Consider clarifying along the lines of: 'historical mapping preserved for API compatibility' for 'c'→'materialized view', 'f'→'composite type', 'm'→'foreign table'. If this inversion is unintentional, this would be the moment to correct with a versioned change."
    }
  ]
}
JSON

# Post the review
gh api "repos/${REPO}/pulls/${PR}/reviews" --method POST --input "$tmpfile"

echo "Review submitted."

Length of output: 225

---

@jnasbyupgrade I’ve completed a full review and submitted a GitHub PR review requesting changes with a consolidated summary. Key findings:

Blocking

• test/expected/routine__.out contains a real psql ERROR, not a passing TAP line.
  • Root cause: test/helpers/enum_mapping.sql calls :s.routine__:kind(:'sample_char') with an untyped literal when kind=type and sample_char='f', which is ambiguous vs the "char"→enum INOUT cast.
  • Fix: cast the sample to "char" in the first Set 1 check, then regenerate expected output.
    Example:
    :s.routine__:kind(:'sample_char'::"char")

Bugs

• Upgrade script privileges: sql/cat_tools--0.2.2--0.3.0.sql.in grants EXECUTE on _cat_tools.function__arg_to_regprocedure and _cat_tools.function__drop_temp without first revoking from PUBLIC (install script has the REVOKE).
  • Add:
    REVOKE EXECUTE ON FUNCTION _cat_tools.function__arg_to_regprocedure(text, text, text) FROM PUBLIC;
    REVOKE EXECUTE ON FUNCTION _cat_tools.function__drop_temp(pg_catalog.regprocedure, text) FROM PUBLIC;
• test/build/upgrade.sql runs BEGIN … ALTER EXTENSION … ROLLBACK. Nothing commits and there’s no version assertion.
  • Add an assertion before ROLLBACK:
    SELECT is((SELECT extversion FROM pg_extension WHERE extname = 'cat_tools'),
    :'EXPECTED_VERSION',
    'Extension updated to expected version');

Docs

• HISTORY.asc: 0.3.0 “Changes” is still // TODO. Please populate before merge.
• README.asc: “Works on Postgres 9.3 and above.” is outdated; this release is PG12+.

Style/minor

• Trailing whitespace in sql/cat_tools.sql.in function bodies (relation__is_temp / relation__is_catalog): FROM pg_catalog.pg_class␠
• Relkind mapping comments in sql/cat_tools--0.3.0.sql.in read like a bug but appear to be historic API-compat behavior (c→materialized view, f→composite type, m→foreign table). Consider clarifying the intent in comments, or realign if this inversion is no longer desired.

If you’d like, I can follow up by:

• Pushing exact patches for enum_mapping.sql and the upgrade REVOKEs.
• Adding the version assertion wiring in CI so EXPECTED_VERSION is shared with the build/upgrade test.
• Drafting the 0.3.0 Changes section based on the commit log.

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.