Horton

Thread

Commits

  1. Add SKIP_LOCKED option to RangeVarGetRelidExtended().

  2. Combine options for RangeVarGetRelidExtended() into a flags argument.

  1. Change RangeVarGetRelidExtended() to take flags argument?

    Andres Freund <andres@anarazel.de> — 2018-03-06T00:53:49Z

    Hi,

https://www.postgresql.org/message-id/7327B413-1A57-477F-A6A0-6FD80CB5482D@amazon.com
adds a SKIP LOCKED option to vacuum. To avoid blocking in
expand_vacuum_rel()'s call of RangeVarGetRelid(), it open codes a
SKIP_LOCKED variant (i.e. RangeVarGetRelidExtended()'s nowait, but
doesn't error out). I don't like that much.

Therefore I wonder if we should consolidate the existing
RangeVarGetRelidExtended() arguments (missing_ok and nowait) into a
flags argument. That'll then allow to add SKIP_LOCKED behaviour.

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.  We could make the return
value more complicated (and return relation oid via parameter) but that
seems like it'd be more invasive.

Greetings,

Andres Freund

  2. Re: Change RangeVarGetRelidExtended() to take flags argument?

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-03-06T00:57:44Z

    Andres Freund <andres@anarazel.de> 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

  3. Re: Change RangeVarGetRelidExtended() to take flags argument?

    Andres Freund <andres@anarazel.de> — 2018-03-06T01:06:59Z

    On 2018-03-05 19:57:44 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> 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

  4. Re: Change RangeVarGetRelidExtended() to take flags argument?

    Michael Paquier <michael@paquier.xyz> — 2018-03-06T01:17:49Z

    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

  5. Re: Change RangeVarGetRelidExtended() to take flags argument?

    Andres Freund <andres@anarazel.de> — 2018-03-06T01:21:11Z

    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

  6. Re: Change RangeVarGetRelidExtended() to take flags argument?

    Michael Paquier <michael@paquier.xyz> — 2018-03-06T01:35:12Z

    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

  7. Re: Change RangeVarGetRelidExtended() to take flags argument?

    Nathan Bossart <bossartn@amazon.com> — 2018-03-07T16:33:07Z

    On 3/5/18, 7:08 PM, "Andres Freund" <andres@anarazel.de> wrote:
> On 2018-03-05 19:57:44 -0500, Tom Lane wrote:
>> Andres Freund <andres@anarazel.de> 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

  8. Re: Change RangeVarGetRelidExtended() to take flags argument?

    Nathan Bossart <bossartn@amazon.com> — 2018-03-22T15:45:23Z

    On 3/7/18, 10:33 AM, "Bossart, Nathan" <bossartn@amazon.com> wrote:
> On 3/5/18, 7:08 PM, "Andres Freund" <andres@anarazel.de> wrote:
>> On 2018-03-05 19:57:44 -0500, Tom Lane wrote:
>>> Andres Freund <andres@anarazel.de> 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.

Here is a set of patches for this approach.

Nathan

  9. Re: Change RangeVarGetRelidExtended() to take flags argument?

    Andres Freund <andres@anarazel.de> — 2018-03-30T00:15:14Z

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

  10. Re: Change RangeVarGetRelidExtended() to take flags argument?

    Michael Paquier <michael@paquier.xyz> — 2018-03-30T02:37:19Z

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

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

  11. Re: Change RangeVarGetRelidExtended() to take flags argument?

    Nathan Bossart <bossartn@amazon.com> — 2018-03-30T17:08:26Z

    Thanks for taking a look.

On 3/29/18, 9:38 PM, "Michael Paquier" <michael@paquier.xyz> wrote:
> On Thu, Mar 29, 2018 at 05:15:14PM -0700, Andres Freund wrote:
>> On 2018-03-22 15:45:23 +0000, Bossart, Nathan wrote:
>>> +	/* 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);

Agreed.  I've changed this in v2 of 0002.

> 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.

I've made this change as well.

> + * 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?

I adjusted the existing log statements to generate a DEBUG message for
missing-ok and skip-locked.

I'll go ahead and post rebased patches for VACUUM (SKIP LOCKED) in the
other thread [0].

Nathan

[0] https://postgr.es/m/20171201160907.27110.74730%40wrigleys.postgresql.org

  12. Re: Change RangeVarGetRelidExtended() to take flags argument?

    Andres Freund <andres@anarazel.de> — 2018-03-30T21:55:45Z

    Hi,

On 2018-03-30 17:08:26 +0000, 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

  13. Re: Change RangeVarGetRelidExtended() to take flags argument?

    Andres Freund <andres@anarazel.de> — 2018-03-31T00:09:42Z

    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 +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.
> 
> 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 +0000, 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

  14. Re: Change RangeVarGetRelidExtended() to take flags argument?

    Nathan Bossart <bossartn@amazon.com> — 2018-04-02T15:04:16Z

    On 3/30/18, 7:09 PM, "Andres Freund" <andres@anarazel.de> wrote:
> Pushed.

Thanks!

Nathan