Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Avoid race condition between "GRANT role" and "DROP ROLE".

  2. Fix pg_dumpall to cope with dangling OIDs in pg_auth_members.

  3. Ensure that pg_auth_members.grantor is always valid.

  1. Major Version Upgrade failure due to orphan roles entries in catalog

    Virender Singla <virender.cse@gmail.com> — 2025-02-11T10:02:42Z

    Hi,

We have identified an issue causing upgrade failures. The following steps
detail how to reproduce the issue:

*Create an orphan role entry*

/* Postgres version:: PostgreSQL 16.6 */
/* The same can be reproduced in version 17 as well */

create role my_group;
create role dropped_member;
begin;
grant my_group to dropped_member;
OTHER SESSION: drop role dropped_member;
BACK IN ORIGINAL SESSION:
commit;

*Upgrade to Postgres v17*

And the upgrade fails with an error :


*GRANT "my_group" TO "" WITH INHERIT TRUE GRANTED BY "postgres";ERROR:
zero-length delimited identifier at or near """"*

The issue seems to be coming from pg_dumpall for building grants during
pg_upgrade.

https://github.com/postgres/postgres/blob/master/src/bin/pg_dump/pg_dumpall.c#L992

-Virender

  2. Re: Major Version Upgrade failure due to orphan roles entries in catalog

    Laurenz Albe <laurenz.albe@cybertec.at> — 2025-02-11T14:20:24Z

    On Tue, 2025-02-11 at 15:32 +0530, Virender Singla wrote:
> We have identified an issue causing upgrade failures. The following steps detail how to reproduce the issue:
> 
> Create an orphan role entry
> 
> /* Postgres version:: PostgreSQL 16.6 */ 
> /* The same can be reproduced in version 17 as well */
> 
> create role my_group;
> create role dropped_member;
> begin;
> grant my_group to dropped_member;
> OTHER SESSION: drop role dropped_member;
> BACK IN ORIGINAL SESSION:
> commit;
> 
> Upgrade to Postgres v17
> 
> And the upgrade fails with an error :
> 
> GRANT "my_group" TO "" WITH INHERIT TRUE GRANTED BY "postgres";
> ERROR: zero-length delimited identifier at or near """"
> 
> The issue seems to be coming from pg_dumpall for building grants during pg_upgrade.
> 
> https://github.com/postgres/postgres/blob/master/src/bin/pg_dump/pg_dumpall.c#L992

I agree that that is a bug.

I guess the GRANT statement should put a FOR KEY SHARE lock on the pg_authid row
for "dropped_member", similar to what we do for foreign keys.

Yours,
Laurenz Albe

-- 

*E-Mail Disclaimer*
Der Inhalt dieser E-Mail ist ausschliesslich fuer den 
bezeichneten Adressaten bestimmt. Wenn Sie nicht der vorgesehene Adressat 
dieser E-Mail oder dessen Vertreter sein sollten, so beachten Sie bitte, 
dass jede Form der Kenntnisnahme, Veroeffentlichung, Vervielfaeltigung oder 
Weitergabe des Inhalts dieser E-Mail unzulaessig ist. Wir bitten Sie, sich 
in diesem Fall mit dem Absender der E-Mail in Verbindung zu setzen.

*CONFIDENTIALITY NOTICE & DISCLAIMER
*This message and any attachment are 
confidential and may be privileged or otherwise protected from disclosure 
and solely for the use of the person(s) or entity to whom it is intended. 
If you have received this message in error and are not the intended 
recipient, please notify the sender immediately and delete this message and 
any attachment from your system. If you are not the intended recipient, be 
advised that any use of this message is prohibited and may be unlawful, and 
you must not copy this message or attachment or disclose the contents to 
any other person.

  3. Re: Major Version Upgrade failure due to orphan roles entries in catalog

    Virender Singla <virender.cse@gmail.com> — 2025-02-13T09:51:18Z

    BTW a similar variant has been fixed in PG 16 but does not fix the above
case.

create role dropped_group;
create role member;
begin;
grant dropped_group to member;
OTHER SESSION: drop role dropped_group;
BACK IN ORIGINAL SESSION:
commit;




On Tue, Feb 11, 2025 at 7:50 PM Laurenz Albe <laurenz.albe@cybertec.at>
wrote:

> On Tue, 2025-02-11 at 15:32 +0530, Virender Singla wrote:
> > We have identified an issue causing upgrade failures. The following
> steps detail how to reproduce the issue:
> >
> > Create an orphan role entry
> >
> > /* Postgres version:: PostgreSQL 16.6 */
> > /* The same can be reproduced in version 17 as well */
> >
> > create role my_group;
> > create role dropped_member;
> > begin;
> > grant my_group to dropped_member;
> > OTHER SESSION: drop role dropped_member;
> > BACK IN ORIGINAL SESSION:
> > commit;
> >
> > Upgrade to Postgres v17
> >
> > And the upgrade fails with an error :
> >
> > GRANT "my_group" TO "" WITH INHERIT TRUE GRANTED BY "postgres";
> > ERROR: zero-length delimited identifier at or near """"
> >
> > The issue seems to be coming from pg_dumpall for building grants during
> pg_upgrade.
> >
> >
> https://github.com/postgres/postgres/blob/master/src/bin/pg_dump/pg_dumpall.c#L992
>
> I agree that that is a bug.
>
> I guess the GRANT statement should put a FOR KEY SHARE lock on the
> pg_authid row
> for "dropped_member", similar to what we do for foreign keys.
>
> Yours,
> Laurenz Albe
>
> --
>
> *E-Mail Disclaimer*
> Der Inhalt dieser E-Mail ist ausschliesslich fuer den
> bezeichneten Adressaten bestimmt. Wenn Sie nicht der vorgesehene Adressat
> dieser E-Mail oder dessen Vertreter sein sollten, so beachten Sie bitte,
> dass jede Form der Kenntnisnahme, Veroeffentlichung, Vervielfaeltigung
> oder
> Weitergabe des Inhalts dieser E-Mail unzulaessig ist. Wir bitten Sie, sich
> in diesem Fall mit dem Absender der E-Mail in Verbindung zu setzen.
>
> *CONFIDENTIALITY NOTICE & DISCLAIMER
> *This message and any attachment are
> confidential and may be privileged or otherwise protected from disclosure
> and solely for the use of the person(s) or entity to whom it is intended.
> If you have received this message in error and are not the intended
> recipient, please notify the sender immediately and delete this message
> and
> any attachment from your system. If you are not the intended recipient, be
> advised that any use of this message is prohibited and may be unlawful,
> and
> you must not copy this message or attachment or disclose the contents to
> any other person.
>

  4. Re: Major Version Upgrade failure due to orphan roles entries in catalog

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-02-13T17:07:25Z

    Laurenz Albe <laurenz.albe@cybertec.at> writes:
> On Tue, 2025-02-11 at 15:32 +0530, Virender Singla wrote:
>> /* Postgres version:: PostgreSQL 16.6 */
>> /* The same can be reproduced in version 17 as well */
>> 
>> create role my_group;
>> create role dropped_member;
>> begin;
>> grant my_group to dropped_member;
>> OTHER SESSION: drop role dropped_member;
>> BACK IN ORIGINAL SESSION:
>> commit;

[ leaves a dangling pg_auth_members entry behind ]

> I guess the GRANT statement should put a FOR KEY SHARE lock on the pg_authid row
> for "dropped_member", similar to what we do for foreign keys.

There is a lock taken already, which is why the DROP ROLE blocks.
What there is not is a recheck that the role still exists once
we have its lock.

I poked into this, and in code terms it seems like the problem is
that AddRoleMems adds a pg_auth_members entry but makes it depend
on only one of the three roles listed in the entry.  That seems
wrong on its face.  The connection to this problem is that the
dependency-adding code would take care of the lock+recheck, if
it only knew that the pg_auth_members entry ought to be treated
as depending on dropped_member.

The code the attached patch is touching is from commit 6566133c5.
I'm not going to blame the bug on that commit, because before that
we had dependencies on *zero* of the three roles; at least it added
one on the grantor.  But I'm wondering whether Robert thought about
the other two roles and explicitly rejected making dependencies
for them, and if so why.

In addition to fixing the described race condition, this patch
means that DROP OWNED BY on either the granted role or the member
will drop the grant.  This seems consistent with the goals of
6566133c5 and with our general approach to dropping privileges
during DROP OWNED BY; but it didn't happen that way before.

I'm wondering a little bit if we could simplify/remove some of the
code in user.c by relying on processing of these dependency entries
to carry the load instead during DROP ROLE.  But it passes check-world
as-is, so maybe leaving well enough alone is best.

			regards, tom lane

  5. Re: Major Version Upgrade failure due to orphan roles entries in catalog

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-02-13T17:16:02Z

    On 2025-Feb-11, Virender Singla wrote:

> And the upgrade fails with an error :
> 
> 
> *GRANT "my_group" TO "" WITH INHERIT TRUE GRANTED BY "postgres";ERROR:
> zero-length delimited identifier at or near """"*
> 
> The issue seems to be coming from pg_dumpall for building grants during
> pg_upgrade.

Hmm, I think fixing the bug as Tom suggests downthread is probably a
good idea, but I think we should in addition change pg_dumpall to avoid
printing a GRANT line if there's no grantee.  Maybe turning one of these LEFT
JOINs into a regular inner join is a sufficient fix for that:

	/* Generate and execute query. */
	printfPQExpBuffer(buf, "SELECT ur.rolname AS role, "
					  "um.rolname AS member, "
					  "ug.oid AS grantorid, "
					  "ug.rolname AS grantor, "
					  "a.admin_option");
	if (dump_grant_options)
		appendPQExpBufferStr(buf, ", a.inherit_option, a.set_option");
	appendPQExpBuffer(buf, " FROM pg_auth_members a "
					  "LEFT JOIN %s ur on ur.oid = a.roleid "
					  "LEFT JOIN %s um on um.oid = a.member "
					  "LEFT JOIN %s ug on ug.oid = a.grantor "
					  "WHERE NOT (ur.rolname ~ '^pg_' AND um.rolname ~ '^pg_')"
					  "ORDER BY 1,2,4", role_catalog, role_catalog, role_catalog);

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"I am amazed at [the pgsql-sql] mailing list for the wonderful support, and
lack of hesitasion in answering a lost soul's question, I just wished the rest
of the mailing list could be like this."                               (Fotis)
              https://postgr.es/m/200606261359.k5QDxE2p004593@auth-smtp.hol.gr

  6. Re: Major Version Upgrade failure due to orphan roles entries in catalog

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-02-13T17:33:07Z

    =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@alvh.no-ip.org> writes:
> Hmm, I think fixing the bug as Tom suggests downthread is probably a
> good idea, but I think we should in addition change pg_dumpall to avoid
> printing a GRANT line if there's no grantee.

On the one hand, my proposed patch can do nothing to fix existing
dangling entries in pg_auth_members, so hacking pg_dump seems like
a good workaround if the problem already exists.  On the other hand,
if we make pg_dump do that then we won't detect future problems of
the same ilk.

> Maybe turning one of these LEFT
> JOINs into a regular inner join is a sufficient fix for that:

Probably change all three, if we're to do this at all.

			regards, tom lane

  7. Re: Major Version Upgrade failure due to orphan roles entries in catalog

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-02-20T22:19:34Z

    =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@alvh.no-ip.org> writes:
> Hmm, I think fixing the bug as Tom suggests downthread is probably a
> good idea, but I think we should in addition change pg_dumpall to avoid
> printing a GRANT line if there's no grantee.  Maybe turning one of these LEFT
> JOINs into a regular inner join is a sufficient fix for that:

After looking at this I thought it was worth a little more code to warn
about the dangling role OID, instead of just silently ignoring it.
Here's a couple of more-polished patches.

I'm unsure whether to back-patch the 0001 patch, as it does imply
more pg_shdepend entries than we have today, so it's sort of a
backdoor catalog change.  But we're mostly interested in the
transient behavior of having a lock+recheck during entry insertion,
so maybe it's fine.  0002 should be back-patched in any case.

(BTW, I was distressed to learn from the code coverage report
that we have zero test coverage of the hardly-trivial logic in
dumpRoleMembership.  I didn't try to address that here.  I did
test this new logic by dint of manually deleting from pg_authid.)

			regards, tom lane

  8. Re: Major Version Upgrade failure due to orphan roles entries in catalog

    Laurenz Albe <laurenz.albe@cybertec.at> — 2025-02-21T07:18:12Z

    On Thu, 2025-02-20 at 17:19 -0500, Tom Lane wrote:
> After looking at this I thought it was worth a little more code to warn
> about the dangling role OID, instead of just silently ignoring it.
> Here's a couple of more-polished patches.
> 
> I'm unsure whether to back-patch the 0001 patch, as it does imply
> more pg_shdepend entries than we have today, so it's sort of a
> backdoor catalog change.  But we're mostly interested in the
> transient behavior of having a lock+recheck during entry insertion,
> so maybe it's fine.  0002 should be back-patched in any case.

I'd say that adding new catalog entries in a way that is compatible
shouldn't be a problem, but I still wouldn't backpatch the 0001 patch,
because it is not necessary.  The orphaned pg_auth_members entry didn't
cause any harm, and a few warnings more during an upgrade shouldn't be
a big problem.

I have one question about the first patch:

> diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
> index 0db174e6f1..0c84886e82 100644
> --- a/src/backend/commands/user.c
> +++ b/src/backend/commands/user.c
> @@ -489,7 +490,7 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
>  	 * Advance command counter so we can see new record; else tests in
>  	 * AddRoleMems may fail.
>  	 */
> -	if (addroleto || adminmembers || rolemembers)
> +	if (addroleto || adminmembers || rolemembers || !superuser())
>  		CommandCounterIncrement();
>  
>  	/* Default grant. */

That change seems unrelated to the problem at hand, and I don't see it
mentioned in the commit message.  Is that an oversight you fixed on the
fly?

Apart from that, the patches look fine.

Yours,
Laurenz Albe

-- 

*E-Mail Disclaimer*
Der Inhalt dieser E-Mail ist ausschliesslich fuer den 
bezeichneten Adressaten bestimmt. Wenn Sie nicht der vorgesehene Adressat 
dieser E-Mail oder dessen Vertreter sein sollten, so beachten Sie bitte, 
dass jede Form der Kenntnisnahme, Veroeffentlichung, Vervielfaeltigung oder 
Weitergabe des Inhalts dieser E-Mail unzulaessig ist. Wir bitten Sie, sich 
in diesem Fall mit dem Absender der E-Mail in Verbindung zu setzen.

*CONFIDENTIALITY NOTICE & DISCLAIMER
*This message and any attachment are 
confidential and may be privileged or otherwise protected from disclosure 
and solely for the use of the person(s) or entity to whom it is intended. 
If you have received this message in error and are not the intended 
recipient, please notify the sender immediately and delete this message and 
any attachment from your system. If you are not the intended recipient, be 
advised that any use of this message is prohibited and may be unlawful, and 
you must not copy this message or attachment or disclose the contents to 
any other person.

  9. Re: Major Version Upgrade failure due to orphan roles entries in catalog

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-02-21T14:41:43Z

    Laurenz Albe <laurenz.albe@cybertec.at> writes:
> I have one question about the first patch:

>> -	if (addroleto || adminmembers || rolemembers)
>> +	if (addroleto || adminmembers || rolemembers || !superuser())
>> 		CommandCounterIncrement();

> That change seems unrelated to the problem at hand, and I don't see it
> mentioned in the commit message.  Is that an oversight you fixed on the
> fly?

Well, kinda, because the patch doesn't work without it.  The
problematic case is where none of those 3 flags are set and also
!superuser, so that we decide we need the default grant implemented a
few lines further down.  That grant now has an explicit reference to
the new roleid, and if we haven't CommandCounterIncrement'ed, the
pg_shdepend code will error out because it doesn't see the catalog
entry for roleid.

			regards, tom lane

  10. Re: Major Version Upgrade failure due to orphan roles entries in catalog

    Laurenz Albe <laurenz.albe@cybertec.at> — 2025-02-21T16:23:24Z

    On Fri, 2025-02-21 at 09:41 -0500, Tom Lane wrote:
> Laurenz Albe <laurenz.albe@cybertec.at> writes:
> > I have one question about the first patch:
> 
> > > -	if (addroleto || adminmembers || rolemembers)
> > > +	if (addroleto || adminmembers || rolemembers || !superuser())
> > > 		CommandCounterIncrement();
> 
> > That change seems unrelated to the problem at hand, and I don't see it
> > mentioned in the commit message.  Is that an oversight you fixed on the
> > fly?
> 
> Well, kinda, because the patch doesn't work without it.  The
> problematic case is where none of those 3 flags are set and also
> !superuser, so that we decide we need the default grant implemented a
> few lines further down.  That grant now has an explicit reference to
> the new roleid, and if we haven't CommandCounterIncrement'ed, the
> pg_shdepend code will error out because it doesn't see the catalog
> entry for roleid.

Thanks for the explanation.  That might be worth a comment.

Yours,
Laurenz Albe

-- 

*E-Mail Disclaimer*
Der Inhalt dieser E-Mail ist ausschliesslich fuer den 
bezeichneten Adressaten bestimmt. Wenn Sie nicht der vorgesehene Adressat 
dieser E-Mail oder dessen Vertreter sein sollten, so beachten Sie bitte, 
dass jede Form der Kenntnisnahme, Veroeffentlichung, Vervielfaeltigung oder 
Weitergabe des Inhalts dieser E-Mail unzulaessig ist. Wir bitten Sie, sich 
in diesem Fall mit dem Absender der E-Mail in Verbindung zu setzen.

*CONFIDENTIALITY NOTICE & DISCLAIMER
*This message and any attachment are 
confidential and may be privileged or otherwise protected from disclosure 
and solely for the use of the person(s) or entity to whom it is intended. 
If you have received this message in error and are not the intended 
recipient, please notify the sender immediately and delete this message and 
any attachment from your system. If you are not the intended recipient, be 
advised that any use of this message is prohibited and may be unlawful, and 
you must not copy this message or attachment or disclose the contents to 
any other person.

  11. Re: Major Version Upgrade failure due to orphan roles entries in catalog

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-02-21T16:31:09Z

    Laurenz Albe <laurenz.albe@cybertec.at> writes:
> Thanks for the explanation.  That might be worth a comment.

The adjacent comment already says

	/*
	 * Advance command counter so we can see new record; else tests in
	 * AddRoleMems may fail.
	 */

so I didn't see anything to add there.  Maybe "We can skip this in
cases where we will not call AddRoleMems"?  Or maybe the better answer
is to conclude that the whole idea of not calling
CommandCounterIncrement unconditionally is too fragile and not worth
expending brain cells on, and just rip out the if-test.

			regards, tom lane

  12. Re: Major Version Upgrade failure due to orphan roles entries in catalog

    Laurenz Albe <laurenz.albe@cybertec.at> — 2025-02-21T21:39:40Z

    On Fri, 2025-02-21 at 11:31 -0500, Tom Lane wrote:
> Laurenz Albe <laurenz.albe@cybertec.at> writes:
> > Thanks for the explanation.  That might be worth a comment.
> 
> The adjacent comment already says
> 
> 	/*
> 	 * Advance command counter so we can see new record; else tests in
> 	 * AddRoleMems may fail.
> 	 */
> 
> so I didn't see anything to add there.  Maybe "We can skip this in
> cases where we will not call AddRoleMems"?  Or maybe the better answer
> is to conclude that the whole idea of not calling
> CommandCounterIncrement unconditionally is too fragile and not worth
> expending brain cells on, and just rip out the if-test.

Both the extra sentence and the simplification feel like an improvement.
I am fine with either.

Yours,
Laurenz Albe

-- 

*E-Mail Disclaimer*
Der Inhalt dieser E-Mail ist ausschliesslich fuer den 
bezeichneten Adressaten bestimmt. Wenn Sie nicht der vorgesehene Adressat 
dieser E-Mail oder dessen Vertreter sein sollten, so beachten Sie bitte, 
dass jede Form der Kenntnisnahme, Veroeffentlichung, Vervielfaeltigung oder 
Weitergabe des Inhalts dieser E-Mail unzulaessig ist. Wir bitten Sie, sich 
in diesem Fall mit dem Absender der E-Mail in Verbindung zu setzen.

*CONFIDENTIALITY NOTICE & DISCLAIMER
*This message and any attachment are 
confidential and may be privileged or otherwise protected from disclosure 
and solely for the use of the person(s) or entity to whom it is intended. 
If you have received this message in error and are not the intended 
recipient, please notify the sender immediately and delete this message and 
any attachment from your system. If you are not the intended recipient, be 
advised that any use of this message is prohibited and may be unlawful, and 
you must not copy this message or attachment or disclose the contents to 
any other person.

  13. Re: Major Version Upgrade failure due to orphan roles entries in catalog

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-02-21T21:45:18Z

    Laurenz Albe <laurenz.albe@cybertec.at> writes:
> On Fri, 2025-02-21 at 11:31 -0500, Tom Lane wrote:
>> ... Or maybe the better answer
>> is to conclude that the whole idea of not calling
>> CommandCounterIncrement unconditionally is too fragile and not worth
>> expending brain cells on, and just rip out the if-test.

> Both the extra sentence and the simplification feel like an improvement.
> I am fine with either.

The more I think about it the more I like just getting rid of the
test.  It'll likely break with every future change to this logic,
until somebody finally gives up on it; so why not now?

I'll make it so.  Thanks for reviewing!

			regards, tom lane

  14. Re: Major Version Upgrade failure due to orphan roles entries in catalog

    Robert Haas <robertmhaas@gmail.com> — 2025-03-04T15:17:07Z

    On Thu, Feb 13, 2025 at 12:07 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> The code the attached patch is touching is from commit 6566133c5.
> I'm not going to blame the bug on that commit, because before that
> we had dependencies on *zero* of the three roles; at least it added
> one on the grantor.  But I'm wondering whether Robert thought about
> the other two roles and explicitly rejected making dependencies
> for them, and if so why.

For some reason, I came to the conclusion that it wasn't needed.
Apparently, that was incorrect.

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