Date: Tue, 22 Jun 2021 12:34:52 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Stephen Frost <sfrost@snowman.net>,
	Alvaro Herrera <alvherre@alvh.no-ip.org>, exclusion@gmail.com,
	pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17062: Assert failed in RemoveRoleFromObjectPolicy() on
 DROP OWNED policy applied to duplicate role
Message-ID: <YNFaXAOshF14RMsa@paquier.xyz>
References: <202106181925.x6ka3pktyblk@alvherre.pgsql>
 <1285334.1624046402@sss.pgh.pa.us>
 <1573181.1624220108@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="xCkxcQcd6vGSHOtf"
Content-Disposition: inline
In-Reply-To: <1573181.1624220108@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/YNFaXAOshF14RMsa%40paquier.xyz>
Precedence: bulk


--xCkxcQcd6vGSHOtf
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Sun, Jun 20, 2021 at 04:15:08PM -0400, Tom Lane wrote:
> Here's a draft patch that gets rid of all the seems-to-me-unnecessary
> stuff in RemoveRoleFromObjectPolicy().  As I said before, the permission
> check seems misguided, the rebuild of non-shared dependencies is a waste
> of cycles, and the table locking is neither necessary nor well designed.
> In this version, if somebody manages to commit a concurrent table drop,
> policy change, etc, you just get a "tuple concurrently deleted" or
> equivalent failure.  That's much like most other DDL-conflict cases.

Hm.  This version still removes all the shared dependencies and
rebuilds them from scratch for each role.  Is that something you
intended to change?

> (I did not look to see if there's any documentation mentioning the
> existing permission check.  If there's not, I don't think we need any
> doc changes, since this seems to me to be strictly less surprising
> than the old behavior.)

Don't see any.

> There remains the question of whether we could preserve the policy
> in the edge case of deleting the last role by letting its polroles go
> to empty.  But that's clearly not going to be a back-patchable change,
> and I'm not terribly interested in working on it personally.

What would be the advantage of keeping the policy around anyway?  The
code behaves like that since its introduction.  If you decide to
change that, note that the current code would fail to empty
pg_policy.polroles as the array gets manipulated only when num_roles >
0.  So this could leave around policies with roles part of it even
after some DROP OWNED query.

-   if (!allowSystemTableMods && IsSystemRelation(rel))
-       ereport(ERROR,
-               (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-                errmsg("permission denied: \"%s\" is a system catalog",
-                       RelationGetRelationName(rel))));
-
-   if (!allowSystemTableMods && IsSystemRelation(rel))
-       ereport(ERROR,
-               (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-                errmsg("permission denied: \"%s\" is a system
-       catalog",
-                       RelationGetRelationName(rel))));
Right.  This looks like a copy-paste coming from
RangeVarCallbackForPolicy() and this removes the need to lock the
relation of a policy.
--
Michael

--xCkxcQcd6vGSHOtf
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmDRWlwACgkQnvQgOdby
QH20dg//SANxtEfSGareKRF+4WF/khBl/n4zkWrHB1cewTMRdDloIHKVUxI9Nk5M
w2Ab+EG1qXZQX9CHndWPYpdRVTzaNFCgXaS08eIi06ETOx8EjKKzdfVY3IBs81Lk
rLBbGtpZgOGzBCs+Bbmh5lJC5f/Zb6J7OaHK36G8m5M6lFbpHL0lcCyUGoN+qr7W
UFIkfIN50QZTeyoUI5a0JVbEt4T/OD6zKzUU86/dRJcq6TPjXa8DS0OuzbiQgEDT
39t+4SXWlHLR4ieaLUhT9h4BFNz8WMwcCixYV6g5zXHua71vpABOl7pHmrCbIKS7
zhuIKqX6SKo8dacmVzMJTw0Kyipr0aGdiNCQp0Ut/gNv1E0QJsf9FqLOXoxpfNY0
cYp/17DOhzMyHPn5Esj5Y4LTYLkxUBZHzy1wTQ5npafOIdc1K8qtgtUIT0Sz5WKF
AE8i6G6Tx1J2HWoh3Pc0xDmPeXuPcFXCxCPgG0gyZCuBfZcL8ArhJVfgJEti2TM6
EiyowQd8XL9vp7S8KjjWIbKOfokPsbGk2unqSPWTxjjJSdUDPNGKwxEsmAflvqDV
vNxIN9nbJaja3IS3nkLeo3S61KNFtTmXTL0AqmuutSixCBsL7B83+B35iES96NnE
GsV+pnOOYHz6rmc2dQvOIZbF5LBuipwXqx/Lg+pr7G6cKR1K92A=
=BZUB
-----END PGP SIGNATURE-----

--xCkxcQcd6vGSHOtf--