- Notifications
You must be signed in to change notification settings - Fork4.9k
Commit403ac22
committed
Harden has_xxx_privilege() functions against concurrent object drops.
The versions of these functions that accept object OIDs are supposedto return NULL, rather than failing, if the target object has beendropped. This makes it safe(r) to use them in queries that scancatalogs, since the functions will be applied to objects that arevisible in the query's snapshot but might now be gone according tothe catalog snapshot. In most cases we implemented this by doinga SearchSysCacheExists test and assuming that if that succeeds, wecan safely invoke the appropriate aclchk.c function, which willimmediately re-fetch the same syscache entry. It was argued thatif the existence test succeeds then the followup fetch must succeedas well, for lack of any intervening AcceptInvalidationMessages call.Alexander Lakhin demonstrated that this is not so whenCATCACHE_FORCE_RELEASE is enabled: the syscache entry will be forciblydropped at the end of SearchSysCacheExists, and then it is possiblefor the catalog snapshot to get advanced while re-fetching the entry.Alexander's test case requires the operation to happen inside aparallel worker, but that seems incidental to the fundamental problem.What remains obscure is whether there is a way for this to happen in anon-debug build. Nonetheless, CATCACHE_FORCE_RELEASE is a very usefultest methodology, so we'd better make the code safe for it.After some discussion we concluded that the most future-proof fixis to give up the assumption that checking SearchSysCacheExists canguarantee success of a later fetch. At best that assumption leadsto fragile code --- for example, has_type_privilege appears brokenfor array types even if you believe the assumption holds. And it'snot even particularly efficient.There had already been some work towards extending the aclchk.cAPIs to include "is_missing" output flags, so this patch extendsthat work to cover all the aclchk.c functions that are used by thehas_xxx_privilege() functions. (This allows getting rid of somead-hoc decisions about not throwing errors in certain places inaclchk.c.)In passing, this fixes the has_sequence_privilege() functions toprovide the same guarantees as their cousins: for some reason theSearchSysCacheExists tests never got added to those.There is more work to do to remove the unsafe coding pattern withSearchSysCacheExists in other places, but this is a prettyself-contained patch so I'll commit it separately.Per bug #18014 from Alexander Lakhin. Given the lack of hard evidencethat there's a bug in non-debug builds, I'm content to fix this onlyin HEAD. (Perhaps we should clean up the has_sequence_privilege()oversight in the back branches, but in the absence of field complaintsI'm not too excited about that either.)Discussion:https://postgr.es/m/18014-28c81cb79d44295d@postgresql.org1 parent22655aa commit403ac22