Subject: Re: pg_decode_message vs skip_empty_xacts and xact_wrote_changes

From: Amit Kapila <[email-masked]>
Date: Fri, 30 Jun 2023 09:55:03 +0530

On Thu, Jun 29, 2023 at 9:40 PM vignesh C <[email-masked]> wrote:
>
> On Thu, 29 Jun 2023 at 09:58, Zhijie Hou (Fujitsu)
> <[email-masked]> wrote:
> >
> > On Thursday, June 29, 2023 12:06 PM vignesh C <[email-masked]> wrote:
> > >
> >
> > 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.
>

I feel the use of last_write has reduced the readability of this part
of the code. It may be that we can add comments to make it clear but I
feel your previous version was much easier to understand.

-- 
With Regards,
Amit Kapila.

---

From: vignesh C <[email-masked]>
Date: Mon, 3 Jul 2023 16:49:40 +0530

> feel your previous version was much easier to understand.

+1 for the first version patch, I also felt the first version is
easily understandable.

Regards,
Vignesh

---

From: Amit Kapila <[email-masked]>
Date: Wed, 5 Jul 2023 14:28:00 +0530

> +1 for the first version patch, I also felt the first version is

Okay, please find the slightly updated version (changed a comment and
commit message). Unless there are more comments, I'll push this in a
day or two.

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Wed, 5 Jul 2023 14:28:56 +0530

> commit message). Unless there are more comments, I'll push this in a

oops, forgot to attach the patch.

-- 
With Regards,
Amit Kapila.

---

From: Ashutosh Bapat <[email-masked]>
Date: Wed, 5 Jul 2023 19:20:06 +0530

> oops, forgot to attach the patch.

I still think that we need to do something so that a new call to
pg_output_begin() automatically takes care of the conditions under
which it should be called. Otherwise, we will introduce a similar
problem in some other place in future.

-- 
Best Wishes,
Ashutosh Bapat

---

From: Amit Kapila <[email-masked]>
Date: Thu, 6 Jul 2023 14:06:32 +0530

> problem in some other place in future.

AFAIU, this problem is because we forget to conditionally call
pg_output_begin() from pg_decode_message() which can happen with or
without moving conditions inside pg_output_begin(). Also, it shouldn't
be done at the expense of complexity. I find the check added by
Vignesh's v2 patch (+ if (!(last_write ^ data->skip_empty_xacts) ||
txndata->xact_wrote_changes)) a bit difficult to understand and more
error-prone. The others like Hou-San also couldn't understand unless
Vignesh gave an explanation. I also thought of avoiding that check.
Basically, IIUC, the check is added because the patch also removed
'data->skip_empty_xacts' check from
pg_decode_begin_txn()/pg_decode_begin_prepare_txn(). Now, if retain
those checks then it is probably okay but again the similar checks are
still split and that doesn't appear to be better than the v1 or v3
patch version. I am not against improving code in this area and
probably we can consider doing it as a separate patch if we have
better ideas instead of combining it with this patch.

-- 
With Regards,
Amit Kapila.