Date: Fri, 18 Jun 2021 12:34:50 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Stephen Frost <sfrost@snowman.net>, 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: <YMwUWpBb26FkNBqW@paquier.xyz>
References: <17062-11f471ae3199ca23@postgresql.org>
 <1066940.1623964553@sss.pgh.pa.us>
 <1067500.1623965562@sss.pgh.pa.us>
 <1068096.1623966678@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="kWgt+16HY5wA7vXq"
Content-Disposition: inline
In-Reply-To: <1068096.1623966678@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/YMwUWpBb26FkNBqW%40paquier.xyz>
Precedence: bulk


--kWgt+16HY5wA7vXq
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Thu, Jun 17, 2021 at 05:51:18PM -0400, Tom Lane wrote:
> While I'm whining ... that function's permissions checks seem
> completely out of line too.  How is it that, if I have the right
> to drop some role, I lose that right if the role is mentioned in
> a policy of some relation I don't own?  It feels like this function
> was written by copy-and-pasting a whole bunch of irrelevant logic.

Hm.  Wouldn't it be better to do something similar to 21378e1f where
we ensure that there are no duplicated role OIDs in the catalogs to
begin with, letting the drop code as it is now?  The array build
happens in policy_role_list_to_array() for CREATE/ALTER POLICY, so
that's more than just a CCI.  And I think that making the DROP OWNED
BY code more reliable with drops of the same role makes the handling
of shdepend entries a bit shaky.  Back-branches would need to handle a
multi-role logic, now nobody has complained either about this bug
issue for years.

> * Locking.  It acquires lock on the policy's relation only after
> it looks up the pg_policy entry.  By that point the entry could
> be gone or modified.

Yeah.  The policy could be gone, or its relations, so it is possible
to hit two elog() here.  rename_policy() does not take a lock on the
policy's relation for one.

> * Why is it expensively reconstructing the dependencies of the
> policy expressions?  Those aren't going to be changed by this
> operation.  AFAICS it ought to be sufficient to remove and
> rebuild the policy's shared dependencies.

For the shdepend entries dropped and then recreated from scratch, I
agree that this is a waste.  That would be simple enough to fix if we
make shdepDropDependency() available, and simpler to call it in loop
with the roles dropped.
--
Michael

--kWgt+16HY5wA7vXq
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmDMFFoACgkQnvQgOdby
QH34Kg/+MKv0pjIx3TfE+VUBWeIhot39+0wg7uts2KBjpmO8WdZqa/HfmKstC3An
9H00CqA5r/0mVW85r2iKwkdOKJyr4GfxcxWlj2t2VLCBRKQpJEUIKsq6vl36+PnD
1OnaP38NpZhn348SRnfadLEJZLraGePDznDAlScM8ncYxSSgk+wpYgQQdHirnzbn
yeg/j/muqtB7hO7wFvABvldkd5E9JwkyrPH7bX2GAmafG3oAIxfp/bT2SSAlNnTq
raGJo3VTZX9T/gBSd2tZW9iHCd7+g2WvKAaTj/1fCDTvtZilJqxuryjXbNhpcaoJ
7fLWeoPmngUS5JKXDN+bszm+fILhHgE7YMVobdWV5+N/ScUsU7jBNo0et7ZlNtw4
lDuDKH5iy7nLW8fDIERj29P4hNMO3PYn/X4UOmBa4x4MvAwtadzLeFvzyFa0jWvE
ZfK73Xs9l6wKSAFwOCrBAnaAQe2UaBdmeSIPCvwUEeZTOlxalJI1Tt6eDYHd6Qx7
RSjtrgxrnLjgo5ZkqUfCigLSDhKaxcvly3WqvQ9W9/HL5iGgj3RQzvvxgfrOastZ
mpKljvu8vZcTZ2tXHmc2DuRqlUHGTJnAKrWktNmcKGrZHsK0f4LKIzsAQag5odGs
iR7r9AZj3nR4qKcD0wMUhwG4s9bCX7shmX9WFZbXp+CUVc1a+R8=
=UGic
-----END PGP SIGNATURE-----

--kWgt+16HY5wA7vXq--