Stop recording pg_shdepend rows lolor cannot describe. - #54
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
a39adb7 to
3829270
Compare
a11af71 to
45592a4
Compare
| lobjId_new, GetUserId()); | ||
|
|
||
| /* Post creation hook for new large object */ | ||
| InvokeObjectPostCreateHook(get_LOLOR_LargeObjectRelationId(), lobjId_new, 0); |
There was a problem hiding this comment.
I guess it was deleted by accident. If so, return this line to the code.
There was a problem hiding this comment.
Deliberate, not accidental. The comment should have said so; an earlier version did and I trimmed it. Restored in dd04002.
Both calls take a classId. Core passes LargeObjectRelationId, a real catalog. This passed get_LOLOR_LargeObjectRelationId(), an ordinary table OID, which is what broke DROP ROLE. Giving that OID to an object access hook misleads sepgsql the same way, since it switches on classId expecting a catalog. There is no correct classId for an object that is not a catalog object, so the hook call has to go too.
There was a problem hiding this comment.
Follow-up: removing those two calls left catalog/objectaccess.h and catalog/dependency.h included but unused. Dropped both in ac37e57, builds clean and tests pass.
| LEFT JOIN pg_catalog.pg_authid a ON a.oid = m.lomowner | ||
| WHERE a.oid IS NULL | ||
| ORDER BY m.oid | ||
| $$ LANGUAGE sql STABLE; |
There was a problem hiding this comment.
Not sure, but why would anyone call such 'migration' functions? Maybe narrow the scope to superusers only?
There was a problem hiding this comment.
It is not a migration function, just a read-only diagnostic. It lists large objects whose owner has been dropped, which matters because this PR stops recording pg_shdepend rows, so DROP ROLE no longer notices them.
You are right on privileges though. It was already restricted, but only as a side effect of reading pg_authid and an extension-owned table, so users got permission denied for table pg_largeobject_metadata. Added an explicit REVOKE in dd04002 to match the migration functions. Now it says permission denied for function check_orphans.
45592a4 to
dd04002
Compare
3829270 to
f6e4c39
Compare
dd04002 to
6e9e6a7
Compare
f6e4c39 to
e1b389f
Compare
6e9e6a7 to
9da897b
Compare
e1b389f to
fbabb89
Compare
f2c67da to
ac37e57
Compare
fbabb89 to
6d675a4
Compare
ac37e57 to
beda067
Compare
6d675a4 to
1952866
Compare
beda067 to
de81cf4
Compare
Creating a large object recorded a shared dependency whose classId was the OID of lolor.pg_largeobject, an ordinary table rather than a catalog the dependency machinery can describe. DROP ROLE on any role that had created one failed with "unrecognized object class", and the rows were never removed because inv_drop() deleted with PERFORM_DELETION_SKIP_ORIGINAL. pg_shdepend is shared across the cluster, so the stored classId was a per-database relation OID meaningless in any other database. Objects in lolor storage are rows in ordinary tables and cannot take part in pg_shdepend at all, so drop the recording and the matching deletion, and remove the rows earlier versions left behind. Ownership is consequently untracked, which is inherent to storing large objects outside the catalogs; add lolor.check_orphans() to report objects whose owner is gone.
de81cf4 to
f25c02e
Compare
|
Pushed fix_orphans() rewritten the way aclnewowner() works: in one UPDATE the old owner is replaced by new_owner as grantee and grantor, coinciding entries merge, then only entries still naming a missing role are dropped. Before, it reassigned lomowner first and then removed every entry the dead owner had granted, so {alice=rw/alice, bob=r/alice} became NULL and bob lost access. Now it gives {carol=rw/carol, bob=r/carol}, same as REASSIGN OWNED. Tests added here for a dead owner with grants, a grant chain through a live grantor, and a new owner that already held a grant. README now covers grantees, running REASSIGN OWNED or DROP OWNED in each lolor database before DROP ROLE, and role OID reuse. |
3 participants
Based on #53.
lolor_inv_create()recorded apg_shdependrow with the OID oflolor.pg_largeobjectas the classId. That is an ordinary table, not a catalog, soDROP ROLEon any role that had created a large object failed with "unsupported object class". The rows were never removed either, andlo_unlink(0)hit the same bad classId.Objects in lolor storage are plain table rows and cannot be in
pg_shdepend, so this stops recording them, removes the deletion that never matched anything, and deletes the rows older versions left behind in the current database.The cost is that
DROP ROLEno longer notices a role that still owns lolor objects or is named in their ACLs. Two helpers cover that:lolor.check_orphans()lists objects whose owner, grantee or grantor is gone, andlolor.fix_orphans(new_owner)repairs them the wayREASSIGN OWNEDwould, keeping the grants the old owner made. Both are superuser only. The README now says to runREASSIGN OWNEDorDROP OWNEDin each lolor database before dropping a role, and warns that role OIDs can be reused.1.3.0 is unreleased, so the SQL goes into
lolor--1.2.2--1.3.0.sqlrather than a new version.Tests cover the permission checks, orphan detection, and three
fix_orphans()cases: a dead owner with grants, a grant chain through a live grantor, and a new owner that already held a grant.