Re: pg_decode_message vs skip_empty_xacts and xact_wrote_changes

On Thu, Jul 6, 2023 at 2:06 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> 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.
>

I have pushed this work. But feel free to propose further
improvements, if you have any better ideas.

--
With Regards,
Amit Kapila.