On Wed, Jul 5, 2023 at 7:20 PM Ashutosh Bapat <ashutosh.bapat....@gmail.com> wrote: > > On Wed, Jul 5, 2023 at 2:29 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > On Wed, Jul 5, 2023 at 2:28 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > > > On Mon, Jul 3, 2023 at 4:49 PM vignesh C <vignes...@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.
