Change RangeVarGetRelidExtended() to take flags argument?
Hi,
/messages/by-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
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
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
Attachments:
v1-0001-Combine-options-for-RangeVarGetRelidExtended-into.patchapplication/octet-stream; name=v1-0001-Combine-options-for-RangeVarGetRelidExtended-into.patchDownload
From 352a442c6fec640b99dbf968198c35cd22a94517 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <bossartn@amazon.com>
Date: Wed, 21 Mar 2018 21:31:22 +0000
Subject: [PATCH v1 1/2] Combine options for RangeVarGetRelidExtended() into a
flags argument.
---
src/backend/catalog/namespace.c | 11 ++++++-----
src/backend/commands/cluster.c | 2 +-
src/backend/commands/indexcmds.c | 4 ++--
src/backend/commands/lockcmds.c | 4 ++--
src/backend/commands/matview.c | 2 +-
src/backend/commands/policy.c | 6 +++---
src/backend/commands/sequence.c | 3 +--
src/backend/commands/tablecmds.c | 15 +++++++--------
src/backend/commands/trigger.c | 2 +-
src/backend/rewrite/rewriteDefine.c | 2 +-
src/backend/tcop/utility.c | 2 +-
src/include/catalog/namespace.h | 10 ++++++++--
12 files changed, 34 insertions(+), 29 deletions(-)
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))
{
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 57f3917..37c83ca 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -115,7 +115,7 @@ cluster(ClusterStmt *stmt, bool isTopLevel)
/* Find, lock, and check permissions on the table */
tableOid = RangeVarGetRelidExtended(stmt->relation,
AccessExclusiveLock,
- false, false,
+ 0,
RangeVarCallbackOwnsTable, NULL);
rel = heap_open(tableOid, NoLock);
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 0a2ab50..ee4def1 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -2091,7 +2091,7 @@ ReindexIndex(RangeVar *indexRelation, int options)
* used here must match the index lock obtained in reindex_index().
*/
indOid = RangeVarGetRelidExtended(indexRelation, AccessExclusiveLock,
- false, false,
+ 0,
RangeVarCallbackForReindexIndex,
(void *) &heapOid);
@@ -2183,7 +2183,7 @@ ReindexTable(RangeVar *relation, int options)
Oid heapOid;
/* The lock level used here should match reindex_relation(). */
- heapOid = RangeVarGetRelidExtended(relation, ShareLock, false, false,
+ heapOid = RangeVarGetRelidExtended(relation, ShareLock, 0,
RangeVarCallbackOwnsTable, NULL);
if (!reindex_relation(heapOid,
diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c
index 6479dcb..a37dc01 100644
--- a/src/backend/commands/lockcmds.c
+++ b/src/backend/commands/lockcmds.c
@@ -57,8 +57,8 @@ LockTableCommand(LockStmt *lockstmt)
bool recurse = rv->inh;
Oid reloid;
- reloid = RangeVarGetRelidExtended(rv, lockstmt->mode, false,
- lockstmt->nowait,
+ reloid = RangeVarGetRelidExtended(rv, lockstmt->mode,
+ (lockstmt->nowait) ? RELID_NOWAIT : 0,
RangeVarCallbackForLockTable,
(void *) &lockstmt->mode);
diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index 23892b1..410d4e5 100644
--- a/src/backend/commands/matview.c
+++ b/src/backend/commands/matview.c
@@ -161,7 +161,7 @@ ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
* Get a lock until end of transaction.
*/
matviewOid = RangeVarGetRelidExtended(stmt->relation,
- lockmode, false, false,
+ lockmode, 0,
RangeVarCallbackOwnsTable, NULL);
matviewRel = heap_open(matviewOid, NoLock);
diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c
index 280a14a..ac96a35 100644
--- a/src/backend/commands/policy.c
+++ b/src/backend/commands/policy.c
@@ -740,7 +740,7 @@ CreatePolicy(CreatePolicyStmt *stmt)
/* Get id of table. Also handles permissions checks. */
table_id = RangeVarGetRelidExtended(stmt->table, AccessExclusiveLock,
- false, false,
+ 0,
RangeVarCallbackForPolicy,
(void *) stmt);
@@ -912,7 +912,7 @@ AlterPolicy(AlterPolicyStmt *stmt)
/* Get id of table. Also handles permissions checks. */
table_id = RangeVarGetRelidExtended(stmt->table, AccessExclusiveLock,
- false, false,
+ 0,
RangeVarCallbackForPolicy,
(void *) stmt);
@@ -1212,7 +1212,7 @@ rename_policy(RenameStmt *stmt)
/* Get id of table. Also handles permissions checks. */
table_id = RangeVarGetRelidExtended(stmt->relation, AccessExclusiveLock,
- false, false,
+ 0,
RangeVarCallbackForPolicy,
(void *) stmt);
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index dcc0aa5..96984e3 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -432,8 +432,7 @@ AlterSequence(ParseState *pstate, AlterSeqStmt *stmt)
/* Open and lock sequence, and check for ownership along the way. */
relid = RangeVarGetRelidExtended(stmt->sequence,
ShareRowExclusiveLock,
- stmt->missing_ok,
- false,
+ (stmt->missing_ok) ? RELID_MISSING_OK : 0,
RangeVarCallbackOwnsRelation,
NULL);
if (relid == InvalidOid)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index f5c744b..128434b 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -1147,8 +1147,7 @@ RemoveRelations(DropStmt *drop)
state.heapOid = InvalidOid;
state.partParentOid = InvalidOid;
state.concurrent = drop->concurrent;
- relOid = RangeVarGetRelidExtended(rel, lockmode, true,
- false,
+ relOid = RangeVarGetRelidExtended(rel, lockmode, RELID_MISSING_OK,
RangeVarCallbackForDropRelation,
(void *) &state);
@@ -2781,7 +2780,7 @@ renameatt(RenameStmt *stmt)
/* lock level taken here should match renameatt_internal */
relid = RangeVarGetRelidExtended(stmt->relation, AccessExclusiveLock,
- stmt->missing_ok, false,
+ (stmt->missing_ok) ? RELID_MISSING_OK : 0,
RangeVarCallbackForRenameAttribute,
NULL);
@@ -2933,7 +2932,7 @@ RenameConstraint(RenameStmt *stmt)
{
/* lock level taken here should match rename_constraint_internal */
relid = RangeVarGetRelidExtended(stmt->relation, AccessExclusiveLock,
- stmt->missing_ok, false,
+ (stmt->missing_ok) ? RELID_MISSING_OK : 0,
RangeVarCallbackForRenameAttribute,
NULL);
if (!OidIsValid(relid))
@@ -2975,7 +2974,7 @@ RenameRelation(RenameStmt *stmt)
* escalation.
*/
relid = RangeVarGetRelidExtended(stmt->relation, AccessExclusiveLock,
- stmt->missing_ok, false,
+ (stmt->missing_ok) ? RELID_MISSING_OK : 0,
RangeVarCallbackForAlterRelation,
(void *) stmt);
@@ -3134,7 +3133,7 @@ CheckTableNotInUse(Relation rel, const char *stmt)
Oid
AlterTableLookupRelation(AlterTableStmt *stmt, LOCKMODE lockmode)
{
- return RangeVarGetRelidExtended(stmt->relation, lockmode, stmt->missing_ok, false,
+ return RangeVarGetRelidExtended(stmt->relation, lockmode, (stmt->missing_ok) ? RELID_MISSING_OK : 0,
RangeVarCallbackForAlterRelation,
(void *) stmt);
}
@@ -12646,7 +12645,7 @@ AlterTableNamespace(AlterObjectSchemaStmt *stmt, Oid *oldschema)
ObjectAddress myself;
relid = RangeVarGetRelidExtended(stmt->relation, AccessExclusiveLock,
- stmt->missing_ok, false,
+ (stmt->missing_ok) ? RELID_MISSING_OK : 0,
RangeVarCallbackForAlterRelation,
(void *) stmt);
@@ -14452,7 +14451,7 @@ ATExecAttachPartitionIdx(List **wqueue, Relation parentIdx, RangeVar *name)
state.parentTblOid = parentIdx->rd_index->indrelid;
state.lockedParentTbl = false;
partIdxId =
- RangeVarGetRelidExtended(name, AccessExclusiveLock, false, false,
+ RangeVarGetRelidExtended(name, AccessExclusiveLock, 0,
RangeVarCallbackForAttachIndex,
(void *) &state);
/* Not there? */
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index fbd176b..5f1f5c5 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -1462,7 +1462,7 @@ renametrig(RenameStmt *stmt)
* release until end of transaction).
*/
relid = RangeVarGetRelidExtended(stmt->relation, AccessExclusiveLock,
- false, false,
+ 0,
RangeVarCallbackForRenameTrigger,
NULL);
diff --git a/src/backend/rewrite/rewriteDefine.c b/src/backend/rewrite/rewriteDefine.c
index 679be60..d81a2ea 100644
--- a/src/backend/rewrite/rewriteDefine.c
+++ b/src/backend/rewrite/rewriteDefine.c
@@ -951,7 +951,7 @@ RenameRewriteRule(RangeVar *relation, const char *oldName,
* release until end of transaction).
*/
relid = RangeVarGetRelidExtended(relation, AccessExclusiveLock,
- false, false,
+ 0,
RangeVarCallbackForRenameRule,
NULL);
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index ed55521..b497c6e 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -1307,7 +1307,7 @@ ProcessUtilitySlow(ParseState *pstate,
: ShareLock;
relid =
RangeVarGetRelidExtended(stmt->relation, lockmode,
- false, false,
+ 0,
RangeVarCallbackOwnsRelation,
NULL);
diff --git a/src/include/catalog/namespace.h b/src/include/catalog/namespace.h
index 5f8cf49..22f8667 100644
--- a/src/include/catalog/namespace.h
+++ b/src/include/catalog/namespace.h
@@ -47,14 +47,20 @@ typedef struct OverrideSearchPath
bool addTemp; /* implicitly prepend temp schema? */
} OverrideSearchPath;
+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;
+
typedef void (*RangeVarGetRelidCallback) (const RangeVar *relation, Oid relId,
Oid oldRelId, void *callback_arg);
#define RangeVarGetRelid(relation, lockmode, missing_ok) \
- RangeVarGetRelidExtended(relation, lockmode, missing_ok, false, NULL, NULL)
+ RangeVarGetRelidExtended(relation, lockmode, (missing_ok) ? RELID_MISSING_OK : 0, NULL, NULL)
extern Oid RangeVarGetRelidExtended(const RangeVar *relation,
- LOCKMODE lockmode, bool missing_ok, bool nowait,
+ LOCKMODE lockmode, int flags,
RangeVarGetRelidCallback callback,
void *callback_arg);
extern Oid RangeVarGetCreationNamespace(const RangeVar *newRelation);
--
2.7.3.AMZN
v1-0002-Add-skip-locked-option-to-RangeVarGetRelidExtende.patchapplication/octet-stream; name=v1-0002-Add-skip-locked-option-to-RangeVarGetRelidExtende.patchDownload
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),
diff --git a/src/include/catalog/namespace.h b/src/include/catalog/namespace.h
index 22f8667..cca99cd 100644
--- a/src/include/catalog/namespace.h
+++ b/src/include/catalog/namespace.h
@@ -50,7 +50,8 @@ typedef struct OverrideSearchPath
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 */
+ RELID_NOWAIT = 1 << 1, /* error if relation cannot be locked */
+ RELID_SKIP_LOCKED = 1 << 2 /* skip if relation cannot be locked */
} RelidOption;
typedef void (*RangeVarGetRelidCallback) (const RangeVar *relation, Oid relId,
--
2.7.3.AMZN
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))
{
...
Show quoted text
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
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]/messages/by-id/20171201160907.27110.74730@wrigleys.postgresql.org.
Nathan
[0]: /messages/by-id/20171201160907.27110.74730@wrigleys.postgresql.org
Attachments:
v2-0001-Combine-options-for-RangeVarGetRelidExtended-into.patchapplication/octet-stream; name=v2-0001-Combine-options-for-RangeVarGetRelidExtended-into.patchDownload
From acb87921a529906944acf990f87a1f90c9e31097 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <bossartn@amazon.com>
Date: Fri, 30 Mar 2018 15:05:22 +0000
Subject: [PATCH v2 1/2] Combine options for RangeVarGetRelidExtended() into a
flags argument.
---
src/backend/catalog/namespace.c | 11 ++++++-----
src/backend/commands/cluster.c | 2 +-
src/backend/commands/indexcmds.c | 4 ++--
src/backend/commands/lockcmds.c | 4 ++--
src/backend/commands/matview.c | 2 +-
src/backend/commands/policy.c | 6 +++---
src/backend/commands/sequence.c | 3 +--
src/backend/commands/tablecmds.c | 15 +++++++--------
src/backend/commands/trigger.c | 2 +-
src/backend/rewrite/rewriteDefine.c | 2 +-
src/backend/tcop/utility.c | 2 +-
src/include/catalog/namespace.h | 10 ++++++++--
12 files changed, 34 insertions(+), 29 deletions(-)
diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
index 52dd400b96..d3eef63d5e 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,
+ uint32 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))
{
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 0f844c00c8..d088dc11a6 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -115,7 +115,7 @@ cluster(ClusterStmt *stmt, bool isTopLevel)
/* Find, lock, and check permissions on the table */
tableOid = RangeVarGetRelidExtended(stmt->relation,
AccessExclusiveLock,
- false, false,
+ 0,
RangeVarCallbackOwnsTable, NULL);
rel = heap_open(tableOid, NoLock);
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 0185970794..e224b91f53 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -2091,7 +2091,7 @@ ReindexIndex(RangeVar *indexRelation, int options)
* used here must match the index lock obtained in reindex_index().
*/
indOid = RangeVarGetRelidExtended(indexRelation, AccessExclusiveLock,
- false, false,
+ 0,
RangeVarCallbackForReindexIndex,
(void *) &heapOid);
@@ -2183,7 +2183,7 @@ ReindexTable(RangeVar *relation, int options)
Oid heapOid;
/* The lock level used here should match reindex_relation(). */
- heapOid = RangeVarGetRelidExtended(relation, ShareLock, false, false,
+ heapOid = RangeVarGetRelidExtended(relation, ShareLock, 0,
RangeVarCallbackOwnsTable, NULL);
if (!reindex_relation(heapOid,
diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c
index 7f25d7498a..c87aca8996 100644
--- a/src/backend/commands/lockcmds.c
+++ b/src/backend/commands/lockcmds.c
@@ -61,8 +61,8 @@ LockTableCommand(LockStmt *lockstmt)
bool recurse = rv->inh;
Oid reloid;
- reloid = RangeVarGetRelidExtended(rv, lockstmt->mode, false,
- lockstmt->nowait,
+ reloid = RangeVarGetRelidExtended(rv, lockstmt->mode,
+ (lockstmt->nowait) ? RELID_NOWAIT : 0,
RangeVarCallbackForLockTable,
(void *) &lockstmt->mode);
diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index 23892b1b81..410d4e5a38 100644
--- a/src/backend/commands/matview.c
+++ b/src/backend/commands/matview.c
@@ -161,7 +161,7 @@ ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
* Get a lock until end of transaction.
*/
matviewOid = RangeVarGetRelidExtended(stmt->relation,
- lockmode, false, false,
+ lockmode, 0,
RangeVarCallbackOwnsTable, NULL);
matviewRel = heap_open(matviewOid, NoLock);
diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c
index cfaf32ccbd..00841b3b8a 100644
--- a/src/backend/commands/policy.c
+++ b/src/backend/commands/policy.c
@@ -743,7 +743,7 @@ CreatePolicy(CreatePolicyStmt *stmt)
/* Get id of table. Also handles permissions checks. */
table_id = RangeVarGetRelidExtended(stmt->table, AccessExclusiveLock,
- false, false,
+ 0,
RangeVarCallbackForPolicy,
(void *) stmt);
@@ -915,7 +915,7 @@ AlterPolicy(AlterPolicyStmt *stmt)
/* Get id of table. Also handles permissions checks. */
table_id = RangeVarGetRelidExtended(stmt->table, AccessExclusiveLock,
- false, false,
+ 0,
RangeVarCallbackForPolicy,
(void *) stmt);
@@ -1215,7 +1215,7 @@ rename_policy(RenameStmt *stmt)
/* Get id of table. Also handles permissions checks. */
table_id = RangeVarGetRelidExtended(stmt->relation, AccessExclusiveLock,
- false, false,
+ 0,
RangeVarCallbackForPolicy,
(void *) stmt);
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index dcc0aa536a..96984e3b2f 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -432,8 +432,7 @@ AlterSequence(ParseState *pstate, AlterSeqStmt *stmt)
/* Open and lock sequence, and check for ownership along the way. */
relid = RangeVarGetRelidExtended(stmt->sequence,
ShareRowExclusiveLock,
- stmt->missing_ok,
- false,
+ (stmt->missing_ok) ? RELID_MISSING_OK : 0,
RangeVarCallbackOwnsRelation,
NULL);
if (relid == InvalidOid)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 83a881eff3..bf501270b5 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -1159,8 +1159,7 @@ RemoveRelations(DropStmt *drop)
state.heapOid = InvalidOid;
state.partParentOid = InvalidOid;
state.concurrent = drop->concurrent;
- relOid = RangeVarGetRelidExtended(rel, lockmode, true,
- false,
+ relOid = RangeVarGetRelidExtended(rel, lockmode, RELID_MISSING_OK,
RangeVarCallbackForDropRelation,
(void *) &state);
@@ -2793,7 +2792,7 @@ renameatt(RenameStmt *stmt)
/* lock level taken here should match renameatt_internal */
relid = RangeVarGetRelidExtended(stmt->relation, AccessExclusiveLock,
- stmt->missing_ok, false,
+ (stmt->missing_ok) ? RELID_MISSING_OK : 0,
RangeVarCallbackForRenameAttribute,
NULL);
@@ -2945,7 +2944,7 @@ RenameConstraint(RenameStmt *stmt)
{
/* lock level taken here should match rename_constraint_internal */
relid = RangeVarGetRelidExtended(stmt->relation, AccessExclusiveLock,
- stmt->missing_ok, false,
+ (stmt->missing_ok) ? RELID_MISSING_OK : 0,
RangeVarCallbackForRenameAttribute,
NULL);
if (!OidIsValid(relid))
@@ -2987,7 +2986,7 @@ RenameRelation(RenameStmt *stmt)
* escalation.
*/
relid = RangeVarGetRelidExtended(stmt->relation, AccessExclusiveLock,
- stmt->missing_ok, false,
+ (stmt->missing_ok) ? RELID_MISSING_OK : 0,
RangeVarCallbackForAlterRelation,
(void *) stmt);
@@ -3146,7 +3145,7 @@ CheckTableNotInUse(Relation rel, const char *stmt)
Oid
AlterTableLookupRelation(AlterTableStmt *stmt, LOCKMODE lockmode)
{
- return RangeVarGetRelidExtended(stmt->relation, lockmode, stmt->missing_ok, false,
+ return RangeVarGetRelidExtended(stmt->relation, lockmode, (stmt->missing_ok) ? RELID_MISSING_OK : 0,
RangeVarCallbackForAlterRelation,
(void *) stmt);
}
@@ -12683,7 +12682,7 @@ AlterTableNamespace(AlterObjectSchemaStmt *stmt, Oid *oldschema)
ObjectAddress myself;
relid = RangeVarGetRelidExtended(stmt->relation, AccessExclusiveLock,
- stmt->missing_ok, false,
+ (stmt->missing_ok) ? RELID_MISSING_OK : 0,
RangeVarCallbackForAlterRelation,
(void *) stmt);
@@ -14613,7 +14612,7 @@ ATExecAttachPartitionIdx(List **wqueue, Relation parentIdx, RangeVar *name)
state.parentTblOid = parentIdx->rd_index->indrelid;
state.lockedParentTbl = false;
partIdxId =
- RangeVarGetRelidExtended(name, AccessExclusiveLock, false, false,
+ RangeVarGetRelidExtended(name, AccessExclusiveLock, 0,
RangeVarCallbackForAttachIndex,
(void *) &state);
/* Not there? */
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 9d8df5986e..a6593f939c 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -1667,7 +1667,7 @@ renametrig(RenameStmt *stmt)
* release until end of transaction).
*/
relid = RangeVarGetRelidExtended(stmt->relation, AccessExclusiveLock,
- false, false,
+ 0,
RangeVarCallbackForRenameTrigger,
NULL);
diff --git a/src/backend/rewrite/rewriteDefine.c b/src/backend/rewrite/rewriteDefine.c
index 679be605f1..d81a2ea342 100644
--- a/src/backend/rewrite/rewriteDefine.c
+++ b/src/backend/rewrite/rewriteDefine.c
@@ -951,7 +951,7 @@ RenameRewriteRule(RangeVar *relation, const char *oldName,
* release until end of transaction).
*/
relid = RangeVarGetRelidExtended(relation, AccessExclusiveLock,
- false, false,
+ 0,
RangeVarCallbackForRenameRule,
NULL);
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index d355bef606..b2dc9d18ea 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -1305,7 +1305,7 @@ ProcessUtilitySlow(ParseState *pstate,
: ShareLock;
relid =
RangeVarGetRelidExtended(stmt->relation, lockmode,
- false, false,
+ 0,
RangeVarCallbackOwnsRelation,
NULL);
diff --git a/src/include/catalog/namespace.h b/src/include/catalog/namespace.h
index 5f8cf4992e..60f110b639 100644
--- a/src/include/catalog/namespace.h
+++ b/src/include/catalog/namespace.h
@@ -47,14 +47,20 @@ typedef struct OverrideSearchPath
bool addTemp; /* implicitly prepend temp schema? */
} OverrideSearchPath;
+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;
+
typedef void (*RangeVarGetRelidCallback) (const RangeVar *relation, Oid relId,
Oid oldRelId, void *callback_arg);
#define RangeVarGetRelid(relation, lockmode, missing_ok) \
- RangeVarGetRelidExtended(relation, lockmode, missing_ok, false, NULL, NULL)
+ RangeVarGetRelidExtended(relation, lockmode, (missing_ok) ? RELID_MISSING_OK : 0, NULL, NULL)
extern Oid RangeVarGetRelidExtended(const RangeVar *relation,
- LOCKMODE lockmode, bool missing_ok, bool nowait,
+ LOCKMODE lockmode, uint32 flags,
RangeVarGetRelidCallback callback,
void *callback_arg);
extern Oid RangeVarGetCreationNamespace(const RangeVar *newRelation);
--
2.16.2
v2-0002-Add-skip-locked-option-to-RangeVarGetRelidExtende.patchapplication/octet-stream; name=v2-0002-Add-skip-locked-option-to-RangeVarGetRelidExtende.patchDownload
From eb349b58ab8ebaa30c7bd0892393c3f062d7c0e0 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <bossartn@amazon.com>
Date: Fri, 30 Mar 2018 16:34:40 +0000
Subject: [PATCH v2 2/2] Add skip-locked option to RangeVarGetRelidExtended().
---
src/backend/catalog/namespace.c | 28 ++++++++++++++++++++++------
src/include/catalog/namespace.h | 3 ++-
2 files changed, 24 insertions(+), 7 deletions(-)
diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
index d3eef63d5e..55ac8b55d7 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) == 0 || (flags & RELID_SKIP_LOCKED) == 0);
+
/*
* We check the catalog name and then ignore it.
*/
@@ -362,20 +372,24 @@ 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))
{
+ int elevel = ((flags & RELID_SKIP_LOCKED) != 0) ? DEBUG1 : ERROR;
+
if (relation->schemaname)
- ereport(ERROR,
+ ereport(elevel,
(errcode(ERRCODE_LOCK_NOT_AVAILABLE),
errmsg("could not obtain lock on relation \"%s.%s\"",
relation->schemaname, relation->relname)));
else
- ereport(ERROR,
+ ereport(elevel,
(errcode(ERRCODE_LOCK_NOT_AVAILABLE),
errmsg("could not obtain lock on relation \"%s\"",
relation->relname)));
+
+ return InvalidOid;
}
/*
@@ -393,15 +407,17 @@ RangeVarGetRelidExtended(const RangeVar *relation, LOCKMODE lockmode,
oldRelId = relId;
}
- if (!OidIsValid(relId) && !missing_ok)
+ if (!OidIsValid(relId))
{
+ int elevel = missing_ok ? DEBUG1 : ERROR;
+
if (relation->schemaname)
- ereport(ERROR,
+ ereport(elevel,
(errcode(ERRCODE_UNDEFINED_TABLE),
errmsg("relation \"%s.%s\" does not exist",
relation->schemaname, relation->relname)));
else
- ereport(ERROR,
+ ereport(elevel,
(errcode(ERRCODE_UNDEFINED_TABLE),
errmsg("relation \"%s\" does not exist",
relation->relname)));
diff --git a/src/include/catalog/namespace.h b/src/include/catalog/namespace.h
index 60f110b639..6347a97c53 100644
--- a/src/include/catalog/namespace.h
+++ b/src/include/catalog/namespace.h
@@ -50,7 +50,8 @@ typedef struct OverrideSearchPath
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 */
+ RELID_NOWAIT = 1 << 1, /* error if relation cannot be locked */
+ RELID_SKIP_LOCKED = 1 << 2 /* skip if relation cannot be locked */
} RelidOption;
typedef void (*RangeVarGetRelidCallback) (const RangeVar *relation, Oid relId,
--
2.16.2
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