Re: Change RangeVarGetRelidExtended() to take flags argument?

Hi Everyone,

On 2018-03-22 15:45:23 +0000, Bossart, Nathan wrote:
> Here is a set of patches for this approach.

To me this looks like a reasonable approach that'd solve the VACUUM
use-case. I think we can live with the API breakage, but I'd like to
have some more comments? Pertinent parts quoted below.

- Andres

> diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
> index 52dd400..edd229d 100644
> --- a/src/backend/catalog/namespace.c
> +++ b/src/backend/catalog/namespace.c
> @@ -206,23 +206,24 @@ static bool MatchNamedCall(HeapTuple proctup, int nargs, List *argnames,
> * Given a RangeVar describing an existing relation,
> * select the proper namespace and look up the relation OID.
> *
> - * If the schema or relation is not found, return InvalidOid if missing_ok
> - * = true, otherwise raise an error.
> + * If the schema or relation is not found, return InvalidOid if the flags contain
> + * RELID_MISSING_OK, otherwise raise an error.
> *
> - * If nowait = true, throw an error if we'd have to wait for a lock.
> + * If the flags contain RELID_NOWAIT, throw an error if we'd have to wait for a lock.
> *
> * Callback allows caller to check permissions or acquire additional locks
> * prior to grabbing the relation lock.
> */
> Oid
> RangeVarGetRelidExtended(const RangeVar *relation, LOCKMODE lockmode,
> - bool missing_ok, bool nowait,
> + int flags,
> RangeVarGetRelidCallback callback, void *callback_arg)
> {
> uint64 inval_count;
> Oid relId;
> Oid oldRelId = InvalidOid;
> bool retry = false;
> + bool missing_ok = ((flags & RELID_MISSING_OK) != 0);
>
> /*
> * We check the catalog name and then ignore it.
> @@ -361,7 +362,7 @@ RangeVarGetRelidExtended(const RangeVar *relation, LOCKMODE lockmode,
> */
> if (!OidIsValid(relId))
> AcceptInvalidationMessages();
> - else if (!nowait)
> + else if ((flags & RELID_NOWAIT) == 0)
> LockRelationOid(relId, lockmode);
> else if (!ConditionalLockRelationOid(relId, lockmode))
> {

...

> From ace71981220b8cae863e8a99b5b14b85cf19ba90 Mon Sep 17 00:00:00 2001
> From: Nathan Bossart <bossartn(at)amazon(dot)com>
> Date: Wed, 21 Mar 2018 22:00:24 +0000
> Subject: [PATCH v1 2/2] Add skip-locked option to RangeVarGetRelidExtended().
>
> ---
> src/backend/catalog/namespace.c | 15 ++++++++++++++-
> src/include/catalog/namespace.h | 3 ++-
> 2 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
> index edd229d..72e069e 100644
> --- a/src/backend/catalog/namespace.c
> +++ b/src/backend/catalog/namespace.c
> @@ -210,6 +210,13 @@ static bool MatchNamedCall(HeapTuple proctup, int nargs, List *argnames,
> * RELID_MISSING_OK, otherwise raise an error.
> *
> * If the flags contain RELID_NOWAIT, throw an error if we'd have to wait for a lock.
> + * If the flags contain RELID_SKIP_LOCKED, return InvalidOid if we'd have to wait for
> + * a lock. The flags cannot contain both RELID_NOWAIT and RELID_SKIP_LOCKED
> + * together.
> + *
> + * Note that if RELID_MISSING_OK and RELID_SKIP_LOCKED are both specified, a return
> + * value of InvalidOid could either mean the relation is missing or it could not be
> + * locked.
> *
> * Callback allows caller to check permissions or acquire additional locks
> * prior to grabbing the relation lock.
> @@ -225,6 +232,9 @@ RangeVarGetRelidExtended(const RangeVar *relation, LOCKMODE lockmode,
> bool retry = false;
> bool missing_ok = ((flags & RELID_MISSING_OK) != 0);
>
> + /* verify that conflicting options are not specified */
> + Assert((flags & (RELID_NOWAIT | RELID_SKIP_LOCKED)) != (RELID_NOWAIT | RELID_SKIP_LOCKED));
> +
> /*
> * We check the catalog name and then ignore it.
> */
> @@ -362,10 +372,13 @@ RangeVarGetRelidExtended(const RangeVar *relation, LOCKMODE lockmode,
> */
> if (!OidIsValid(relId))
> AcceptInvalidationMessages();
> - else if ((flags & RELID_NOWAIT) == 0)
> + else if ((flags & (RELID_NOWAIT | RELID_SKIP_LOCKED)) == 0)
> LockRelationOid(relId, lockmode);
> else if (!ConditionalLockRelationOid(relId, lockmode))
> {
> + if ((flags & RELID_SKIP_LOCKED) != 0)
> + return InvalidOid;
> +
> if (relation->schemaname)
> ereport(ERROR,
> (errcode(ERRCODE_LOCK_NOT_AVAILABLE),