Message-ID: <b0459688caa95c77f439bebf3b30341169986369.camel@cybertec.at>
Subject: Re: Major Version Upgrade failure due to orphan roles entries in
 catalog
From: Laurenz Albe <laurenz.albe@cybertec.at>
To: Tom Lane <tgl@sss.pgh.pa.us>, =?ISO-8859-1?Q?=C1lvaro?= Herrera
	 <alvherre@alvh.no-ip.org>
Cc: Virender Singla <virender.cse@gmail.com>,
 pgsql-bugs@lists.postgresql.org,  Aniket Jha <aniketkumarj@gmail.com>
Date: Fri, 21 Feb 2025 08:18:12 +0100
In-Reply-To: <2939991.1740089974@sss.pgh.pa.us>
References: <202502131716.7mgkcnrem2hn@alvherre.pgsql>
	 <2939991.1740089974@sss.pgh.pa.us>
User-Agent: Evolution 3.54.3 (3.54.3-1.fc41) 
MIME-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Archived-At: <https://www.postgresql.org/message-id/b0459688caa95c77f439bebf3b30341169986369.camel%40cybertec.at>
Precedence: bulk

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.