Re: Segfault on logical replication to partitioned table with foreign children

2022-11-03 Thread Ilya Gladyshev 
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 
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

Re: Segfault on logical replication to partitioned table with foreign children

2022-11-02 Thread Tom Lane 
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

Re: Segfault on logical replication to partitioned table with foreign children

2022-11-01 Thread Tom Lane 
Ilya Gladyshev  writes:
> [ v2-0001-check-relkind-of-subscription-target-recursively.patch ]

Hmm.  I like Shi yu's way better (formal patch attached).  Checking
at CREATE/ALTER SUBSCRIPTION is much more complicated, and it's really
insufficient, because what if someone adds a new partition after
setting up the subscription?

I get the argument about it being a useful check for simple mistakes,
but I don't entirely buy that argument, because I think there are
potential use-cases that it'd disallow needlessly.  Imagine a
partitioned table that receives replication updates, but only into
the "current" partition; older partitions are basically static.
Now suppose you'd like to offload some of that old seldom-used data
to another server, and make those partitions into foreign tables
so you can still access it at need.  Such a setup will work perfectly
fine today, but this patch would break it.

So I think what we want is to check when we identify the partition.
Maybe Shi yu missed a place or two to check, but I verified that the
attached stops the crash.

There'd still be value in refactoring to avoid premature lookup
of the namespace name, but that's just micro-optimization.

regards, tom lane

diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index 5250ae7f54..17b9c42ecf 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -2176,6 +2176,15 @@ apply_handle_tuple_routing(ApplyExecutionData *edata,
 	Assert(partrelinfo != NULL);
 	partrel = partrelinfo->ri_RelationDesc;
 
+	/*
+	 * Check for supported relkind.  We need this since partitions might be of
+	 * 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));
+
 	/*
 	 * To perform any of the operations below, the tuple must match the
 	 * partition's rowtype. Convert if needed or just copy, using a dedicated

Re: Segfault on logical replication to partitioned table with foreign children

2022-10-31 Thread Ilya Gladyshev 
On Mon, 2022-10-31 at 03:20 +, shiy.f...@fujitsu.com wrote:
> On Sun, Oct 30, 2022 9:39 PM Tom Lane  wrote:
> > 
> > What I'm wondering about is whether we can refactor this code
> > to avoid so many usually-useless catalog lookups.  Pulling the
> > namespace name, in particular, is expensive and we generally
> > are not going to need the result.  In the child-rel case it'd
> > be much better to pass the opened relation and let the error-check
> > subroutine work from that.  Maybe we should just do it like that
> > at the start of the recursion, too?  Or pass the relid and let
> > the subroutine look up the names only in the error case.
> > 
> > A completely different line of thought is that this doesn't seem
> > like a terribly bulletproof fix, since children could get added to
> > a partitioned table after we look.  Maybe it'd be better to check
> > the relkind at the last moment before we do something that depends
> > on a child table being a relation.
> > 
> 
> I agree. So maybe we can add this check in
> apply_handle_tuple_routing().
> 
> diff --git a/src/backend/replication/logical/worker.c
> b/src/backend/replication/logical/worker.c
> index 5250ae7f54..e941b68e4b 100644
> --- a/src/backend/replication/logical/worker.c
> +++ b/src/backend/replication/logical/worker.c
> @@ -2176,6 +2176,10 @@ apply_handle_tuple_routing(ApplyExecutionData
> *edata,
>     Assert(partrelinfo != NULL);
>     partrel = partrelinfo->ri_RelationDesc;
> 
> +   /* Check for supported relkind. */
> +   CheckSubscriptionRelkind(partrel->rd_rel->relkind,
> +    relmapentry-
> >remoterel.nspname, relmapentry->remoterel.relname);
> +
>     /*
>  * To perform any of the operations below, the tuple must
> match the
>  * partition's rowtype. Convert if needed or just copy, using
> a dedicated
> 
> 
> Regards,
> Shi yu

I have verified that the current patch handles the attaching of new
partitions to the target partitioned table by throwing an error on
attempt to insert into a foreign table inside the logical replication
worker. I have refactored the code to minimize cache lookups, but I am
yet to write the tests for this. See the attached patch for the
refactored version.


From 56086185324e12e9db6add40f84bc7e60867f1d6 Mon Sep 17 00:00:00 2001
From: Ilya Gladyshev 
Date: Tue, 1 Nov 2022 01:42:44 +0400
Subject: [PATCH v2] check relkind of subscription target recursively

---
 src/backend/commands/subscriptioncmds.c| 18 
 src/backend/executor/execReplication.c | 51 ++
 src/backend/replication/logical/relation.c |  6 +--
 src/include/executor/executor.h|  4 +-
 4 files changed, 57 insertions(+), 22 deletions(-)

diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index f0cec2ad5e..2ae89f9e03 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -700,12 +700,13 @@ CreateSubscription(ParseState *pstate, CreateSubscriptionStmt *stmt,
 			{
 RangeVar   *rv = (RangeVar *) lfirst(lc);
 Oid			relid;
+Relation rel;
 
 relid = RangeVarGetRelid(rv, AccessShareLock, false);
-
-/* Check for supported relkind. */
-CheckSubscriptionRelkind(get_rel_relkind(relid),
-		 rv->schemaname, rv->relname);
+rel = RelationIdGetRelation(relid);
+/* Check for supported relkind recursively. */
+CheckSubscriptionRelation(rel, rv->schemaname, rv->relname);
+RelationClose(rel);
 
 AddSubscriptionRelState(subid, relid, table_state,
 		InvalidXLogRecPtr);
@@ -861,12 +862,13 @@ AlterSubscription_refresh(Subscription *sub, bool copy_data,
 		{
 			RangeVar   *rv = (RangeVar *) lfirst(lc);
 			Oid			relid;
+			Relation rel;
 
 			relid = RangeVarGetRelid(rv, AccessShareLock, false);
-
-			/* Check for supported relkind. */
-			CheckSubscriptionRelkind(get_rel_relkind(relid),
-	 rv->schemaname, rv->relname);
+			rel = RelationIdGetRelation(relid);
+			/* Check for supported relkind recursively. */
+			CheckSubscriptionRelation(rel, rv->schemaname, rv->relname);
+			RelationClose(rel);
 
 			pubrel_local_oids[off++] = relid;
 
diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c
index 6014f2e248..9da0b6b3a1 100644
--- a/src/backend/executor/execReplication.c
+++ b/src/backend/executor/execReplication.c
@@ -16,9 +16,11 @@
 
 #include "access/genam.h"
 #include "access/relscan.h"
+#include "access/table.h"
 #include "access/tableam.h"
 #include "access/transam.h"
 #include "access/xact.h"
+#include "catalog/pg_inherits.h"
 #include "commands/trigger.h"
 #include "executor/executor.h"
 #include "executor/nodeModifyTable.h"
@@ -645,20 +647,51 @@ CheckCmdReplicaIdentity(Relation rel, CmdType cmd)
  errhint("To enable deleting from the table, set REPLICA IDENTITY using ALTER TABLE.")));
 }
 
-
-/*
- * Check if we support writing into specific

Re: Segfault on logical replication to partitioned table with foreign children

2022-10-31 Thread Ilya Gladyshev 
On Sun, 2022-10-30 at 16:52 +0100, Alvaro Herrera wrote:
> On 2022-Oct-28, ilya.v.gladys...@gmail.com wrote:
> 
> > This will cause a segfault or raise an assert, because inserting
> > into
> > foreign tables via logical replication is not possible. The
> > solution I
> > propose is to add recursive checks of relkind for children of a
> > target,
> > if the target is a partitioned table.
> 
> However, I imagine that the only reason we don't support this is that
> the code hasn't been written yet. I think it would be better to write
> that code, so that we don't have to raise any error at all (unless
> the
> foreign table is something that doesn't support DML, in which case we
> would have to raise an error).  Of course, we still have to fix it in
> backbranches, but we can just do it as a targeted check at the moment
> of
> insert/update, not at the moment of subscription create/alter.
> 

Sure, this patch is just a quick fix. A proper implementation of
logical replication into foreign tables would be a much more difficult
undertaking. I think this patch is simple enough, the checks in the
patch are performed both on subscription DDL and when the relation is
opened for logical replication, so it gives both early feedback and
last-minute checks as well. All the code infrastructure for these kinds
of checks is already in place, so I think it's a good idea to use it.

P.S. sorry, duplicating the message, forgot to cc the mailing list the
first time

Re: Segfault on logical replication to partitioned table with foreign children

2022-10-31 Thread ilya . v . gladyshev 


On Sun, 2022-10-30 at 09:39 -0400, Tom Lane wrote:
> Dilip Kumar  writes:
> > Yes, this looks like a bug and your fix seems correct to me.  It
> > would
> > be nice to add a test case for this scenario.
> 
> A test case doesn't seem that exciting to me.  If we were trying to
> make it actually work, then yeah, but throwing an error isn't that
> useful to test.  The code will be exercised by replication to a
> regular partitioned table (I assume we do have tests for that).
> 
> What I'm wondering about is whether we can refactor this code
> to avoid so many usually-useless catalog lookups.  Pulling the
> namespace name, in particular, is expensive and we generally
> are not going to need the result.  In the child-rel case it'd
> be much better to pass the opened relation and let the error-check
> subroutine work from that.  Maybe we should just do it like that
> at the start of the recursion, too?  Or pass the relid and let
> the subroutine look up the names only in the error case.

Sure, I think passing in the opened relation is a good idea.

> A completely different line of thought is that this doesn't seem
> like a terribly bulletproof fix, since children could get added to
> a partitioned table after we look.  Maybe it'd be better to check
> the relkind at the last moment before we do something that depends
> on a child table being a relation.

These checks are run both on subscription DDL commands, which is good
to get some early feedback, and inside logical_rel_open(), right before
something useful is about to get done to the relation, so we should be
good here. I think some tests would actually be nice to verify this,
but I don't really have a strong opinion about it.

I'll refactor the patch and post a bit later.

Re: Segfault on logical replication to partitioned table with foreign children

2022-10-31 Thread Dilip Kumar 
On Sun, Oct 30, 2022 at 7:09 PM Tom Lane  wrote:
>
> Dilip Kumar  writes:
> > Yes, this looks like a bug and your fix seems correct to me.  It would
> > be nice to add a test case for this scenario.
>
> A test case doesn't seem that exciting to me.  If we were trying to
> make it actually work, then yeah, but throwing an error isn't that
> useful to test.  The code will be exercised by replication to a
> regular partitioned table (I assume we do have tests for that).

That's true, but we missed this case because of the absence of the
test case so I thought at least we can add it now to catch any future
bug in case of any behavior change.

> A completely different line of thought is that this doesn't seem
> like a terribly bulletproof fix, since children could get added to
> a partitioned table after we look.  Maybe it'd be better to check
> the relkind at the last moment before we do something that depends
> on a child table being a relation.

+1

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

RE: Segfault on logical replication to partitioned table with foreign children

2022-10-30 Thread shiy.f...@fujitsu.com 
On Sun, Oct 30, 2022 9:39 PM Tom Lane  wrote:
> 
> What I'm wondering about is whether we can refactor this code
> to avoid so many usually-useless catalog lookups.  Pulling the
> namespace name, in particular, is expensive and we generally
> are not going to need the result.  In the child-rel case it'd
> be much better to pass the opened relation and let the error-check
> subroutine work from that.  Maybe we should just do it like that
> at the start of the recursion, too?  Or pass the relid and let
> the subroutine look up the names only in the error case.
> 
> A completely different line of thought is that this doesn't seem
> like a terribly bulletproof fix, since children could get added to
> a partitioned table after we look.  Maybe it'd be better to check
> the relkind at the last moment before we do something that depends
> on a child table being a relation.
> 

I agree. So maybe we can add this check in apply_handle_tuple_routing().

diff --git a/src/backend/replication/logical/worker.c 
b/src/backend/replication/logical/worker.c
index 5250ae7f54..e941b68e4b 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -2176,6 +2176,10 @@ apply_handle_tuple_routing(ApplyExecutionData *edata,
Assert(partrelinfo != NULL);
partrel = partrelinfo->ri_RelationDesc;

+   /* Check for supported relkind. */
+   CheckSubscriptionRelkind(partrel->rd_rel->relkind,
+
relmapentry->remoterel.nspname, relmapentry->remoterel.relname);
+
/*
 * To perform any of the operations below, the tuple must match the
 * partition's rowtype. Convert if needed or just copy, using a 
dedicated


Regards,
Shi yu

Re: Segfault on logical replication to partitioned table with foreign children

2022-10-30 Thread Alvaro Herrera 
On 2022-Oct-28, ilya.v.gladys...@gmail.com wrote:

> This will cause a segfault or raise an assert, because inserting into
> foreign tables via logical replication is not possible. The solution I
> propose is to add recursive checks of relkind for children of a target,
> if the target is a partitioned table.

However, I imagine that the only reason we don't support this is that
the code hasn't been written yet. I think it would be better to write
that code, so that we don't have to raise any error at all (unless the
foreign table is something that doesn't support DML, in which case we
would have to raise an error).  Of course, we still have to fix it in
backbranches, but we can just do it as a targeted check at the moment of
insert/update, not at the moment of subscription create/alter.

-- 
Álvaro Herrera   48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"The eagle never lost so much time, as
when he submitted to learn of the crow." (William Blake)

Re: Segfault on logical replication to partitioned table with foreign children

2022-10-30 Thread Tom Lane 
Dilip Kumar  writes:
> Yes, this looks like a bug and your fix seems correct to me.  It would
> be nice to add a test case for this scenario.

A test case doesn't seem that exciting to me.  If we were trying to
make it actually work, then yeah, but throwing an error isn't that
useful to test.  The code will be exercised by replication to a
regular partitioned table (I assume we do have tests for that).

What I'm wondering about is whether we can refactor this code
to avoid so many usually-useless catalog lookups.  Pulling the
namespace name, in particular, is expensive and we generally
are not going to need the result.  In the child-rel case it'd
be much better to pass the opened relation and let the error-check
subroutine work from that.  Maybe we should just do it like that
at the start of the recursion, too?  Or pass the relid and let
the subroutine look up the names only in the error case.

A completely different line of thought is that this doesn't seem
like a terribly bulletproof fix, since children could get added to
a partitioned table after we look.  Maybe it'd be better to check
the relkind at the last moment before we do something that depends
on a child table being a relation.

regards, tom lane

Re: Segfault on logical replication to partitioned table with foreign children

2022-10-30 Thread Dilip Kumar 
On Sat, Oct 29, 2022 at 1:01 AM  wrote:
>
> Right now it is possible to add a partitioned table with foreign tables
> as its children as a target of a subscription. It can lead to an assert
> (or a segfault, if compiled without asserts) on a logical replication
> worker when the worker attempts to insert the data received via
> replication into the foreign table. Reproduce with caution, the worker
> is going to crash and restart indefinitely. The setup:

Yes, this looks like a bug and your fix seems correct to me.  It would
be nice to add a test case for this scenario.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Segfault on logical replication to partitioned table with foreign children

2022-10-28 Thread ilya . v . gladyshev 
Hi,

Right now it is possible to add a partitioned table with foreign tables
as its children as a target of a subscription. It can lead to an assert
(or a segfault, if compiled without asserts) on a logical replication
worker when the worker attempts to insert the data received via
replication into the foreign table. Reproduce with caution, the worker
is going to crash and restart indefinitely. The setup:

Publisher on 5432 port:

CREATE TABLE parent (id int, num int);
CREATE PUBLICATION parent_pub FOR TABLE parent;

Subscriber on 5433 port:

CREATE EXTENSION postgres_fdw;
CREATE SERVER loopback foreign data wrapper postgres_fdw options (host
'127.0.0.1', port '5433', dbname 'postgres');
CREATE USER MAPPING FOR CURRENT_USER SERVER loopback;
CREATE TABLE parent (id int, num int) partition by range (id);
CREATE FOREIGN TABLE p1 PARTITION OF parent DEFAULT SERVER loopback;
CREATE TABLE p1_loc(id int, num int);
CREATE SUBSCRIPTION parent_sub CONNECTION 'host=127.0.0.1 port=5432
dbname=postgres' PUBLICATION parent_pub;

Then run an insert on the publisher: INSERT INTO parent VALUES (1, 1);

This will cause a segfault or raise an assert, because inserting into
foreign tables via logical replication is not possible. The solution I
propose is to add recursive checks of relkind for children of a target,
if the target is a partitioned table. I have attached a patch for this
and managed to reproduce this on REL_14_STABLE as well, not sure if a
patch for that version is also needed.

Kind Regards,
Ilya Gladyshev

From 96f1fce8fb50f527b958de13a60f7324dd1ef052 Mon Sep 17 00:00:00 2001
From: Ilya Gladyshev 
Date: Fri, 28 Oct 2022 23:25:20 +0400
Subject: [PATCH] check relkind of subscription target recursively

---
 src/backend/commands/subscriptioncmds.c| 13 ---
 src/backend/executor/execReplication.c | 45 ++
 src/backend/replication/logical/relation.c |  7 ++--
 src/include/executor/executor.h|  4 +-
 4 files changed, 51 insertions(+), 18 deletions(-)

diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index f0cec2ad5e..e2dd6425e4 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -703,9 +703,10 @@ CreateSubscription(ParseState *pstate, CreateSubscriptionStmt *stmt,
 
 relid = RangeVarGetRelid(rv, AccessShareLock, false);
 
-/* Check for supported relkind. */
-CheckSubscriptionRelkind(get_rel_relkind(relid),
-		 rv->schemaname, rv->relname);
+/* Check for supported relkind recursively. */
+CheckSubscriptionRelation(relid,
+		  get_rel_relkind(relid),
+		  rv->schemaname, rv->relname);
 
 AddSubscriptionRelState(subid, relid, table_state,
 		InvalidXLogRecPtr);
@@ -864,9 +865,9 @@ AlterSubscription_refresh(Subscription *sub, bool copy_data,
 
 			relid = RangeVarGetRelid(rv, AccessShareLock, false);
 
-			/* Check for supported relkind. */
-			CheckSubscriptionRelkind(get_rel_relkind(relid),
-	 rv->schemaname, rv->relname);
+			/* Check for supported relkind recursively. */
+			CheckSubscriptionRelation(relid, get_rel_relkind(relid),
+	  rv->schemaname, rv->relname);
 
 			pubrel_local_oids[off++] = relid;
 
diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c
index 6014f2e248..98bc4a6618 100644
--- a/src/backend/executor/execReplication.c
+++ b/src/backend/executor/execReplication.c
@@ -16,9 +16,11 @@
 
 #include "access/genam.h"
 #include "access/relscan.h"
+#include "access/table.h"
 #include "access/tableam.h"
 #include "access/transam.h"
 #include "access/xact.h"
+#include "catalog/pg_inherits.h"
 #include "commands/trigger.h"
 #include "executor/executor.h"
 #include "executor/nodeModifyTable.h"
@@ -645,13 +647,7 @@ CheckCmdReplicaIdentity(Relation rel, CmdType cmd)
  errhint("To enable deleting from the table, set REPLICA IDENTITY using ALTER TABLE.")));
 }
 
-
-/*
- * Check if we support writing into specific relkind.
- *
- * The nspname and relname are only needed for error reporting.
- */
-void
+static void
 CheckSubscriptionRelkind(char relkind, const char *nspname,
 		 const char *relname)
 {
@@ -662,3 +658,38 @@ CheckSubscriptionRelkind(char relkind, const char *nspname,
 		nspname, relname),
  errdetail_relkind_not_supported(relkind)));
 }
+
+/*
+ * Recursively check if we support writing into specific relkind.
+ *
+ * The nspname and relname are only needed for error reporting.
+ */
+void
+CheckSubscriptionRelation(Oid relid, char relkind, const char *nspname,
+		  const char *relname)
+{
+	CheckSubscriptionRelkind(relkind, nspname, relname);
+
+	if (relkind == RELKIND_PARTITIONED_TABLE)
+	{
+		List *inheritors;
+		ListCell *lc;
+
+		inheritors = find_all_inheritors(relid,
+		 AccessShareLock,
+		 NULL);
+		foreach (lc, inheritors)
+		{
+			Oid child_oid = lfirst_oid(lc);
+			Relation child_rel =