Re: Change RangeVarGetRelidExtended() to take flags argument?

[Bossart, Nathan]

On 3/30/18, 7:09 PM, "Andres Freund"  wrote:
> Pushed.

Thanks!

Nathan

Re: Change RangeVarGetRelidExtended() to take flags argument?

[Andres Freund]

On 2018-03-30 11:37:19 +0900, Michael Paquier wrote:
> On Thu, Mar 29, 2018 at 05:15:14PM -0700, Andres Freund wrote:
> > On 2018-03-22 15:45:23 +, 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.
> 
> I just looked at the proposed patches, that looks like a sensible
> approach.
> 
> >> +  /* verify that conflicting options are not specified */
> >> +  Assert((flags & (RELID_NOWAIT | RELID_SKIP_LOCKED)) != (RELID_NOWAIT | 
> >> RELID_SKIP_LOCKED));
> >> +
> 
> This is more readable as follows I think:
> Assert((flags & RELID_NOWAIT) == 0 || (flags & RELID_SKIP_LOCKED) == 0);

I found Assert(!((flags & RELID_NOWAIT) && (flags & RELID_SKIP_LOCKED)))
easier. I've removed a number of of parens from, in my opinion,
over-parenthized statements.

On 2018-03-30 14:55:45 -0700, Andres Freund wrote:
> On 2018-03-30 17:08:26 +, Bossart, Nathan wrote:
> > +typedef enum RelidOption
> > +{
> > +   RELID_MISSING_OK = 1 << 0,  /* don't error if relation doesn't 
> > exist */
> > +   RELID_NOWAIT = 1 << 1   /* error if relation cannot be locked */
> > +} RelidOption;
> 
> I don't like the Relid prefix here. RangeVarGetRelid deals with
> *rangevars*, and returns a relation oid. ISTM it'd be more accurate to
> call this RV_* or RVID_*. Counterarguments, preferences?

Went with RVR_*

Pushed.

Greetings,

Andres Freund

Re: Change RangeVarGetRelidExtended() to take flags argument?

[Andres Freund]

Hi,

On 2018-03-30 17:08:26 +, Bossart, Nathan wrote:
> +typedef enum RelidOption
> +{
> + RELID_MISSING_OK = 1 << 0,  /* don't error if relation doesn't 
> exist */
> + RELID_NOWAIT = 1 << 1   /* error if relation cannot be locked */
> +} RelidOption;

I don't like the Relid prefix here. RangeVarGetRelid deals with
*rangevars*, and returns a relation oid. ISTM it'd be more accurate to
call this RV_* or RVID_*. Counterarguments, preferences?

Other than that this looks good, and I plan to push it later today.

Andres

Re: Change RangeVarGetRelidExtended() to take flags argument?

[Michael Paquier]

On Thu, Mar 29, 2018 at 05:15:14PM -0700, Andres Freund wrote:
> On 2018-03-22 15:45:23 +, 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.

I just looked at the proposed patches, that looks like a sensible
approach.

>> +/* verify that conflicting options are not specified */
>> +Assert((flags & (RELID_NOWAIT | RELID_SKIP_LOCKED)) != (RELID_NOWAIT | 
>> RELID_SKIP_LOCKED));
>> +

This is more readable as follows I think:
Assert((flags & RELID_NOWAIT) == 0 || (flags & RELID_SKIP_LOCKED) == 0);

>>  /*
>>   * 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),

That looks correct to me.

I would suggest to use uint8, uint16 or uint32 for the flags of
RangeVarGetRelidExtended instead of int.  That's the practice in other
similar APIs with control flags.

+ * 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.
Perhaps we could generate a DEBUG message to help with making the
difference for developers?

So, +1 to simplify and break the interface.
--
Michael


signature.asc
Description: PGP signature

Re: Change RangeVarGetRelidExtended() to take flags argument?

[Andres Freund]

Hi Everyone,

On 2018-03-22 15:45:23 +, 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;
>   boolretry = false;
> + boolmissing_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 
> Date: Wed, 21 Mar 2018 22:00:24 +
> 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,
>   boolretry = false;
>   boolmissing_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,
>   
> 

Re: Change RangeVarGetRelidExtended() to take flags argument?

[Bossart, Nathan]

On 3/5/18, 7:08 PM, "Andres Freund"  wrote:
> On 2018-03-05 19:57:44 -0500, Tom Lane wrote:
>> Andres Freund  writes:
>>> One wrinkle in that plan is that it'd not be trivial to discern whether
>>> a lock couldn't be acquired or whether the object vanished.  I don't
>>> really have good idea how to tackle that yet.
>> Do we really care which case applies?
>
> I think there might be cases where we do. As expand_vacuum_rel()
> wouldn't use missing_ok = true, it'd not be an issue for this specific
> callsite though.

I think it might be enough to simply note the ambiguity of returning
InvalidOid when skip-locked and missing-ok are both specified.  Even
if RangeVarGetRelidExtended() did return whether skip-locked or
missing-ok applied, such a caller would likely not be able to trust
it anyway, as no lock would be held.

Nathan

Re: Change RangeVarGetRelidExtended() to take flags argument?

[Michael Paquier]

On Mon, Mar 05, 2018 at 05:21:11PM -0800, Andres Freund wrote:
> Hi,
> 
> On 2018-03-06 10:17:49 +0900, Michael Paquier wrote:
> > On Mon, Mar 05, 2018 at 05:06:59PM -0800, Andres Freund wrote:
> > > Yea, that's a concern. OTOH, it doesn't seem nice to grow duplicates of
> > > similar code. It'd not be too hard to move RangeVarGetRelidExtended()
> > > code into RangeVarGetRelidInternal() and add
> > > RangeVarGetRelidTryLock(). Not sure if that's any better.  Or just add
> > > RangeVarGetRelidExtended2() :)
> > 
> > FWIW, it would have been nice to switch RangeVarGetRelidExtended
> 
> What exactly do you mean with the paste tense here?

s/paste/past/?  I mean "When RangeVarGetRelidExtended was created."

>> so as it handles a set of uint8 flags as one of its arguments.
> 
> Right, that's what I was proposing. Although I'd just go for uint32,
> there's no benefit in uint8 here.

No objection to what you are suggested here.

>> Avoiding a new flavor of RangevarGet would be also nice, now
>> RangeVarGetRelidExtended() is likely popular enough in extensions that
>> much things would break.
> 
> I can't follow?

Please, let's not have RangeVarGetRelidTryLock(), RangeVarGetRelidFoo()
or RangeVarGetRelidHoge().  This would make a third API designed at
doing the same thing...
--
Michael


signature.asc
Description: PGP signature

Re: Change RangeVarGetRelidExtended() to take flags argument?

[Andres Freund]

Hi,

On 2018-03-06 10:17:49 +0900, Michael Paquier wrote:
> On Mon, Mar 05, 2018 at 05:06:59PM -0800, Andres Freund wrote:
> > Yea, that's a concern. OTOH, it doesn't seem nice to grow duplicates of
> > similar code. It'd not be too hard to move RangeVarGetRelidExtended()
> > code into RangeVarGetRelidInternal() and add
> > RangeVarGetRelidTryLock(). Not sure if that's any better.  Or just add
> > RangeVarGetRelidExtended2() :)
> 
> FWIW, it would have been nice to switch RangeVarGetRelidExtended

What exactly do you mean with the paste tense here?


> so as it handles a set of uint8 flags as one of its arguments.

Right, that's what I was proposing. Although I'd just go for uint32,
there's no benefit in uint8 here.


> Avoiding a new flavor of RangevarGet would be also nice, now
> RangeVarGetRelidExtended() is likely popular enough in extensions that
> much things would break.

I can't follow?


Greetings,

Andres Freund

Re: Change RangeVarGetRelidExtended() to take flags argument?

[Michael Paquier]

On Mon, Mar 05, 2018 at 05:06:59PM -0800, Andres Freund wrote:
> Yea, that's a concern. OTOH, it doesn't seem nice to grow duplicates of
> similar code. It'd not be too hard to move RangeVarGetRelidExtended()
> code into RangeVarGetRelidInternal() and add
> RangeVarGetRelidTryLock(). Not sure if that's any better.  Or just add
> RangeVarGetRelidExtended2() :)

FWIW, it would have been nice to switch RangeVarGetRelidExtended so as
it handles a set of uint8 flags as one of its arguments.  missing_ok and
nowait could be part of that.  Avoiding a new flavor of RangevarGet
would be also nice, now RangeVarGetRelidExtended() is likely popular
enough in extensions that much things would break.
--
Michael


signature.asc
Description: PGP signature

Re: Change RangeVarGetRelidExtended() to take flags argument?

[Andres Freund]

On 2018-03-05 19:57:44 -0500, Tom Lane wrote:
> Andres Freund  writes:
> > One wrinkle in that plan is that it'd not be trivial to discern whether
> > a lock couldn't be acquired or whether the object vanished.  I don't
> > really have good idea how to tackle that yet.
> 
> Do we really care which case applies?

I think there might be cases where we do. As expand_vacuum_rel()
wouldn't use missing_ok = true, it'd not be an issue for this specific
callsite though.


> But having to mess with the semantics of RangeVarGetRelidExtended seems
> like a good reason not to go down this path...

Yea, that's a concern. OTOH, it doesn't seem nice to grow duplicates of
similar code. It'd not be too hard to move RangeVarGetRelidExtended()
code into RangeVarGetRelidInternal() and add
RangeVarGetRelidTryLock(). Not sure if that's any better.  Or just add
RangeVarGetRelidExtended2() :)

Greetings,

Andres Freund

Re: Change RangeVarGetRelidExtended() to take flags argument?

[Tom Lane]

Andres Freund  writes:
> One wrinkle in that plan is that it'd not be trivial to discern whether
> a lock couldn't be acquired or whether the object vanished.  I don't
> really have good idea how to tackle that yet.

Do we really care which case applies?

But having to mess with the semantics of RangeVarGetRelidExtended seems
like a good reason not to go down this path...

regards, tom lane