From: Michael Paquier <michael@paquier.xyz>
To: Joel Jacobson <joel@compiler.org>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Postgres hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: pgsql: Add pg_get_acl() to get the ACL for a database object
Date: Wed, 10 Jul 2024 10:25:49 +0900
Message-ID: <Zo3jHUYX9juFYtON@paquier.xyz> (raw)
In-Reply-To: <aa46ce00-17ad-4a87-938f-c66196800129@app.fastmail.com>
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>
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.
>
> 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
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
Attachments:
signature.asc (application/pgp-signature, 832B) download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-committers@postgresql.org
Cc: michael@paquier.xyz, joel@compiler.org, tgl@sss.pgh.pa.us, pgsql-hackers@lists.postgresql.org
Subject: Re: pgsql: Add pg_get_acl() to get the ACL for a database object
In-Reply-To: <Zo3jHUYX9juFYtON@paquier.xyz>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox