Re: Major Version Upgrade failure due to orphan roles entries in catalog - Mailing list pgsql-bugs

=?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

From b8f0dff492cfe689e8516b0353d004f483838640 Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Thu, 20 Feb 2025 15:34:55 -0500
Subject: [PATCH v1 1/2] Avoid race condition between "GRANT role" and "DROP
 ROLE".

Concurrently dropping either the granted role or the grantee
does not stop GRANT from completing, instead resulting in a
dangling role reference in pg_auth_members.  That's relatively
harmless in the short run, but it does result in garbage
output from pg_dumpall and therefore problems for pg_upgrade.

This patch deals with the problem by adding the granted and grantee
roles as explicit shared dependencies of the pg_auth_members entry.
That's a bit indirect, but it works because the pg_shdepend code
applies the necessary locking and rechecking.

Commit 6566133c5 previously established similar handling for
the grantor column of pg_auth_members; it's not clear why it
didn't cover the other two role OID columns.

A side-effect of this approach is that DROP OWNED BY will now
drop pg_auth_members entries that mention the target role as
either the granted or grantee role.  That's clearly appropriate
for the grantee, and it doesn't seem too far out of line for
the granted role.  (One could argue that this makes DropRole's
code to auto-drop pg_auth_members entries unnecessary, but
I chose to leave it in place since perhaps some people's
workflows expect that to work without a DROP OWNED BY.)

Reported-by: Virender Singla <virender.cse@gmail.com>
Discussion: https://postgr.es/m/CAM6Zo8woa62ZFHtMKox6a4jb8qQ=w87R2L0K8347iE-juQL2EA@mail.gmail.com
Backpatch-through: TBD
---
 doc/src/sgml/ref/drop_owned.sgml         |  2 +-
 src/backend/commands/user.c              | 22 +++++++++++++----
 src/test/regress/expected/privileges.out | 30 ++++++++++++++++++++++++
 src/test/regress/sql/privileges.sql      | 15 ++++++++++++
 4 files changed, 63 insertions(+), 6 deletions(-)

diff --git a/doc/src/sgml/ref/drop_owned.sgml b/doc/src/sgml/ref/drop_owned.sgml
index efda01a39e..46e1c229ec 100644
--- a/doc/src/sgml/ref/drop_owned.sgml
+++ b/doc/src/sgml/ref/drop_owned.sgml
@@ -33,7 +33,7 @@ DROP OWNED BY { <replaceable class="parameter">name</replaceable> | CURRENT_ROLE
    database that are owned by one of the specified roles. Any
    privileges granted to the given roles on objects in the current
    database or on shared objects (databases, tablespaces, configuration
-   parameters) will also be revoked.
+   parameters, or other roles) will also be revoked.
   </para>
  </refsect1>

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
@@ -30,6 +30,7 @@
 #include "commands/defrem.h"
 #include "commands/seclabel.h"
 #include "commands/user.h"
+#include "lib/qunique.h"
 #include "libpq/crypt.h"
 #include "miscadmin.h"
 #include "storage/lmgr.h"
@@ -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. */
@@ -1904,7 +1905,8 @@ AddRoleMems(Oid currentUserId, const char *rolename, Oid roleid,
         else
         {
             Oid            objectId;
-            Oid           *newmembers = palloc(sizeof(Oid));
+            Oid           *newmembers = (Oid *) palloc(3 * sizeof(Oid));
+            int            nnewmembers;

             /*
              * The values for these options can be taken directly from 'popt'.
@@ -1946,12 +1948,22 @@ AddRoleMems(Oid currentUserId, const char *rolename, Oid roleid,
                                     new_record, new_record_nulls);
             CatalogTupleInsert(pg_authmem_rel, tuple);

-            /* updateAclDependencies wants to pfree array inputs */
-            newmembers[0] = grantorId;
+            /*
+             * Record dependencies on the roleid, member, and grantor, as if a
+             * pg_auth_members entry were an object ACL.
+             * updateAclDependencies() requires an input array that is
+             * palloc'd (it will free it), sorted, and de-duped.
+             */
+            newmembers[0] = roleid;
+            newmembers[1] = memberid;
+            newmembers[2] = grantorId;
+            qsort(newmembers, 3, sizeof(Oid), oid_cmp);
+            nnewmembers = qunique(newmembers, 3, sizeof(Oid), oid_cmp);
+
             updateAclDependencies(AuthMemRelationId, objectId,
                                   0, InvalidOid,
                                   0, NULL,
-                                  1, newmembers);
+                                  nnewmembers, newmembers);
         }

         /* CCI after each change, in case there are duplicates in list */
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index 6b01313101..a76256405f 100644
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -113,6 +113,36 @@ CREATE USER regress_priv_user2;
 CREATE USER regress_priv_user3;
 CREATE USER regress_priv_user4;
 CREATE USER regress_priv_user5;
+-- DROP OWNED should also act on granted and granted-to roles
+GRANT regress_priv_user1 TO regress_priv_user2;
+GRANT regress_priv_user2 TO regress_priv_user3;
+SELECT roleid::regrole, member::regrole FROM pg_auth_members
+  WHERE roleid IN ('regress_priv_user1'::regrole,'regress_priv_user2'::regrole)
+  ORDER BY roleid::regrole::text;
+       roleid       |       member
+--------------------+--------------------
+ regress_priv_user1 | regress_priv_user2
+ regress_priv_user2 | regress_priv_user3
+(2 rows)
+
+REASSIGN OWNED BY regress_priv_user2 TO regress_priv_user4;  -- no effect
+SELECT roleid::regrole, member::regrole FROM pg_auth_members
+  WHERE roleid IN ('regress_priv_user1'::regrole,'regress_priv_user2'::regrole)
+  ORDER BY roleid::regrole::text;
+       roleid       |       member
+--------------------+--------------------
+ regress_priv_user1 | regress_priv_user2
+ regress_priv_user2 | regress_priv_user3
+(2 rows)
+
+DROP OWNED BY regress_priv_user2;  -- removes both grants
+SELECT roleid::regrole, member::regrole FROM pg_auth_members
+  WHERE roleid IN ('regress_priv_user1'::regrole,'regress_priv_user2'::regrole)
+  ORDER BY roleid::regrole::text;
+ roleid | member
+--------+--------
+(0 rows)
+
 GRANT pg_read_all_data TO regress_priv_user6;
 GRANT pg_write_all_data TO regress_priv_user7;
 GRANT pg_read_all_settings TO regress_priv_user8 WITH ADMIN OPTION;
diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql
index 60e7443bf5..d195aaf137 100644
--- a/src/test/regress/sql/privileges.sql
+++ b/src/test/regress/sql/privileges.sql
@@ -90,6 +90,21 @@ CREATE USER regress_priv_user3;
 CREATE USER regress_priv_user4;
 CREATE USER regress_priv_user5;

+-- DROP OWNED should also act on granted and granted-to roles
+GRANT regress_priv_user1 TO regress_priv_user2;
+GRANT regress_priv_user2 TO regress_priv_user3;
+SELECT roleid::regrole, member::regrole FROM pg_auth_members
+  WHERE roleid IN ('regress_priv_user1'::regrole,'regress_priv_user2'::regrole)
+  ORDER BY roleid::regrole::text;
+REASSIGN OWNED BY regress_priv_user2 TO regress_priv_user4;  -- no effect
+SELECT roleid::regrole, member::regrole FROM pg_auth_members
+  WHERE roleid IN ('regress_priv_user1'::regrole,'regress_priv_user2'::regrole)
+  ORDER BY roleid::regrole::text;
+DROP OWNED BY regress_priv_user2;  -- removes both grants
+SELECT roleid::regrole, member::regrole FROM pg_auth_members
+  WHERE roleid IN ('regress_priv_user1'::regrole,'regress_priv_user2'::regrole)
+  ORDER BY roleid::regrole::text;
+
 GRANT pg_read_all_data TO regress_priv_user6;
 GRANT pg_write_all_data TO regress_priv_user7;
 GRANT pg_read_all_settings TO regress_priv_user8 WITH ADMIN OPTION;
--
2.43.5

From 937b1ebb7c8eeb022385c51ec13ea206bd97d2a3 Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Thu, 20 Feb 2025 17:07:34 -0500
Subject: [PATCH v1 2/2] Fix pg_dumpall to cope with dangling OIDs in
 pg_auth_members.

In view of the bug fixed by the previous patch, we need to allow
for the possibility of role OIDs in pg_auth_members that don't
exist in pg_authid.  As pg_dumpall stands, it will emit invalid
commands like 'GRANT foo TO ""', which causes pg_upgrade to fail.
Fix it to emit warnings and skip those GRANTs, instead.

There was some discussion of removing the problem by changing
dumpRoleMembership's query to use JOIN not LEFT JOIN, but that
would result in silently ignoring such entries.  It seems better
to produce a warning.

Reported-by: Virender Singla <virender.cse@gmail.com>
Discussion: https://postgr.es/m/CAM6Zo8woa62ZFHtMKox6a4jb8qQ=w87R2L0K8347iE-juQL2EA@mail.gmail.com
Backpatch-through: TBD
---
 src/bin/pg_dump/pg_dumpall.c | 66 +++++++++++++++++++++++++++++++-----
 1 file changed, 57 insertions(+), 9 deletions(-)

diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
index b993b05cc2..b9b22c47be 100644
--- a/src/bin/pg_dump/pg_dumpall.c
+++ b/src/bin/pg_dump/pg_dumpall.c
@@ -986,6 +986,13 @@ dumpRoleMembership(PGconn *conn)
                 total;
     bool        dump_grantors;
     bool        dump_grant_options;
+    int            i_role;
+    int            i_member;
+    int            i_grantor;
+    int            i_roleid;
+    int            i_memberid;
+    int            i_grantorid;
+    int            i_admin_option;
     int            i_inherit_option;
     int            i_set_option;

@@ -995,6 +1002,10 @@ dumpRoleMembership(PGconn *conn)
      * they didn't have ADMIN OPTION on the role, or a user that no longer
      * existed. To avoid dump and restore failures, don't dump the grantor
      * when talking to an old server version.
+     *
+     * Also, in older versions the roleid and/or member could be role OIDs
+     * that no longer exist.  If we find such cases, print a warning and skip
+     * the entry.
      */
     dump_grantors = (PQserverVersion(conn) >= 160000);

@@ -1006,8 +1017,10 @@ dumpRoleMembership(PGconn *conn)
     /* Generate and execute query. */
     printfPQExpBuffer(buf, "SELECT ur.rolname AS role, "
                       "um.rolname AS member, "
-                      "ug.oid AS grantorid, "
                       "ug.rolname AS grantor, "
+                      "a.roleid AS roleid, "
+                      "a.member AS memberid, "
+                      "a.grantor AS grantorid, "
                       "a.admin_option");
     if (dump_grant_options)
         appendPQExpBufferStr(buf, ", a.inherit_option, a.set_option");
@@ -1016,8 +1029,15 @@ dumpRoleMembership(PGconn *conn)
                       "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);
+                      "ORDER BY 1,2,3", role_catalog, role_catalog, role_catalog);
     res = executeQuery(conn, buf->data);
+    i_role = PQfnumber(res, "role");
+    i_member = PQfnumber(res, "member");
+    i_grantor = PQfnumber(res, "grantor");
+    i_roleid = PQfnumber(res, "roleid");
+    i_memberid = PQfnumber(res, "memberid");
+    i_grantorid = PQfnumber(res, "grantorid");
+    i_admin_option = PQfnumber(res, "admin_option");
     i_inherit_option = PQfnumber(res, "inherit_option");
     i_set_option = PQfnumber(res, "set_option");

@@ -1041,24 +1061,32 @@ dumpRoleMembership(PGconn *conn)
     total = PQntuples(res);
     while (start < total)
     {
-        char       *role = PQgetvalue(res, start, 0);
+        char       *role = PQgetvalue(res, start, i_role);
         int            i;
         bool       *done;
         int            remaining;
         int            prev_remaining = 0;
         rolename_hash *ht;

+        /* If we hit a null roleid, we're done (nulls sort to the end). */
+        if (PQgetisnull(res, start, i_role))
+        {
+            /* translator: %s represents a numeric role OID */
+            pg_log_warning("found dangling pg_auth_members entry for role %s",
+                           PQgetvalue(res, start, i_roleid));
+            break;
+        }
+
         /* All memberships for a single role should be adjacent. */
         for (end = start; end < total; ++end)
         {
             char       *otherrole;

-            otherrole = PQgetvalue(res, end, 0);
+            otherrole = PQgetvalue(res, end, i_role);
             if (strcmp(role, otherrole) != 0)
                 break;
         }

-        role = PQgetvalue(res, start, 0);
         remaining = end - start;
         done = pg_malloc0(remaining * sizeof(bool));
         ht = rolename_create(remaining, NULL);
@@ -1098,10 +1126,30 @@ dumpRoleMembership(PGconn *conn)
                 if (done[i - start])
                     continue;

-                member = PQgetvalue(res, i, 1);
-                grantorid = PQgetvalue(res, i, 2);
-                grantor = PQgetvalue(res, i, 3);
-                admin_option = PQgetvalue(res, i, 4);
+                /* Complain about, then ignore, entries with dangling OIDs. */
+                if (PQgetisnull(res, i, i_member))
+                {
+                    /* translator: %s represents a numeric role OID */
+                    pg_log_warning("found dangling pg_auth_members entry for role %s",
+                                   PQgetvalue(res, i, i_memberid));
+                    done[i - start] = true;
+                    --remaining;
+                    continue;
+                }
+                if (PQgetisnull(res, i, i_grantor))
+                {
+                    /* translator: %s represents a numeric role OID */
+                    pg_log_warning("found dangling pg_auth_members entry for role %s",
+                                   PQgetvalue(res, i, i_grantorid));
+                    done[i - start] = true;
+                    --remaining;
+                    continue;
+                }
+
+                member = PQgetvalue(res, i, i_member);
+                grantor = PQgetvalue(res, i, i_grantor);
+                grantorid = PQgetvalue(res, i, i_grantorid);
+                admin_option = PQgetvalue(res, i, i_admin_option);
                 if (dump_grant_options)
                     set_option = PQgetvalue(res, i, i_set_option);

--
2.43.5