Re: pg_decode_message vs skip_empty_xacts and xact_wrote_changes

On Wed, Jul 5, 2023 at 7:20 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Wed, Jul 5, 2023 at 2:29 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Wed, Jul 5, 2023 at 2:28 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Mon, Jul 3, 2023 at 4:49 PM vignesh C <vignesh21@gmail.com> wrote:
> > > >
> > > > +1 for the first version patch, I also felt the first version is
> > > > easily understandable.
> > > >
> > >
> > > 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.
> > >
> >
> > 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.
>

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.