MIME-Version: 1.0
References: <202502131716.7mgkcnrem2hn@alvherre.pgsql> <2939991.1740089974@sss.pgh.pa.us>
In-Reply-To: <2939991.1740089974@sss.pgh.pa.us>
From: Robert Haas <robertmhaas@gmail.com>
Date: Wed, 25 Feb 2026 08:59:04 -0500
Message-ID: <CA+TgmoauoiW4ydDhdrseg+DD4Kwha=+TSZp18BrJeHKx3o1Fdw@mail.gmail.com>
Subject: Re: Major Version Upgrade failure due to orphan roles entries in catalog
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: =?UTF-8?Q?=C3=81lvaro_Herrera?= <alvherre@alvh.no-ip.org>, 
	Virender Singla <virender.cse@gmail.com>, pgsql-bugs@lists.postgresql.org, 
	Aniket Jha <aniketkumarj@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CA%2BTgmoauoiW4ydDhdrseg%2BDD4Kwha%3D%2BTSZp18BrJeHKx3o1Fdw%40mail.gmail.com>
Precedence: bulk

On Thu, Feb 20, 2025 at 5:19=E2=80=AFPM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 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 recently learned of a case where this commit caused role grants to
be erroneously emitted from the output of pg_dumpall. In the case in
question, a v16 pg_dumpall was used against an older server.  Hence,
dump_grantors was false, and any generated GRANT commands would not
have included in the grantor anyway. Nevertheless, this logic caused
those grants to be skipped altogether:

+                if (PQgetisnull(res, i, i_grantor))
+                {
+                    /* translator: %s represents a numeric role OID */
+                    pg_log_warning("found orphaned pg_auth_members
entry for role %s",
+                                   PQgetvalue(res, i, i_grantorid));
+                    done[i - start] =3D true;
+                    --remaining;
+                    continue;
+                }

I don't think this logic makes sense. In pre-16 releases, we don't
even try to maintain the grantor field properly. Consider this test
case:

create role foo;
create role bar;
create role baz createrole;
set role baz;
grant foo to bar;
reset role;
drop role baz;

If you do this on v15 and then run v15's pg_dumpall, it will dump
"GRANT foo to bar", with no GRANTOR clause due to the PQgetisnull()
gating that logic. v16's pg_dumpall will dump nothing and emit a
warning instead. Arguably, pre-v16 pg_dumpall shouldn't EVER be
dumping the grantor since the grantorid could be an old role OID that
has been recycled for a new role, and relying on that for anything
security-critical seems like a mistake, but that behavior is also
longstanding. But omitting the grant altogether seems like an
overreaction. I understand that we need to do that when the *member*
is invalid, of course; in that case, there's no alternative. But
that's not the case for the grantor.

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