Re: Skipping logical replication transactions on subscriber side

On Wed, Mar 16, 2022 at 1:20 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Wed, Mar 16, 2022 at 7:58 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Wed, Mar 16, 2022 at 6:03 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > >
> > > On Tue, Mar 15, 2022 at 7:18 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > > >
> > > > On Tue, Mar 15, 2022 at 11:43 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > > > >
> > > >
> > > > 6.
> > > > @@ -1583,7 +1649,8 @@ apply_handle_insert(StringInfo s)
> > > > TupleTableSlot *remoteslot;
> > > > MemoryContext oldctx;
> > > >
> > > > - if (handle_streamed_transaction(LOGICAL_REP_MSG_INSERT, s))
> > > > + if (is_skipping_changes() ||
> > > >
> > > > Is there a reason to keep the skip_changes check here and in other DML
> > > > operations instead of at one central place in apply_dispatch?
> > >
> > > Since we already have the check of applying the change on the spot at
> > > the beginning of the handlers I feel it's better to add
> > > is_skipping_changes() to the check than add a new if statement to
> > > apply_dispatch, but do you prefer to check it in one central place in
> > > apply_dispatch?
> > >
> >
> > I think either way is fine. I just wanted to know the reason, your
> > current change looks okay to me.
> >
> > Some questions/comments
> > ======================
> >
>
> Some cosmetic suggestions:
> ======================
> 1.
> +# Create subscriptions. Both subscription sets disable_on_error to on
> +# so that they get disabled when a conflict occurs.
> +$node_subscriber->safe_psql(
> + 'postgres',
> + qq[
> +CREATE SUBSCRIPTION $subname CONNECTION '$publisher_connstr'
> PUBLICATION tap_pub WITH (streaming = on, two_phase = on,
> disable_on_error = on);
> +]);
>
> I don't understand what you mean by 'Both subscription ...' in the
> above comments.

Fixed.

>
> 2.
> + # Check the log indicating that successfully skipped the transaction,
>
> How about slightly rephrasing this to: "Check the log to ensure that
> the transaction is skipped...."?

Fixed.

I've attached an updated version patch.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/