From: Ilya Gladyshev <ilya.v.gladyshev@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: shiy.fnst@fujitsu.com <shiy.fnst@fujitsu.com>
Cc: Dilip Kumar <dilipbalaut@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
Subject: Re: Segfault on logical replication to partitioned table with foreign children
Date: Thu, 03 Nov 2022 12:45:08 +0400
Message-ID: <83d682e61ee79925db600bbbd9f62182471f49b4.camel@gmail.com> (raw)
In-Reply-To: <1787918.1667407062@sss.pgh.pa.us>
References: <6b93e3748ba43298694f376ca8797279d7945e29.camel@gmail.com>
<CAFiTN-vvnrvHeUibhwhnJNgTyE70DcBTgXPws3iO7HVM9TWANg@mail.gmail.com>
<188734.1667137161@sss.pgh.pa.us>
<OSZPR01MB6310B855B1B90FEE433A98F4FD379@OSZPR01MB6310.jpnprd01.prod.outlook.com>
<04b5212cd549ee3c8efc63f6a92d85d5463bfdf1.camel@gmail.com>
<1361869.1667329814@sss.pgh.pa.us>
<1787918.1667407062@sss.pgh.pa.us>
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.
Attachments:0001-Delay-namespace-and-relname-lookups-until-error.patch (text/x-patch, 5.5K) download
From 004c63a8eba777be739f062cdc9b7ddcf2eac253 Mon Sep 17 00:00:00 2001
From: Ilya Gladyshev <ilya.v.gladyshev@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.cindex 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.cindex 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.cindex 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.cindex 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.hindex 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: ilya.v.gladyshev@gmail.com, tgl@sss.pgh.pa.us, shiy.fnst@fujitsu.com, dilipbalaut@gmail.com, pgsql-hackers@lists.postgresql.org
Subject: Re: Segfault on logical replication to partitioned table with foreign children
In-Reply-To: <83d682e61ee79925db600bbbd9f62182471f49b4.camel@gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox