On Mon, 2022-10-31 at 03:20 +0000, shiy.fnst@fujitsu.com wrote:
> On Sun, Oct 30, 2022 9:39 PM Tom Lane <tgl@sss.pgh.pa.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
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.
