On Wed, 2022-11-02 at 12:37 -0400, Tom Lane wrote:
> Since we're getting pretty close to the next set of back-branch
> releases,
> I went ahead and pushed a minimal fix along the lines suggested by
> Shi Yu.
> (I realized that there's a second ExecFindPartition call in worker.c
> that
> also needs a check.) We still can at leisure think about refactoring
> CheckSubscriptionRelkind to avoid unnecessary lookups. I think that
> is something we should do only in HEAD; it'll just be a marginal
> savings,
> not worth the risks of API changes in stable branches. The other
> loose
> end is whether to worry about a regression test case. I'm inclined
> not
> to bother. The only thing that isn't getting exercised is the actual
> ereport, which probably isn't in great need of routine testing.
>
> regards, tom lane
I agree that early checks limit some of the functionality that was
available before, so I guess the only way to preserve it is to do only
the last-minute checks after routing, fair enough. As for the
refactoring of the premature lookup, I have done some work on that in
the previous patch, I think we can just use it. Attached a separate
patch with the refactoring.
From 004c63a8eba777be739f062cdc9b7ddcf2eac253 Mon Sep 17 00:00:00 2001
From: Ilya Gladyshev <ilya.v.gladys...@gmail.com>
Date: Thu, 3 Nov 2022 11:39:24 +0400
Subject: [PATCH] Delay namespace and relname lookups until error
---
src/backend/commands/subscriptioncmds.c | 12 ++++++++----
src/backend/executor/execReplication.c | 13 ++++++++++---
src/backend/replication/logical/relation.c | 2 +-
src/backend/replication/logical/worker.c | 8 ++------
src/include/executor/executor.h | 2 +-
5 files changed, 22 insertions(+), 15 deletions(-)
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index f0cec2ad5e..0c3ad698eb 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -700,12 +700,14 @@ CreateSubscription(ParseState *pstate, CreateSubscriptionStmt *stmt,
{
RangeVar *rv = (RangeVar *) lfirst(lc);
Oid relid;
+ Relation sub_rel;
relid = RangeVarGetRelid(rv, AccessShareLock, false);
+ sub_rel = RelationIdGetRelation(relid);
/* Check for supported relkind. */
- CheckSubscriptionRelkind(get_rel_relkind(relid),
- rv->schemaname, rv->relname);
+ CheckSubscriptionRelkind(sub_rel, rv->schemaname, rv->relname);
+ RelationClose(sub_rel);
AddSubscriptionRelState(subid, relid, table_state,
InvalidXLogRecPtr);
@@ -861,12 +863,14 @@ AlterSubscription_refresh(Subscription *sub, bool copy_data,
{
RangeVar *rv = (RangeVar *) lfirst(lc);
Oid relid;
+ Relation sub_rel;
relid = RangeVarGetRelid(rv, AccessShareLock, false);
+ sub_rel = RelationIdGetRelation(relid);
/* Check for supported relkind. */
- CheckSubscriptionRelkind(get_rel_relkind(relid),
- rv->schemaname, rv->relname);
+ CheckSubscriptionRelkind(sub_rel, rv->schemaname, rv->relname);
+ RelationClose(sub_rel);
pubrel_local_oids[off++] = relid;
diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c
index 6014f2e248..a0a2d326db 100644
--- a/src/backend/executor/execReplication.c
+++ b/src/backend/executor/execReplication.c
@@ -649,16 +649,23 @@ CheckCmdReplicaIdentity(Relation rel, CmdType cmd)
/*
* Check if we support writing into specific relkind.
*
- * The nspname and relname are only needed for error reporting.
+ * If nspname and relname are not NULL, they are used for error reporting, otherwise these values are fetched from relcache.
*/
void
-CheckSubscriptionRelkind(char relkind, const char *nspname,
- const char *relname)
+CheckSubscriptionRelkind(Relation rel, const char *nspname, const char *relname)
{
+ char relkind = rel->rd_rel->relkind;
+
if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE)
+ {
+ if (relname == NULL)
+ relname = RelationGetRelationName(rel);
+ if (nspname == NULL)
+ nspname = get_namespace_name(RelationGetNamespace(rel));
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("cannot use relation \"%s.%s\" as logical replication target",
nspname, relname),
errdetail_relkind_not_supported(relkind)));
+ }
}
diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c
index e989047681..e98031e272 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -393,7 +393,7 @@ logicalrep_rel_open(LogicalRepRelId remoteid, LOCKMODE lockmode)
entry->localreloid = relid;
/* Check for supported relkind. */
- CheckSubscriptionRelkind(entry->localrel->rd_rel->relkind,
+ CheckSubscriptionRelkind(entry->localrel,
remoterel->nspname, remoterel->relname);
/*
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index e48a3f589a..e00e383968 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -2181,9 +2181,7 @@ apply_handle_tuple_routing(ApplyExecutionData *edata,
* unsupported relkinds; and the set of partitions can change, so checking
* at CREATE/ALTER SUBSCRIPTION would be insufficient.
*/
- CheckSubscriptionRelkind(partrel->rd_rel->relkind,
- get_namespace_name(RelationGetNamespace(partrel)),
- RelationGetRelationName(partrel));
+ CheckSubscriptionRelkind(partrel, NULL, NULL);
/*
* To perform any of the operations below, the tuple must match the
@@ -2334,9 +2332,7 @@ apply_handle_tuple_routing(ApplyExecutionData *edata,
partrel_new = partrelinfo_new->ri_RelationDesc;
/* Check that new partition also has supported relkind. */
- CheckSubscriptionRelkind(partrel_new->rd_rel->relkind,
- get_namespace_name(RelationGetNamespace(partrel_new)),
- RelationGetRelationName(partrel_new));
+ CheckSubscriptionRelkind(partrel_new, NULL, NULL);
/* DELETE old tuple found in the old partition. */
apply_handle_delete_internal(edata, partrelinfo,
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index ed95ed1176..188e783b5e 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -646,7 +646,7 @@ extern void ExecSimpleRelationDelete(ResultRelInfo *resultRelInfo,
TupleTableSlot *searchslot);
extern void CheckCmdReplicaIdentity(Relation rel, CmdType cmd);
-extern void CheckSubscriptionRelkind(char relkind, const char *nspname,
+extern void CheckSubscriptionRelkind(Relation rel, const char *nspname,
const char *relname);
/*
--
2.30.2
