Обсуждение: [MASSMAIL]DROP OWNED BY fails to clean out pg_init_privs grants
I wondered why buildfarm member copperhead has started to fail
xversion-upgrade-HEAD-HEAD tests. I soon reproduced the problem here:
pg_restore: creating ACL "regress_pg_dump_schema.TYPE "test_type""
pg_restore: while PROCESSING TOC:
pg_restore: from TOC entry 4355; 0 0 ACL TYPE "test_type" buildfarm
pg_restore: error: could not execute query: ERROR: role "74603" does not exist
Command was: SELECT pg_catalog.binary_upgrade_set_record_init_privs(true);
GRANT ALL ON TYPE "regress_pg_dump_schema"."test_type" TO "74603";
SELECT pg_catalog.binary_upgrade_set_record_init_privs(false);
REVOKE ALL ON TYPE "regress_pg_dump_schema"."test_type" FROM "74603";
(So now I'm wondering why *only* copperhead has shown this so far.
Are our other cross-version-upgrade testing animals AWOL?)
I believe this is a longstanding problem that was exposed by accident
by commit 936e3fa37. If you run "make installcheck" in HEAD's
src/test/modules/test_pg_dump, and then poke around in the leftover
contrib_regression database, you can find dangling grants in
pg_init_privs:
contrib_regression=# table pg_init_privs;
objoid | classoid | objsubid | privtype | initprivs
--------+----------+----------+----------+--------------------------------------
---------------------------
...
es}
43134 | 1259 | 0 | e | {postgres=rwU/postgres,43125=U/postgr
es}
43128 | 1259 | 0 | e | {postgres=arwdDxtm/postgres,43125=r/p
ostgres}
...
The fact that the DROP ROLE added by 936e3fa37 succeeded indicates
that these role references weren't captured in pg_shdepend.
I imagine that we also lack code that would allow DROP OWNED BY to
follow up on such entries if they existed, but I've not checked that
for sure. In any case, there's probably a nontrivial amount of code
to be written to make this work.
Given the lack of field complaints, I suspect that extension scripts
simply don't grant privileges to random roles that aren't the
extension's owner. So I wonder a little bit if this is even worth
fixing, as opposed to blocking off somehow. But probably we should
first try to fix it.
I doubt this is something we'll have fixed by Monday, so I will
go add an open item for it.
regards, tom lane
On Fri, Apr 05, 2024 at 07:10:59PM -0400, Tom Lane wrote:
> I wondered why buildfarm member copperhead has started to fail
> xversion-upgrade-HEAD-HEAD tests. I soon reproduced the problem here:
>
> pg_restore: creating ACL "regress_pg_dump_schema.TYPE "test_type""
> pg_restore: while PROCESSING TOC:
> pg_restore: from TOC entry 4355; 0 0 ACL TYPE "test_type" buildfarm
> pg_restore: error: could not execute query: ERROR: role "74603" does not exist
> Command was: SELECT pg_catalog.binary_upgrade_set_record_init_privs(true);
> GRANT ALL ON TYPE "regress_pg_dump_schema"."test_type" TO "74603";
> SELECT pg_catalog.binary_upgrade_set_record_init_privs(false);
> REVOKE ALL ON TYPE "regress_pg_dump_schema"."test_type" FROM "74603";
>
> (So now I'm wondering why *only* copperhead has shown this so far.
> Are our other cross-version-upgrade testing animals AWOL?)
>
> I believe this is a longstanding problem that was exposed by accident
> by commit 936e3fa37. If you run "make installcheck" in HEAD's
> src/test/modules/test_pg_dump, and then poke around in the leftover
> contrib_regression database, you can find dangling grants in
> pg_init_privs:
>
> contrib_regression=# table pg_init_privs;
> objoid | classoid | objsubid | privtype | initprivs
>
> --------+----------+----------+----------+--------------------------------------
> ---------------------------
> ...
> es}
> 43134 | 1259 | 0 | e | {postgres=rwU/postgres,43125=U/postgr
> es}
> 43128 | 1259 | 0 | e | {postgres=arwdDxtm/postgres,43125=r/p
> ostgres}
> ...
>
> The fact that the DROP ROLE added by 936e3fa37 succeeded indicates
> that these role references weren't captured in pg_shdepend.
> I imagine that we also lack code that would allow DROP OWNED BY to
> follow up on such entries if they existed, but I've not checked that
> for sure. In any case, there's probably a nontrivial amount of code
> to be written to make this work.
>
> Given the lack of field complaints, I suspect that extension scripts
> simply don't grant privileges to random roles that aren't the
> extension's owner. So I wonder a little bit if this is even worth
> fixing, as opposed to blocking off somehow. But probably we should
> first try to fix it.
This sounds closely-related to the following thread:
https://www.postgresql.org/message-id/flat/1573808483712.96817%40Optiver.com
Noah Misch <noah@leadboat.com> writes: > On Fri, Apr 05, 2024 at 07:10:59PM -0400, Tom Lane wrote: >> The fact that the DROP ROLE added by 936e3fa37 succeeded indicates >> that these role references weren't captured in pg_shdepend. >> I imagine that we also lack code that would allow DROP OWNED BY to >> follow up on such entries if they existed, but I've not checked that >> for sure. In any case, there's probably a nontrivial amount of code >> to be written to make this work. >> >> Given the lack of field complaints, I suspect that extension scripts >> simply don't grant privileges to random roles that aren't the >> extension's owner. So I wonder a little bit if this is even worth >> fixing, as opposed to blocking off somehow. But probably we should >> first try to fix it. > This sounds closely-related to the following thread: > https://www.postgresql.org/message-id/flat/1573808483712.96817%40Optiver.com Oh, interesting, I'd forgotten that thread completely. So Stephen was pushing back against dealing with the case because he thought that the SQL commands issued in that example should not have produced pg_init_privs entries in the first place. Which nobody else wanted to opine on, so the thread stalled. However, in the case of the test_pg_dump extension, the test_pg_dump--1.0.sql script absolutely did grant those privileges so it's very hard for me to think that they shouldn't be listed in pg_init_privs. Hence, I think we've accidentally stumbled across a case where we do need all that mechanism --- unless somebody wants to argue that what test_pg_dump--1.0.sql is doing should be disallowed. regards, tom lane
> On 6 Apr 2024, at 01:10, Tom Lane <tgl@sss.pgh.pa.us> wrote: > (So now I'm wondering why *only* copperhead has shown this so far. > Are our other cross-version-upgrade testing animals AWOL?) Clicking around searching for Xversion animals I didn't spot any, but without access to the database it's nontrivial to know which animal does what. > I doubt this is something we'll have fixed by Monday, so I will > go add an open item for it. +1. Having opened the can of worms I'll have a look at it next week. -- Daniel Gustafsson
Daniel Gustafsson <daniel@yesql.se> writes:
>> On 6 Apr 2024, at 01:10, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> (So now I'm wondering why *only* copperhead has shown this so far.
>> Are our other cross-version-upgrade testing animals AWOL?)
> Clicking around searching for Xversion animals I didn't spot any, but without
> access to the database it's nontrivial to know which animal does what.
I believe I see why this is (or isn't) happening. The animals
currently running xversion tests are copperhead, crake, drongo,
and fairywren. copperhead is using the makefiles while the others
are using meson. And I find this in
src/test/modules/test_pg_dump/meson.build (from 3f0e786cc):
# doesn't delete its user
'runningcheck': false,
So the meson animals are not running the test that sets up the
problematic data.
I think we should remove the above, since (a) the reason to have
it is gone, and (b) it seems really bad that the set of tests
run by meson is different from that run by the makefiles.
However, once we do that, those other three animals will presumably go
red, greatly complicating detection of any Windows-specific problems.
So I'm inclined to not do it till just before we intend to commit
a fix for the underlying problem. (Enough before that we can confirm
that they do go red.)
Speaking of which ...
>> I doubt this is something we'll have fixed by Monday, so I will
>> go add an open item for it.
> +1. Having opened the can of worms I'll have a look at it next week.
... were you going to look at it? I can take a whack if it's
too far down your priority list.
regards, tom lane
> On 21 Apr 2024, at 23:08, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> ... were you going to look at it? I can take a whack if it's too far down your priority list.
Yeah, I’m working on a patchset right now.
./daniel
> On 21 Apr 2024, at 23:08, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Daniel Gustafsson <daniel@yesql.se> writes: >>> On 6 Apr 2024, at 01:10, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> (So now I'm wondering why *only* copperhead has shown this so far. >>> Are our other cross-version-upgrade testing animals AWOL?) > >> Clicking around searching for Xversion animals I didn't spot any, but without >> access to the database it's nontrivial to know which animal does what. > > I believe I see why this is (or isn't) happening. The animals > currently running xversion tests are copperhead, crake, drongo, > and fairywren. copperhead is using the makefiles while the others > are using meson. And I find this in > src/test/modules/test_pg_dump/meson.build (from 3f0e786cc): > > # doesn't delete its user > 'runningcheck': false, > > So the meson animals are not running the test that sets up the > problematic data. ugh =/ > I think we should remove the above, since (a) the reason to have > it is gone, and (b) it seems really bad that the set of tests > run by meson is different from that run by the makefiles. Agreed. > However, once we do that, those other three animals will presumably go > red, greatly complicating detection of any Windows-specific problems. > So I'm inclined to not do it till just before we intend to commit > a fix for the underlying problem. (Enough before that we can confirm > that they do go red.) Agreed, we definitely want that but compromising the ability to find Windows issues at this point in the cycle seems bad. > ... were you going to look at it? I can take a whack if it's > too far down your priority list. I took a look at this, reading code and the linked thread. My gut feeling is that Stephen is right in that the underlying bug is these privileges ending up in pg_init_privs to begin with. That being said, I wasn't able to fix that in a way that doesn't seem like a terrible hack. The attached POC hack fixes it for me but I'm not sure how to fix it properly. Your wisdom would be much appreciated. Clusters which already has such entries aren't helped by a fix for this though, fixing that would either require pg_dump to skip them, or pg_upgrade to have a check along with instructions for fixing the issue. Not sure what's the best strategy here, the lack of complaints could indicate this isn't terribly common so spending cycles on it for every pg_dump might be excessive compared to a pg_upgrade check? -- Daniel Gustafsson
Вложения
Daniel Gustafsson <daniel@yesql.se> writes:
> On 21 Apr 2024, at 23:08, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> So the meson animals are not running the test that sets up the
>> problematic data.
> I took a look at this, reading code and the linked thread. My gut feeling is
> that Stephen is right in that the underlying bug is these privileges ending up
> in pg_init_privs to begin with. That being said, I wasn't able to fix that in
> a way that doesn't seem like a terrible hack.
Hmm, can't we put the duplicate logic inside recordExtensionInitPriv?
Even if these calls need a different result from others, adding a flag
parameter seems superior to having N copies of the logic.
A bigger problem though is that I think you are addressing the
original complaint from the older thread, which while it's a fine
thing to fix seems orthogonal to the failure we're seeing in the
buildfarm. The buildfarm's problem is not that we're recording
incorrect pg_init_privs entries, it's that when we do create such
entries we're failing to show their dependency on the grantee role
in pg_shdepend. We've missed spotting that so far because it's
so seldom that pg_init_privs entries reference any but built-in
roles (or at least roles that'd likely outlive the extension).
regards, tom lane
I wrote:
> A bigger problem though is that I think you are addressing the
> original complaint from the older thread, which while it's a fine
> thing to fix seems orthogonal to the failure we're seeing in the
> buildfarm. The buildfarm's problem is not that we're recording
> incorrect pg_init_privs entries, it's that when we do create such
> entries we're failing to show their dependency on the grantee role
> in pg_shdepend. We've missed spotting that so far because it's
> so seldom that pg_init_privs entries reference any but built-in
> roles (or at least roles that'd likely outlive the extension).
Here's a draft patch that attacks that. It seems to fix the
problem with test_pg_dump: no dangling pg_init_privs grants
are left behind.
A lot of the changes here are just involved with needing to pass the
object's owner OID to recordExtensionInitPriv so that it can be passed
to updateAclDependencies. One thing I'm a bit worried about is that
some of the new code assumes that all object types that are of
interest here will have catcaches on OID, so that it's possible to
fetch the owner OID for a generic object-with-privileges using the
catcache and objectaddress.c's tables of object properties. That
assumption seems to exist already, eg ExecGrant_common also assumes
it, but it's not obvious that it must be so.
regards, tom lane
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 2907079e2a..c8cb46c5b9 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -7184,6 +7184,20 @@ SCRAM-SHA-256$<replaceable><iteration count></replaceable>:<replaceable>&l
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><symbol>SHARED_DEPENDENCY_INITACL</symbol> (<literal>i</literal>)</term>
+ <listitem>
+ <para>
+ The referenced object (which must be a role) is mentioned in a
+ <link linkend="catalog-pg-init-privs"><structname>pg_init_privs</structname></link>
+ entry for the dependent object.
+ (A <symbol>SHARED_DEPENDENCY_INITACL</symbol> entry is not made for
+ the owner of the object, since the owner will have
+ a <symbol>SHARED_DEPENDENCY_OWNER</symbol> entry anyway.)
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><symbol>SHARED_DEPENDENCY_POLICY</symbol> (<literal>r</literal>)</term>
<listitem>
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 7abf3c2a74..04c41c0c14 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -165,9 +165,9 @@ static AclMode pg_type_aclmask_ext(Oid type_oid, Oid roleid,
AclMode mask, AclMaskHow how,
bool *is_missing);
static void recordExtensionInitPriv(Oid objoid, Oid classoid, int objsubid,
- Acl *new_acl);
+ Oid ownerId, Acl *new_acl);
static void recordExtensionInitPrivWorker(Oid objoid, Oid classoid, int objsubid,
- Acl *new_acl);
+ Oid ownerId, Acl *new_acl);
/*
@@ -1790,7 +1790,7 @@ ExecGrant_Attribute(InternalGrant *istmt, Oid relOid, const char *relname,
CatalogTupleUpdate(attRelation, &newtuple->t_self, newtuple);
/* Update initial privileges for extensions */
- recordExtensionInitPriv(relOid, RelationRelationId, attnum,
+ recordExtensionInitPriv(relOid, RelationRelationId, attnum, ownerId,
ACL_NUM(new_acl) > 0 ? new_acl : NULL);
/* Update the shared dependency ACL info */
@@ -2050,7 +2050,8 @@ ExecGrant_Relation(InternalGrant *istmt)
CatalogTupleUpdate(relation, &newtuple->t_self, newtuple);
/* Update initial privileges for extensions */
- recordExtensionInitPriv(relOid, RelationRelationId, 0, new_acl);
+ recordExtensionInitPriv(relOid, RelationRelationId, 0,
+ ownerId, new_acl);
/* Update the shared dependency ACL info */
updateAclDependencies(RelationRelationId, relOid, 0,
@@ -2251,7 +2252,7 @@ ExecGrant_common(InternalGrant *istmt, Oid classid, AclMode default_privs,
CatalogTupleUpdate(relation, &newtuple->t_self, newtuple);
/* Update initial privileges for extensions */
- recordExtensionInitPriv(objectid, classid, 0, new_acl);
+ recordExtensionInitPriv(objectid, classid, 0, ownerId, new_acl);
/* Update the shared dependency ACL info */
updateAclDependencies(classid,
@@ -2403,7 +2404,8 @@ ExecGrant_Largeobject(InternalGrant *istmt)
CatalogTupleUpdate(relation, &newtuple->t_self, newtuple);
/* Update initial privileges for extensions */
- recordExtensionInitPriv(loid, LargeObjectRelationId, 0, new_acl);
+ recordExtensionInitPriv(loid, LargeObjectRelationId, 0,
+ ownerId, new_acl);
/* Update the shared dependency ACL info */
updateAclDependencies(LargeObjectRelationId,
@@ -2575,7 +2577,7 @@ ExecGrant_Parameter(InternalGrant *istmt)
/* Update initial privileges for extensions */
recordExtensionInitPriv(parameterId, ParameterAclRelationId, 0,
- new_acl);
+ ownerId, new_acl);
/* Update the shared dependency ACL info */
updateAclDependencies(ParameterAclRelationId, parameterId, 0,
@@ -4463,6 +4465,7 @@ recordExtObjInitPriv(Oid objoid, Oid classoid)
}
recordExtensionInitPrivWorker(objoid, classoid, curr_att,
+ pg_class_tuple->relowner,
DatumGetAclP(attaclDatum));
ReleaseSysCache(attTuple);
@@ -4475,6 +4478,7 @@ recordExtObjInitPriv(Oid objoid, Oid classoid)
/* Add the record, if any, for the top-level object */
if (!isNull)
recordExtensionInitPrivWorker(objoid, classoid, 0,
+ pg_class_tuple->relowner,
DatumGetAclP(aclDatum));
ReleaseSysCache(tuple);
@@ -4485,6 +4489,7 @@ recordExtObjInitPriv(Oid objoid, Oid classoid)
Datum aclDatum;
bool isNull;
HeapTuple tuple;
+ Form_pg_largeobject_metadata form_lo_meta;
ScanKeyData entry[1];
SysScanDesc scan;
Relation relation;
@@ -4509,6 +4514,7 @@ recordExtObjInitPriv(Oid objoid, Oid classoid)
tuple = systable_getnext(scan);
if (!HeapTupleIsValid(tuple))
elog(ERROR, "could not find tuple for large object %u", objoid);
+ form_lo_meta = (Form_pg_largeobject_metadata) GETSTRUCT(tuple);
aclDatum = heap_getattr(tuple,
Anum_pg_largeobject_metadata_lomacl,
@@ -4517,6 +4523,7 @@ recordExtObjInitPriv(Oid objoid, Oid classoid)
/* Add the record, if any, for the top-level object */
if (!isNull)
recordExtensionInitPrivWorker(objoid, classoid, 0,
+ form_lo_meta->lomowner,
DatumGetAclP(aclDatum));
systable_endscan(scan);
@@ -4524,24 +4531,29 @@ recordExtObjInitPriv(Oid objoid, Oid classoid)
/* This will error on unsupported classoid. */
else if (get_object_attnum_acl(classoid) != InvalidAttrNumber)
{
+ int cacheid;
+ Oid ownerId;
Datum aclDatum;
bool isNull;
HeapTuple tuple;
- tuple = SearchSysCache1(get_object_catcache_oid(classoid),
- ObjectIdGetDatum(objoid));
+ cacheid = get_object_catcache_oid(classoid);
+ tuple = SearchSysCache1(cacheid, ObjectIdGetDatum(objoid));
if (!HeapTupleIsValid(tuple))
elog(ERROR, "cache lookup failed for %s %u",
get_object_class_descr(classoid), objoid);
- aclDatum = SysCacheGetAttr(get_object_catcache_oid(classoid), tuple,
+ ownerId = DatumGetObjectId(SysCacheGetAttrNotNull(cacheid,
+ tuple,
+ get_object_attnum_owner(classoid)));
+ aclDatum = SysCacheGetAttr(cacheid, tuple,
get_object_attnum_acl(classoid),
&isNull);
/* Add the record, if any, for the top-level object */
if (!isNull)
recordExtensionInitPrivWorker(objoid, classoid, 0,
- DatumGetAclP(aclDatum));
+ ownerId, DatumGetAclP(aclDatum));
ReleaseSysCache(tuple);
}
@@ -4554,6 +4566,8 @@ recordExtObjInitPriv(Oid objoid, Oid classoid)
void
removeExtObjInitPriv(Oid objoid, Oid classoid)
{
+ Oid ownerId;
+
/*
* If this is a relation then we need to see if there are any sub-objects
* (eg: columns) for it and, if so, be sure to call
@@ -4568,6 +4582,7 @@ removeExtObjInitPriv(Oid objoid, Oid classoid)
if (!HeapTupleIsValid(tuple))
elog(ERROR, "cache lookup failed for relation %u", objoid);
pg_class_tuple = (Form_pg_class) GETSTRUCT(tuple);
+ ownerId = pg_class_tuple->relowner;
/*
* Indexes don't have permissions, neither do the pg_class rows for
@@ -4604,7 +4619,8 @@ removeExtObjInitPriv(Oid objoid, Oid classoid)
/* when removing, remove all entries, even dropped columns */
- recordExtensionInitPrivWorker(objoid, classoid, curr_att, NULL);
+ recordExtensionInitPrivWorker(objoid, classoid, curr_att,
+ ownerId, NULL);
ReleaseSysCache(attTuple);
}
@@ -4612,9 +4628,35 @@ removeExtObjInitPriv(Oid objoid, Oid classoid)
ReleaseSysCache(tuple);
}
+ else
+ {
+ /* Must find out the owner's OID the hard way */
+ AttrNumber ownerattnum;
+ int cacheid;
+ HeapTuple tuple;
+
+ /*
+ * If the object is of a type that has no owner, it should not have
+ * any pg_init_privs entry either.
+ */
+ ownerattnum = get_object_attnum_owner(classoid);
+ if (ownerattnum == InvalidAttrNumber)
+ return;
+ cacheid = get_object_catcache_oid(classoid);
+ tuple = SearchSysCache1(cacheid, ObjectIdGetDatum(objoid));
+ if (!HeapTupleIsValid(tuple))
+ elog(ERROR, "cache lookup failed for %s %u",
+ get_object_class_descr(classoid), objoid);
+
+ ownerId = DatumGetObjectId(SysCacheGetAttrNotNull(cacheid,
+ tuple,
+ ownerattnum));
+
+ ReleaseSysCache(tuple);
+ }
/* Remove the record, if any, for the top-level object */
- recordExtensionInitPrivWorker(objoid, classoid, 0, NULL);
+ recordExtensionInitPrivWorker(objoid, classoid, 0, ownerId, NULL);
}
/*
@@ -4626,7 +4668,8 @@ removeExtObjInitPriv(Oid objoid, Oid classoid)
* Pass in the object OID, the OID of the class (the OID of the table which
* the object is defined in) and the 'sub' id of the object (objsubid), if
* any. If there is no 'sub' id (they are currently only used for columns of
- * tables) then pass in '0'. Finally, pass in the complete ACL to store.
+ * tables) then pass in '0'. Also pass the OID of the object's owner.
+ * Finally, pass in the complete ACL to store.
*
* If an ACL already exists for this object/sub-object then we will replace
* it with what is passed in.
@@ -4635,7 +4678,8 @@ removeExtObjInitPriv(Oid objoid, Oid classoid)
* removed, if one is found.
*/
static void
-recordExtensionInitPriv(Oid objoid, Oid classoid, int objsubid, Acl *new_acl)
+recordExtensionInitPriv(Oid objoid, Oid classoid, int objsubid,
+ Oid ownerId, Acl *new_acl)
{
/*
* Generally, we only record the initial privileges when an extension is
@@ -4648,7 +4692,7 @@ recordExtensionInitPriv(Oid objoid, Oid classoid, int objsubid, Acl *new_acl)
if (!creating_extension && !binary_upgrade_record_init_privs)
return;
- recordExtensionInitPrivWorker(objoid, classoid, objsubid, new_acl);
+ recordExtensionInitPrivWorker(objoid, classoid, objsubid, ownerId, new_acl);
}
/*
@@ -4664,14 +4708,23 @@ recordExtensionInitPriv(Oid objoid, Oid classoid, int objsubid, Acl *new_acl)
* EXTENSION ... ADD/DROP.
*/
static void
-recordExtensionInitPrivWorker(Oid objoid, Oid classoid, int objsubid, Acl *new_acl)
+recordExtensionInitPrivWorker(Oid objoid, Oid classoid, int objsubid,
+ Oid ownerId, Acl *new_acl)
{
Relation relation;
ScanKeyData key[3];
SysScanDesc scan;
HeapTuple tuple;
HeapTuple oldtuple;
+ int noldmembers;
+ int nnewmembers;
+ Oid *oldmembers;
+ Oid *newmembers;
+ /* We'll need the role membership of the new ACL. */
+ nnewmembers = aclmembers(new_acl, &newmembers);
+
+ /* Search pg_init_privs for an existing entry. */
relation = table_open(InitPrivsRelationId, RowExclusiveLock);
ScanKeyInit(&key[0],
@@ -4699,6 +4752,23 @@ recordExtensionInitPrivWorker(Oid objoid, Oid classoid, int objsubid, Acl *new_a
Datum values[Natts_pg_init_privs] = {0};
bool nulls[Natts_pg_init_privs] = {0};
bool replace[Natts_pg_init_privs] = {0};
+ Datum oldAclDatum;
+ bool isNull;
+ Acl *old_acl;
+
+ /* Update pg_shdepend for roles mentioned in the old/new ACLs. */
+ oldAclDatum = heap_getattr(oldtuple, Anum_pg_init_privs_initprivs,
+ RelationGetDescr(relation), &isNull);
+ if (!isNull)
+ old_acl = DatumGetAclP(oldAclDatum);
+ else
+ old_acl = NULL; /* this case shouldn't happen, probably */
+ noldmembers = aclmembers(old_acl, &oldmembers);
+
+ updateInitAclDependencies(classoid, objoid, objsubid,
+ ownerId,
+ noldmembers, oldmembers,
+ nnewmembers, newmembers);
/* If we have a new ACL to set, then update the row with it. */
if (new_acl)
@@ -4744,6 +4814,15 @@ recordExtensionInitPrivWorker(Oid objoid, Oid classoid, int objsubid, Acl *new_a
tuple = heap_form_tuple(RelationGetDescr(relation), values, nulls);
CatalogTupleInsert(relation, tuple);
+
+ /* Update pg_shdepend, too. */
+ noldmembers = 0;
+ oldmembers = NULL;
+
+ updateInitAclDependencies(classoid, objoid, objsubid,
+ ownerId,
+ noldmembers, oldmembers,
+ nnewmembers, newmembers);
}
}
@@ -4754,3 +4833,138 @@ recordExtensionInitPrivWorker(Oid objoid, Oid classoid, int objsubid, Acl *new_a
table_close(relation, RowExclusiveLock);
}
+
+/*
+ * RemoveRoleFromInitPriv
+ *
+ * Used by shdepDropOwned to remove mentions of a role in pg_init_privs.
+ */
+void
+RemoveRoleFromInitPriv(Oid roleid, Oid classid, Oid objid, int32 objsubid)
+{
+ Relation rel;
+ ScanKeyData key[3];
+ SysScanDesc scan;
+ HeapTuple oldtuple;
+ int cacheid;
+ HeapTuple objtuple;
+ Oid ownerId;
+ Datum oldAclDatum;
+ bool isNull;
+ Acl *old_acl;
+ Acl *new_acl;
+ HeapTuple newtuple;
+ int noldmembers;
+ int nnewmembers;
+ Oid *oldmembers;
+ Oid *newmembers;
+
+ /* Search for existing pg_init_privs entry for the target object. */
+ rel = table_open(InitPrivsRelationId, RowExclusiveLock);
+
+ ScanKeyInit(&key[0],
+ Anum_pg_init_privs_objoid,
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(objid));
+ ScanKeyInit(&key[1],
+ Anum_pg_init_privs_classoid,
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(classid));
+ ScanKeyInit(&key[2],
+ Anum_pg_init_privs_objsubid,
+ BTEqualStrategyNumber, F_INT4EQ,
+ Int32GetDatum(objsubid));
+
+ scan = systable_beginscan(rel, InitPrivsObjIndexId, true,
+ NULL, 3, key);
+
+ /* There should exist only one entry or none. */
+ oldtuple = systable_getnext(scan);
+
+ if (!HeapTupleIsValid(oldtuple))
+ {
+ /*
+ * Hmm, why are we here if there's no entry? But pack up and go away
+ * quietly.
+ */
+ systable_endscan(scan);
+ table_close(rel, RowExclusiveLock);
+ return;
+ }
+
+ /* Get a writable copy of the existing ACL. */
+ oldAclDatum = heap_getattr(oldtuple, Anum_pg_init_privs_initprivs,
+ RelationGetDescr(rel), &isNull);
+ if (!isNull)
+ old_acl = DatumGetAclPCopy(oldAclDatum);
+ else
+ old_acl = NULL; /* this case shouldn't happen, probably */
+
+ /*
+ * We need the members of both old and new ACLs so we can correct the
+ * shared dependency information. Collect data before
+ * merge_acl_with_grant throws away old_acl.
+ */
+ noldmembers = aclmembers(old_acl, &oldmembers);
+
+ /* Must find out the owner's OID the hard way. */
+ cacheid = get_object_catcache_oid(classid);
+ objtuple = SearchSysCache1(cacheid, ObjectIdGetDatum(objid));
+ if (!HeapTupleIsValid(objtuple))
+ elog(ERROR, "cache lookup failed for %s %u",
+ get_object_class_descr(classid), objid);
+
+ ownerId = DatumGetObjectId(SysCacheGetAttrNotNull(cacheid,
+ objtuple,
+ get_object_attnum_owner(classid)));
+ ReleaseSysCache(objtuple);
+
+ /*
+ * Generate new ACL. Grantor of rights is always the same as the owner.
+ */
+ new_acl = merge_acl_with_grant(old_acl,
+ false, /* is_grant */
+ false, /* grant_option */
+ DROP_RESTRICT,
+ list_make1_oid(roleid),
+ ACLITEM_ALL_PRIV_BITS,
+ ownerId,
+ ownerId);
+
+ /* If we end with an empty ACL, delete the pg_init_privs entry. */
+ if (new_acl == NULL || ACL_NUM(new_acl) == 0)
+ {
+ CatalogTupleDelete(rel, &oldtuple->t_self);
+ }
+ else
+ {
+ Datum values[Natts_pg_init_privs] = {0};
+ bool nulls[Natts_pg_init_privs] = {0};
+ bool replaces[Natts_pg_init_privs] = {0};
+
+ /* Update existing entry. */
+ values[Anum_pg_init_privs_initprivs - 1] = PointerGetDatum(new_acl);
+ replaces[Anum_pg_init_privs_initprivs - 1] = true;
+
+ newtuple = heap_modify_tuple(oldtuple, RelationGetDescr(rel),
+ values, nulls, replaces);
+ CatalogTupleUpdate(rel, &newtuple->t_self, newtuple);
+ }
+
+ /*
+ * Update the shared dependency ACL info.
+ */
+ nnewmembers = aclmembers(new_acl, &newmembers);
+
+ updateInitAclDependencies(classid, objid, objsubid,
+ ownerId,
+ noldmembers, oldmembers,
+ nnewmembers, newmembers);
+
+ systable_endscan(scan);
+
+ /* prevent error when processing objects multiple times */
+ CommandCounterIncrement();
+
+ table_close(rel, RowExclusiveLock);
+}
diff --git a/src/backend/catalog/pg_shdepend.c b/src/backend/catalog/pg_shdepend.c
index cb31590339..20bcfd779b 100644
--- a/src/backend/catalog/pg_shdepend.c
+++ b/src/backend/catalog/pg_shdepend.c
@@ -84,6 +84,11 @@ static void shdepChangeDep(Relation sdepRel,
Oid classid, Oid objid, int32 objsubid,
Oid refclassid, Oid refobjid,
SharedDependencyType deptype);
+static void updateAclDependenciesWorker(Oid classId, Oid objectId,
+ int32 objsubId, Oid ownerId,
+ SharedDependencyType deptype,
+ int noldmembers, Oid *oldmembers,
+ int nnewmembers, Oid *newmembers);
static void shdepAddDependency(Relation sdepRel,
Oid classId, Oid objectId, int32 objsubId,
Oid refclassId, Oid refobjId,
@@ -340,6 +345,11 @@ changeDependencyOnOwner(Oid classId, Oid objectId, Oid newOwnerId)
AuthIdRelationId, newOwnerId,
SHARED_DEPENDENCY_ACL);
+ /* The same applies to SHARED_DEPENDENCY_INITACL */
+ shdepDropDependency(sdepRel, classId, objectId, 0, true,
+ AuthIdRelationId, newOwnerId,
+ SHARED_DEPENDENCY_INITACL);
+
table_close(sdepRel, RowExclusiveLock);
}
@@ -478,6 +488,38 @@ updateAclDependencies(Oid classId, Oid objectId, int32 objsubId,
Oid ownerId,
int noldmembers, Oid *oldmembers,
int nnewmembers, Oid *newmembers)
+{
+ updateAclDependenciesWorker(classId, objectId, objsubId,
+ ownerId, SHARED_DEPENDENCY_ACL,
+ noldmembers, oldmembers,
+ nnewmembers, newmembers);
+}
+
+/*
+ * updateInitAclDependencies
+ * Update the pg_shdepend info for a pg_init_privs entry.
+ *
+ * Exactly like updateAclDependencies, except we are considering a
+ * pg_init_privs ACL for the specified object.
+ */
+void
+updateInitAclDependencies(Oid classId, Oid objectId, int32 objsubId,
+ Oid ownerId,
+ int noldmembers, Oid *oldmembers,
+ int nnewmembers, Oid *newmembers)
+{
+ updateAclDependenciesWorker(classId, objectId, objsubId,
+ ownerId, SHARED_DEPENDENCY_INITACL,
+ noldmembers, oldmembers,
+ nnewmembers, newmembers);
+}
+
+/* Common code for the above two functions */
+static void
+updateAclDependenciesWorker(Oid classId, Oid objectId, int32 objsubId,
+ Oid ownerId, SharedDependencyType deptype,
+ int noldmembers, Oid *oldmembers,
+ int nnewmembers, Oid *newmembers)
{
Relation sdepRel;
int i;
@@ -513,7 +555,7 @@ updateAclDependencies(Oid classId, Oid objectId, int32 objsubId,
shdepAddDependency(sdepRel, classId, objectId, objsubId,
AuthIdRelationId, roleid,
- SHARED_DEPENDENCY_ACL);
+ deptype);
}
/* Drop no-longer-used old dependencies */
@@ -532,7 +574,7 @@ updateAclDependencies(Oid classId, Oid objectId, int32 objsubId,
shdepDropDependency(sdepRel, classId, objectId, objsubId,
false, /* exact match on objsubId */
AuthIdRelationId, roleid,
- SHARED_DEPENDENCY_ACL);
+ deptype);
}
table_close(sdepRel, RowExclusiveLock);
@@ -1249,6 +1291,8 @@ storeObjectDescription(StringInfo descs,
appendStringInfo(descs, _("owner of %s"), objdesc);
else if (deptype == SHARED_DEPENDENCY_ACL)
appendStringInfo(descs, _("privileges for %s"), objdesc);
+ else if (deptype == SHARED_DEPENDENCY_INITACL)
+ appendStringInfo(descs, _("initial privileges for %s"), objdesc);
else if (deptype == SHARED_DEPENDENCY_POLICY)
appendStringInfo(descs, _("target of %s"), objdesc);
else if (deptype == SHARED_DEPENDENCY_TABLESPACE)
@@ -1431,6 +1475,14 @@ shdepDropOwned(List *roleids, DropBehavior behavior)
add_exact_object_address(&obj, deleteobjs);
}
break;
+ case SHARED_DEPENDENCY_INITACL:
+ /* Shouldn't see a role grant here */
+ Assert(sdepForm->classid != AuthMemRelationId);
+ RemoveRoleFromInitPriv(roleid,
+ sdepForm->classid,
+ sdepForm->objid,
+ sdepForm->objsubid);
+ break;
}
}
diff --git a/src/include/catalog/dependency.h b/src/include/catalog/dependency.h
index ec654010d4..e0dcc0b069 100644
--- a/src/include/catalog/dependency.h
+++ b/src/include/catalog/dependency.h
@@ -56,11 +56,17 @@ typedef enum DependencyType
* created for the owner of an object; hence two objects may be linked by
* one or the other, but not both, of these dependency types.)
*
- * (c) a SHARED_DEPENDENCY_POLICY entry means that the referenced object is
+ * (c) a SHARED_DEPENDENCY_INITACL entry means that the referenced object is
+ * a role mentioned in a pg_init_privs entry for the dependent object. The
+ * referenced object must be a pg_authid entry. (SHARED_DEPENDENCY_INITACL
+ * entries are not created for the owner of an object; hence two objects may
+ * be linked by one or the other, but not both, of these dependency types.)
+ *
+ * (d) a SHARED_DEPENDENCY_POLICY entry means that the referenced object is
* a role mentioned in a policy object. The referenced object must be a
* pg_authid entry.
*
- * (d) a SHARED_DEPENDENCY_TABLESPACE entry means that the referenced
+ * (e) a SHARED_DEPENDENCY_TABLESPACE entry means that the referenced
* object is a tablespace mentioned in a relation without storage. The
* referenced object must be a pg_tablespace entry. (Relations that have
* storage don't need this: they are protected by the existence of a physical
@@ -73,6 +79,7 @@ typedef enum SharedDependencyType
{
SHARED_DEPENDENCY_OWNER = 'o',
SHARED_DEPENDENCY_ACL = 'a',
+ SHARED_DEPENDENCY_INITACL = 'i',
SHARED_DEPENDENCY_POLICY = 'r',
SHARED_DEPENDENCY_TABLESPACE = 't',
SHARED_DEPENDENCY_INVALID = 0,
@@ -201,6 +208,11 @@ extern void updateAclDependencies(Oid classId, Oid objectId, int32 objsubId,
int noldmembers, Oid *oldmembers,
int nnewmembers, Oid *newmembers);
+extern void updateInitAclDependencies(Oid classId, Oid objectId, int32 objsubId,
+ Oid ownerId,
+ int noldmembers, Oid *oldmembers,
+ int nnewmembers, Oid *newmembers);
+
extern bool checkSharedDependencies(Oid classId, Oid objectId,
char **detail_msg, char **detail_log_msg);
diff --git a/src/include/utils/acl.h b/src/include/utils/acl.h
index 3a0baf3039..1a554c6699 100644
--- a/src/include/utils/acl.h
+++ b/src/include/utils/acl.h
@@ -276,6 +276,8 @@ extern void aclcheck_error_type(AclResult aclerr, Oid typeOid);
extern void recordExtObjInitPriv(Oid objoid, Oid classoid);
extern void removeExtObjInitPriv(Oid objoid, Oid classoid);
+extern void RemoveRoleFromInitPriv(Oid roleid,
+ Oid classid, Oid objid, int32 objsubid);
/* ownercheck routines just return true (owner) or false (not) */
I wrote:
> Here's a draft patch that attacks that. It seems to fix the
> problem with test_pg_dump: no dangling pg_init_privs grants
> are left behind.
Here's a v2 that attempts to add some queries to test_pg_dump.sql
to provide visual verification that pg_shdepend and pg_init_privs
are updated correctly during DROP OWNED BY. It's a little bit
nasty to look at the ACL column of pg_init_privs, because that text
involves the bootstrap superuser's name which is site-dependent.
What I did to try to make the test stable is
replace(initprivs::text, current_user, 'postgres') AS initprivs
This is of course not bulletproof: with a sufficiently weird
bootstrap superuser name, we could get false matches to parts
of "regress_dump_test_role" or to privilege strings. That
seems unlikely enough to live with, but I wonder if anybody has
a better idea.
regards, tom lane
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 2907079e2a..c8cb46c5b9 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -7184,6 +7184,20 @@ SCRAM-SHA-256$<replaceable><iteration count></replaceable>:<replaceable>&l
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><symbol>SHARED_DEPENDENCY_INITACL</symbol> (<literal>i</literal>)</term>
+ <listitem>
+ <para>
+ The referenced object (which must be a role) is mentioned in a
+ <link linkend="catalog-pg-init-privs"><structname>pg_init_privs</structname></link>
+ entry for the dependent object.
+ (A <symbol>SHARED_DEPENDENCY_INITACL</symbol> entry is not made for
+ the owner of the object, since the owner will have
+ a <symbol>SHARED_DEPENDENCY_OWNER</symbol> entry anyway.)
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><symbol>SHARED_DEPENDENCY_POLICY</symbol> (<literal>r</literal>)</term>
<listitem>
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 7abf3c2a74..04c41c0c14 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -165,9 +165,9 @@ static AclMode pg_type_aclmask_ext(Oid type_oid, Oid roleid,
AclMode mask, AclMaskHow how,
bool *is_missing);
static void recordExtensionInitPriv(Oid objoid, Oid classoid, int objsubid,
- Acl *new_acl);
+ Oid ownerId, Acl *new_acl);
static void recordExtensionInitPrivWorker(Oid objoid, Oid classoid, int objsubid,
- Acl *new_acl);
+ Oid ownerId, Acl *new_acl);
/*
@@ -1790,7 +1790,7 @@ ExecGrant_Attribute(InternalGrant *istmt, Oid relOid, const char *relname,
CatalogTupleUpdate(attRelation, &newtuple->t_self, newtuple);
/* Update initial privileges for extensions */
- recordExtensionInitPriv(relOid, RelationRelationId, attnum,
+ recordExtensionInitPriv(relOid, RelationRelationId, attnum, ownerId,
ACL_NUM(new_acl) > 0 ? new_acl : NULL);
/* Update the shared dependency ACL info */
@@ -2050,7 +2050,8 @@ ExecGrant_Relation(InternalGrant *istmt)
CatalogTupleUpdate(relation, &newtuple->t_self, newtuple);
/* Update initial privileges for extensions */
- recordExtensionInitPriv(relOid, RelationRelationId, 0, new_acl);
+ recordExtensionInitPriv(relOid, RelationRelationId, 0,
+ ownerId, new_acl);
/* Update the shared dependency ACL info */
updateAclDependencies(RelationRelationId, relOid, 0,
@@ -2251,7 +2252,7 @@ ExecGrant_common(InternalGrant *istmt, Oid classid, AclMode default_privs,
CatalogTupleUpdate(relation, &newtuple->t_self, newtuple);
/* Update initial privileges for extensions */
- recordExtensionInitPriv(objectid, classid, 0, new_acl);
+ recordExtensionInitPriv(objectid, classid, 0, ownerId, new_acl);
/* Update the shared dependency ACL info */
updateAclDependencies(classid,
@@ -2403,7 +2404,8 @@ ExecGrant_Largeobject(InternalGrant *istmt)
CatalogTupleUpdate(relation, &newtuple->t_self, newtuple);
/* Update initial privileges for extensions */
- recordExtensionInitPriv(loid, LargeObjectRelationId, 0, new_acl);
+ recordExtensionInitPriv(loid, LargeObjectRelationId, 0,
+ ownerId, new_acl);
/* Update the shared dependency ACL info */
updateAclDependencies(LargeObjectRelationId,
@@ -2575,7 +2577,7 @@ ExecGrant_Parameter(InternalGrant *istmt)
/* Update initial privileges for extensions */
recordExtensionInitPriv(parameterId, ParameterAclRelationId, 0,
- new_acl);
+ ownerId, new_acl);
/* Update the shared dependency ACL info */
updateAclDependencies(ParameterAclRelationId, parameterId, 0,
@@ -4463,6 +4465,7 @@ recordExtObjInitPriv(Oid objoid, Oid classoid)
}
recordExtensionInitPrivWorker(objoid, classoid, curr_att,
+ pg_class_tuple->relowner,
DatumGetAclP(attaclDatum));
ReleaseSysCache(attTuple);
@@ -4475,6 +4478,7 @@ recordExtObjInitPriv(Oid objoid, Oid classoid)
/* Add the record, if any, for the top-level object */
if (!isNull)
recordExtensionInitPrivWorker(objoid, classoid, 0,
+ pg_class_tuple->relowner,
DatumGetAclP(aclDatum));
ReleaseSysCache(tuple);
@@ -4485,6 +4489,7 @@ recordExtObjInitPriv(Oid objoid, Oid classoid)
Datum aclDatum;
bool isNull;
HeapTuple tuple;
+ Form_pg_largeobject_metadata form_lo_meta;
ScanKeyData entry[1];
SysScanDesc scan;
Relation relation;
@@ -4509,6 +4514,7 @@ recordExtObjInitPriv(Oid objoid, Oid classoid)
tuple = systable_getnext(scan);
if (!HeapTupleIsValid(tuple))
elog(ERROR, "could not find tuple for large object %u", objoid);
+ form_lo_meta = (Form_pg_largeobject_metadata) GETSTRUCT(tuple);
aclDatum = heap_getattr(tuple,
Anum_pg_largeobject_metadata_lomacl,
@@ -4517,6 +4523,7 @@ recordExtObjInitPriv(Oid objoid, Oid classoid)
/* Add the record, if any, for the top-level object */
if (!isNull)
recordExtensionInitPrivWorker(objoid, classoid, 0,
+ form_lo_meta->lomowner,
DatumGetAclP(aclDatum));
systable_endscan(scan);
@@ -4524,24 +4531,29 @@ recordExtObjInitPriv(Oid objoid, Oid classoid)
/* This will error on unsupported classoid. */
else if (get_object_attnum_acl(classoid) != InvalidAttrNumber)
{
+ int cacheid;
+ Oid ownerId;
Datum aclDatum;
bool isNull;
HeapTuple tuple;
- tuple = SearchSysCache1(get_object_catcache_oid(classoid),
- ObjectIdGetDatum(objoid));
+ cacheid = get_object_catcache_oid(classoid);
+ tuple = SearchSysCache1(cacheid, ObjectIdGetDatum(objoid));
if (!HeapTupleIsValid(tuple))
elog(ERROR, "cache lookup failed for %s %u",
get_object_class_descr(classoid), objoid);
- aclDatum = SysCacheGetAttr(get_object_catcache_oid(classoid), tuple,
+ ownerId = DatumGetObjectId(SysCacheGetAttrNotNull(cacheid,
+ tuple,
+ get_object_attnum_owner(classoid)));
+ aclDatum = SysCacheGetAttr(cacheid, tuple,
get_object_attnum_acl(classoid),
&isNull);
/* Add the record, if any, for the top-level object */
if (!isNull)
recordExtensionInitPrivWorker(objoid, classoid, 0,
- DatumGetAclP(aclDatum));
+ ownerId, DatumGetAclP(aclDatum));
ReleaseSysCache(tuple);
}
@@ -4554,6 +4566,8 @@ recordExtObjInitPriv(Oid objoid, Oid classoid)
void
removeExtObjInitPriv(Oid objoid, Oid classoid)
{
+ Oid ownerId;
+
/*
* If this is a relation then we need to see if there are any sub-objects
* (eg: columns) for it and, if so, be sure to call
@@ -4568,6 +4582,7 @@ removeExtObjInitPriv(Oid objoid, Oid classoid)
if (!HeapTupleIsValid(tuple))
elog(ERROR, "cache lookup failed for relation %u", objoid);
pg_class_tuple = (Form_pg_class) GETSTRUCT(tuple);
+ ownerId = pg_class_tuple->relowner;
/*
* Indexes don't have permissions, neither do the pg_class rows for
@@ -4604,7 +4619,8 @@ removeExtObjInitPriv(Oid objoid, Oid classoid)
/* when removing, remove all entries, even dropped columns */
- recordExtensionInitPrivWorker(objoid, classoid, curr_att, NULL);
+ recordExtensionInitPrivWorker(objoid, classoid, curr_att,
+ ownerId, NULL);
ReleaseSysCache(attTuple);
}
@@ -4612,9 +4628,35 @@ removeExtObjInitPriv(Oid objoid, Oid classoid)
ReleaseSysCache(tuple);
}
+ else
+ {
+ /* Must find out the owner's OID the hard way */
+ AttrNumber ownerattnum;
+ int cacheid;
+ HeapTuple tuple;
+
+ /*
+ * If the object is of a type that has no owner, it should not have
+ * any pg_init_privs entry either.
+ */
+ ownerattnum = get_object_attnum_owner(classoid);
+ if (ownerattnum == InvalidAttrNumber)
+ return;
+ cacheid = get_object_catcache_oid(classoid);
+ tuple = SearchSysCache1(cacheid, ObjectIdGetDatum(objoid));
+ if (!HeapTupleIsValid(tuple))
+ elog(ERROR, "cache lookup failed for %s %u",
+ get_object_class_descr(classoid), objoid);
+
+ ownerId = DatumGetObjectId(SysCacheGetAttrNotNull(cacheid,
+ tuple,
+ ownerattnum));
+
+ ReleaseSysCache(tuple);
+ }
/* Remove the record, if any, for the top-level object */
- recordExtensionInitPrivWorker(objoid, classoid, 0, NULL);
+ recordExtensionInitPrivWorker(objoid, classoid, 0, ownerId, NULL);
}
/*
@@ -4626,7 +4668,8 @@ removeExtObjInitPriv(Oid objoid, Oid classoid)
* Pass in the object OID, the OID of the class (the OID of the table which
* the object is defined in) and the 'sub' id of the object (objsubid), if
* any. If there is no 'sub' id (they are currently only used for columns of
- * tables) then pass in '0'. Finally, pass in the complete ACL to store.
+ * tables) then pass in '0'. Also pass the OID of the object's owner.
+ * Finally, pass in the complete ACL to store.
*
* If an ACL already exists for this object/sub-object then we will replace
* it with what is passed in.
@@ -4635,7 +4678,8 @@ removeExtObjInitPriv(Oid objoid, Oid classoid)
* removed, if one is found.
*/
static void
-recordExtensionInitPriv(Oid objoid, Oid classoid, int objsubid, Acl *new_acl)
+recordExtensionInitPriv(Oid objoid, Oid classoid, int objsubid,
+ Oid ownerId, Acl *new_acl)
{
/*
* Generally, we only record the initial privileges when an extension is
@@ -4648,7 +4692,7 @@ recordExtensionInitPriv(Oid objoid, Oid classoid, int objsubid, Acl *new_acl)
if (!creating_extension && !binary_upgrade_record_init_privs)
return;
- recordExtensionInitPrivWorker(objoid, classoid, objsubid, new_acl);
+ recordExtensionInitPrivWorker(objoid, classoid, objsubid, ownerId, new_acl);
}
/*
@@ -4664,14 +4708,23 @@ recordExtensionInitPriv(Oid objoid, Oid classoid, int objsubid, Acl *new_acl)
* EXTENSION ... ADD/DROP.
*/
static void
-recordExtensionInitPrivWorker(Oid objoid, Oid classoid, int objsubid, Acl *new_acl)
+recordExtensionInitPrivWorker(Oid objoid, Oid classoid, int objsubid,
+ Oid ownerId, Acl *new_acl)
{
Relation relation;
ScanKeyData key[3];
SysScanDesc scan;
HeapTuple tuple;
HeapTuple oldtuple;
+ int noldmembers;
+ int nnewmembers;
+ Oid *oldmembers;
+ Oid *newmembers;
+ /* We'll need the role membership of the new ACL. */
+ nnewmembers = aclmembers(new_acl, &newmembers);
+
+ /* Search pg_init_privs for an existing entry. */
relation = table_open(InitPrivsRelationId, RowExclusiveLock);
ScanKeyInit(&key[0],
@@ -4699,6 +4752,23 @@ recordExtensionInitPrivWorker(Oid objoid, Oid classoid, int objsubid, Acl *new_a
Datum values[Natts_pg_init_privs] = {0};
bool nulls[Natts_pg_init_privs] = {0};
bool replace[Natts_pg_init_privs] = {0};
+ Datum oldAclDatum;
+ bool isNull;
+ Acl *old_acl;
+
+ /* Update pg_shdepend for roles mentioned in the old/new ACLs. */
+ oldAclDatum = heap_getattr(oldtuple, Anum_pg_init_privs_initprivs,
+ RelationGetDescr(relation), &isNull);
+ if (!isNull)
+ old_acl = DatumGetAclP(oldAclDatum);
+ else
+ old_acl = NULL; /* this case shouldn't happen, probably */
+ noldmembers = aclmembers(old_acl, &oldmembers);
+
+ updateInitAclDependencies(classoid, objoid, objsubid,
+ ownerId,
+ noldmembers, oldmembers,
+ nnewmembers, newmembers);
/* If we have a new ACL to set, then update the row with it. */
if (new_acl)
@@ -4744,6 +4814,15 @@ recordExtensionInitPrivWorker(Oid objoid, Oid classoid, int objsubid, Acl *new_a
tuple = heap_form_tuple(RelationGetDescr(relation), values, nulls);
CatalogTupleInsert(relation, tuple);
+
+ /* Update pg_shdepend, too. */
+ noldmembers = 0;
+ oldmembers = NULL;
+
+ updateInitAclDependencies(classoid, objoid, objsubid,
+ ownerId,
+ noldmembers, oldmembers,
+ nnewmembers, newmembers);
}
}
@@ -4754,3 +4833,138 @@ recordExtensionInitPrivWorker(Oid objoid, Oid classoid, int objsubid, Acl *new_a
table_close(relation, RowExclusiveLock);
}
+
+/*
+ * RemoveRoleFromInitPriv
+ *
+ * Used by shdepDropOwned to remove mentions of a role in pg_init_privs.
+ */
+void
+RemoveRoleFromInitPriv(Oid roleid, Oid classid, Oid objid, int32 objsubid)
+{
+ Relation rel;
+ ScanKeyData key[3];
+ SysScanDesc scan;
+ HeapTuple oldtuple;
+ int cacheid;
+ HeapTuple objtuple;
+ Oid ownerId;
+ Datum oldAclDatum;
+ bool isNull;
+ Acl *old_acl;
+ Acl *new_acl;
+ HeapTuple newtuple;
+ int noldmembers;
+ int nnewmembers;
+ Oid *oldmembers;
+ Oid *newmembers;
+
+ /* Search for existing pg_init_privs entry for the target object. */
+ rel = table_open(InitPrivsRelationId, RowExclusiveLock);
+
+ ScanKeyInit(&key[0],
+ Anum_pg_init_privs_objoid,
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(objid));
+ ScanKeyInit(&key[1],
+ Anum_pg_init_privs_classoid,
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(classid));
+ ScanKeyInit(&key[2],
+ Anum_pg_init_privs_objsubid,
+ BTEqualStrategyNumber, F_INT4EQ,
+ Int32GetDatum(objsubid));
+
+ scan = systable_beginscan(rel, InitPrivsObjIndexId, true,
+ NULL, 3, key);
+
+ /* There should exist only one entry or none. */
+ oldtuple = systable_getnext(scan);
+
+ if (!HeapTupleIsValid(oldtuple))
+ {
+ /*
+ * Hmm, why are we here if there's no entry? But pack up and go away
+ * quietly.
+ */
+ systable_endscan(scan);
+ table_close(rel, RowExclusiveLock);
+ return;
+ }
+
+ /* Get a writable copy of the existing ACL. */
+ oldAclDatum = heap_getattr(oldtuple, Anum_pg_init_privs_initprivs,
+ RelationGetDescr(rel), &isNull);
+ if (!isNull)
+ old_acl = DatumGetAclPCopy(oldAclDatum);
+ else
+ old_acl = NULL; /* this case shouldn't happen, probably */
+
+ /*
+ * We need the members of both old and new ACLs so we can correct the
+ * shared dependency information. Collect data before
+ * merge_acl_with_grant throws away old_acl.
+ */
+ noldmembers = aclmembers(old_acl, &oldmembers);
+
+ /* Must find out the owner's OID the hard way. */
+ cacheid = get_object_catcache_oid(classid);
+ objtuple = SearchSysCache1(cacheid, ObjectIdGetDatum(objid));
+ if (!HeapTupleIsValid(objtuple))
+ elog(ERROR, "cache lookup failed for %s %u",
+ get_object_class_descr(classid), objid);
+
+ ownerId = DatumGetObjectId(SysCacheGetAttrNotNull(cacheid,
+ objtuple,
+ get_object_attnum_owner(classid)));
+ ReleaseSysCache(objtuple);
+
+ /*
+ * Generate new ACL. Grantor of rights is always the same as the owner.
+ */
+ new_acl = merge_acl_with_grant(old_acl,
+ false, /* is_grant */
+ false, /* grant_option */
+ DROP_RESTRICT,
+ list_make1_oid(roleid),
+ ACLITEM_ALL_PRIV_BITS,
+ ownerId,
+ ownerId);
+
+ /* If we end with an empty ACL, delete the pg_init_privs entry. */
+ if (new_acl == NULL || ACL_NUM(new_acl) == 0)
+ {
+ CatalogTupleDelete(rel, &oldtuple->t_self);
+ }
+ else
+ {
+ Datum values[Natts_pg_init_privs] = {0};
+ bool nulls[Natts_pg_init_privs] = {0};
+ bool replaces[Natts_pg_init_privs] = {0};
+
+ /* Update existing entry. */
+ values[Anum_pg_init_privs_initprivs - 1] = PointerGetDatum(new_acl);
+ replaces[Anum_pg_init_privs_initprivs - 1] = true;
+
+ newtuple = heap_modify_tuple(oldtuple, RelationGetDescr(rel),
+ values, nulls, replaces);
+ CatalogTupleUpdate(rel, &newtuple->t_self, newtuple);
+ }
+
+ /*
+ * Update the shared dependency ACL info.
+ */
+ nnewmembers = aclmembers(new_acl, &newmembers);
+
+ updateInitAclDependencies(classid, objid, objsubid,
+ ownerId,
+ noldmembers, oldmembers,
+ nnewmembers, newmembers);
+
+ systable_endscan(scan);
+
+ /* prevent error when processing objects multiple times */
+ CommandCounterIncrement();
+
+ table_close(rel, RowExclusiveLock);
+}
diff --git a/src/backend/catalog/pg_shdepend.c b/src/backend/catalog/pg_shdepend.c
index cb31590339..20bcfd779b 100644
--- a/src/backend/catalog/pg_shdepend.c
+++ b/src/backend/catalog/pg_shdepend.c
@@ -84,6 +84,11 @@ static void shdepChangeDep(Relation sdepRel,
Oid classid, Oid objid, int32 objsubid,
Oid refclassid, Oid refobjid,
SharedDependencyType deptype);
+static void updateAclDependenciesWorker(Oid classId, Oid objectId,
+ int32 objsubId, Oid ownerId,
+ SharedDependencyType deptype,
+ int noldmembers, Oid *oldmembers,
+ int nnewmembers, Oid *newmembers);
static void shdepAddDependency(Relation sdepRel,
Oid classId, Oid objectId, int32 objsubId,
Oid refclassId, Oid refobjId,
@@ -340,6 +345,11 @@ changeDependencyOnOwner(Oid classId, Oid objectId, Oid newOwnerId)
AuthIdRelationId, newOwnerId,
SHARED_DEPENDENCY_ACL);
+ /* The same applies to SHARED_DEPENDENCY_INITACL */
+ shdepDropDependency(sdepRel, classId, objectId, 0, true,
+ AuthIdRelationId, newOwnerId,
+ SHARED_DEPENDENCY_INITACL);
+
table_close(sdepRel, RowExclusiveLock);
}
@@ -478,6 +488,38 @@ updateAclDependencies(Oid classId, Oid objectId, int32 objsubId,
Oid ownerId,
int noldmembers, Oid *oldmembers,
int nnewmembers, Oid *newmembers)
+{
+ updateAclDependenciesWorker(classId, objectId, objsubId,
+ ownerId, SHARED_DEPENDENCY_ACL,
+ noldmembers, oldmembers,
+ nnewmembers, newmembers);
+}
+
+/*
+ * updateInitAclDependencies
+ * Update the pg_shdepend info for a pg_init_privs entry.
+ *
+ * Exactly like updateAclDependencies, except we are considering a
+ * pg_init_privs ACL for the specified object.
+ */
+void
+updateInitAclDependencies(Oid classId, Oid objectId, int32 objsubId,
+ Oid ownerId,
+ int noldmembers, Oid *oldmembers,
+ int nnewmembers, Oid *newmembers)
+{
+ updateAclDependenciesWorker(classId, objectId, objsubId,
+ ownerId, SHARED_DEPENDENCY_INITACL,
+ noldmembers, oldmembers,
+ nnewmembers, newmembers);
+}
+
+/* Common code for the above two functions */
+static void
+updateAclDependenciesWorker(Oid classId, Oid objectId, int32 objsubId,
+ Oid ownerId, SharedDependencyType deptype,
+ int noldmembers, Oid *oldmembers,
+ int nnewmembers, Oid *newmembers)
{
Relation sdepRel;
int i;
@@ -513,7 +555,7 @@ updateAclDependencies(Oid classId, Oid objectId, int32 objsubId,
shdepAddDependency(sdepRel, classId, objectId, objsubId,
AuthIdRelationId, roleid,
- SHARED_DEPENDENCY_ACL);
+ deptype);
}
/* Drop no-longer-used old dependencies */
@@ -532,7 +574,7 @@ updateAclDependencies(Oid classId, Oid objectId, int32 objsubId,
shdepDropDependency(sdepRel, classId, objectId, objsubId,
false, /* exact match on objsubId */
AuthIdRelationId, roleid,
- SHARED_DEPENDENCY_ACL);
+ deptype);
}
table_close(sdepRel, RowExclusiveLock);
@@ -1249,6 +1291,8 @@ storeObjectDescription(StringInfo descs,
appendStringInfo(descs, _("owner of %s"), objdesc);
else if (deptype == SHARED_DEPENDENCY_ACL)
appendStringInfo(descs, _("privileges for %s"), objdesc);
+ else if (deptype == SHARED_DEPENDENCY_INITACL)
+ appendStringInfo(descs, _("initial privileges for %s"), objdesc);
else if (deptype == SHARED_DEPENDENCY_POLICY)
appendStringInfo(descs, _("target of %s"), objdesc);
else if (deptype == SHARED_DEPENDENCY_TABLESPACE)
@@ -1431,6 +1475,14 @@ shdepDropOwned(List *roleids, DropBehavior behavior)
add_exact_object_address(&obj, deleteobjs);
}
break;
+ case SHARED_DEPENDENCY_INITACL:
+ /* Shouldn't see a role grant here */
+ Assert(sdepForm->classid != AuthMemRelationId);
+ RemoveRoleFromInitPriv(roleid,
+ sdepForm->classid,
+ sdepForm->objid,
+ sdepForm->objsubid);
+ break;
}
}
diff --git a/src/include/catalog/dependency.h b/src/include/catalog/dependency.h
index ec654010d4..e0dcc0b069 100644
--- a/src/include/catalog/dependency.h
+++ b/src/include/catalog/dependency.h
@@ -56,11 +56,17 @@ typedef enum DependencyType
* created for the owner of an object; hence two objects may be linked by
* one or the other, but not both, of these dependency types.)
*
- * (c) a SHARED_DEPENDENCY_POLICY entry means that the referenced object is
+ * (c) a SHARED_DEPENDENCY_INITACL entry means that the referenced object is
+ * a role mentioned in a pg_init_privs entry for the dependent object. The
+ * referenced object must be a pg_authid entry. (SHARED_DEPENDENCY_INITACL
+ * entries are not created for the owner of an object; hence two objects may
+ * be linked by one or the other, but not both, of these dependency types.)
+ *
+ * (d) a SHARED_DEPENDENCY_POLICY entry means that the referenced object is
* a role mentioned in a policy object. The referenced object must be a
* pg_authid entry.
*
- * (d) a SHARED_DEPENDENCY_TABLESPACE entry means that the referenced
+ * (e) a SHARED_DEPENDENCY_TABLESPACE entry means that the referenced
* object is a tablespace mentioned in a relation without storage. The
* referenced object must be a pg_tablespace entry. (Relations that have
* storage don't need this: they are protected by the existence of a physical
@@ -73,6 +79,7 @@ typedef enum SharedDependencyType
{
SHARED_DEPENDENCY_OWNER = 'o',
SHARED_DEPENDENCY_ACL = 'a',
+ SHARED_DEPENDENCY_INITACL = 'i',
SHARED_DEPENDENCY_POLICY = 'r',
SHARED_DEPENDENCY_TABLESPACE = 't',
SHARED_DEPENDENCY_INVALID = 0,
@@ -201,6 +208,11 @@ extern void updateAclDependencies(Oid classId, Oid objectId, int32 objsubId,
int noldmembers, Oid *oldmembers,
int nnewmembers, Oid *newmembers);
+extern void updateInitAclDependencies(Oid classId, Oid objectId, int32 objsubId,
+ Oid ownerId,
+ int noldmembers, Oid *oldmembers,
+ int nnewmembers, Oid *newmembers);
+
extern bool checkSharedDependencies(Oid classId, Oid objectId,
char **detail_msg, char **detail_log_msg);
diff --git a/src/include/utils/acl.h b/src/include/utils/acl.h
index 3a0baf3039..1a554c6699 100644
--- a/src/include/utils/acl.h
+++ b/src/include/utils/acl.h
@@ -276,6 +276,8 @@ extern void aclcheck_error_type(AclResult aclerr, Oid typeOid);
extern void recordExtObjInitPriv(Oid objoid, Oid classoid);
extern void removeExtObjInitPriv(Oid objoid, Oid classoid);
+extern void RemoveRoleFromInitPriv(Oid roleid,
+ Oid classid, Oid objid, int32 objsubid);
/* ownercheck routines just return true (owner) or false (not) */
diff --git a/src/test/modules/test_pg_dump/expected/test_pg_dump.out
b/src/test/modules/test_pg_dump/expected/test_pg_dump.out
index f57c96aeb9..be71822ece 100644
--- a/src/test/modules/test_pg_dump/expected/test_pg_dump.out
+++ b/src/test/modules/test_pg_dump/expected/test_pg_dump.out
@@ -62,6 +62,59 @@ GRANT SELECT ON ft1 TO regress_dump_test_role;
GRANT UPDATE ON test_pg_dump_mv1 TO regress_dump_test_role;
GRANT USAGE ON SCHEMA test_pg_dump_s1 TO regress_dump_test_role;
GRANT USAGE ON TYPE test_pg_dump_e1 TO regress_dump_test_role;
+SELECT pg_describe_object(classoid,objoid,objsubid) AS obj,
+ replace(initprivs::text, current_user, 'postgres') AS initprivs
+ FROM pg_init_privs WHERE privtype = 'e' ORDER BY 1;
+ obj | initprivs
+----------------------------------------------------+---------------------------------------------------------------------
+ column col1 of table regress_pg_dump_table | {=r/postgres}
+ function regress_pg_dump_schema.test_agg(smallint) |
{=X/postgres,postgres=X/postgres,regress_dump_test_role=X/postgres}
+ function regress_pg_dump_schema.test_func() |
{=X/postgres,postgres=X/postgres,regress_dump_test_role=X/postgres}
+ function wgo_then_no_access() | {=X/postgres,postgres=X/postgres,pg_signal_backend=X*/postgres}
+ sequence regress_pg_dump_schema.test_seq | {postgres=rwU/postgres,regress_dump_test_role=U/postgres}
+ sequence regress_pg_dump_seq | {postgres=rwU/postgres,regress_dump_test_role=U/postgres}
+ sequence regress_seq_dumpable | {postgres=rwU/postgres,=r/postgres}
+ sequence wgo_then_regular | {postgres=rwU/postgres,pg_signal_backend=rw*U*/postgres}
+ table regress_pg_dump_schema.test_table | {postgres=arwdDxtm/postgres,regress_dump_test_role=r/postgres}
+ table regress_pg_dump_table | {postgres=arwdDxtm/postgres,regress_dump_test_role=r/postgres}
+ table regress_table_dumpable | {postgres=arwdDxtm/postgres,=r/postgres}
+ type regress_pg_dump_schema.test_type |
{=U/postgres,postgres=U/postgres,regress_dump_test_role=U/postgres}
+(12 rows)
+
+SELECT pg_describe_object(classid,objid,objsubid) AS obj,
+ pg_describe_object(refclassid,refobjid,0) AS refobj,
+ deptype
+ FROM pg_shdepend JOIN pg_database d ON dbid = d.oid
+ WHERE d.datname = current_database()
+ ORDER BY 1, 3;
+ obj | refobj | deptype
+----------------------------------------------------+-----------------------------+---------
+ column c1 of foreign table ft1 | role regress_dump_test_role | a
+ column c1 of table test_pg_dump_t1 | role regress_dump_test_role | a
+ foreign table ft1 | role regress_dump_test_role | a
+ foreign-data wrapper dummy | role regress_dump_test_role | a
+ function regress_pg_dump_schema.test_agg(smallint) | role regress_dump_test_role | a
+ function regress_pg_dump_schema.test_agg(smallint) | role regress_dump_test_role | i
+ function regress_pg_dump_schema.test_func() | role regress_dump_test_role | a
+ function regress_pg_dump_schema.test_func() | role regress_dump_test_role | i
+ function test_pg_dump(integer) | role regress_dump_test_role | a
+ materialized view test_pg_dump_mv1 | role regress_dump_test_role | a
+ schema test_pg_dump_s1 | role regress_dump_test_role | a
+ sequence regress_pg_dump_schema.test_seq | role regress_dump_test_role | a
+ sequence regress_pg_dump_schema.test_seq | role regress_dump_test_role | i
+ sequence regress_pg_dump_seq | role regress_dump_test_role | a
+ sequence regress_pg_dump_seq | role regress_dump_test_role | i
+ server s0 | role regress_dump_test_role | a
+ table regress_pg_dump_schema.test_table | role regress_dump_test_role | a
+ table regress_pg_dump_schema.test_table | role regress_dump_test_role | i
+ table regress_pg_dump_table | role regress_dump_test_role | a
+ table regress_pg_dump_table | role regress_dump_test_role | i
+ type regress_pg_dump_schema.test_type | role regress_dump_test_role | a
+ type regress_pg_dump_schema.test_type | role regress_dump_test_role | i
+ type test_pg_dump_e1 | role regress_dump_test_role | a
+ view test_pg_dump_v1 | role regress_dump_test_role | a
+(24 rows)
+
ALTER EXTENSION test_pg_dump ADD ACCESS METHOD gist2;
ALTER EXTENSION test_pg_dump ADD AGGREGATE newavg(int4);
ALTER EXTENSION test_pg_dump ADD CAST (text AS casttesttype);
@@ -92,4 +145,33 @@ ALTER EXTENSION test_pg_dump DROP TABLE test_pg_dump_t1;
ALTER EXTENSION test_pg_dump DROP TYPE test_pg_dump_e1;
ALTER EXTENSION test_pg_dump DROP VIEW test_pg_dump_v1;
DROP OWNED BY regress_dump_test_role RESTRICT;
+SELECT pg_describe_object(classoid,objoid,objsubid) AS obj,
+ replace(initprivs::text, current_user, 'postgres') AS initprivs
+ FROM pg_init_privs WHERE privtype = 'e' ORDER BY 1;
+ obj | initprivs
+----------------------------------------------------+-----------------------------------------------------------------
+ column col1 of table regress_pg_dump_table | {=r/postgres}
+ function regress_pg_dump_schema.test_agg(smallint) | {=X/postgres,postgres=X/postgres}
+ function regress_pg_dump_schema.test_func() | {=X/postgres,postgres=X/postgres}
+ function wgo_then_no_access() | {=X/postgres,postgres=X/postgres,pg_signal_backend=X*/postgres}
+ sequence regress_pg_dump_schema.test_seq | {postgres=rwU/postgres}
+ sequence regress_pg_dump_seq | {postgres=rwU/postgres}
+ sequence regress_seq_dumpable | {postgres=rwU/postgres,=r/postgres}
+ sequence wgo_then_regular | {postgres=rwU/postgres,pg_signal_backend=rw*U*/postgres}
+ table regress_pg_dump_schema.test_table | {postgres=arwdDxtm/postgres}
+ table regress_pg_dump_table | {postgres=arwdDxtm/postgres}
+ table regress_table_dumpable | {postgres=arwdDxtm/postgres,=r/postgres}
+ type regress_pg_dump_schema.test_type | {=U/postgres,postgres=U/postgres}
+(12 rows)
+
+SELECT pg_describe_object(classid,objid,objsubid) AS obj,
+ pg_describe_object(refclassid,refobjid,0) AS refobj,
+ deptype
+ FROM pg_shdepend JOIN pg_database d ON dbid = d.oid
+ WHERE d.datname = current_database()
+ ORDER BY 1, 3;
+ obj | refobj | deptype
+-----+--------+---------
+(0 rows)
+
DROP ROLE regress_dump_test_role;
diff --git a/src/test/modules/test_pg_dump/sql/test_pg_dump.sql b/src/test/modules/test_pg_dump/sql/test_pg_dump.sql
index 4f1eb9d429..0d48256883 100644
--- a/src/test/modules/test_pg_dump/sql/test_pg_dump.sql
+++ b/src/test/modules/test_pg_dump/sql/test_pg_dump.sql
@@ -75,6 +75,16 @@ GRANT UPDATE ON test_pg_dump_mv1 TO regress_dump_test_role;
GRANT USAGE ON SCHEMA test_pg_dump_s1 TO regress_dump_test_role;
GRANT USAGE ON TYPE test_pg_dump_e1 TO regress_dump_test_role;
+SELECT pg_describe_object(classoid,objoid,objsubid) AS obj,
+ replace(initprivs::text, current_user, 'postgres') AS initprivs
+ FROM pg_init_privs WHERE privtype = 'e' ORDER BY 1;
+SELECT pg_describe_object(classid,objid,objsubid) AS obj,
+ pg_describe_object(refclassid,refobjid,0) AS refobj,
+ deptype
+ FROM pg_shdepend JOIN pg_database d ON dbid = d.oid
+ WHERE d.datname = current_database()
+ ORDER BY 1, 3;
+
ALTER EXTENSION test_pg_dump ADD ACCESS METHOD gist2;
ALTER EXTENSION test_pg_dump ADD AGGREGATE newavg(int4);
ALTER EXTENSION test_pg_dump ADD CAST (text AS casttesttype);
@@ -108,4 +118,15 @@ ALTER EXTENSION test_pg_dump DROP TYPE test_pg_dump_e1;
ALTER EXTENSION test_pg_dump DROP VIEW test_pg_dump_v1;
DROP OWNED BY regress_dump_test_role RESTRICT;
+
+SELECT pg_describe_object(classoid,objoid,objsubid) AS obj,
+ replace(initprivs::text, current_user, 'postgres') AS initprivs
+ FROM pg_init_privs WHERE privtype = 'e' ORDER BY 1;
+SELECT pg_describe_object(classid,objid,objsubid) AS obj,
+ pg_describe_object(refclassid,refobjid,0) AS refobj,
+ deptype
+ FROM pg_shdepend JOIN pg_database d ON dbid = d.oid
+ WHERE d.datname = current_database()
+ ORDER BY 1, 3;
+
DROP ROLE regress_dump_test_role;
> On 28 Apr 2024, at 20:52, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > I wrote: >> Here's a draft patch that attacks that. It seems to fix the >> problem with test_pg_dump: no dangling pg_init_privs grants >> are left behind. Reading this I can't find any sharp edges, and I prefer your changes to recordExtensionInitPriv over the version I had half-baked by the time you had this finished. Trying to break it with the testcases I had devised also failed, so +1. > Here's a v2 that attempts to add some queries to test_pg_dump.sql > to provide visual verification that pg_shdepend and pg_init_privs > are updated correctly during DROP OWNED BY. It's a little bit > nasty to look at the ACL column of pg_init_privs, because that text > involves the bootstrap superuser's name which is site-dependent. > What I did to try to make the test stable is > > replace(initprivs::text, current_user, 'postgres') AS initprivs Maybe that part warrants a small comment in the testfile to keep it from sending future readers into rabbitholes? > This is of course not bulletproof: with a sufficiently weird > bootstrap superuser name, we could get false matches to parts > of "regress_dump_test_role" or to privilege strings. That > seems unlikely enough to live with, but I wonder if anybody has > a better idea. I think that will be bulletproof enough to keep it working in the buildfarm and among 99% of hackers. -- Daniel Gustafsson
Daniel Gustafsson <daniel@yesql.se> writes:
>> On 28 Apr 2024, at 20:52, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> ... It's a little bit
>> nasty to look at the ACL column of pg_init_privs, because that text
>> involves the bootstrap superuser's name which is site-dependent.
>> What I did to try to make the test stable is
>> replace(initprivs::text, current_user, 'postgres') AS initprivs
> Maybe that part warrants a small comment in the testfile to keep it from
> sending future readers into rabbitholes?
Agreed.
>> This is of course not bulletproof: with a sufficiently weird
>> bootstrap superuser name, we could get false matches to parts
>> of "regress_dump_test_role" or to privilege strings. That
>> seems unlikely enough to live with, but I wonder if anybody has
>> a better idea.
> I think that will be bulletproof enough to keep it working in the buildfarm and
> among 99% of hackers.
It occurred to me to use "aclexplode" to expand the initprivs, and
then we can substitute names with simple equality tests. The test
query is a bit more complicated, but I feel better about it.
v3 attached also has a bit more work on code comments.
regards, tom lane
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 2907079e2a..c8cb46c5b9 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -7184,6 +7184,20 @@ SCRAM-SHA-256$<replaceable><iteration count></replaceable>:<replaceable>&l
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><symbol>SHARED_DEPENDENCY_INITACL</symbol> (<literal>i</literal>)</term>
+ <listitem>
+ <para>
+ The referenced object (which must be a role) is mentioned in a
+ <link linkend="catalog-pg-init-privs"><structname>pg_init_privs</structname></link>
+ entry for the dependent object.
+ (A <symbol>SHARED_DEPENDENCY_INITACL</symbol> entry is not made for
+ the owner of the object, since the owner will have
+ a <symbol>SHARED_DEPENDENCY_OWNER</symbol> entry anyway.)
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><symbol>SHARED_DEPENDENCY_POLICY</symbol> (<literal>r</literal>)</term>
<listitem>
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 7abf3c2a74..e6cc720579 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -165,9 +165,9 @@ static AclMode pg_type_aclmask_ext(Oid type_oid, Oid roleid,
AclMode mask, AclMaskHow how,
bool *is_missing);
static void recordExtensionInitPriv(Oid objoid, Oid classoid, int objsubid,
- Acl *new_acl);
+ Oid ownerId, Acl *new_acl);
static void recordExtensionInitPrivWorker(Oid objoid, Oid classoid, int objsubid,
- Acl *new_acl);
+ Oid ownerId, Acl *new_acl);
/*
@@ -1447,7 +1447,19 @@ SetDefaultACL(InternalDefaultACL *iacls)
/*
* RemoveRoleFromObjectACL
*
- * Used by shdepDropOwned to remove mentions of a role in ACLs
+ * Used by shdepDropOwned to remove mentions of a role in ACLs.
+ *
+ * Notice that this doesn't accept an objsubid parameter, which is a bit bogus
+ * since the pg_shdepend record that caused us to call it certainly had one.
+ * If, for example, pg_shdepend records the existence of a permission on
+ * mytable.mycol, this function will effectively issue a REVOKE ALL ON TABLE
+ * mytable. That gets the job done because (per SQL spec) such a REVOKE also
+ * revokes per-column permissions. We could not recreate a situation where
+ * the role has table-level but not column-level permissions; but it's okay
+ * (for now anyway) because this is only used when we're dropping the role
+ * and so all its permissions everywhere must go away. At worst it's a bit
+ * inefficient if the role has column permissions on several columns of the
+ * same table.
*/
void
RemoveRoleFromObjectACL(Oid roleid, Oid classid, Oid objid)
@@ -1790,7 +1802,7 @@ ExecGrant_Attribute(InternalGrant *istmt, Oid relOid, const char *relname,
CatalogTupleUpdate(attRelation, &newtuple->t_self, newtuple);
/* Update initial privileges for extensions */
- recordExtensionInitPriv(relOid, RelationRelationId, attnum,
+ recordExtensionInitPriv(relOid, RelationRelationId, attnum, ownerId,
ACL_NUM(new_acl) > 0 ? new_acl : NULL);
/* Update the shared dependency ACL info */
@@ -2050,7 +2062,8 @@ ExecGrant_Relation(InternalGrant *istmt)
CatalogTupleUpdate(relation, &newtuple->t_self, newtuple);
/* Update initial privileges for extensions */
- recordExtensionInitPriv(relOid, RelationRelationId, 0, new_acl);
+ recordExtensionInitPriv(relOid, RelationRelationId, 0,
+ ownerId, new_acl);
/* Update the shared dependency ACL info */
updateAclDependencies(RelationRelationId, relOid, 0,
@@ -2251,7 +2264,7 @@ ExecGrant_common(InternalGrant *istmt, Oid classid, AclMode default_privs,
CatalogTupleUpdate(relation, &newtuple->t_self, newtuple);
/* Update initial privileges for extensions */
- recordExtensionInitPriv(objectid, classid, 0, new_acl);
+ recordExtensionInitPriv(objectid, classid, 0, ownerId, new_acl);
/* Update the shared dependency ACL info */
updateAclDependencies(classid,
@@ -2403,7 +2416,8 @@ ExecGrant_Largeobject(InternalGrant *istmt)
CatalogTupleUpdate(relation, &newtuple->t_self, newtuple);
/* Update initial privileges for extensions */
- recordExtensionInitPriv(loid, LargeObjectRelationId, 0, new_acl);
+ recordExtensionInitPriv(loid, LargeObjectRelationId, 0,
+ ownerId, new_acl);
/* Update the shared dependency ACL info */
updateAclDependencies(LargeObjectRelationId,
@@ -2575,7 +2589,7 @@ ExecGrant_Parameter(InternalGrant *istmt)
/* Update initial privileges for extensions */
recordExtensionInitPriv(parameterId, ParameterAclRelationId, 0,
- new_acl);
+ ownerId, new_acl);
/* Update the shared dependency ACL info */
updateAclDependencies(ParameterAclRelationId, parameterId, 0,
@@ -4463,6 +4477,7 @@ recordExtObjInitPriv(Oid objoid, Oid classoid)
}
recordExtensionInitPrivWorker(objoid, classoid, curr_att,
+ pg_class_tuple->relowner,
DatumGetAclP(attaclDatum));
ReleaseSysCache(attTuple);
@@ -4475,6 +4490,7 @@ recordExtObjInitPriv(Oid objoid, Oid classoid)
/* Add the record, if any, for the top-level object */
if (!isNull)
recordExtensionInitPrivWorker(objoid, classoid, 0,
+ pg_class_tuple->relowner,
DatumGetAclP(aclDatum));
ReleaseSysCache(tuple);
@@ -4485,6 +4501,7 @@ recordExtObjInitPriv(Oid objoid, Oid classoid)
Datum aclDatum;
bool isNull;
HeapTuple tuple;
+ Form_pg_largeobject_metadata form_lo_meta;
ScanKeyData entry[1];
SysScanDesc scan;
Relation relation;
@@ -4509,6 +4526,7 @@ recordExtObjInitPriv(Oid objoid, Oid classoid)
tuple = systable_getnext(scan);
if (!HeapTupleIsValid(tuple))
elog(ERROR, "could not find tuple for large object %u", objoid);
+ form_lo_meta = (Form_pg_largeobject_metadata) GETSTRUCT(tuple);
aclDatum = heap_getattr(tuple,
Anum_pg_largeobject_metadata_lomacl,
@@ -4517,6 +4535,7 @@ recordExtObjInitPriv(Oid objoid, Oid classoid)
/* Add the record, if any, for the top-level object */
if (!isNull)
recordExtensionInitPrivWorker(objoid, classoid, 0,
+ form_lo_meta->lomowner,
DatumGetAclP(aclDatum));
systable_endscan(scan);
@@ -4524,24 +4543,29 @@ recordExtObjInitPriv(Oid objoid, Oid classoid)
/* This will error on unsupported classoid. */
else if (get_object_attnum_acl(classoid) != InvalidAttrNumber)
{
+ int cacheid;
+ Oid ownerId;
Datum aclDatum;
bool isNull;
HeapTuple tuple;
- tuple = SearchSysCache1(get_object_catcache_oid(classoid),
- ObjectIdGetDatum(objoid));
+ cacheid = get_object_catcache_oid(classoid);
+ tuple = SearchSysCache1(cacheid, ObjectIdGetDatum(objoid));
if (!HeapTupleIsValid(tuple))
elog(ERROR, "cache lookup failed for %s %u",
get_object_class_descr(classoid), objoid);
- aclDatum = SysCacheGetAttr(get_object_catcache_oid(classoid), tuple,
+ ownerId = DatumGetObjectId(SysCacheGetAttrNotNull(cacheid,
+ tuple,
+ get_object_attnum_owner(classoid)));
+ aclDatum = SysCacheGetAttr(cacheid, tuple,
get_object_attnum_acl(classoid),
&isNull);
/* Add the record, if any, for the top-level object */
if (!isNull)
recordExtensionInitPrivWorker(objoid, classoid, 0,
- DatumGetAclP(aclDatum));
+ ownerId, DatumGetAclP(aclDatum));
ReleaseSysCache(tuple);
}
@@ -4554,6 +4578,8 @@ recordExtObjInitPriv(Oid objoid, Oid classoid)
void
removeExtObjInitPriv(Oid objoid, Oid classoid)
{
+ Oid ownerId;
+
/*
* If this is a relation then we need to see if there are any sub-objects
* (eg: columns) for it and, if so, be sure to call
@@ -4568,6 +4594,7 @@ removeExtObjInitPriv(Oid objoid, Oid classoid)
if (!HeapTupleIsValid(tuple))
elog(ERROR, "cache lookup failed for relation %u", objoid);
pg_class_tuple = (Form_pg_class) GETSTRUCT(tuple);
+ ownerId = pg_class_tuple->relowner;
/*
* Indexes don't have permissions, neither do the pg_class rows for
@@ -4604,7 +4631,8 @@ removeExtObjInitPriv(Oid objoid, Oid classoid)
/* when removing, remove all entries, even dropped columns */
- recordExtensionInitPrivWorker(objoid, classoid, curr_att, NULL);
+ recordExtensionInitPrivWorker(objoid, classoid, curr_att,
+ ownerId, NULL);
ReleaseSysCache(attTuple);
}
@@ -4612,9 +4640,35 @@ removeExtObjInitPriv(Oid objoid, Oid classoid)
ReleaseSysCache(tuple);
}
+ else
+ {
+ /* Must find out the owner's OID the hard way */
+ AttrNumber ownerattnum;
+ int cacheid;
+ HeapTuple tuple;
+
+ /*
+ * If the object is of a kind that has no owner, it should not have
+ * any pg_init_privs entry either.
+ */
+ ownerattnum = get_object_attnum_owner(classoid);
+ if (ownerattnum == InvalidAttrNumber)
+ return;
+ cacheid = get_object_catcache_oid(classoid);
+ tuple = SearchSysCache1(cacheid, ObjectIdGetDatum(objoid));
+ if (!HeapTupleIsValid(tuple))
+ elog(ERROR, "cache lookup failed for %s %u",
+ get_object_class_descr(classoid), objoid);
+
+ ownerId = DatumGetObjectId(SysCacheGetAttrNotNull(cacheid,
+ tuple,
+ ownerattnum));
+
+ ReleaseSysCache(tuple);
+ }
/* Remove the record, if any, for the top-level object */
- recordExtensionInitPrivWorker(objoid, classoid, 0, NULL);
+ recordExtensionInitPrivWorker(objoid, classoid, 0, ownerId, NULL);
}
/*
@@ -4626,7 +4680,8 @@ removeExtObjInitPriv(Oid objoid, Oid classoid)
* Pass in the object OID, the OID of the class (the OID of the table which
* the object is defined in) and the 'sub' id of the object (objsubid), if
* any. If there is no 'sub' id (they are currently only used for columns of
- * tables) then pass in '0'. Finally, pass in the complete ACL to store.
+ * tables) then pass in '0'. Also pass the OID of the object's owner.
+ * Finally, pass in the complete ACL to store.
*
* If an ACL already exists for this object/sub-object then we will replace
* it with what is passed in.
@@ -4635,7 +4690,8 @@ removeExtObjInitPriv(Oid objoid, Oid classoid)
* removed, if one is found.
*/
static void
-recordExtensionInitPriv(Oid objoid, Oid classoid, int objsubid, Acl *new_acl)
+recordExtensionInitPriv(Oid objoid, Oid classoid, int objsubid,
+ Oid ownerId, Acl *new_acl)
{
/*
* Generally, we only record the initial privileges when an extension is
@@ -4648,7 +4704,7 @@ recordExtensionInitPriv(Oid objoid, Oid classoid, int objsubid, Acl *new_acl)
if (!creating_extension && !binary_upgrade_record_init_privs)
return;
- recordExtensionInitPrivWorker(objoid, classoid, objsubid, new_acl);
+ recordExtensionInitPrivWorker(objoid, classoid, objsubid, ownerId, new_acl);
}
/*
@@ -4664,14 +4720,23 @@ recordExtensionInitPriv(Oid objoid, Oid classoid, int objsubid, Acl *new_acl)
* EXTENSION ... ADD/DROP.
*/
static void
-recordExtensionInitPrivWorker(Oid objoid, Oid classoid, int objsubid, Acl *new_acl)
+recordExtensionInitPrivWorker(Oid objoid, Oid classoid, int objsubid,
+ Oid ownerId, Acl *new_acl)
{
Relation relation;
ScanKeyData key[3];
SysScanDesc scan;
HeapTuple tuple;
HeapTuple oldtuple;
+ int noldmembers;
+ int nnewmembers;
+ Oid *oldmembers;
+ Oid *newmembers;
+
+ /* We'll need the role membership of the new ACL. */
+ nnewmembers = aclmembers(new_acl, &newmembers);
+ /* Search pg_init_privs for an existing entry. */
relation = table_open(InitPrivsRelationId, RowExclusiveLock);
ScanKeyInit(&key[0],
@@ -4699,6 +4764,23 @@ recordExtensionInitPrivWorker(Oid objoid, Oid classoid, int objsubid, Acl *new_a
Datum values[Natts_pg_init_privs] = {0};
bool nulls[Natts_pg_init_privs] = {0};
bool replace[Natts_pg_init_privs] = {0};
+ Datum oldAclDatum;
+ bool isNull;
+ Acl *old_acl;
+
+ /* Update pg_shdepend for roles mentioned in the old/new ACLs. */
+ oldAclDatum = heap_getattr(oldtuple, Anum_pg_init_privs_initprivs,
+ RelationGetDescr(relation), &isNull);
+ if (!isNull)
+ old_acl = DatumGetAclP(oldAclDatum);
+ else
+ old_acl = NULL; /* this case shouldn't happen, probably */
+ noldmembers = aclmembers(old_acl, &oldmembers);
+
+ updateInitAclDependencies(classoid, objoid, objsubid,
+ ownerId,
+ noldmembers, oldmembers,
+ nnewmembers, newmembers);
/* If we have a new ACL to set, then update the row with it. */
if (new_acl)
@@ -4744,6 +4826,15 @@ recordExtensionInitPrivWorker(Oid objoid, Oid classoid, int objsubid, Acl *new_a
tuple = heap_form_tuple(RelationGetDescr(relation), values, nulls);
CatalogTupleInsert(relation, tuple);
+
+ /* Update pg_shdepend, too. */
+ noldmembers = 0;
+ oldmembers = NULL;
+
+ updateInitAclDependencies(classoid, objoid, objsubid,
+ ownerId,
+ noldmembers, oldmembers,
+ nnewmembers, newmembers);
}
}
@@ -4754,3 +4845,138 @@ recordExtensionInitPrivWorker(Oid objoid, Oid classoid, int objsubid, Acl *new_a
table_close(relation, RowExclusiveLock);
}
+
+/*
+ * RemoveRoleFromInitPriv
+ *
+ * Used by shdepDropOwned to remove mentions of a role in pg_init_privs.
+ */
+void
+RemoveRoleFromInitPriv(Oid roleid, Oid classid, Oid objid, int32 objsubid)
+{
+ Relation rel;
+ ScanKeyData key[3];
+ SysScanDesc scan;
+ HeapTuple oldtuple;
+ int cacheid;
+ HeapTuple objtuple;
+ Oid ownerId;
+ Datum oldAclDatum;
+ bool isNull;
+ Acl *old_acl;
+ Acl *new_acl;
+ HeapTuple newtuple;
+ int noldmembers;
+ int nnewmembers;
+ Oid *oldmembers;
+ Oid *newmembers;
+
+ /* Search for existing pg_init_privs entry for the target object. */
+ rel = table_open(InitPrivsRelationId, RowExclusiveLock);
+
+ ScanKeyInit(&key[0],
+ Anum_pg_init_privs_objoid,
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(objid));
+ ScanKeyInit(&key[1],
+ Anum_pg_init_privs_classoid,
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(classid));
+ ScanKeyInit(&key[2],
+ Anum_pg_init_privs_objsubid,
+ BTEqualStrategyNumber, F_INT4EQ,
+ Int32GetDatum(objsubid));
+
+ scan = systable_beginscan(rel, InitPrivsObjIndexId, true,
+ NULL, 3, key);
+
+ /* There should exist only one entry or none. */
+ oldtuple = systable_getnext(scan);
+
+ if (!HeapTupleIsValid(oldtuple))
+ {
+ /*
+ * Hmm, why are we here if there's no entry? But pack up and go away
+ * quietly.
+ */
+ systable_endscan(scan);
+ table_close(rel, RowExclusiveLock);
+ return;
+ }
+
+ /* Get a writable copy of the existing ACL. */
+ oldAclDatum = heap_getattr(oldtuple, Anum_pg_init_privs_initprivs,
+ RelationGetDescr(rel), &isNull);
+ if (!isNull)
+ old_acl = DatumGetAclPCopy(oldAclDatum);
+ else
+ old_acl = NULL; /* this case shouldn't happen, probably */
+
+ /*
+ * We need the members of both old and new ACLs so we can correct the
+ * shared dependency information. Collect data before
+ * merge_acl_with_grant throws away old_acl.
+ */
+ noldmembers = aclmembers(old_acl, &oldmembers);
+
+ /* Must find out the owner's OID the hard way. */
+ cacheid = get_object_catcache_oid(classid);
+ objtuple = SearchSysCache1(cacheid, ObjectIdGetDatum(objid));
+ if (!HeapTupleIsValid(objtuple))
+ elog(ERROR, "cache lookup failed for %s %u",
+ get_object_class_descr(classid), objid);
+
+ ownerId = DatumGetObjectId(SysCacheGetAttrNotNull(cacheid,
+ objtuple,
+ get_object_attnum_owner(classid)));
+ ReleaseSysCache(objtuple);
+
+ /*
+ * Generate new ACL. Grantor of rights is always the same as the owner.
+ */
+ new_acl = merge_acl_with_grant(old_acl,
+ false, /* is_grant */
+ false, /* grant_option */
+ DROP_RESTRICT,
+ list_make1_oid(roleid),
+ ACLITEM_ALL_PRIV_BITS,
+ ownerId,
+ ownerId);
+
+ /* If we end with an empty ACL, delete the pg_init_privs entry. */
+ if (new_acl == NULL || ACL_NUM(new_acl) == 0)
+ {
+ CatalogTupleDelete(rel, &oldtuple->t_self);
+ }
+ else
+ {
+ Datum values[Natts_pg_init_privs] = {0};
+ bool nulls[Natts_pg_init_privs] = {0};
+ bool replaces[Natts_pg_init_privs] = {0};
+
+ /* Update existing entry. */
+ values[Anum_pg_init_privs_initprivs - 1] = PointerGetDatum(new_acl);
+ replaces[Anum_pg_init_privs_initprivs - 1] = true;
+
+ newtuple = heap_modify_tuple(oldtuple, RelationGetDescr(rel),
+ values, nulls, replaces);
+ CatalogTupleUpdate(rel, &newtuple->t_self, newtuple);
+ }
+
+ /*
+ * Update the shared dependency ACL info.
+ */
+ nnewmembers = aclmembers(new_acl, &newmembers);
+
+ updateInitAclDependencies(classid, objid, objsubid,
+ ownerId,
+ noldmembers, oldmembers,
+ nnewmembers, newmembers);
+
+ systable_endscan(scan);
+
+ /* prevent error when processing objects multiple times */
+ CommandCounterIncrement();
+
+ table_close(rel, RowExclusiveLock);
+}
diff --git a/src/backend/catalog/pg_shdepend.c b/src/backend/catalog/pg_shdepend.c
index cb31590339..20bcfd779b 100644
--- a/src/backend/catalog/pg_shdepend.c
+++ b/src/backend/catalog/pg_shdepend.c
@@ -84,6 +84,11 @@ static void shdepChangeDep(Relation sdepRel,
Oid classid, Oid objid, int32 objsubid,
Oid refclassid, Oid refobjid,
SharedDependencyType deptype);
+static void updateAclDependenciesWorker(Oid classId, Oid objectId,
+ int32 objsubId, Oid ownerId,
+ SharedDependencyType deptype,
+ int noldmembers, Oid *oldmembers,
+ int nnewmembers, Oid *newmembers);
static void shdepAddDependency(Relation sdepRel,
Oid classId, Oid objectId, int32 objsubId,
Oid refclassId, Oid refobjId,
@@ -340,6 +345,11 @@ changeDependencyOnOwner(Oid classId, Oid objectId, Oid newOwnerId)
AuthIdRelationId, newOwnerId,
SHARED_DEPENDENCY_ACL);
+ /* The same applies to SHARED_DEPENDENCY_INITACL */
+ shdepDropDependency(sdepRel, classId, objectId, 0, true,
+ AuthIdRelationId, newOwnerId,
+ SHARED_DEPENDENCY_INITACL);
+
table_close(sdepRel, RowExclusiveLock);
}
@@ -478,6 +488,38 @@ updateAclDependencies(Oid classId, Oid objectId, int32 objsubId,
Oid ownerId,
int noldmembers, Oid *oldmembers,
int nnewmembers, Oid *newmembers)
+{
+ updateAclDependenciesWorker(classId, objectId, objsubId,
+ ownerId, SHARED_DEPENDENCY_ACL,
+ noldmembers, oldmembers,
+ nnewmembers, newmembers);
+}
+
+/*
+ * updateInitAclDependencies
+ * Update the pg_shdepend info for a pg_init_privs entry.
+ *
+ * Exactly like updateAclDependencies, except we are considering a
+ * pg_init_privs ACL for the specified object.
+ */
+void
+updateInitAclDependencies(Oid classId, Oid objectId, int32 objsubId,
+ Oid ownerId,
+ int noldmembers, Oid *oldmembers,
+ int nnewmembers, Oid *newmembers)
+{
+ updateAclDependenciesWorker(classId, objectId, objsubId,
+ ownerId, SHARED_DEPENDENCY_INITACL,
+ noldmembers, oldmembers,
+ nnewmembers, newmembers);
+}
+
+/* Common code for the above two functions */
+static void
+updateAclDependenciesWorker(Oid classId, Oid objectId, int32 objsubId,
+ Oid ownerId, SharedDependencyType deptype,
+ int noldmembers, Oid *oldmembers,
+ int nnewmembers, Oid *newmembers)
{
Relation sdepRel;
int i;
@@ -513,7 +555,7 @@ updateAclDependencies(Oid classId, Oid objectId, int32 objsubId,
shdepAddDependency(sdepRel, classId, objectId, objsubId,
AuthIdRelationId, roleid,
- SHARED_DEPENDENCY_ACL);
+ deptype);
}
/* Drop no-longer-used old dependencies */
@@ -532,7 +574,7 @@ updateAclDependencies(Oid classId, Oid objectId, int32 objsubId,
shdepDropDependency(sdepRel, classId, objectId, objsubId,
false, /* exact match on objsubId */
AuthIdRelationId, roleid,
- SHARED_DEPENDENCY_ACL);
+ deptype);
}
table_close(sdepRel, RowExclusiveLock);
@@ -1249,6 +1291,8 @@ storeObjectDescription(StringInfo descs,
appendStringInfo(descs, _("owner of %s"), objdesc);
else if (deptype == SHARED_DEPENDENCY_ACL)
appendStringInfo(descs, _("privileges for %s"), objdesc);
+ else if (deptype == SHARED_DEPENDENCY_INITACL)
+ appendStringInfo(descs, _("initial privileges for %s"), objdesc);
else if (deptype == SHARED_DEPENDENCY_POLICY)
appendStringInfo(descs, _("target of %s"), objdesc);
else if (deptype == SHARED_DEPENDENCY_TABLESPACE)
@@ -1431,6 +1475,14 @@ shdepDropOwned(List *roleids, DropBehavior behavior)
add_exact_object_address(&obj, deleteobjs);
}
break;
+ case SHARED_DEPENDENCY_INITACL:
+ /* Shouldn't see a role grant here */
+ Assert(sdepForm->classid != AuthMemRelationId);
+ RemoveRoleFromInitPriv(roleid,
+ sdepForm->classid,
+ sdepForm->objid,
+ sdepForm->objsubid);
+ break;
}
}
diff --git a/src/include/catalog/dependency.h b/src/include/catalog/dependency.h
index ec654010d4..e0dcc0b069 100644
--- a/src/include/catalog/dependency.h
+++ b/src/include/catalog/dependency.h
@@ -56,11 +56,17 @@ typedef enum DependencyType
* created for the owner of an object; hence two objects may be linked by
* one or the other, but not both, of these dependency types.)
*
- * (c) a SHARED_DEPENDENCY_POLICY entry means that the referenced object is
+ * (c) a SHARED_DEPENDENCY_INITACL entry means that the referenced object is
+ * a role mentioned in a pg_init_privs entry for the dependent object. The
+ * referenced object must be a pg_authid entry. (SHARED_DEPENDENCY_INITACL
+ * entries are not created for the owner of an object; hence two objects may
+ * be linked by one or the other, but not both, of these dependency types.)
+ *
+ * (d) a SHARED_DEPENDENCY_POLICY entry means that the referenced object is
* a role mentioned in a policy object. The referenced object must be a
* pg_authid entry.
*
- * (d) a SHARED_DEPENDENCY_TABLESPACE entry means that the referenced
+ * (e) a SHARED_DEPENDENCY_TABLESPACE entry means that the referenced
* object is a tablespace mentioned in a relation without storage. The
* referenced object must be a pg_tablespace entry. (Relations that have
* storage don't need this: they are protected by the existence of a physical
@@ -73,6 +79,7 @@ typedef enum SharedDependencyType
{
SHARED_DEPENDENCY_OWNER = 'o',
SHARED_DEPENDENCY_ACL = 'a',
+ SHARED_DEPENDENCY_INITACL = 'i',
SHARED_DEPENDENCY_POLICY = 'r',
SHARED_DEPENDENCY_TABLESPACE = 't',
SHARED_DEPENDENCY_INVALID = 0,
@@ -201,6 +208,11 @@ extern void updateAclDependencies(Oid classId, Oid objectId, int32 objsubId,
int noldmembers, Oid *oldmembers,
int nnewmembers, Oid *newmembers);
+extern void updateInitAclDependencies(Oid classId, Oid objectId, int32 objsubId,
+ Oid ownerId,
+ int noldmembers, Oid *oldmembers,
+ int nnewmembers, Oid *newmembers);
+
extern bool checkSharedDependencies(Oid classId, Oid objectId,
char **detail_msg, char **detail_log_msg);
diff --git a/src/include/utils/acl.h b/src/include/utils/acl.h
index 3a0baf3039..1a554c6699 100644
--- a/src/include/utils/acl.h
+++ b/src/include/utils/acl.h
@@ -276,6 +276,8 @@ extern void aclcheck_error_type(AclResult aclerr, Oid typeOid);
extern void recordExtObjInitPriv(Oid objoid, Oid classoid);
extern void removeExtObjInitPriv(Oid objoid, Oid classoid);
+extern void RemoveRoleFromInitPriv(Oid roleid,
+ Oid classid, Oid objid, int32 objsubid);
/* ownercheck routines just return true (owner) or false (not) */
diff --git a/src/test/modules/test_pg_dump/expected/test_pg_dump.out
b/src/test/modules/test_pg_dump/expected/test_pg_dump.out
index f57c96aeb9..fcfa78aafc 100644
--- a/src/test/modules/test_pg_dump/expected/test_pg_dump.out
+++ b/src/test/modules/test_pg_dump/expected/test_pg_dump.out
@@ -62,6 +62,113 @@ GRANT SELECT ON ft1 TO regress_dump_test_role;
GRANT UPDATE ON test_pg_dump_mv1 TO regress_dump_test_role;
GRANT USAGE ON SCHEMA test_pg_dump_s1 TO regress_dump_test_role;
GRANT USAGE ON TYPE test_pg_dump_e1 TO regress_dump_test_role;
+-- Substitute for current user's name to keep test output consistent
+SELECT s.obj,
+ CASE WHEN a.grantor::regrole::name = current_user THEN 'postgres'
+ ELSE a.grantor::regrole END,
+ CASE WHEN a.grantee::regrole::name = current_user THEN 'postgres'
+ ELSE a.grantee::regrole END,
+ a.privilege_type, a.is_grantable
+FROM
+ (SELECT pg_describe_object(classoid,objoid,objsubid) AS obj, initprivs
+ FROM pg_init_privs WHERE privtype = 'e' ORDER BY 1) s,
+ aclexplode(s.initprivs) a;
+ obj | grantor | grantee | privilege_type |
is_grantable
+----------------------------------------------------+----------+------------------------+----------------+--------------
+ column col1 of table regress_pg_dump_table | postgres | - | SELECT | f
+ function regress_pg_dump_schema.test_agg(smallint) | postgres | - | EXECUTE | f
+ function regress_pg_dump_schema.test_agg(smallint) | postgres | postgres | EXECUTE | f
+ function regress_pg_dump_schema.test_agg(smallint) | postgres | regress_dump_test_role | EXECUTE | f
+ function regress_pg_dump_schema.test_func() | postgres | - | EXECUTE | f
+ function regress_pg_dump_schema.test_func() | postgres | postgres | EXECUTE | f
+ function regress_pg_dump_schema.test_func() | postgres | regress_dump_test_role | EXECUTE | f
+ function wgo_then_no_access() | postgres | - | EXECUTE | f
+ function wgo_then_no_access() | postgres | postgres | EXECUTE | f
+ function wgo_then_no_access() | postgres | pg_signal_backend | EXECUTE | t
+ sequence regress_pg_dump_schema.test_seq | postgres | postgres | SELECT | f
+ sequence regress_pg_dump_schema.test_seq | postgres | postgres | UPDATE | f
+ sequence regress_pg_dump_schema.test_seq | postgres | postgres | USAGE | f
+ sequence regress_pg_dump_schema.test_seq | postgres | regress_dump_test_role | USAGE | f
+ sequence regress_pg_dump_seq | postgres | postgres | SELECT | f
+ sequence regress_pg_dump_seq | postgres | postgres | UPDATE | f
+ sequence regress_pg_dump_seq | postgres | postgres | USAGE | f
+ sequence regress_pg_dump_seq | postgres | regress_dump_test_role | USAGE | f
+ sequence regress_seq_dumpable | postgres | postgres | SELECT | f
+ sequence regress_seq_dumpable | postgres | postgres | UPDATE | f
+ sequence regress_seq_dumpable | postgres | postgres | USAGE | f
+ sequence regress_seq_dumpable | postgres | - | SELECT | f
+ sequence wgo_then_regular | postgres | postgres | SELECT | f
+ sequence wgo_then_regular | postgres | postgres | UPDATE | f
+ sequence wgo_then_regular | postgres | postgres | USAGE | f
+ sequence wgo_then_regular | postgres | pg_signal_backend | SELECT | f
+ sequence wgo_then_regular | postgres | pg_signal_backend | UPDATE | t
+ sequence wgo_then_regular | postgres | pg_signal_backend | USAGE | t
+ table regress_pg_dump_schema.test_table | postgres | postgres | INSERT | f
+ table regress_pg_dump_schema.test_table | postgres | postgres | SELECT | f
+ table regress_pg_dump_schema.test_table | postgres | postgres | UPDATE | f
+ table regress_pg_dump_schema.test_table | postgres | postgres | DELETE | f
+ table regress_pg_dump_schema.test_table | postgres | postgres | TRUNCATE | f
+ table regress_pg_dump_schema.test_table | postgres | postgres | REFERENCES | f
+ table regress_pg_dump_schema.test_table | postgres | postgres | TRIGGER | f
+ table regress_pg_dump_schema.test_table | postgres | postgres | MAINTAIN | f
+ table regress_pg_dump_schema.test_table | postgres | regress_dump_test_role | SELECT | f
+ table regress_pg_dump_table | postgres | postgres | INSERT | f
+ table regress_pg_dump_table | postgres | postgres | SELECT | f
+ table regress_pg_dump_table | postgres | postgres | UPDATE | f
+ table regress_pg_dump_table | postgres | postgres | DELETE | f
+ table regress_pg_dump_table | postgres | postgres | TRUNCATE | f
+ table regress_pg_dump_table | postgres | postgres | REFERENCES | f
+ table regress_pg_dump_table | postgres | postgres | TRIGGER | f
+ table regress_pg_dump_table | postgres | postgres | MAINTAIN | f
+ table regress_pg_dump_table | postgres | regress_dump_test_role | SELECT | f
+ table regress_table_dumpable | postgres | postgres | INSERT | f
+ table regress_table_dumpable | postgres | postgres | SELECT | f
+ table regress_table_dumpable | postgres | postgres | UPDATE | f
+ table regress_table_dumpable | postgres | postgres | DELETE | f
+ table regress_table_dumpable | postgres | postgres | TRUNCATE | f
+ table regress_table_dumpable | postgres | postgres | REFERENCES | f
+ table regress_table_dumpable | postgres | postgres | TRIGGER | f
+ table regress_table_dumpable | postgres | postgres | MAINTAIN | f
+ table regress_table_dumpable | postgres | - | SELECT | f
+ type regress_pg_dump_schema.test_type | postgres | - | USAGE | f
+ type regress_pg_dump_schema.test_type | postgres | postgres | USAGE | f
+ type regress_pg_dump_schema.test_type | postgres | regress_dump_test_role | USAGE | f
+(58 rows)
+
+SELECT pg_describe_object(classid,objid,objsubid) AS obj,
+ pg_describe_object(refclassid,refobjid,0) AS refobj,
+ deptype
+ FROM pg_shdepend JOIN pg_database d ON dbid = d.oid
+ WHERE d.datname = current_database()
+ ORDER BY 1, 3;
+ obj | refobj | deptype
+----------------------------------------------------+-----------------------------+---------
+ column c1 of foreign table ft1 | role regress_dump_test_role | a
+ column c1 of table test_pg_dump_t1 | role regress_dump_test_role | a
+ foreign table ft1 | role regress_dump_test_role | a
+ foreign-data wrapper dummy | role regress_dump_test_role | a
+ function regress_pg_dump_schema.test_agg(smallint) | role regress_dump_test_role | a
+ function regress_pg_dump_schema.test_agg(smallint) | role regress_dump_test_role | i
+ function regress_pg_dump_schema.test_func() | role regress_dump_test_role | a
+ function regress_pg_dump_schema.test_func() | role regress_dump_test_role | i
+ function test_pg_dump(integer) | role regress_dump_test_role | a
+ materialized view test_pg_dump_mv1 | role regress_dump_test_role | a
+ schema test_pg_dump_s1 | role regress_dump_test_role | a
+ sequence regress_pg_dump_schema.test_seq | role regress_dump_test_role | a
+ sequence regress_pg_dump_schema.test_seq | role regress_dump_test_role | i
+ sequence regress_pg_dump_seq | role regress_dump_test_role | a
+ sequence regress_pg_dump_seq | role regress_dump_test_role | i
+ server s0 | role regress_dump_test_role | a
+ table regress_pg_dump_schema.test_table | role regress_dump_test_role | a
+ table regress_pg_dump_schema.test_table | role regress_dump_test_role | i
+ table regress_pg_dump_table | role regress_dump_test_role | a
+ table regress_pg_dump_table | role regress_dump_test_role | i
+ type regress_pg_dump_schema.test_type | role regress_dump_test_role | a
+ type regress_pg_dump_schema.test_type | role regress_dump_test_role | i
+ type test_pg_dump_e1 | role regress_dump_test_role | a
+ view test_pg_dump_v1 | role regress_dump_test_role | a
+(24 rows)
+
ALTER EXTENSION test_pg_dump ADD ACCESS METHOD gist2;
ALTER EXTENSION test_pg_dump ADD AGGREGATE newavg(int4);
ALTER EXTENSION test_pg_dump ADD CAST (text AS casttesttype);
@@ -92,4 +199,80 @@ ALTER EXTENSION test_pg_dump DROP TABLE test_pg_dump_t1;
ALTER EXTENSION test_pg_dump DROP TYPE test_pg_dump_e1;
ALTER EXTENSION test_pg_dump DROP VIEW test_pg_dump_v1;
DROP OWNED BY regress_dump_test_role RESTRICT;
+-- Substitute for current user's name to keep test output consistent
+SELECT s.obj,
+ CASE WHEN a.grantor::regrole::name = current_user THEN 'postgres'
+ ELSE a.grantor::regrole END,
+ CASE WHEN a.grantee::regrole::name = current_user THEN 'postgres'
+ ELSE a.grantee::regrole END,
+ a.privilege_type, a.is_grantable
+FROM
+ (SELECT pg_describe_object(classoid,objoid,objsubid) AS obj, initprivs
+ FROM pg_init_privs WHERE privtype = 'e' ORDER BY 1) s,
+ aclexplode(s.initprivs) a;
+ obj | grantor | grantee | privilege_type | is_grantable
+----------------------------------------------------+----------+-------------------+----------------+--------------
+ column col1 of table regress_pg_dump_table | postgres | - | SELECT | f
+ function regress_pg_dump_schema.test_agg(smallint) | postgres | - | EXECUTE | f
+ function regress_pg_dump_schema.test_agg(smallint) | postgres | postgres | EXECUTE | f
+ function regress_pg_dump_schema.test_func() | postgres | - | EXECUTE | f
+ function regress_pg_dump_schema.test_func() | postgres | postgres | EXECUTE | f
+ function wgo_then_no_access() | postgres | - | EXECUTE | f
+ function wgo_then_no_access() | postgres | postgres | EXECUTE | f
+ function wgo_then_no_access() | postgres | pg_signal_backend | EXECUTE | t
+ sequence regress_pg_dump_schema.test_seq | postgres | postgres | SELECT | f
+ sequence regress_pg_dump_schema.test_seq | postgres | postgres | UPDATE | f
+ sequence regress_pg_dump_schema.test_seq | postgres | postgres | USAGE | f
+ sequence regress_pg_dump_seq | postgres | postgres | SELECT | f
+ sequence regress_pg_dump_seq | postgres | postgres | UPDATE | f
+ sequence regress_pg_dump_seq | postgres | postgres | USAGE | f
+ sequence regress_seq_dumpable | postgres | postgres | SELECT | f
+ sequence regress_seq_dumpable | postgres | postgres | UPDATE | f
+ sequence regress_seq_dumpable | postgres | postgres | USAGE | f
+ sequence regress_seq_dumpable | postgres | - | SELECT | f
+ sequence wgo_then_regular | postgres | postgres | SELECT | f
+ sequence wgo_then_regular | postgres | postgres | UPDATE | f
+ sequence wgo_then_regular | postgres | postgres | USAGE | f
+ sequence wgo_then_regular | postgres | pg_signal_backend | SELECT | f
+ sequence wgo_then_regular | postgres | pg_signal_backend | UPDATE | t
+ sequence wgo_then_regular | postgres | pg_signal_backend | USAGE | t
+ table regress_pg_dump_schema.test_table | postgres | postgres | INSERT | f
+ table regress_pg_dump_schema.test_table | postgres | postgres | SELECT | f
+ table regress_pg_dump_schema.test_table | postgres | postgres | UPDATE | f
+ table regress_pg_dump_schema.test_table | postgres | postgres | DELETE | f
+ table regress_pg_dump_schema.test_table | postgres | postgres | TRUNCATE | f
+ table regress_pg_dump_schema.test_table | postgres | postgres | REFERENCES | f
+ table regress_pg_dump_schema.test_table | postgres | postgres | TRIGGER | f
+ table regress_pg_dump_schema.test_table | postgres | postgres | MAINTAIN | f
+ table regress_pg_dump_table | postgres | postgres | INSERT | f
+ table regress_pg_dump_table | postgres | postgres | SELECT | f
+ table regress_pg_dump_table | postgres | postgres | UPDATE | f
+ table regress_pg_dump_table | postgres | postgres | DELETE | f
+ table regress_pg_dump_table | postgres | postgres | TRUNCATE | f
+ table regress_pg_dump_table | postgres | postgres | REFERENCES | f
+ table regress_pg_dump_table | postgres | postgres | TRIGGER | f
+ table regress_pg_dump_table | postgres | postgres | MAINTAIN | f
+ table regress_table_dumpable | postgres | postgres | INSERT | f
+ table regress_table_dumpable | postgres | postgres | SELECT | f
+ table regress_table_dumpable | postgres | postgres | UPDATE | f
+ table regress_table_dumpable | postgres | postgres | DELETE | f
+ table regress_table_dumpable | postgres | postgres | TRUNCATE | f
+ table regress_table_dumpable | postgres | postgres | REFERENCES | f
+ table regress_table_dumpable | postgres | postgres | TRIGGER | f
+ table regress_table_dumpable | postgres | postgres | MAINTAIN | f
+ table regress_table_dumpable | postgres | - | SELECT | f
+ type regress_pg_dump_schema.test_type | postgres | - | USAGE | f
+ type regress_pg_dump_schema.test_type | postgres | postgres | USAGE | f
+(51 rows)
+
+SELECT pg_describe_object(classid,objid,objsubid) AS obj,
+ pg_describe_object(refclassid,refobjid,0) AS refobj,
+ deptype
+ FROM pg_shdepend JOIN pg_database d ON dbid = d.oid
+ WHERE d.datname = current_database()
+ ORDER BY 1, 3;
+ obj | refobj | deptype
+-----+--------+---------
+(0 rows)
+
DROP ROLE regress_dump_test_role;
diff --git a/src/test/modules/test_pg_dump/sql/test_pg_dump.sql b/src/test/modules/test_pg_dump/sql/test_pg_dump.sql
index 4f1eb9d429..41f1d8dfc5 100644
--- a/src/test/modules/test_pg_dump/sql/test_pg_dump.sql
+++ b/src/test/modules/test_pg_dump/sql/test_pg_dump.sql
@@ -75,6 +75,24 @@ GRANT UPDATE ON test_pg_dump_mv1 TO regress_dump_test_role;
GRANT USAGE ON SCHEMA test_pg_dump_s1 TO regress_dump_test_role;
GRANT USAGE ON TYPE test_pg_dump_e1 TO regress_dump_test_role;
+-- Substitute for current user's name to keep test output consistent
+SELECT s.obj,
+ CASE WHEN a.grantor::regrole::name = current_user THEN 'postgres'
+ ELSE a.grantor::regrole END,
+ CASE WHEN a.grantee::regrole::name = current_user THEN 'postgres'
+ ELSE a.grantee::regrole END,
+ a.privilege_type, a.is_grantable
+FROM
+ (SELECT pg_describe_object(classoid,objoid,objsubid) AS obj, initprivs
+ FROM pg_init_privs WHERE privtype = 'e' ORDER BY 1) s,
+ aclexplode(s.initprivs) a;
+SELECT pg_describe_object(classid,objid,objsubid) AS obj,
+ pg_describe_object(refclassid,refobjid,0) AS refobj,
+ deptype
+ FROM pg_shdepend JOIN pg_database d ON dbid = d.oid
+ WHERE d.datname = current_database()
+ ORDER BY 1, 3;
+
ALTER EXTENSION test_pg_dump ADD ACCESS METHOD gist2;
ALTER EXTENSION test_pg_dump ADD AGGREGATE newavg(int4);
ALTER EXTENSION test_pg_dump ADD CAST (text AS casttesttype);
@@ -108,4 +126,23 @@ ALTER EXTENSION test_pg_dump DROP TYPE test_pg_dump_e1;
ALTER EXTENSION test_pg_dump DROP VIEW test_pg_dump_v1;
DROP OWNED BY regress_dump_test_role RESTRICT;
+
+-- Substitute for current user's name to keep test output consistent
+SELECT s.obj,
+ CASE WHEN a.grantor::regrole::name = current_user THEN 'postgres'
+ ELSE a.grantor::regrole END,
+ CASE WHEN a.grantee::regrole::name = current_user THEN 'postgres'
+ ELSE a.grantee::regrole END,
+ a.privilege_type, a.is_grantable
+FROM
+ (SELECT pg_describe_object(classoid,objoid,objsubid) AS obj, initprivs
+ FROM pg_init_privs WHERE privtype = 'e' ORDER BY 1) s,
+ aclexplode(s.initprivs) a;
+SELECT pg_describe_object(classid,objid,objsubid) AS obj,
+ pg_describe_object(refclassid,refobjid,0) AS refobj,
+ deptype
+ FROM pg_shdepend JOIN pg_database d ON dbid = d.oid
+ WHERE d.datname = current_database()
+ ORDER BY 1, 3;
+
DROP ROLE regress_dump_test_role;
> On 29 Apr 2024, at 21:15, Tom Lane <tgl@sss.pgh.pa.us> wrote: > It occurred to me to use "aclexplode" to expand the initprivs, and > then we can substitute names with simple equality tests. The test > query is a bit more complicated, but I feel better about it. Nice, I didn't even remember that function existed. I agree that it's an improvement even at the increased query complexity. > v3 attached also has a bit more work on code comments. LGTM. -- Daniel Gustafsson
Daniel Gustafsson <daniel@yesql.se> writes:
> On 29 Apr 2024, at 21:15, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> v3 attached also has a bit more work on code comments.
> LGTM.
Pushed, thanks for reviewing!
regards, tom lane
I wrote:
> Pushed, thanks for reviewing!
Argh, I forgot I'd meant to push b0c5b215d first not second.
Oh well, it was only neatnik-ism that made me want to see
those other animals fail --- and a lot of the buildfarm is
red right now for $other_reasons anyway.
regards, tom lane
On Monday, April 29, 2024, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Daniel Gustafsson <daniel@yesql.se> writes:
>> On 28 Apr 2024, at 20:52, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> This is of course not bulletproof: with a sufficiently weird
>> bootstrap superuser name, we could get false matches to parts
>> of "regress_dump_test_role" or to privilege strings. That
>> seems unlikely enough to live with, but I wonder if anybody has
>> a better idea.
> I think that will be bulletproof enough to keep it working in the buildfarm and
> among 99% of hackers.
It occurred to me to use "aclexplode" to expand the initprivs, and
then we can substitute names with simple equality tests. The test
query is a bit more complicated, but I feel better about it.
My solution to this was to rely on the fact that the bootstrap superuser is assigned OID 10 regardless of its name.
David J.
"David G. Johnston" <david.g.johnston@gmail.com> writes:
> My solution to this was to rely on the fact that the bootstrap superuser is
> assigned OID 10 regardless of its name.
Yeah, I wrote it that way to start with too, but reconsidered
because
(1) I don't like hard-coding numeric OIDs. We can avoid that in C
code but it's harder to do in SQL.
(2) It's not clear to me that this test couldn't be run by a
non-bootstrap superuser. I think "current_user" is actually
the correct thing for the role executing the test.
regards, tom lane
On Monday, April 29, 2024, Tom Lane <tgl@sss.pgh.pa.us> wrote:
"David G. Johnston" <david.g.johnston@gmail.com> writes:
> My solution to this was to rely on the fact that the bootstrap superuser is
> assigned OID 10 regardless of its name.
Yeah, I wrote it that way to start with too, but reconsidered
because
(1) I don't like hard-coding numeric OIDs. We can avoid that in C
code but it's harder to do in SQL.
If the tests don’t involve, e.g., the predefined role pg_monitor and its grantor of the memberships in the other predefined roles, this indeed can be avoided. So I think my test still needs to check for 10 even if some other superuser is allowed to produce the test output since a key output in my case was the bootstrap superuser and the initdb roles.
(2) It's not clear to me that this test couldn't be run by a
non-bootstrap superuser. I think "current_user" is actually
the correct thing for the role executing the test.
Agreed, testing against current_role is correct if the things being queried were created while executing the test. I would need to do this as well to remove the current requirement that my tests be run by the bootstrap superuser.
David J.
While the 'DROP OWNED BY fails to clean out pg_init_privs grants'
issue is now fixed,we have a similar issue with REASSIGN OWNED BY that
is still there:
Tested on fresh git checkout om May 20th
test=# create user privtestuser superuser;
CREATE ROLE
test=# set role privtestuser;
SET
test=# create extension pg_stat_statements ;
CREATE EXTENSION
test=# select * from pg_init_privs where privtype ='e';
objoid | classoid | objsubid | privtype |
initprivs
--------+----------+----------+----------+------------------------------------------------------
16405 | 1259 | 0 | e |
{privtestuser=arwdDxtm/privtestuser,=r/privtestuser}
16422 | 1259 | 0 | e |
{privtestuser=arwdDxtm/privtestuser,=r/privtestuser}
16427 | 1255 | 0 | e | {privtestuser=X/privtestuser}
(3 rows)
test=# reset role;
RESET
test=# reassign owned by privtestuser to hannuk;
REASSIGN OWNED
test=# select * from pg_init_privs where privtype ='e';
objoid | classoid | objsubid | privtype |
initprivs
--------+----------+----------+----------+------------------------------------------------------
16405 | 1259 | 0 | e |
{privtestuser=arwdDxtm/privtestuser,=r/privtestuser}
16422 | 1259 | 0 | e |
{privtestuser=arwdDxtm/privtestuser,=r/privtestuser}
16427 | 1255 | 0 | e | {privtestuser=X/privtestuser}
(3 rows)
test=# drop user privtestuser ;
DROP ROLE
test=# select * from pg_init_privs where privtype ='e';
objoid | classoid | objsubid | privtype | initprivs
--------+----------+----------+----------+---------------------------------
16405 | 1259 | 0 | e | {16390=arwdDxtm/16390,=r/16390}
16422 | 1259 | 0 | e | {16390=arwdDxtm/16390,=r/16390}
16427 | 1255 | 0 | e | {16390=X/16390}
(3 rows)
This will cause pg_dump to produce something that cant be loaded back
into the database:
CREATE EXTENSION IF NOT EXISTS pg_stat_statements WITH SCHEMA public;
...
REVOKE ALL ON TABLE public.pg_stat_statements FROM "16390";
...
And this will, among other things, break pg_upgrade.
-----
Hannu
On Tue, Apr 30, 2024 at 6:40 AM David G. Johnston
<david.g.johnston@gmail.com> wrote:
>
> On Monday, April 29, 2024, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>
>> "David G. Johnston" <david.g.johnston@gmail.com> writes:
>> > My solution to this was to rely on the fact that the bootstrap superuser is
>> > assigned OID 10 regardless of its name.
>>
>> Yeah, I wrote it that way to start with too, but reconsidered
>> because
>>
>> (1) I don't like hard-coding numeric OIDs. We can avoid that in C
>> code but it's harder to do in SQL.
>
>
> If the tests don’t involve, e.g., the predefined role pg_monitor and its grantor of the memberships in the other
predefinedroles, this indeed can be avoided. So I think my test still needs to check for 10 even if some other
superuseris allowed to produce the test output since a key output in my case was the bootstrap superuser and the initdb
roles.
>
>>
>> (2) It's not clear to me that this test couldn't be run by a
>> non-bootstrap superuser. I think "current_user" is actually
>> the correct thing for the role executing the test.
>
>
> Agreed, testing against current_role is correct if the things being queried were created while executing the test. I
wouldneed to do this as well to remove the current requirement that my tests be run by the bootstrap superuser.
>
> David J.
>
Hannu Krosing <hannuk@google.com> writes:
> While the 'DROP OWNED BY fails to clean out pg_init_privs grants'
> issue is now fixed,we have a similar issue with REASSIGN OWNED BY that
> is still there:
Ugh, how embarrassing. I'll take a look tomorrow, if no one
beats me to it.
regards, tom lane
> On 24 May 2024, at 01:01, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Hannu Krosing <hannuk@google.com> writes: >> While the 'DROP OWNED BY fails to clean out pg_init_privs grants' >> issue is now fixed,we have a similar issue with REASSIGN OWNED BY that >> is still there: > > Ugh, how embarrassing. I'll take a look tomorrow, if no one > beats me to it. I had a look, but I didn't beat you to a fix since it's not immediately clear to me how this should work for REASSING OWNED (DROP OWNED seems a simpler case). Should REASSIGN OWNED alter the rows in pg_shdepend matching init privs from SHARED_DEPENDENCY_OWNER to SHARED_DEPENDENCY_INITACL, so that these can be mopped up with a later DROP OWNED? Trying this in a POC patch it fails with RemoveRoleFromInitPriv not removing the rows, shortcircuiting that for a test seems to make it work but is it the right approach? -- Daniel Gustafsson
Daniel Gustafsson <daniel@yesql.se> writes:
> I had a look, but I didn't beat you to a fix since it's not immediately clear
> to me how this should work for REASSING OWNED (DROP OWNED seems a simpler
> case). Should REASSIGN OWNED alter the rows in pg_shdepend matching init privs
> from SHARED_DEPENDENCY_OWNER to SHARED_DEPENDENCY_INITACL, so that these can be
> mopped up with a later DROP OWNED? Trying this in a POC patch it fails with
> RemoveRoleFromInitPriv not removing the rows, shortcircuiting that for a test
> seems to make it work but is it the right approach?
I've tentatively concluded that I shouldn't have modeled
SHARED_DEPENDENCY_INITACL so closely on SHARED_DEPENDENCY_ACL,
in particular the decision that we don't need such an entry if
there's also SHARED_DEPENDENCY_OWNER. I think one reason we
can get away with omitting a SHARED_DEPENDENCY_ACL entry for the
owner is that the object's normal ACL is part of its primary
catalog row, so it goes away automatically if the object is
dropped. But obviously that's not true for a pg_init_privs
entry. I can see two routes to a solution:
1. Create SHARED_DEPENDENCY_INITACL, if applicable, whether the
role is the object's owner or not. Then, clearing out the
pg_shdepend entry cues us to go delete the pg_init_privs entry.
2. Just always search pg_init_privs for relevant entries
when dropping an object.
I don't especially like #2 on performance grounds, but it has
a lot fewer moving parts than #1. In particular, there's some
handwaving in changeDependencyOnOwner() about why we should
drop SHARED_DEPENDENCY_ACL when changing owner, and I've not
wrapped my head around how those concerns map to INITACL
if we treat it in this different way.
Another point: shdepReassignOwned explicitly does not touch grants
or default ACLs. It feels like the same should be true of
pg_init_privs entries, or at least if not, why not? In that case
there's nothing to be done in shdepReassignOwned (although maybe its
comments should be adjusted to mention this explicitly). The bug is
just that DROP OWNED isn't getting rid of the entries because there's
no INITACL entry to cue it to do so.
Another thing I'm wondering about right now is privileges on global
objects (roles, databases, tablespaces). The fine manual says
"Although an extension script is not prohibited from creating such
objects, if it does so they will not be tracked as part of the
extension". Presumably, that also means that no pg_init_privs
entries are made; but do we do that correctly?
Anyway, -ENOCAFFEINE for the moment. I'll look more later.
regards, tom lane
> On 24 May 2024, at 16:20, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I've tentatively concluded that I shouldn't have modeled > SHARED_DEPENDENCY_INITACL so closely on SHARED_DEPENDENCY_ACL, > in particular the decision that we don't need such an entry if > there's also SHARED_DEPENDENCY_OWNER. +1, in light of this report I think we need to go back on that. > I can see two routes to a solution: > > 1. Create SHARED_DEPENDENCY_INITACL, if applicable, whether the > role is the object's owner or not. Then, clearing out the > pg_shdepend entry cues us to go delete the pg_init_privs entry. > > 2. Just always search pg_init_privs for relevant entries > when dropping an object. > > I don't especially like #2 on performance grounds, but it has > a lot fewer moving parts than #1. #1 is more elegant, but admittedly also more complicated. An unscientific guess is that a majority of objects dropped won't have init privs, making the extra scan in #2 quite possibly more than academic. #2 could however be backported and solve the issue in existing clusters. > Another point: shdepReassignOwned explicitly does not touch grants > or default ACLs. It feels like the same should be true of > pg_init_privs entries, Agreed, I can't see why pg_init_privs should be treated differently. > Another thing I'm wondering about right now is privileges on global > objects (roles, databases, tablespaces). The fine manual says > "Although an extension script is not prohibited from creating such > objects, if it does so they will not be tracked as part of the > extension". Presumably, that also means that no pg_init_privs > entries are made; but do we do that correctly? I'm away from a tree to check, but that does warrant investigation. If we don't have a test for it already then it might be worth constructing something to catch that. -- Daniel Gustafsson
Daniel Gustafsson <daniel@yesql.se> writes:
> On 24 May 2024, at 16:20, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Another point: shdepReassignOwned explicitly does not touch grants
>> or default ACLs. It feels like the same should be true of
>> pg_init_privs entries,
> Agreed, I can't see why pg_init_privs should be treated differently.
Thinking about this some more: the point of pg_init_privs is to record
an object's privileges as they stood at the end of CREATE EXTENSION
(or extension update), with the goal that pg_dump should be able to
compute the delta between that and the object's current privileges
and emit GRANT/REVOKE commands to restore those current privileges
after a fresh extension install. (We slide gently past the question
of whether the fresh extension install is certain to create privileges
matching the previous pg_init_privs entry.) So this goal seems to
mean that neither ALTER OWNER nor REASSIGN OWNED should touch
pg_init_privs at all, as that would break its function of recording
a historical state. Only DROP OWNED should get rid of pg_init_privs
grants, and that only because there's no choice -- if the role is
about to go away, we can't hang on to a reference to its OID.
However ... then what are the implications of doing ALTER OWNER on
an extension-owned object? Is pg_dump supposed to recognize that
that's happened and replay it too? If not, is it sane at all to
try to restore the current privileges, which are surely dependent
on the current owner? I kind of doubt that that's possible at all,
and even if it is it might result in security issues. It seems
like pg_init_privs has missed a critical thing, which is to record
the original owner not only the original privileges.
(Alternatively, maybe we should forbid ALTER OWNER on extension-owned
objects? Or at least on those having pg_init_privs entries?)
I'm wondering too about this scenario:
1. CREATE EXTENSION installs an object and sets some initial privileges.
2. DBA manually modifies the object's privileges.
3. ALTER EXTENSION UPDATE further modifies the object's privileges.
I think what will happen is that at the end of ALTER EXTENSION,
we'll store the object's current ACL verbatim in pg_init_privs,
therefore including the effects of step 2. This seems undesirable,
but I'm not sure how to get around it.
Anyway, this is starting to look like the sort of can of worms
best not opened post-beta1. v17 has made some things better in this
area, and I don't think it's made anything worse; so maybe we should
declare victory for the moment and hope to address these additional
concerns later. I've added an open item though.
regards, tom lane
On Fri, May 24, 2024 at 11:59 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Thinking about this some more: the point of pg_init_privs is to record > an object's privileges as they stood at the end of CREATE EXTENSION > (or extension update), with the goal that pg_dump should be able to > compute the delta between that and the object's current privileges > and emit GRANT/REVOKE commands to restore those current privileges > after a fresh extension install. (We slide gently past the question > of whether the fresh extension install is certain to create privileges > matching the previous pg_init_privs entry.) +1 to all of this. > So this goal seems to > mean that neither ALTER OWNER nor REASSIGN OWNED should touch > pg_init_privs at all, as that would break its function of recording > a historical state. Only DROP OWNED should get rid of pg_init_privs > grants, and that only because there's no choice -- if the role is > about to go away, we can't hang on to a reference to its OID. But I would have thought that the right thing to do to pg_init_privs here would be essentially s/$OLDOWNER/$NEWOWNER/g. I know I'm late to the party here, but why is that idea wrong? -- Robert Haas EDB: http://www.enterprisedb.com
Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, May 24, 2024 at 11:59 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> So this goal seems to
>> mean that neither ALTER OWNER nor REASSIGN OWNED should touch
>> pg_init_privs at all, as that would break its function of recording
>> a historical state. Only DROP OWNED should get rid of pg_init_privs
>> grants, and that only because there's no choice -- if the role is
>> about to go away, we can't hang on to a reference to its OID.
> But I would have thought that the right thing to do to pg_init_privs
> here would be essentially s/$OLDOWNER/$NEWOWNER/g.
Doesn't seem right to me. That will give pg_dump the wrong idea
of what the initial privileges actually were, and I don't see how
it can construct correct delta GRANT/REVOKE on the basis of false
information. During the dump reload, the extension will be
recreated with the original owner (I think), causing its objects'
privileges to go back to the original pg_init_privs values.
Applying a delta that starts from some other state seems pretty
questionable in that case.
It could be that if we expect pg_dump to issue an ALTER OWNER
to move ownership of the altered extension object to its new
owner, and only then apply its computed delta GRANT/REVOKEs,
then indeed the right thing is for the original ALTER OWNER
to apply s/$OLDOWNER/$NEWOWNER/g to pg_init_privs. I've not
thought this through in complete detail, but it feels like
that might work, because the reload-time ALTER OWNER would
apply exactly that change to both the object's ACL and its
pg_init_privs, and then the delta is starting from the right state.
Of course, pg_dump can't do that right now because it lacks the
information that such an ALTER is needed.
Although ... this is tickling a recollection that pg_dump doesn't
try very hard to run CREATE EXTENSION with the same owner that
the extension had originally. That's a leftover from the times
when basically all extensions required superuser to install,
and of course one superuser is as good as the next. There might
be some work we have to do on that side too if we want to up
our game in this area.
Another case that's likely not handled well is what if the extension
really shouldn't have its original owner (e.g. you're using
--no-owner). If it's restored under a new owner then the
pg_init_privs data certainly doesn't apply, and it feels like it'd
be mostly luck if the precomputed delta GRANT/REVOKEs lead to a
state you like.
regards, tom lane
On Fri, May 24, 2024 at 2:57 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Doesn't seem right to me. That will give pg_dump the wrong idea > of what the initial privileges actually were, and I don't see how > it can construct correct delta GRANT/REVOKE on the basis of false > information. During the dump reload, the extension will be > recreated with the original owner (I think), causing its objects' > privileges to go back to the original pg_init_privs values. Oh! That does seem like it would make what I said wrong, but how would it even know who the original owner was? Shouldn't we be recreating the object with the owner it had at dump time? > Although ... this is tickling a recollection that pg_dump doesn't > try very hard to run CREATE EXTENSION with the same owner that > the extension had originally. That's a leftover from the times > when basically all extensions required superuser to install, > and of course one superuser is as good as the next. There might > be some work we have to do on that side too if we want to up > our game in this area. Hmm, yeah. > Another case that's likely not handled well is what if the extension > really shouldn't have its original owner (e.g. you're using > --no-owner). If it's restored under a new owner then the > pg_init_privs data certainly doesn't apply, and it feels like it'd > be mostly luck if the precomputed delta GRANT/REVOKEs lead to a > state you like. I'm not sure exactly how this computation works, but if tgl granted nmisch privileges on an object and the extension is now owned by rhaas, it would seem like the right thing to do would be for rhaas to grant nmisch those same privileges. Conversely if tgl started with privileges to do X and Y and later was granted privileges to do Z and we dump and restore such that the extension is owned by rhaas, I'd presume rhaas would end up with those same privileges. I'm probably too far from the code to give terribly useful advice here, but I think the expected behavior is that the new owner replaces the old one for all purposes relating to the owned object(s). At least, I can't currently see what else makes any sense. -- Robert Haas EDB: http://www.enterprisedb.com
Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, May 24, 2024 at 2:57 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Doesn't seem right to me. That will give pg_dump the wrong idea
>> of what the initial privileges actually were, and I don't see how
>> it can construct correct delta GRANT/REVOKE on the basis of false
>> information. During the dump reload, the extension will be
>> recreated with the original owner (I think), causing its objects'
>> privileges to go back to the original pg_init_privs values.
> Oh! That does seem like it would make what I said wrong, but how would
> it even know who the original owner was? Shouldn't we be recreating
> the object with the owner it had at dump time?
Keep in mind that the whole point here is for the pg_dump script to
just say "CREATE EXTENSION foo", not to mess with the individual
objects therein. So the objects are (probably) going to be owned by
the user that issued CREATE EXTENSION.
In the original conception, that was the end of it: what you got for
the member objects was whatever state CREATE EXTENSION left behind.
The idea of pg_init_privs is to support dump/reload of subsequent
manual alterations of privileges for extension-created objects.
I'm not, at this point, 100% certain that that's a fully realizable
goal. But I definitely think it's insane to expect that to work
without also tracking changes in the ownership of said objects.
Maybe forbidding ALTER OWNER on extension-owned objects isn't
such a bad idea?
regards, tom lane
On Fri, May 24, 2024 at 10:00 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Robert Haas <robertmhaas@gmail.com> writes: > > On Fri, May 24, 2024 at 2:57 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > >> Doesn't seem right to me. That will give pg_dump the wrong idea > >> of what the initial privileges actually were, and I don't see how > >> it can construct correct delta GRANT/REVOKE on the basis of false > >> information. During the dump reload, the extension will be > >> recreated with the original owner (I think), causing its objects' > >> privileges to go back to the original pg_init_privs values. > > > Oh! That does seem like it would make what I said wrong, but how would > > it even know who the original owner was? Shouldn't we be recreating > > the object with the owner it had at dump time? > > Keep in mind that the whole point here is for the pg_dump script to > just say "CREATE EXTENSION foo", not to mess with the individual > objects therein. So the objects are (probably) going to be owned by > the user that issued CREATE EXTENSION. > > In the original conception, that was the end of it: what you got for > the member objects was whatever state CREATE EXTENSION left behind. > The idea of pg_init_privs is to support dump/reload of subsequent > manual alterations of privileges for extension-created objects. > I'm not, at this point, 100% certain that that's a fully realizable > goal. The issue became visible because pg_dump issued a bogus REVOKE ALL ON TABLE public.pg_stat_statements FROM "16390"; Maybe the right place for a fix is in pg_dump and the fix would be to *not* issue REVOKE ALL ON <any object> FROM <non-existing users> ? Or alternatively change REVOKE to treat non-existing users as a no-op ? Also, the pg_init_privs entry should either go away or at least be changed at the point when the user referenced in init-privs is dropped. Having an pg_init_privs entry referencing a non-existing user is certainly of no practical use. Or maybe we should change the user at that point to NULL or some special non-existing-user-id ? > But I definitely think it's insane to expect that to work > without also tracking changes in the ownership of said objects. > > Maybe forbidding ALTER OWNER on extension-owned objects isn't > such a bad idea?
Hannu Krosing <hannuk@google.com> writes:
> Having an pg_init_privs entry referencing a non-existing user is
> certainly of no practical use.
Sure, that's not up for debate. What I think we're discussing
right now is
1. What other cases are badly handled by the pg_init_privs
mechanisms.
2. How much of that is practical to fix in v17, seeing that
it's all long-standing bugs and we're already past beta1.
I kind of doubt that the answer to #2 is "all of it".
But perhaps we can do better than "none of it".
regards, tom lane
On Sat, May 25, 2024 at 4:48 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Hannu Krosing <hannuk@google.com> writes: > > Having an pg_init_privs entry referencing a non-existing user is > > certainly of no practical use. > > Sure, that's not up for debate. What I think we're discussing > right now is > > 1. What other cases are badly handled by the pg_init_privs > mechanisms. > > 2. How much of that is practical to fix in v17, seeing that > it's all long-standing bugs and we're already past beta1. > > I kind of doubt that the answer to #2 is "all of it". > But perhaps we can do better than "none of it". Putting the fix either in pg_dump or making REVOKE tolerate non-existing users would definitely be most practical / useful fixes, as these would actually allow pg_upgrade to v17 to work without changing anything in older versions. Currently one already can revoke a privilege that is not there in the first place, with the end state being that the privilege (still) does not exist. This does not even generate a warning. Extending this to revoking from users that do not exist does not seem any different on conceptual level, though I understand that implementation would be very different as it needs catching the user lookup error from a very different part of the code. That said, it would be better if we can have something that would be easy to backport something that would make pg_upgrade work for all supported versions. Making REVOKE silently ignore revoking from non-existing users would improve general robustness but could conceivably change behaviour if somebody relies on it in their workflows. Regards, Hannu
Attached is a minimal patch to allow missing roles in REVOKE command
This should fix the pg_upgrade issue and also a case where somebody
has dropped a role you are trying to revoke privileges from :
smalltest=# create table revoketest();
CREATE TABLE
smalltest=# revoke select on revoketest from bob;
WARNING: ignoring REVOKE FROM a missing role "bob"
REVOKE
smalltest=# create user bob;
CREATE ROLE
smalltest=# grant select on revoketest to bob;
GRANT
smalltest=# \du
List of roles
Role name | Attributes
-----------+------------------------------------------------------------
bob |
hannuk | Superuser, Create role, Create DB, Replication, Bypass RLS
smalltest=# \dp
Access privileges
Schema | Name | Type | Access privileges | Column
privileges | Policies
--------+------------+-------+------------------------+-------------------+----------
public | revoketest | table | hannuk=arwdDxtm/hannuk+| |
| | | bob=r/hannuk | |
public | vacwatch | table | | |
(2 rows)
smalltest=# revoke select on revoketest from bob, joe;
WARNING: ignoring REVOKE FROM a missing role "joe"
REVOKE
smalltest=# \dp
Access privileges
Schema | Name | Type | Access privileges | Column
privileges | Policies
--------+------------+-------+------------------------+-------------------+----------
public | revoketest | table | hannuk=arwdDxtm/hannuk | |
public | vacwatch | table | | |
(2 rows)
On Sun, May 26, 2024 at 12:05 AM Hannu Krosing <hannuk@google.com> wrote:
>
> On Sat, May 25, 2024 at 4:48 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > Hannu Krosing <hannuk@google.com> writes:
> > > Having an pg_init_privs entry referencing a non-existing user is
> > > certainly of no practical use.
> >
> > Sure, that's not up for debate. What I think we're discussing
> > right now is
> >
> > 1. What other cases are badly handled by the pg_init_privs
> > mechanisms.
> >
> > 2. How much of that is practical to fix in v17, seeing that
> > it's all long-standing bugs and we're already past beta1.
> >
> > I kind of doubt that the answer to #2 is "all of it".
> > But perhaps we can do better than "none of it".
>
> Putting the fix either in pg_dump or making REVOKE tolerate
> non-existing users would definitely be most practical / useful fixes,
> as these would actually allow pg_upgrade to v17 to work without
> changing anything in older versions.
>
> Currently one already can revoke a privilege that is not there in the
> first place, with the end state being that the privilege (still) does
> not exist.
> This does not even generate a warning.
>
> Extending this to revoking from users that do not exist does not seem
> any different on conceptual level, though I understand that
> implementation would be very different as it needs catching the user
> lookup error from a very different part of the code.
>
> That said, it would be better if we can have something that would be
> easy to backport something that would make pg_upgrade work for all
> supported versions.
> Making REVOKE silently ignore revoking from non-existing users would
> improve general robustness but could conceivably change behaviour if
> somebody relies on it in their workflows.
>
> Regards,
> Hannu
Вложения
Hannu Krosing <hannuk@google.com> writes:
> Attached is a minimal patch to allow missing roles in REVOKE command
FTR, I think this is a very bad idea.
It might be OK if we added some kind of IF EXISTS option,
but I'm not eager about that concept either.
The right thing here is to fix the backend so that pg_dump doesn't
see these bogus ACLs.
regards, tom lane
> On 26 May 2024, at 23:25, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Hannu Krosing <hannuk@google.com> writes: >> Attached is a minimal patch to allow missing roles in REVOKE command > > FTR, I think this is a very bad idea. Agreed, this is papering over a bug. If we are worried about pg_upgrade it would be better to add a check to pg_upgrade which detects this case and advices the user how to deal with it. -- Daniel Gustafsson
Hi Daniel,
pg_upgrade is just one important user of pg_dump which is the one that
generates REVOKE for a non-existent role.
We should definitely also fix pg_dump, likely just checking that the
role exists when generating REVOKE commands (may be a good practice
for other cases too so instead of casting to ::regrole do the actual
join)
## here is the fix for pg_dump
While flying to Vancouver I looked around in pg_dump code, and it
looks like the easiest way to mitigate the dangling pg_init_priv
entries is to replace the query in pg_dump with one that filters out
invalid entries
The current query is at line 9336:
/* Fetch initial-privileges data */
if (fout->remoteVersion >= 90600)
{
printfPQExpBuffer(query,
"SELECT objoid, classoid, objsubid, privtype, initprivs "
"FROM pg_init_privs");
res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
And we need the same but filtering out invalid aclitems from initprivs
something like this
WITH q AS (
SELECT objoid, classoid, objsubid, privtype, unnest(initprivs) AS
initpriv FROM saved_init_privs
)
SELECT objoid, classoid, objsubid, privtype, array_agg(initpriv) as initprivs
FROM q
WHERE is_valid_value_for_type(initpriv::text, 'aclitem')
GROUP BY 1,2,3,4;
### The proposed re[placement query:
Unfortunately we do not have an existing
is_this_a_valid_value_for_type(value text, type text, OUT res boolean)
function, so for a read-only workaround the following seems to work:
Here I first collect the initprivs array elements which fail the
conversion to text and back into an array and store it in GUC
pg_dump.bad_aclitems
Then I use this stored list to filter out the bad ones in the actual query.
DO $$
DECLARE
aclitem_text text;
bad_aclitems text[] = '{}';
BEGIN
FOR aclitem_text IN
SELECT DISTINCT unnest(initprivs)::text FROM pg_init_privs
LOOP
BEGIN /* try to convert back to aclitem */
PERFORM aclitem_text::aclitem;
EXCEPTION WHEN OTHERS THEN /* collect bad aclitems */
bad_aclitems := bad_aclitems || ARRAY[aclitem_text];
END;
END LOOP;
IF bad_aclitems != '{}' THEN
RAISE WARNING 'Ignoring bad aclitems "%" in pg_init_privs', bad_aclitems;
END IF;
PERFORM set_config('pg_dump.bad_aclitems', bad_aclitems::text,
false); -- true for trx-local
END;
$$;
WITH q AS (
SELECT objoid, classoid, objsubid, privtype, unnest(initprivs) AS
initpriv FROM pg_init_privs
)
SELECT objoid, classoid, objsubid, privtype, array_agg(initpriv) AS initprivs
FROM q
WHERE NOT initpriv::text = ANY
(current_setting('pg_dump.bad_aclitems')::text[])
GROUP BY 1,2,3,4;
--
Hannu
On Sun, May 26, 2024 at 11:27 PM Daniel Gustafsson <daniel@yesql.se> wrote:
>
> > On 26 May 2024, at 23:25, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > Hannu Krosing <hannuk@google.com> writes:
> >> Attached is a minimal patch to allow missing roles in REVOKE command
> >
> > FTR, I think this is a very bad idea.
>
> Agreed, this is papering over a bug. If we are worried about pg_upgrade it
> would be better to add a check to pg_upgrade which detects this case and
> advices the user how to deal with it.
>
> --
> Daniel Gustafsson
>