Re: Handle infinite recursion in logical replication setup

On Thu, Mar 31, 2022 at 2:52 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Thu, Mar 31, 2022 at 9:14 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Wed, Mar 30, 2022 at 7:40 PM Ashutosh Bapat
> > <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
> > > >
> > > > The changes for the same are available int the v5 patch available at [1].
> > > > [1] - https://www.postgresql.org/message-id/CALDaNm3wCf0YcvVo%2BgHMGpupk9K6WKJxCyLUvhPC2GkPKRZUWA%40mail.gmail.com
> > > >
> > >
> > > cb->truncate_cb = pg_decode_truncate;
> > > cb->commit_cb = pg_decode_commit_txn;
> > > cb->filter_by_origin_cb = pg_decode_filter;
> > > + cb->filter_remote_origin_cb = pg_decode_filter_remote_origin;
> > >
> > > Why do we need a new hook? Can we use filter_by_origin_cb?
> > >
> >
> > I also think there is no need for a new hook in this case but I might
> > also be missing something that Vignesh has in his mind.
> >
> > > Also it looks like
> > > implementation of pg_decode_filter and pg_decode_filter_remote_origin is same,
> > > unless my eyes are deceiving me.
> > >
> > > - <literal>binary</literal>, <literal>streaming</literal>, and
> > > - <literal>disable_on_error</literal>.
> > > + <literal>binary</literal>, <literal>streaming</literal>,
> > > + <literal>disable_on_error</literal> and
> > > + <literal>publish_local_only</literal>.
> > >
> > > "publish_local_only" as a "subscription" option looks odd. Should it be
> > > "subscribe_local_only"?
> > >
> >
> > I also think "subscribe_local_only" fits better here.
> >
>
> About 0002 patch,
> Commit message:
> ------
> If a subscription is created to Node1 from Node3 with publish_local_only
> and copy_data as ON, then throw an error so that user can handle
> creation of subscription with table having consistent data.
> ------
> Do you want to refer to Node2 instead of Node3 here as Node3 doesn't
> make sense in the description?

It should be Node3, I have added more details in the commit message
mentioning about the scenario.

> Also, you haven't explained anywhere in the patch why
> 'publish_local_only' (or whatever we call it) won't work for initial
> sync? IIUC, it is because we can identify origin changes only based on
> WAL and initial sync directly copies data from the heap. Am, I missing
> something here?

I have added the explanation where we are throwing the error.

The v6 patch at [1] has the changes for the same.
[1] - https://www.postgresql.org/message-id/CALDaNm16eBx2BjLFjfFHSU4pb25HmgV--M692OPgH_91Dwn%3D2g%40mail.gmail.com

Regards,
Vignesh