RE: Skipping logical replication transactions on subscriber side

On Wednesday, March 16, 2022 11:33 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Tue, Mar 15, 2022 at 7:30 PM osumi.takamichi@fujitsu.com
> <osumi.takamichi@fujitsu.com> wrote:
> >
> > On Tuesday, March 15, 2022 3:13 PM Masahiko Sawada
> <sawada.mshk@gmail.com> wrote:
> > > I've attached an updated version patch.
> >
> > A couple of minor comments on v14.
> >
> > (1) apply_handle_commit_internal
> >
> >
> > +       if (is_skipping_changes())
> > +       {
> > +               stop_skipping_changes();
> > +
> > +               /*
> > +                * Start a new transaction to clear the subskipxid, if not
> started
> > +                * yet. The transaction is committed below.
> > +                */
> > +               if (!IsTransactionState())
> > +                       StartTransactionCommand();
> > +       }
> > +
> >
> > I suppose we can move this condition check and stop_skipping_changes()
> > call to the inside of the block we enter when IsTransactionState() returns
> true.
> >
> > As the comment of apply_handle_commit_internal() mentions, it's the
> > helper function for apply_handle_commit() and
> > apply_handle_stream_commit().
> >
> > Then, I couldn't think that both callers don't open a transaction
> > before the call of apply_handle_commit_internal().
> > For applying spooled messages, we call begin_replication_step as well.
> >
> > I can miss something, but timing when we receive COMMIT message
> > without opening a transaction, would be the case of empty transactions
> > where the subscription (and its subscription worker) is not interested.
> >
> 
> I think when we skip non-streamed transactions we don't start a transaction.
> So, if we do what you are suggesting, we will miss to clear the skip_lsn after
> skipping the transaction.
OK, this is what I missed.

On the other hand, what I was worried about is that
empty transaction can start skipping changes,
if the subskiplsn is equal to the finish LSN for
the empty transaction. The reason is we call
maybe_start_skipping_changes even for empty ones
and set skip_xact_finish_lsn by the finish LSN in that case.

I checked I could make this happen with debugger and some logs for LSN.
What I did is just having two pairs of pub/sub
and conduct a change for one of them,
after I set a breakpoint in the logicalrep_write_begin
on the walsender that will issue an empty transaction.
Then, I check the finish LSN of it and
conduct an alter subscription skip lsn command with this LSN value.
As a result, empty transaction calls stop_skipping_changes
in the apply_handle_commit_internal and then
enter the block for IsTransactionState == true,
which would not happen before applying the patch.

Also, this behavior looks contradicted with some comments in worker.c
"The subskiplsn is cleared after successfully skipping the transaction
or applying non-empty transaction." so, I was just confused and
wrote the above comment.

I think this would not happen in practice, then
it might be OK without a special measure for this,
but I wasn't sure.


Best Regards,
	Takamichi Osumi