From: Tom Lane <tgl@sss.pgh.pa.us>
To: Stephen Frost <sfrost@snowman.net>
cc: Michael Paquier <michael@paquier.xyz>, exclusion@gmail.com,
        pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17062: Assert failed in RemoveRoleFromObjectPolicy() on DROP
 OWNED policy applied to duplicate role
In-reply-to: 
 <CAOuzzgpmUhyS4T3bG7+rtF7ZLPd0kp+-piCengitHdMN1-PHLg@mail.gmail.com>
References: <17062-11f471ae3199ca23@postgresql.org>
 <1066940.1623964553@sss.pgh.pa.us> <1067500.1623965562@sss.pgh.pa.us>
 <1068096.1623966678@sss.pgh.pa.us> <YMwUWpBb26FkNBqW@paquier.xyz>
 <1233107.1624040308@sss.pgh.pa.us>
 <CAOuzzgo_-Vz7OfavbBZuf5uDCh986Q2wDOjWUtvs7YkTvdKKEw@mail.gmail.com>
 <1233946.1624041420@sss.pgh.pa.us>
 <CAOuzzgpmUhyS4T3bG7+rtF7ZLPd0kp+-piCengitHdMN1-PHLg@mail.gmail.com>
Comments: In-reply-to Stephen Frost <sfrost@snowman.net>
	message dated "Fri, 18 Jun 2021 14:45:43 -0400"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <1284970.1624045917.0@sss.pgh.pa.us>
Date: Fri, 18 Jun 2021 15:53:14 -0400
Message-ID: <1285021.1624045994@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/1285021.1624045994%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1284970.1624045917.1@sss.pgh.pa.us>

Here's a stop-the-bleeding patch that just gets rid of the assertion
failure and the tuple-updated-by-self problem.  I think this is
reasonable to slip in for beta2, although the other issues we
mentioned seem to require more thought.

(For ease of review, I've not re-pgindented.)

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="fix-RemoveRoleFromObjectPolicy-1.patch";
	charset="us-ascii"
Content-ID: <1284970.1624045917.2@sss.pgh.pa.us>
Content-Description: fix-RemoveRoleFromObjectPolicy-1.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c
index 5d469309ce..abb6c66316 100644
--- a/src/backend/commands/policy.c
+++ b/src/backend/commands/policy.c
@@ -18,6 +18,7 @@
 #include "access/relation.h"
 #include "access/sysattr.h"
 #include "access/table.h"
+#include "access/xact.h"
 #include "catalog/catalog.h"
 #include "catalog/dependency.h"
 #include "catalog/indexing.h"
@@ -425,7 +426,6 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oi=
d policy_id)
 	Oid			relid;
 	Relation	rel;
 	ArrayType  *policy_roles;
-	int			num_roles;
 	Datum		roles_datum;
 	bool		attr_isnull;
 	bool		noperm =3D true;
@@ -481,11 +481,6 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, O=
id policy_id)
 =

 	policy_roles =3D DatumGetArrayTypePCopy(roles_datum);
 =

-	/* We should be removing exactly one entry from the roles array */
-	num_roles =3D ARR_DIMS(policy_roles)[0] - 1;
-
-	Assert(num_roles >=3D 0);
-
 	/* Must own relation. */
 	if (pg_class_ownercheck(relid, GetUserId()))
 		noperm =3D false;			/* user is allowed to modify this policy */
@@ -497,15 +492,13 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, =
Oid policy_id)
 						NameStr(((Form_pg_policy) GETSTRUCT(tuple))->polname),
 						RelationGetRelationName(rel))));
 =

-	/*
-	 * If multiple roles exist on this policy, then remove the one we were
-	 * asked to and leave the rest.
-	 */
-	if (!noperm && num_roles > 0)
+	/* Skip the rest if we lack permissions. */
+	if (!noperm)
 	{
 		int			i,
 					j;
 		Oid		   *roles =3D (Oid *) ARR_DATA_PTR(policy_roles);
+		int			num_roles =3D ARR_DIMS(policy_roles)[0];
 		Datum	   *role_oids;
 		char	   *qual_value;
 		Node	   *qual_expr;
@@ -582,16 +575,22 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, =
Oid policy_id)
 		else
 			with_check_qual =3D NULL;
 =

-		/* Rebuild the roles array to then update the pg_policy tuple with */
+		/*
+		 * Rebuild the roles array, without any mentions of the target role.
+		 * Ordinarily there'd be exactly one, but we must cope with duplicate
+		 * mentions, since CREATE/ALTER POLICY historically have allowed that.
+		 */
 		role_oids =3D (Datum *) palloc(num_roles * sizeof(Datum));
-		for (i =3D 0, j =3D 0; i < ARR_DIMS(policy_roles)[0]; i++)
-			/* Copy over all of the roles which are not the one being removed */
+		for (i =3D 0, j =3D 0; i < num_roles; i++)
+		{
 			if (roles[i] !=3D roleid)
 				role_oids[j++] =3D ObjectIdGetDatum(roles[i]);
+		}
+		num_roles =3D j;
 =

-		/* We should have only removed the one role */
-		Assert(j =3D=3D num_roles);
-
+		/* If no roles remain, we have to drop the policy instead. */
+		if (num_roles > 0)
+		{
 		/* This is the array for the new tuple */
 		role_ids =3D construct_array(role_oids, num_roles, OIDOID,
 								   sizeof(Oid), true, TYPALIGN_INT);
@@ -646,8 +645,15 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, O=
id policy_id)
 =

 		heap_freetuple(new_tuple);
 =

+		/* Make updates visible */
+		CommandCounterIncrement();
+
 		/* Invalidate Relation Cache */
 		CacheInvalidateRelcache(rel);
+
+		/* Report that policy should not be dropped */
+		noperm =3D true;
+	}
 	}
 =

 	/* Clean up. */
@@ -657,7 +663,7 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oi=
d policy_id)
 =

 	table_close(pg_policy_rel, RowExclusiveLock);
 =

-	return (noperm || num_roles > 0);
+	return noperm;
 }
 =

 /*

------- =_aaaaaaaaaa0--