From: Tom Lane <tgl@sss.pgh.pa.us>
To: Ilya Gladyshev <ilya.v.gladyshev@gmail.com>
cc: "shiy.fnst@fujitsu.com" <shiy.fnst@fujitsu.com>,
        Dilip Kumar <dilipbalaut@gmail.com>,
        "pgsql-hackers@lists.postgresql.org"
 <pgsql-hackers@lists.postgresql.org>
Subject: Re: Segfault on logical replication to partitioned table with foreign
 children
In-reply-to: <04b5212cd549ee3c8efc63f6a92d85d5463bfdf1.camel@gmail.com>
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>
Comments: In-reply-to Ilya Gladyshev <ilya.v.gladyshev@gmail.com>
	message dated "Tue, 01 Nov 2022 02:02:42 +0400"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <1361523.1667329669.0@sss.pgh.pa.us>
Date: Tue, 01 Nov 2022 15:10:14 -0400
Message-ID: <1361869.1667329814@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/1361869.1667329814%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1361523.1667329669.1@sss.pgh.pa.us>

Ilya Gladyshev <ilya.v.gladyshev@gmail.com> 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


------- =_aaaaaaaaaa0
Content-Type: text/x-diff;
	name="v3-0001-check-target-partition-relkind.patch";
	charset="us-ascii"
Content-ID: <1361523.1667329669.2@sss.pgh.pa.us>
Content-Description: v3-0001-check-target-partition-relkind.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/replication/logical/worker.c b/src/backend/replic=
ation/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 *edat=
a,
 	Assert(partrelinfo !=3D NULL);
 	partrel =3D 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 checki=
ng
+	 * 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 dedicate=
d

------- =_aaaaaaaaaa0--