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
signature.asc
Description: PGP signature
