Re: Change RangeVarGetRelidExtended() to take flags argument? - Mailing list pgsql-hackers

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@amazon.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),