Skip to content

Commit

Permalink
Harden has_xxx_privilege() functions against concurrent object drops.
Browse files Browse the repository at this point in the history
The versions of these functions that accept object OIDs are supposed
to return NULL, rather than failing, if the target object has been
dropped.  This makes it safe(r) to use them in queries that scan
catalogs, since the functions will be applied to objects that are
visible in the query's snapshot but might now be gone according to
the catalog snapshot.  In most cases we implemented this by doing
a SearchSysCacheExists test and assuming that if that succeeds, we
can safely invoke the appropriate aclchk.c function, which will
immediately re-fetch the same syscache entry.  It was argued that
if the existence test succeeds then the followup fetch must succeed
as well, for lack of any intervening AcceptInvalidationMessages call.

Alexander Lakhin demonstrated that this is not so when
CATCACHE_FORCE_RELEASE is enabled: the syscache entry will be forcibly
dropped at the end of SearchSysCacheExists, and then it is possible
for the catalog snapshot to get advanced while re-fetching the entry.
Alexander's test case requires the operation to happen inside a
parallel worker, but that seems incidental to the fundamental problem.
What remains obscure is whether there is a way for this to happen in a
non-debug build.  Nonetheless, CATCACHE_FORCE_RELEASE is a very useful
test methodology, so we'd better make the code safe for it.

After some discussion we concluded that the most future-proof fix
is to give up the assumption that checking SearchSysCacheExists can
guarantee success of a later fetch.  At best that assumption leads
to fragile code --- for example, has_type_privilege appears broken
for array types even if you believe the assumption holds.  And it's
not even particularly efficient.

There had already been some work towards extending the aclchk.c
APIs to include "is_missing" output flags, so this patch extends
that work to cover all the aclchk.c functions that are used by the
has_xxx_privilege() functions.  (This allows getting rid of some
ad-hoc decisions about not throwing errors in certain places in
aclchk.c.)

In passing, this fixes the has_sequence_privilege() functions to
provide the same guarantees as their cousins: for some reason the
SearchSysCacheExists tests never got added to those.

There is more work to do to remove the unsafe coding pattern with
SearchSysCacheExists in other places, but this is a pretty
self-contained patch so I'll commit it separately.

Per bug #18014 from Alexander Lakhin.  Given the lack of hard evidence
that there's a bug in non-debug builds, I'm content to fix this only
in HEAD.  (Perhaps we should clean up the has_sequence_privilege()
oversight in the back branches, but in the absence of field complaints
I'm not too excited about that either.)

Discussion: https://postgr.es/m/18014-28c81cb79d44295d@postgresql.org
  • Loading branch information
tglsfdc committed Oct 14, 2023
1 parent 22655aa commit 403ac22
Show file tree
Hide file tree
Showing 3 changed files with 402 additions and 160 deletions.

0 comments on commit 403ac22

Please sign in to comment.