Thread: Change RangeVarGetRelidExtended() to take flags argument?
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
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
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
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
Attachment
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
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
Attachment
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
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
Attachment
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),
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
Attachment
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
Attachment
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
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
On 3/30/18, 7:09 PM, "Andres Freund" <andres@anarazel.de> wrote: > Pushed. Thanks! Nathan