Remove reg* pseudotype columns; add object info functions - #17
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
105ee2d to
aa60049
Compare
…ript, test/install/load.sql three-mode loader, dependency-guard anchor, and a permanent schema-qualification pgTAP assertion Implements advanced-extension-testing.md checklist items 1-6 on top of PR #17 (reconcile-object-functions): - PGXNTOOL_ENABLE_TEST_INSTALL / PGXNTOOL_ENABLE_VERIFY_RESULTS set explicitly; TEST_LOAD_SOURCE (fresh/update/existing) + TEST_UPDATE_FROM/TO make vars, parse-time validated, propagated as placeholder GUCs via PGOPTIONS; `make test-update` wrapper. - test/install/load.sql: single committed-once installer for the extension, covering all three load modes, including a drop-first reset (with pg_temp.drop_role() for the extension's own global roles) and an existing-mode presence/version assertion. - sql/object_reference--0.1.0--stable.sql: hand-authored update script (there was previously no update path at all from the only real historical release to current). Recreates every function/view that changed via the same private-helper-schema bootstrap/teardown convention the fresh install uses, so the update path is verified byte-for-byte structurally identical to a fresh install (function bodies, comments, ACLs, table/view columns). - Makefile: DATA += sql/object_reference--0.1.0.sql (pgxntool#48 workaround, needed for CREATE EXTENSION ... VERSION '0.1.0' to work at all); a conditional count_nulls install step for the update-mode floor only (0.1.0's own install script still needs it, even though current object_reference no longer requires it). - test/finish.sql: one permanent pgTAP assertion (modeled on pg_count_nulls'/extension_tools' own schema-qualification checks) proving object_reference/_object_reference are never resolved via search_path. - Moved the pre-existing raw-source-load sanity check (test/sql/zzz_build.sql) to test/build/, pgxntool's own test-build feature: it needs a schema-free database to create `object_reference` manually in, which the committed-once installer above no longer provides in the shared main-suite database. Dependency-guard anchor for a future existing-mode CI job: a view typed on _object_reference.object's row type (object_reference-owned, never dropped or redefined by the update script) blocks a non-CASCADE DROP EXTENSION; manually proven to block the drop (and to keep blocking it after the update path) as part of this PR's own verification, not committed as CI machinery yet. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
aa60049 to
03d21be
Compare
…ript, test/install/load.sql three-mode loader, dependency-guard anchor, and a permanent schema-qualification pgTAP assertion Implements advanced-extension-testing.md checklist items 1-6 on top of PR #17 (reconcile-object-functions): - PGXNTOOL_ENABLE_TEST_INSTALL / PGXNTOOL_ENABLE_VERIFY_RESULTS set explicitly; TEST_LOAD_SOURCE (fresh/update/existing) + TEST_UPDATE_FROM/TO make vars, parse-time validated, propagated as placeholder GUCs via PGOPTIONS; `make test-update` wrapper. - test/install/load.sql: single committed-once installer for the extension, covering all three load modes, including a drop-first reset (with pg_temp.drop_role() for the extension's own global roles) and an existing-mode presence/version assertion. - sql/object_reference--0.1.0--stable.sql: hand-authored update script (there was previously no update path at all from the only real historical release to current). Recreates every function/view that changed via the same private-helper-schema bootstrap/teardown convention the fresh install uses, so the update path is verified byte-for-byte structurally identical to a fresh install (function bodies, comments, ACLs, table/view columns). - Makefile: DATA += sql/object_reference--0.1.0.sql (pgxntool#48 workaround, needed for CREATE EXTENSION ... VERSION '0.1.0' to work at all); a conditional count_nulls install step for the update-mode floor only (0.1.0's own install script still needs it, even though current object_reference no longer requires it). - test/finish.sql: one permanent pgTAP assertion (modeled on pg_count_nulls'/extension_tools' own schema-qualification checks) proving object_reference/_object_reference are never resolved via search_path. - Moved the pre-existing raw-source-load sanity check (test/sql/zzz_build.sql) to test/build/, pgxntool's own test-build feature: it needs a schema-free database to create `object_reference` manually in, which the committed-once installer above no longer provides in the shared main-suite database. Dependency-guard anchor for a future existing-mode CI job: a view typed on _object_reference.object's row type (object_reference-owned, never dropped or redefined by the update script) blocks a non-CASCADE DROP EXTENSION; manually proven to block the drop (and to keep blocking it after the update path) as part of this PR's own verification, not committed as CI machinery yet. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Claude finished @jnasbyupgrade's task in 8m 2s —— View job Claude's Review
CI is green on this PR ( What looks solid:
One non-blocking simplification worth considering: in No blocking issues found. Nice, focused reconciliation PR — the count_nulls removal and reg* collapse are handled consistently across every file that touched them. |
|
Claude finished @jnasbyupgrade's task in 2m 8s —— View job Claude's Review
CI is green (
Both commits are small, self-contained, and consistent with the rest of the stack (frozen |
|
Claude finished @jnasbyupgrade's task in 1m 36s —— View job Claude finished @jnasbyupgrade's task in 3m 12s —— Reviewing latest changes
CI is green (
Converts the This is a pure style/formatting fix with no behavioral change. No issues found — this addresses everything raised in prior review rounds (the · Branch: |
|
Claude finished @jnasbyupgrade's task in 1m 48s —— View job Reviewing latest changes
CI is green (
One thing worth a mention purely for context: this commit touches META.in.json/META.json, whereas earlier review rounds on this PR noted those files were untouched — that's expected here since this commit is fixing pre-existing packaging metadata issues (tracked as #35) rather than being part of the reg*-column/object-info-function reconciliation work; it doesn't conflict with the "frozen No issues found. This is a clean, well-scoped documentation and metadata fix. |
pgxn install --unstable cat_tools resolves to the newest release actually published to the PGXN package index, which is still 0.2.1 (2017) and fails standalone on modern PostgreSQL with "column oid specified more than once" at CREATE EXTENSION. A fixed release, 0.3.0, is tagged in cat_tools' own git repo but hasn't been uploaded to PGXN yet, so the Makefile's cat_tools target now clones Postgres-Extensions/cat_tools at the 0.3.0 tag and builds/installs it directly. Since this is the first time object_reference's suite has actually run against a real, working cat_tools, two small fallout fixes are needed: - cat_tools.function__arg_types_text() is deprecated in 0.3.0 in favor of cat_tools.routine__parse_arg_types_text() (identical signature/body, just renamed, deprecated one emits a WARNING on every call). Switched object_reference's one call site to the non-deprecated name. - cat_tools 0.3.0's object_type enum grew two new members, "partitioned table" and "partitioned index". pg_get_object_address() doesn't recognize either (only the base table/index types they derive from), so object_reference classifies them as unsupported, matching object_reference.unsupported()'s existing handling of "event trigger" for the same reason. test/sql/all.sql's sanity-check of the unsupported set is updated to match. sql/object_reference--stable.sql and test/expected/zzz_build.out are regenerated (make results) to match. Extracted from PR #5, which had scope-crept into also carrying this fix alongside the actual CI/pgxn-tools migration; splitting it out here so it can be reviewed and merged independently.
…ls 0.3.0 pgxn install --unstable cat_tools now resolves to cat_tools 0.3.0 directly (confirmed against the live PGXN index and by a clean CREATE EXTENSION cat_tools; both locally and, once pushed, in the actual object_reference CI run for #5's slimmed CI-migration branch -- it went fully green without this Makefile change at all). The PGXN package index being stuck at the broken, 2017-era 0.2.1 release was true when this fix was first written, but isn't true anymore, so the git-clone-from-tag workaround has nothing left to work around. Keeping it would leave a Makefile comment describing a problem that no longer exists. The SQL/test fixes (renamed function call, new object_type enum members classified as unsupported) are unaffected -- those are needed regardless of how cat_tools 0.3.0 gets installed.
Reconciles the substantive feature delta from new_features (PR #2) onto the current 'stable' baseline (post PR #5/#16: pgxn-tools testing, cat_tools 0.3.0, and the linter): - _object_reference._object_oid: drop the per-catalog regclass/regconfig/ regdictionary/regnamespace/regoperator/regprocedure/regtype columns and their unique indexes plus the count_nulls-backed null_count trigger that enforced "exactly one is set". classid is now plain oid and object_oid (also NOT NULL) is the sole identifier column, so there's nothing left to arbitrate between. - _object_reference._object_v / _object_v__for_update: drop the reg* columns from the column list to match. - _object_reference._object_oid__add: replace the dynamic, format()-built INSERT that picked a reg* column based on cat_tools.object__reg_type() with a plain INSERT into object_oid. - Drop the count_nulls search_path DO block (dead now that the trigger using it is gone) and the count_nulls dependency throughout (control, Makefile, test setup). - Add object_reference.object__describe()/object__identity(), thin wrappers around pg_describe_object()/pg_identify_object(); and object__cleanup(), which best-effort deletes an object record (ignoring foreign_key_violation if it's still referenced elsewhere). Wire object__cleanup() up to a new AFTER DELETE trigger on object_group__object so removing an object from its last group automatically attempts cleanup. - _object_v__for_update (the getsert core): refuse to track objects living in a pg_temp*/pg_toast_temp* schema, since a tracked reference would outlive the temporary object it points to. - test/sql/object_group.sql: switch the two scratch tables from TEMP to regular tables (object__getsert now rejects temp objects) and add coverage for the new automatic-cleanup trigger. - test/sql/base.sql: replace the count_nulls-relocation test (relocation was already unsupported and the whole extension no longer depends on count_nulls) with coverage for object_oid, object__describe(), object__identity(), and temp-object rejection. sql/object_reference--0.1.0.sql (the frozen historical release) and the META files are untouched. default_version stays 'stable'; sql/object_reference--stable.sql is regenerated to match sql/object_reference.sql. make lint and make test (including the dump/restore test) pass on both PostgreSQL 12 and 17. Supersedes PR #2 (new_features) and, for the update/upgrade test infrastructure built on top of it, sets up the rebuild of PR #3. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…> 5) The __remove section runs 5 assertions (bogus_group=2, throws_ok=1, lives_ok=2) but the plan math counted +4 + 2 = 6. Confirmed via GitHub's own CI run for this branch: PostgreSQL 12 reported "planned 30 tests but ran 29" under make verify-results, the real pgtap-aware gate -- invisible to plain pg_regress diffing since the stale count was already baked into the committed expected-output file. test/expected/object_group.out regenerated via make results, not hand-edited. Co-Authored-By: Claude <noreply@anthropic.com>
…sages sql/object_reference.sql's many %TYPE column references (e.g. _object_oid.objid%TYPE) each fire a NOTICE at parse time, spamming CREATE EXTENSION's output for no reason a normal install needs to see. SET LOCAL (not SET) so it reverts automatically at the end of CREATE EXTENSION's implicit transaction rather than leaking into the installing session. Co-Authored-By: Claude <noreply@anthropic.com>
Before this stack, _object_oid had six separate reg*-typed columns (one per object class) plus object_oid as a plain-oid projection of whichever one applied to a given row -- a real distinction, since the reg* columns bought per-class dump/rename safety and object_oid bought a uniform plain-oid value for cross-class queries. This PR already collapsed those six reg* columns down to the classid/objid/objsubid triple (mirroring pg_get_object_address()'s own output), which removed the only thing object_oid used to distill -- _object_oid__add's insert already just set it to objid unconditionally, and nothing else ever wrote to this table. classid/objid/objsubid live in the private _object_reference schema, so there's no external API stability concern with removing a column here. Drops the now-vacuous objid_must_match CHECK along with the column, and updates test/sql/base.sql's assertion to check objid directly. test/expected/base.out and test/expected/zzz_build.out regenerated via make results, not hand-edited. Co-Authored-By: Claude <noreply@anthropic.com>
CODE_STYLE.md requires /* ... */ for multi-line SQL comments, never multi-line -- line comments. Applies to the client_min_messages explanation. test/expected/zzz_build.out regenerated via make results to reflect the resulting line-number shift in temp_load.not_sql's WARNING output. Co-Authored-By: Claude <noreply@anthropic.com>
Adds a README documenting installation, roles, key concepts, and the full public API, carried forward from PR #2 and verified against the current schema (object__describe/identity/cleanup, capture__*, object group functions). Fixes META.in.json (source for the generated META.json): adds cat_tools to runtime.requires (a real, current dependency per object_reference.control), updates decibel/* org URLs to Postgres-Extensions/*, and bumps the stated PostgreSQL build requirement from 9.5.0 to 12.0.0, matching the floor already established by this repo's CI matrix (derived from cat_tools's own 12.0.0 build requirement). Co-Authored-By: Claude <noreply@anthropic.com>
…lict The rebase onto master (after #16's linter merge) resolved a conflict in the commented-out cat_tools schema-restriction block by keeping a plain /* instead of master's /* EXCLUDED CODE: ... -- without it, the linter demands " * " prefixes on every line of the commented-out DO block, producing 15 false findings. make lint is clean again. Co-Authored-By: Claude <noreply@anthropic.com>
683cb57 to
f4bb263
Compare
|
Claude finished @jnasbyupgrade's task in 1m 24s —— View job Reviewing latest changes
CI is green (
During the rebase onto master (which now includes #16's SQL style linter), a conflict in the commented-out I checked this against the linter's existing usage elsewhere in the repo ( Confirmed the overall PR diff against No issues found. |
… test An extension's own install script shouldn't unilaterally change a session setting on its caller's behalf, even scoped via SET LOCAL -- that's the caller's call to make, not the extension's. Moved the NOTICE suppression out of sql/object_reference.sql entirely; it turns out only zzz_build's raw \i load (not the normal CREATE EXTENSION path every other test uses) ever surfaced the %TYPE resolution NOTICEs in the first place, so the fix belongs in that one test file, wrapped in the transaction setup.sql already opens. test/expected/zzz_build.out regenerated via make results (line-number shift only, from the extension script shrinking). Co-Authored-By: Claude <noreply@anthropic.com>
|
Claude finished @jnasbyupgrade's task in 1m 11s —— View job Reviewing latest changes
CI is green (
This is a good call — it correctly narrows the workaround's blast radius to the one test that needed it, rather than having the shipped extension mutate a session-level setting on every install. No issues found. |
Drop the
reg*pseudotype columns (regclass/regconfig/regdictionary/regnamespace/regoperator/regprocedure/regtype) from_object_reference._object_oidin favor of a single plainoidcolumn (object_oid) and a plainoidclassid, removing the now-unnecessarycount_nulls-backednull_counttrigger that used to arbitrate between them.count_nullsdependency entirely (the search_path workaround DO block,object_reference.control'srequires, the Makefile install target, and test setup) — the trigger above was its only reason for being there.object_reference.object__describe()/object__identity()(thin wrappers aroundpg_describe_object()/pg_identify_object()), andobject__cleanup()(best-effort delete, ignoringforeign_key_violation), wired to a newAFTER DELETEtrigger onobject_group__objectso removing an object from its last group auto-attempts cleanup.object__getsert's core (_object_v__for_update) now refuses to track objects living in apg_temp*/pg_toast_temp*schema.object_group.sql's scratch tables switch fromTEMPto regular tables (temp objects are now rejected) plus new coverage for the cleanup trigger;base.sqlswaps the now-moot count_nulls-relocation test for coverage ofobject_oid,object__describe()/object__identity(), and temp-object rejection._object_reference._object_oid.object_oidwas later dropped too — it always equaledobjidand never diverged.sql/object_reference--0.1.0.sql(the frozen historical release) is untouched — frozen versioned SQL files are never hand-edited after release.default_versionstays'stable';sql/object_reference--stable.sqlis regenerated fromsql/object_reference.sqlviamake.Also adds
README.md(installation, roles/security, key concepts, and the full public API reference) and fixes packaging metadata inMETA.in.json/META.json: addscat_toolstoruntime.requires(a real, current dependency perobject_reference.control), updatesdecibel/*org URLs toPostgres-Extensions/*, and bumps the stated PostgreSQL build requirement from9.5.0to12.0.0, matching the floor this repo's CI matrix already exercises (derived fromcat_tools's own build requirement).Fixes #35
Co-Authored-By: Claude noreply@anthropic.com