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

On Sun, Oct 30, 2022 9:39 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> 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