Feedback-ID: i0fe9450f:Fastmail
Date: Wed, 10 Jul 2024 10:25:49 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Joel Jacobson <joel@compiler.org>
Cc: Tom Lane <tgl@sss.pgh.pa.us>,
	Postgres hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: pgsql: Add pg_get_acl() to get the ACL for a database object
Message-ID: <Zo3jHUYX9juFYtON@paquier.xyz>
References: <E1sPHWu-000GPc-0z@gemulon.postgresql.org>
 <3408967.1720107891@sss.pgh.pa.us>
 <f2688fb7-add6-43da-b1df-97312a451893@app.fastmail.com>
 <ZoctyZzE3UlUSer_@paquier.xyz>
 <f2539bff-64be-47f0-9f0b-df85d3cc0432@app.fastmail.com>
 <ZoukijRBef4m-VMO@paquier.xyz>
 <aa46ce00-17ad-4a87-938f-c66196800129@app.fastmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="2q4Jn0aFsftoO1Ci"
Content-Disposition: inline
In-Reply-To: <aa46ce00-17ad-4a87-938f-c66196800129@app.fastmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/Zo3jHUYX9juFYtON%40paquier.xyz>
Precedence: bulk


--2q4Jn0aFsftoO1Ci
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Mon, Jul 08, 2024 at 11:55:28AM +0200, Joel Jacobson wrote:
> Thanks, nice simplifications.
> I agree the tests you removed are not that interesting.
>=20
> Looks good to me.

On second review, I have spotted a use-after-free for the case of an
attribute ACL in both v1 and v2, as we would finish by releasing the
syscache entry before returning the ACL datum.  For builds with the
flags -DRELCACHE_FORCE_RELEASE -DCATCACHE_FORCE_RELEASE, like the
buildfarm member prion, this would cause lookup failures in the
function when building the result because the datum is borked.

At the end, I have settled down on using SearchSysCacheCopyAttNum().
It requires a heap_freetuple(), which we already don't care much about
in the existing callers of get_catalog_object_by_oid() because these
are short-lived, with bonus points because this routine uses
SearchSysCacheAttNum() that has a check for attisdropped, making the=20
code of pg_get_acl() even simpler.

The usual trick I use to check a fix with this configuration is to use
two builds because initdb is insanely slow if caches are released: one
with the flags and one without them.  I have used the build without
the flags to initialize a cluster with the objects and their ACLs.
Then, the second build is used only to execute all the scenarios of
pg_get_acl(), which are much cheaper to execute.
--
Michael

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmaN4x0ACgkQnvQgOdby
QH2w+Q//QFFoQpwDE+s/uUGu4RJa8Ysl4VjhfKCpf7Ffk+g5M6OCXnWetUHQbcyy
77PhPt9exvgPiP2ntRlvlpI2TgfGxiVv0jBAB/OJpf0mfghu09mSKvX7Z73QJ/Qy
Nb2g93i/hL9nx2UjdwzlttGeV9QeGGVondANtQqSw7S8s1Ze6OIUoNP3TmeE1Lsj
aDSDgKeOyIB7DWHoe54v2cP3qvIcO8tUeMOTJ4SKgVe2l4X9bE51aHA5jO4HdGDm
2RIC3gSnnHvFJJOpdjwikLwbdKX37zZewqAeYraDkeZw6x3eJEVsBvouOOvHiaH9
37gUZ9KZqzw9lKbramw6FHWF9xrWM9WC2Cm8gnvZZEwQjSRbRTnqLQSEZigXzE8u
yxxH09Z7+ylg7sPPDM4IolH3dZb81tYtV51MdYAcKsC3WbWj+xybYFUUUmqLTg4l
kG5xmRYlzr0lFP0O3z6PbQX9g+NMbGhieC7NbNOKxQzQ0snj7ao+TdPMm7BsloiY
NzoRkYG6wBUtHTFDiaIDMdtZWINK8Ba/iHfaO5/d+rfSrDZXzEw41Se6q6yJi2Nj
7zq/2zhrrMJHk/YxqJVe4zdhoRA0kNpycUTJtKxA7OsYrP+hvw7wkX0iOBSn3W3r
EtTX4ruTkNTLKiqh3cKe8bM2H2AD1k+KiBtspQ2QJUxCo/zc61g=
=Fgwv
-----END PGP SIGNATURE-----

--2q4Jn0aFsftoO1Ci--