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

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

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