RE: Skipping logical replication transactions on subscriber side

On Thursday, March 17, 2022 7:56 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
On Thu, Mar 17, 2022 at 5:52 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> wrote:
> >
> > On Thu, Mar 17, 2022 at 12:39 PM osumi(dot)takamichi(at)fujitsu(dot)com
> > <osumi(dot)takamichi(at)fujitsu(dot)com> wrote:
> > >
> > > On Thursday, March 17, 2022 3:04 PM Amit Kapila
> <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > > > On Wed, Mar 16, 2022 at 1:53 PM Masahiko Sawada
> > > > <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > > > >
> > > > > I've attached an updated version patch.
> > > > >
> > > >
> > > > The patch LGTM. I have made minor changes in comments and docs in
> > > > the attached patch. Kindly let me know what you think of the attached?
> > > Hi, thank you for the patch. Few minor comments.
> > >
> > >
> > > (3) apply_handle_commit_internal
> > >
> > ...
> > >
> > > I feel if we move those two functions at the end of the
> > > apply_handle_commit and apply_handle_stream_commit, then we will
> > > have more aligned codes and improve readability.
> > >
>
> I think we cannot just move them to the end of apply_handle_commit() and
> apply_handle_stream_commit(). Because if we do that, we end up missing
> updating replication_session_origin_lsn/timestamp when clearing the
> subskiplsn if we're skipping a non-stream transaction.
>
> Basically, the apply worker differently handles 2pc transactions and non-2pc
> transactions; we always prepare even empty transactions whereas we don't
> commit empty non-2pc transactions. So I think we don’t have to handle both in
> the same way.
Okay. Thank you so much for your explanation.
Then the code looks good to me.

Best Regards,
Takamichi Osumi