Re: pg_decode_message vs skip_empty_xacts and xact_wrote_changes

On Thu, 29 Jun 2023 at 09:58, Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> On Thursday, June 29, 2023 12:06 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Wed, 28 Jun 2023 at 19:26, Ashutosh Bapat
> > <ashutosh.bapat.oss@gmail.com> wrote:
> > >
> > > Hi Vignesh,
> > > Thanks for working on this.
> > >
> > > On Wed, Jun 28, 2023 at 4:52 PM vignesh C <vignesh21@gmail.com> wrote:
> > > >
> > > > Here is a patch having the fix for the same. I have not added any
> > > > tests as the existing tests cover this scenario. The same issue is
> > > > present in back branches too.
> > >
> > > Interesting, we have a test for this scenario and it accepts erroneous
> > > output :).
> > >
> > > > v1-0001-Call-pg_output_begin-in-pg_decode_message-if-it-i_master.pat
> > > > ch can be applied on master, PG15 and PG14,
> > > > v1-0001-Call-pg_output_begin-in-pg_decode_message-if-it-i_PG13.patch
> > > > patch can be applied on PG13, PG12 and PG11.
> > > > Thoughts?
> > >
> > > I noticed this when looking at Tomas's patches for logical decoding of
> > > sequences. The code block you have added is repeated in
> > > pg_decode_change() and pg_decode_truncate(). It might be better to
> > > push the conditions in pg_output_begin() itself so that any future
> > > callsite of pg_output_begin() automatically takes care of these
> > > conditions.
> >
> > Thanks for the comments, here is an updated patch handling the above issue.
>
> Thanks for the patches.
>
> I tried to understand the following check:
>
>         /*
>          * If asked to skip empty transactions, we'll emit BEGIN at the point
>          * where the first operation is received for this transaction.
>          */
> -       if (data->skip_empty_xacts)
> +       if (!(last_write ^ data->skip_empty_xacts) || txndata->xact_wrote_changes)
>                 return;
>
> I might miss something, but would you mind elaborating on why we use "last_write" in this check?

last_write is used to indicate if it is begin/"begin
prepare"(last_write is true) or change/truncate/message(last_write is
false).

We have specified logical XNOR which will be true for the following conditions:
Condition1: last_write && data->skip_empty_xacts  -> If it is
begin/begin prepare and user has specified skip empty transactions, we
will return from here, so that the begin message can be appended at
the point where the first operation is received for this transaction.
Condition2: !last_write && !data->skip_empty_xacts -> If it is
change/truncate or message and user has not specified skip empty
transactions, we will return from here as we would have appended the
begin earlier itself.
The txndata->xact_w6rote_changes will be set after the first operation
is received for this transaction during which we would have outputted
the begin message, this condition is to skip outputting begin message
if the begin message was already outputted.

Regards,
Vignesh