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
