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(at)leadboat(dot)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(at)sss(dot)pgh(dot)pa(dot)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(at)yesql(dot)se> writes:
>> On 6 Apr 2024, at 01:10, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)sss(dot)pgh(dot)pa(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Daniel Gustafsson <daniel(at)yesql(dot)se> writes:
>>> On 6 Apr 2024, at 01:10, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)yesql(dot)se> writes:
> On 21 Apr 2024, at 23:08, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

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

> On 28 Apr 2024, at 20:52, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)yesql(dot)se> writes:
>> On 28 Apr 2024, at 20:52, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

> On 29 Apr 2024, at 21:15, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)yesql(dot)se> writes:
> On 29 Apr 2024, at 21:15, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Daniel Gustafsson <daniel(at)yesql(dot)se> writes:
> >> On 28 Apr 2024, at 20:52, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(dot)g(dot)johnston(at)gmail(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> "David G. Johnston" <david(dot)g(dot)johnston(at)gmail(dot)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(dot)g(dot)johnston(at)gmail(dot)com> wrote:
>
> On Monday, April 29, 2024, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>
>> "David G. Johnston" <david(dot)g(dot)johnston(at)gmail(dot)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.
>

Hannu Krosing <hannuk(at)google(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Hannu Krosing <hannuk(at)google(dot)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(at)yesql(dot)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(at)sss(dot)pgh(dot)pa(dot)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(at)yesql(dot)se> writes:
> On 24 May 2024, at 16:20, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)sss(dot)pgh(dot)pa(dot)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(at)gmail(dot)com> writes:
> On Fri, May 24, 2024 at 11:59 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)sss(dot)pgh(dot)pa(dot)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(at)gmail(dot)com> writes:
> On Fri, May 24, 2024 at 2:57 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > On Fri, May 24, 2024 at 2:57 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)google(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Hannu Krosing <hannuk(at)google(dot)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(at)google(dot)com> wrote:
>
> On Sat, May 25, 2024 at 4:48 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >
> > Hannu Krosing <hannuk(at)google(dot)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(at)google(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Hannu Krosing <hannuk(at)google(dot)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(at)yesql(dot)se> wrote:
>
> > On 26 May 2024, at 23:25, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >
> > Hannu Krosing <hannuk(at)google(dot)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
>

On Tue, May 28, 2024 at 9:06 PM Hannu Krosing <hannuk(at)google(dot)com> wrote:
> 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

+1 for this approach. I agree with Tom that fixing this in REVOKE is a
bad plan; REVOKE is used by way too many things other than pg_dump,
and the behavior change is not in general desirable.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Fri, May 24, 2024 at 4:00 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > 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?

I think the root of my confusion, or at least one of the roots of my
confusion, was whether we were talking about altering the extension or
the objects within the extension. If somebody creates an extension as
user nitin and afterwards changes the ownership to user lucia, then I
would hope for the same pg_init_privs contents as if user lucia had
created the extension in the first place. I think that might be hard
to achieve in complex cases, if the extension changes the ownership of
some objects during the creation process, or if some ownership has
been changed afterward. But in the simple case where nothing like that
happens, it sounds possible. If, on the other hand, somebody creates
the extension and modifies the ownership of an object in the
extension, then I agree that pg_init_privs shouldn't be updated.
However, it also seems pretty clear that pg_init_privs is not
recording enough state for pg_dump to mimic the ownership change,
because it only records the original privileges, not the original
ownership.

So, can we recreate the original privileges without recreating the
original ownership? It doesn't really seem like this is going to work
in general. If nitin creates the extension and grants privileges to
lucia, and then the ownership of the extension is changed to swara,
then nitin is no longer a valid grantor. Even if we could fix that by
modifying the grants to substitute swara for nitin, that could create
impermissible circularities in the permissions graph. Maybe there are
some scenarios where we can fix things up, but it doesn't seem
possible in general.

In a perfect world, the fix here is probably to have pg_init_privs or
something similar record the ownership as well as the permissions, but
that is not back-patchable and nobody's on the hook to fix up somebody
else's feature. So what do we do? Imposing a constraint that you can't
change the ownership of an extension-owned object, as you propose,
seems fairly likely to break a few existing extension scripts, and
also won't fix existing instances that are already broken, but maybe
it's still worth considering if we don't have a better idea. Another
line of thinking might be to somehow nerf pg_init_privs, or the use of
it, so that we don't even try to cover this case e.g. if the ownership
is changed, we nuke the pg_init_privs entry or resnap it to the
current state, and the dump fails to recreate the original state but
we get out from under that by declaring the case unsupported (with
appropriate documentation changes, hopefully). pg_init_privs seems
adequate for normal system catalog entries where the ownership
shouldn't ever change from the bootstrap superuser, but extensions
seem like they require more infrastructure.

I think the only thing we absolutely have to fix here is the dangling
ACL references. Those are a hazard, because the OID could be reused by
an unrelated role, which seems like it could even give rise to
security concerns. Making the whole pg_init_privs system actually work
for this case case would be even better, but that seems to require
filling in at least one major hole the original design, which is a lot
to ask for (1) a back-patched fix or (2) a patch that someone who is
not the original author has to try to write and be responsible for
despite not being the one who created the problem.

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> I think the only thing we absolutely have to fix here is the dangling
> ACL references.

Here's a draft patch that takes care of Hannu's example, and I think
it fixes other potential dangling-reference scenarios too.

I'm not sure whether I like the direction this is going, but I do
think we may not be able to do a lot more than this for v17.

The core change is to install SHARED_DEPENDENCY_INITACL entries in
pg_shdepend for all references to non-pinned roles in pg_init_privs,
whether they are the object's owner or not. Doing that ensures that
we can't drop a role that is still referenced there, and it provides
a basis for shdepDropOwned and shdepReassignOwned to take some kind
of action for such references.

The semantics I've implemented on top of that are:

* ALTER OWNER does not touch pg_init_privs entries.

* REASSIGN OWNED replaces pg_init_privs references with the new
role, whether the references are as grantor or grantee.

* DROP OWNED removes pg_init_privs mentions of the doomed role
(as grantor or grantee), removing the pg_init_privs entry
altogether if it goes to zero clauses. (This is what happened
already, but only if if a SHARED_DEPENDENCY_INITACL entry existed.)

I'm not terribly thrilled with this, because it's still possible
to get into a state where a pg_init_privs entry is based on
an owning role that's no longer the owner: you just have to use
ALTER OWNER directly rather than via REASSIGN OWNED. While
I don't think the backend has much problem with that, it probably
will confuse pg_dump to some extent. However, such cases are
going to confuse pg_dump anyway for reasons discussed upthread,
namely that we don't really support dump/restore of extensions
where not all the objects are owned by the extension owner.
I'm content to leave that in the pile of unfinished work for now.

An alternative definition could be that ALTER OWNER also replaces
old role with new in pg_init_privs entries. That would reduce
the surface for confusing pg_dump a little bit, but I don't think
that I like it better, for two reasons:

* ALTER OWNER would have to probe pg_init_acl for potential
entries every time, which would be wasted work for most ALTERs.

* This gets away from the notion that pg_init_privs should be
a historical record of the state that existed at the end of CREATE
EXTENSION. Now, maybe that notion is unworkable anyway, but
I don't want to let go of it before we're sure about that.

A couple of more-minor points for review:

* As this stands, updateInitAclDependencies() no longer pays any
attention to its ownerId argument, and in one place I depend on
that to skip doing a rather expensive lookup of the current object
owner. Perhaps we should remove that argument altogether, and
in consequence simplify some other callers too? However, that
would only help much if we were willing to revert 534287403's
changes to pass the object's owner ID to recordExtensionInitPriv,
which I'm hesitant to do because I suspect we'll end up wanting
to record the owner ID explicitly in pg_init_privs entries.
On the third hand, maybe we'll never do that, so perhaps we should
revert those changes for now; some of them add nontrivial lookup
costs.

* In shdepReassignOwned, I refactored to move the switch on
sdepForm->classid into a new subroutine. We could have left
it where it is, but it would need a couple more tab stops of
indentation which felt like too much. It's in the eye of
the beholder though.

Comments?

regards, tom lane

I wrote:
> The semantics I've implemented on top of that are:
> * ALTER OWNER does not touch pg_init_privs entries.
> * REASSIGN OWNED replaces pg_init_privs references with the new
> role, whether the references are as grantor or grantee.
> * DROP OWNED removes pg_init_privs mentions of the doomed role
> (as grantor or grantee), removing the pg_init_privs entry
> altogether if it goes to zero clauses. (This is what happened
> already, but only if if a SHARED_DEPENDENCY_INITACL entry existed.)

> I'm not terribly thrilled with this, because it's still possible
> to get into a state where a pg_init_privs entry is based on
> an owning role that's no longer the owner: you just have to use
> ALTER OWNER directly rather than via REASSIGN OWNED. While
> I don't think the backend has much problem with that, it probably
> will confuse pg_dump to some extent.

I poked at this some more, and I'm now moderately convinced that
this is a good place to stop for v17, primarily because ALTER OWNER
doesn't touch pg_init_privs in the older branches either. Thus,
while we've not made things better for pg_dump, at least we've not
introduced a new behavior it will have to account for in the future.

I experimented with this test scenario (to set up, do
"make install" in src/test/modules/test_pg_dump):

-----
create role regress_dump_test_role;
create user test_super superuser;
create user joe;
create database tpd;
\c tpd test_super
create extension test_pg_dump;
alter function regress_pg_dump_schema.test_func() owner to joe;
\df+ regress_pg_dump_schema.test_func()
-----

The \df+ will correctly show that test_func() is owned by joe
and has ACL

| Access privileges |
+------------------------------+
| =X/joe +|
| joe=X/joe +|
| regress_dump_test_role=X/joe |

Now, if you pg_dump this database, what you get is

CREATE EXTENSION IF NOT EXISTS test_pg_dump WITH SCHEMA public;
...
--
-- Name: FUNCTION test_func(); Type: ACL; Schema: regress_pg_dump_schema; Owner: joe
--

REVOKE ALL ON FUNCTION regress_pg_dump_schema.test_func() FROM PUBLIC;
REVOKE ALL ON FUNCTION regress_pg_dump_schema.test_func() FROM test_super;
REVOKE ALL ON FUNCTION regress_pg_dump_schema.test_func() FROM regress_dump_test_role;
GRANT ALL ON FUNCTION regress_pg_dump_schema.test_func() TO joe;
GRANT ALL ON FUNCTION regress_pg_dump_schema.test_func() TO PUBLIC;
GRANT ALL ON FUNCTION regress_pg_dump_schema.test_func() TO regress_dump_test_role;

So pg_dump realizes that the privileges are not what they were,
but it's fairly confused about what to do about it. And it really
can't get that right without better modeling (er, more than none at
all) of the ownership of extension-member objects. If you restore
this dump as postgres, you'll find that test_func is now owned by
postgres and has ACL

| Access privileges |
+-----------------------------------+
| postgres=X/postgres +|
| joe=X/postgres +|
| =X/postgres +|
| regress_dump_test_role=X/postgres |

The grantees are okay, more or less, but we've totally failed to
replicate the owner/grantor. But this is exactly the same as what
you get if you do the experiment in v16 or before. Given the lack
of complaints about that, I think it's okay to stop here for now.
We've at least made REASSIGN OWNED work better.

regards, tom lane

> On 15 Jun 2024, at 01:46, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> I think the only thing we absolutely have to fix here is the dangling
>> ACL references.
>
> Here's a draft patch that takes care of Hannu's example, and I think
> it fixes other potential dangling-reference scenarios too.
>
> I'm not sure whether I like the direction this is going, but I do
> think we may not be able to do a lot more than this for v17.

Agreed.

> The core change is to install SHARED_DEPENDENCY_INITACL entries in
> pg_shdepend for all references to non-pinned roles in pg_init_privs,
> whether they are the object's owner or not. Doing that ensures that
> we can't drop a role that is still referenced there, and it provides
> a basis for shdepDropOwned and shdepReassignOwned to take some kind
> of action for such references.

I wonder if this will break any tools/scripts in prod which relies on the
previous (faulty) behaviour. It will be interesting to see if anything shows
up on -bugs. Off the cuff it seems like a good idea judging by where we are
and what we can fix with it.

> I'm not terribly thrilled with this, because it's still possible
> to get into a state where a pg_init_privs entry is based on
> an owning role that's no longer the owner: you just have to use
> ALTER OWNER directly rather than via REASSIGN OWNED. While
> I don't think the backend has much problem with that, it probably
> will confuse pg_dump to some extent. However, such cases are
> going to confuse pg_dump anyway for reasons discussed upthread,
> namely that we don't really support dump/restore of extensions
> where not all the objects are owned by the extension owner.
> I'm content to leave that in the pile of unfinished work for now.

+1

> An alternative definition could be that ALTER OWNER also replaces
> old role with new in pg_init_privs entries. That would reduce
> the surface for confusing pg_dump a little bit, but I don't think
> that I like it better, for two reasons:
>
> * ALTER OWNER would have to probe pg_init_acl for potential
> entries every time, which would be wasted work for most ALTERs.

Unless it would magically fix all the pg_dump problems I'd prefer to avoid
this.

> * As this stands, updateInitAclDependencies() no longer pays any
> attention to its ownerId argument, and in one place I depend on
> that to skip doing a rather expensive lookup of the current object
> owner. Perhaps we should remove that argument altogether, and
> in consequence simplify some other callers too? However, that
> would only help much if we were willing to revert 534287403's
> changes to pass the object's owner ID to recordExtensionInitPriv,
> which I'm hesitant to do because I suspect we'll end up wanting
> to record the owner ID explicitly in pg_init_privs entries.
> On the third hand, maybe we'll never do that, so perhaps we should
> revert those changes for now; some of them add nontrivial lookup
> costs.

I wonder if it's worth reverting passing the owner ID for v17 and revisiting
that in 18 if we work on recording the ID. Shaving a few catalog lookups is
generally worthwhile, doing them without needing the result for the next five
years might bite us.

Re-reading 534287403 I wonder about this hunk in RemoveRoleFromInitPriv:

+ if (!isNull)
+ old_acl = DatumGetAclPCopy(oldAclDatum);
+ else
+ old_acl = NULL; /* this case shouldn't happen, probably */

I wonder if we should Assert() on old_acl being NULL? I can't imagine a case
where it should legitimately be that and catching such in development might be
useful for catching stray bugs?

> * In shdepReassignOwned, I refactored to move the switch on
> sdepForm->classid into a new subroutine. We could have left
> it where it is, but it would need a couple more tab stops of
> indentation which felt like too much. It's in the eye of
> the beholder though.

I prefer the new way.

+1 on going ahead with this patch. There is more work to do but I agree that
this about all that makes sense in v17 at this point.

--
Daniel Gustafsson

Daniel Gustafsson <daniel(at)yesql(dot)se> writes:
> On 15 Jun 2024, at 01:46, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> The core change is to install SHARED_DEPENDENCY_INITACL entries in
>> pg_shdepend for all references to non-pinned roles in pg_init_privs,
>> whether they are the object's owner or not. Doing that ensures that
>> we can't drop a role that is still referenced there, and it provides
>> a basis for shdepDropOwned and shdepReassignOwned to take some kind
>> of action for such references.

> I wonder if this will break any tools/scripts in prod which relies on the
> previous (faulty) behaviour. It will be interesting to see if anything shows
> up on -bugs. Off the cuff it seems like a good idea judging by where we are
> and what we can fix with it.

Considering that SHARED_DEPENDENCY_INITACL has existed for less than
two months, it's hard to believe that any outside code has grown any
dependencies on it, much less that it couldn't be adjusted readily.

> I wonder if it's worth reverting passing the owner ID for v17 and revisiting
> that in 18 if we work on recording the ID. Shaving a few catalog lookups is
> generally worthwhile, doing them without needing the result for the next five
> years might bite us.

Yeah, that was the direction I was leaning in, too. I'll commit the
revert of that separately, so that un-reverting it shouldn't be too
painful if we eventually decide to do so.

> Re-reading 534287403 I wonder about this hunk in RemoveRoleFromInitPriv:

> + if (!isNull)
> + old_acl = DatumGetAclPCopy(oldAclDatum);
> + else
> + old_acl = NULL; /* this case shouldn't happen, probably */

> I wonder if we should Assert() on old_acl being NULL? I can't imagine a case
> where it should legitimately be that and catching such in development might be
> useful for catching stray bugs?

Hmm, yeah. I was trying to be agnostic about whether it's okay for a
pg_init_privs ACL to be NULL ... but I can't imagine a real use for
that either, and the new patch does add some code that's effectively
assuming it isn't. Agreed, let's be uniform about insisting !isNull.

> +1 on going ahead with this patch. There is more work to do but I agree that
> this about all that makes sense in v17 at this point.

Thanks for reviewing!

regards, tom lane

> On 17 Jun 2024, at 16:56, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Daniel Gustafsson <daniel(at)yesql(dot)se> writes:

>> I wonder if this will break any tools/scripts in prod which relies on the
>> previous (faulty) behaviour. It will be interesting to see if anything shows
>> up on -bugs. Off the cuff it seems like a good idea judging by where we are
>> and what we can fix with it.
>
> Considering that SHARED_DEPENDENCY_INITACL has existed for less than
> two months, it's hard to believe that any outside code has grown any
> dependencies on it, much less that it couldn't be adjusted readily.

Doh, I was thinking about it backwards, clearly not a worry =)

>> I wonder if it's worth reverting passing the owner ID for v17 and revisiting
>> that in 18 if we work on recording the ID. Shaving a few catalog lookups is
>> generally worthwhile, doing them without needing the result for the next five
>> years might bite us.
>
> Yeah, that was the direction I was leaning in, too. I'll commit the
> revert of that separately, so that un-reverting it shouldn't be too
> painful if we eventually decide to do so.

Sounds good.

--
Daniel Gustafsson

Hi Tom,

Is there anything that could be back-patched with reasonable effort ?

--
Hannu

On Mon, Jun 17, 2024 at 6:37 PM Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
>
> > On 17 Jun 2024, at 16:56, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > Daniel Gustafsson <daniel(at)yesql(dot)se> writes:
>
> >> I wonder if this will break any tools/scripts in prod which relies on the
> >> previous (faulty) behaviour. It will be interesting to see if anything shows
> >> up on -bugs. Off the cuff it seems like a good idea judging by where we are
> >> and what we can fix with it.
> >
> > Considering that SHARED_DEPENDENCY_INITACL has existed for less than
> > two months, it's hard to believe that any outside code has grown any
> > dependencies on it, much less that it couldn't be adjusted readily.
>
> Doh, I was thinking about it backwards, clearly not a worry =)
>
> >> I wonder if it's worth reverting passing the owner ID for v17 and revisiting
> >> that in 18 if we work on recording the ID. Shaving a few catalog lookups is
> >> generally worthwhile, doing them without needing the result for the next five
> >> years might bite us.
> >
> > Yeah, that was the direction I was leaning in, too. I'll commit the
> > revert of that separately, so that un-reverting it shouldn't be too
> > painful if we eventually decide to do so.
>
> Sounds good.
>
> --
> Daniel Gustafsson
>

Hannu Krosing <hannuk(at)google(dot)com> writes:
> Is there anything that could be back-patched with reasonable effort ?

Afraid not. The whole thing is dependent on pg_shdepend entries
that won't exist in older branches.

regards, tom lane

Then maybe we should put a query / function in the release notes to
clean up the existing mess.

Thinking of it we should do it anyway, as the patch only prevents new
messiness from happening and does not fix existing issues.

I could share a query to update the pg_init_privs with non-existent
role to replace it with the owner of the object if we figure out a
correct place to publish it.

---
Hannu

On Thu, Jun 20, 2024 at 5:35 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Hannu Krosing <hannuk(at)google(dot)com> writes:
> > Is there anything that could be back-patched with reasonable effort ?
>
> Afraid not. The whole thing is dependent on pg_shdepend entries
> that won't exist in older branches.
>
> regards, tom lane

Or perhaps we should still also patch pg_dump to ignore the aclentries
which refer to roles that do not exist in the database ?

On Thu, Jun 20, 2024 at 7:41 PM Hannu Krosing <hannuk(at)google(dot)com> wrote:
>
> Then maybe we should put a query / function in the release notes to
> clean up the existing mess.
>
> Thinking of it we should do it anyway, as the patch only prevents new
> messiness from happening and does not fix existing issues.
>
> I could share a query to update the pg_init_privs with non-existent
> role to replace it with the owner of the object if we figure out a
> correct place to publish it.
>
> ---
> Hannu
>
>
>
>
> On Thu, Jun 20, 2024 at 5:35 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >
> > Hannu Krosing <hannuk(at)google(dot)com> writes:
> > > Is there anything that could be back-patched with reasonable effort ?
> >
> > Afraid not. The whole thing is dependent on pg_shdepend entries
> > that won't exist in older branches.
> >
> > regards, tom lane

Hannu Krosing <hannuk(at)google(dot)com> writes:
> Or perhaps we should still also patch pg_dump to ignore the aclentries
> which refer to roles that do not exist in the database ?

I didn't want to do that before, and I still don't. Given that this
issue has existed since pg_init_privs was invented (9.6) without
prior reports, I don't think it's a big enough problem in practice
to be worth taking extraordinary actions for.

regards, tom lane

On Thu, Jun 20, 2024 at 2:25 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Hannu Krosing <hannuk(at)google(dot)com> writes:
> > Or perhaps we should still also patch pg_dump to ignore the aclentries
> > which refer to roles that do not exist in the database ?
>
> I didn't want to do that before, and I still don't. Given that this
> issue has existed since pg_init_privs was invented (9.6) without
> prior reports, I don't think it's a big enough problem in practice
> to be worth taking extraordinary actions for.

If we don't fix it in the code and we don't document it anywhere, the
next person who hits it is going to have to try to discover the fact
that there's a problem from the pgsql-hackers archives. That doesn't
seem good. I don't have an educated opinion about what we should do
here specifically, and I realize that we don't have any official place
to document known issues, but it can be pretty inconvenient for users
not to know about them.

--
Robert Haas
EDB: http://www.enterprisedb.com

It does happen with some regularity.

At least one large cloud database provider I know of saw this more
than once a month until the mitigations were integrated in the major
version upgrade process.

It is possible that making database upgrades easier via better
automation is what made this turn up more, as now less experienced /
non-DBA types are more comfortable doing the version upgrades, whereas
before it would be something done by a person who can also diagnose it
and manually fix pg_init_privs.

Still it would be nice to have some public support for users of
non-managed PostgreSQL databases as well

On Thu, Jun 20, 2024 at 8:25 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Hannu Krosing <hannuk(at)google(dot)com> writes:
> > Or perhaps we should still also patch pg_dump to ignore the aclentries
> > which refer to roles that do not exist in the database ?
>
> I didn't want to do that before, and I still don't. Given that this
> issue has existed since pg_init_privs was invented (9.6) without
> prior reports, I don't think it's a big enough problem in practice
> to be worth taking extraordinary actions for.
>
> regards, tom lane

On Thu, Jun 20, 2024 at 3:43 PM Hannu Krosing <hannuk(at)google(dot)com> wrote:
> Still it would be nice to have some public support for users of
> non-managed PostgreSQL databases as well

+1.

--
Robert Haas
EDB: http://www.enterprisedb.com

 
>On Thu, Jun 20, 2024 at 3:43PM Hannu Krosing < hannuk(at)google(dot)com > wrote:
>> Still it would be nice to have some public support for users of
>> non-managed PostgreSQL databases as well
>+1.
>
>--
>Robert Haas
>EDB: http://www.enterprisedb.com
Hello! I have recently been researching postgres build using meson. And I came across a failure of the src/test/modules/test_pg_dump-running test.
You can see regression.diffs in attached file.
It turned out that the test fails if the cluster is initialized by custom user. In my case by user postgres.
Script to reproduce test_pg_fump failure:
---
meson setup -Dprefix=$PGPREFIX build
 
ninja -j64 -C build install >/dev/null
ninja -j64 -C build install-test-files >/dev/null
 
initdb -U postgres -k -D $PGPREFIX/data
pg_ctl -D $PGPREFIX/data -l logfile start
 
psql -U postgres -c "CREATE USER test SUPERUSER"
psql -U postgres -c "CREATE DATABASE test"
 
meson test --setup running --suite postgresql:test_pg_dump-running -C build/
---
You can catch the same failure if build using make and run make installcheck -C src/test/modules/test_pg_dump.
 
I have looked at the test and found queries like below:
---
SELECT pg_describe_object(classid,objid,objsubid) COLLATE "C" 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;
---
This query does not expect that test database may already contain some information about custom user that ran test_pg_dump-running.
--
Egor Chindyaskin
Postgres Professional: https://postgrespro.com
 

=?UTF-8?B?0JXQs9C+0YAg0KfQuNC90LTRj9GB0LrQuNC9?= <kyzevan23(at)mail(dot)ru> writes:
> This query does not expect that test database may already contain some information about custom user that ran test_pg_dump-running.

I'm perfectly content to reject this as being an abuse of the test
case. Our TAP tests are built on the assumption that they use
databases created within the test case. Apparently, you've found a
way to use the meson test infrastructure to execute a TAP test in
the equivalent of "make installcheck" rather than "make check" mode.
I am unwilling to buy into the proposition that our TAP tests should
be proof against doing that after making arbitrary changes to the
database's initial state. If anything, the fact that this is possible
is a bug in our meson scripts.

regards, tom lane

Hello!

IIUC the regression test test_pg_dump [1] fails, see the attached
regression.diffs:

diff -U3
/Users/test/Work/postgrespro/src/test/modules/test_pg_dump/expected/test_pg_dump.out
/Users/test/Work/postgrespro/build/testrun/test_pg_dump-running/regress/results/test_pg_dump.out
---
/Users/test/Work/postgrespro/src/test/modules/test_pg_dump/expected/test_pg_dump.out 2024-09-12
15:02:26.345434331 +0700
+++
/Users/test/Work/postgrespro/build/testrun/test_pg_dump-running/regress/results/test_pg_dump.out 2024-09-12
15:42:09.341520173 +0700

[1]
https://github.com/postgres/postgres/blob/master/src/test/modules/test_pg_dump/sql/test_pg_dump.sql

On 2024-09-18 07:31, Tom Lane wrote:
> =?UTF-8?B?0JXQs9C+0YAg0KfQuNC90LTRj9GB0LrQuNC9?= <kyzevan23(at)mail(dot)ru>
> writes:
>> This query does not expect that test database may already contain some
>> information about custom user that ran test_pg_dump-running.
>
> I'm perfectly content to reject this as being an abuse of the test
> case. Our TAP tests are built on the assumption that they use
> databases created within the test case. Apparently, you've found a
> way to use the meson test infrastructure to execute a TAP test in
> the equivalent of "make installcheck" rather than "make check" mode.
> I am unwilling to buy into the proposition that our TAP tests should
> be proof against doing that after making arbitrary changes to the
> database's initial state. If anything, the fact that this is possible
> is a bug in our meson scripts.
>
> regards, tom lane

--
Marina Polyakova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company