Обсуждение: Add macros for ReorderBufferTXN toptxn

Hi,

During a recent code review, I was confused multiple times by the
toptxn member of ReorderBufferTXN, which is defined only for
sub-transactions.

e.g. txn->toptxn member == NULL means the txn is a top level txn.
e.g. txn->toptxn member != NULL means the txn is not a top level txn

It makes sense if you squint and read it slowly enough, but IMO it's
too easy to accidentally misinterpret the meaning when reading code
that uses this member.

~

Such code can be made easier to read just by introducing some simple macros:

#define isa_toptxn(rbtxn) (rbtxn->toptxn == NULL)
#define isa_subtxn(rbtxn) (rbtxn->toptxn != NULL)
#define get_toptxn(rbtxn) (isa_subtxn(rbtxn) ? rbtxn->toptxn : rbtxn)

~

PSA a small patch that does this.

(Tests OK using make check-world)

Thoughts?

------
Kind Regards,
Peter Smith.
Fujitsu Australia

On Fri, Mar 10, 2023 at 4:36 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> During a recent code review, I was confused multiple times by the
> toptxn member of ReorderBufferTXN, which is defined only for
> sub-transactions.
>
> e.g. txn->toptxn member == NULL means the txn is a top level txn.
> e.g. txn->toptxn member != NULL means the txn is not a top level txn
>
> It makes sense if you squint and read it slowly enough, but IMO it's
> too easy to accidentally misinterpret the meaning when reading code
> that uses this member.
>
> ~
>
> Such code can be made easier to read just by introducing some simple macros:
>
> #define isa_toptxn(rbtxn) (rbtxn->toptxn == NULL)
> #define isa_subtxn(rbtxn) (rbtxn->toptxn != NULL)
> #define get_toptxn(rbtxn) (isa_subtxn(rbtxn) ? rbtxn->toptxn : rbtxn)
>
> ~
>
> PSA a small patch that does this.
>

I also find it will make code easier to read. So, +1 to the idea. I'll
do the detailed review and test next week unless there are objections
to the idea.

--
With Regards,
Amit Kapila.

On Fri, 10 Mar 2023 at 04:36, Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi,
>
> During a recent code review, I was confused multiple times by the
> toptxn member of ReorderBufferTXN, which is defined only for
> sub-transactions.
>
> e.g. txn->toptxn member == NULL means the txn is a top level txn.
> e.g. txn->toptxn member != NULL means the txn is not a top level txn
>
> It makes sense if you squint and read it slowly enough, but IMO it's
> too easy to accidentally misinterpret the meaning when reading code
> that uses this member.
>
> ~
>
> Such code can be made easier to read just by introducing some simple macros:
>
> #define isa_toptxn(rbtxn) (rbtxn->toptxn == NULL)
> #define isa_subtxn(rbtxn) (rbtxn->toptxn != NULL)
> #define get_toptxn(rbtxn) (isa_subtxn(rbtxn) ? rbtxn->toptxn : rbtxn)
>
> ~
>
> PSA a small patch that does this.
>
> (Tests OK using make check-world)
>
> Thoughts?

Few comments:
1) Can we move the macros along with the other macros present in this
file, just above this structure, similar to the macros added for
txn_flags:
        /* Toplevel transaction for this subxact (NULL for top-level). */
+#define isa_toptxn(rbtxn) (rbtxn->toptxn == NULL)
+#define isa_subtxn(rbtxn) (rbtxn->toptxn != NULL)
+#define get_toptxn(rbtxn) (isa_subtxn(rbtxn) ? rbtxn->toptxn : rbtxn)

2) The macro name can be changed to rbtxn_is_toptxn, rbtxn_is_subtxn
and rbtxn_get_toptxn to keep it consistent with others:
        /* Toplevel transaction for this subxact (NULL for top-level). */
+#define isa_toptxn(rbtxn) (rbtxn->toptxn == NULL)
+#define isa_subtxn(rbtxn) (rbtxn->toptxn != NULL)
+#define get_toptxn(rbtxn) (isa_subtxn(rbtxn) ? rbtxn->toptxn : rbtxn)

3) We could add separate comments for each of the macros:
        /* Toplevel transaction for this subxact (NULL for top-level). */
+#define isa_toptxn(rbtxn) (rbtxn->toptxn == NULL)
+#define isa_subtxn(rbtxn) (rbtxn->toptxn != NULL)
+#define get_toptxn(rbtxn) (isa_subtxn(rbtxn) ? rbtxn->toptxn : rbtxn)

4) We check if txn->toptxn is not null twice here both in if condition
and in the assignment, we could retain the assignment operation as
earlier to remove the 2nd check:
-       if (txn->toptxn)
-               txn = txn->toptxn;
+       if (isa_subtxn(txn))
+               txn = get_toptxn(txn);

We could avoid one check again by:
+       if (isa_subtxn(txn))
+               txn = txn->toptxn;

Regards,
Vignesh

Thanks for the review!

On Mon, Mar 13, 2023 at 6:19 PM vignesh C <vignesh21@gmail.com> wrote:
...
> Few comments:
> 1) Can we move the macros along with the other macros present in this
> file, just above this structure, similar to the macros added for
> txn_flags:
>         /* Toplevel transaction for this subxact (NULL for top-level). */
> +#define isa_toptxn(rbtxn) (rbtxn->toptxn == NULL)
> +#define isa_subtxn(rbtxn) (rbtxn->toptxn != NULL)
> +#define get_toptxn(rbtxn) (isa_subtxn(rbtxn) ? rbtxn->toptxn : rbtxn)
>
> 2) The macro name can be changed to rbtxn_is_toptxn, rbtxn_is_subtxn
> and rbtxn_get_toptxn to keep it consistent with others:
>         /* Toplevel transaction for this subxact (NULL for top-level). */
> +#define isa_toptxn(rbtxn) (rbtxn->toptxn == NULL)
> +#define isa_subtxn(rbtxn) (rbtxn->toptxn != NULL)
> +#define get_toptxn(rbtxn) (isa_subtxn(rbtxn) ? rbtxn->toptxn : rbtxn)
>
> 3) We could add separate comments for each of the macros:
>         /* Toplevel transaction for this subxact (NULL for top-level). */
> +#define isa_toptxn(rbtxn) (rbtxn->toptxn == NULL)
> +#define isa_subtxn(rbtxn) (rbtxn->toptxn != NULL)
> +#define get_toptxn(rbtxn) (isa_subtxn(rbtxn) ? rbtxn->toptxn : rbtxn)
>

All the above are fixed as suggested.

> 4) We check if txn->toptxn is not null twice here both in if condition
> and in the assignment, we could retain the assignment operation as
> earlier to remove the 2nd check:
> -       if (txn->toptxn)
> -               txn = txn->toptxn;
> +       if (isa_subtxn(txn))
> +               txn = get_toptxn(txn);
>
> We could avoid one check again by:
> +       if (isa_subtxn(txn))
> +               txn = txn->toptxn;
>

Yeah, that is true, but I chose not to keep the original assignment in
this case mainly because then it is consistent with the other changed
code ---  e.g. Every other direct member assignment/access of the
'toptxn' member now hides behind the macros so I did not want this
single place to be the odd one out. TBH, I don't think 1 extra check
is of any significance, but it is not a problem to change like you
suggested if other people also want it done that way.

------
Kind Regards,
Peter Smith.
Fujitsu Australia.

On Tue, 14 Mar 2023 at 12:37, Peter Smith <smithpb2250@gmail.com> wrote:
>
> Thanks for the review!
>
> On Mon, Mar 13, 2023 at 6:19 PM vignesh C <vignesh21@gmail.com> wrote:
> ...
> > Few comments:
> > 1) Can we move the macros along with the other macros present in this
> > file, just above this structure, similar to the macros added for
> > txn_flags:
> >         /* Toplevel transaction for this subxact (NULL for top-level). */
> > +#define isa_toptxn(rbtxn) (rbtxn->toptxn == NULL)
> > +#define isa_subtxn(rbtxn) (rbtxn->toptxn != NULL)
> > +#define get_toptxn(rbtxn) (isa_subtxn(rbtxn) ? rbtxn->toptxn : rbtxn)
> >
> > 2) The macro name can be changed to rbtxn_is_toptxn, rbtxn_is_subtxn
> > and rbtxn_get_toptxn to keep it consistent with others:
> >         /* Toplevel transaction for this subxact (NULL for top-level). */
> > +#define isa_toptxn(rbtxn) (rbtxn->toptxn == NULL)
> > +#define isa_subtxn(rbtxn) (rbtxn->toptxn != NULL)
> > +#define get_toptxn(rbtxn) (isa_subtxn(rbtxn) ? rbtxn->toptxn : rbtxn)
> >
> > 3) We could add separate comments for each of the macros:
> >         /* Toplevel transaction for this subxact (NULL for top-level). */
> > +#define isa_toptxn(rbtxn) (rbtxn->toptxn == NULL)
> > +#define isa_subtxn(rbtxn) (rbtxn->toptxn != NULL)
> > +#define get_toptxn(rbtxn) (isa_subtxn(rbtxn) ? rbtxn->toptxn : rbtxn)
> >
>
> All the above are fixed as suggested.
>
> > 4) We check if txn->toptxn is not null twice here both in if condition
> > and in the assignment, we could retain the assignment operation as
> > earlier to remove the 2nd check:
> > -       if (txn->toptxn)
> > -               txn = txn->toptxn;
> > +       if (isa_subtxn(txn))
> > +               txn = get_toptxn(txn);
> >
> > We could avoid one check again by:
> > +       if (isa_subtxn(txn))
> > +               txn = txn->toptxn;
> >
>
> Yeah, that is true, but I chose not to keep the original assignment in
> this case mainly because then it is consistent with the other changed
> code ---  e.g. Every other direct member assignment/access of the
> 'toptxn' member now hides behind the macros so I did not want this
> single place to be the odd one out. TBH, I don't think 1 extra check
> is of any significance, but it is not a problem to change like you
> suggested if other people also want it done that way.

The same issue exists here too:
1)
-       if (toptxn != NULL && !rbtxn_has_catalog_changes(toptxn))
+       if (rbtxn_is_subtxn(txn))
        {
-               toptxn->txn_flags |= RBTXN_HAS_CATALOG_CHANGES;
-               dclist_push_tail(&rb->catchange_txns, &toptxn->catchange_node);
+               ReorderBufferTXN *toptxn = rbtxn_get_toptxn(txn);

2)
 -       if (change->txn->toptxn)
-               topxid = change->txn->toptxn->xid;
+       if (rbtxn_is_subtxn(change->txn))
+               topxid = rbtxn_get_toptxn(change->txn)->xid;

If you plan to fix, bothe the above also should be handled.

3) The comment on top of rbtxn_get_toptxn could be kept similar in
both the below places. I know it is not because of your change, but
since it is a very small change probably we could include it along
with this patch:
@@ -717,10 +717,7 @@ ReorderBufferProcessPartialChange(ReorderBuffer
*rb, ReorderBufferTXN *txn,
                return;

        /* Get the top transaction. */
-       if (txn->toptxn != NULL)
-               toptxn = txn->toptxn;
-       else
-               toptxn = txn;
+       toptxn = rbtxn_get_toptxn(txn);

        /*
         * Indicate a partial change for toast inserts.  The change will be
@@ -812,10 +809,7 @@ ReorderBufferQueueChange(ReorderBuffer *rb,
TransactionId xid, XLogRecPtr lsn,
                ReorderBufferTXN *toptxn;

                /* get the top transaction */
-               if (txn->toptxn != NULL)
-                       toptxn = txn->toptxn;
-               else
-                       toptxn = txn;
+               toptxn = rbtxn_get_toptxn(txn);

Regards,
Vignesh

On Tue, Mar 14, 2023 at 12:37 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Thanks for the review!
>
> On Mon, Mar 13, 2023 at 6:19 PM vignesh C <vignesh21@gmail.com> wrote:
> ...
>
> > 4) We check if txn->toptxn is not null twice here both in if condition
> > and in the assignment, we could retain the assignment operation as
> > earlier to remove the 2nd check:
> > -       if (txn->toptxn)
> > -               txn = txn->toptxn;
> > +       if (isa_subtxn(txn))
> > +               txn = get_toptxn(txn);
> >
> > We could avoid one check again by:
> > +       if (isa_subtxn(txn))
> > +               txn = txn->toptxn;
> >
>
> Yeah, that is true, but I chose not to keep the original assignment in
> this case mainly because then it is consistent with the other changed
> code ---  e.g. Every other direct member assignment/access of the
> 'toptxn' member now hides behind the macros so I did not want this
> single place to be the odd one out. TBH, I don't think 1 extra check
> is of any significance, but it is not a problem to change like you
> suggested if other people also want it done that way.
>

Can't we directly use rbtxn_get_toptxn() for this case? I think that
way code will look neat. I see that it is not exactly matching the
existing check so you might be worried but I feel the new code will
achieve the same purpose and will be easy to follow.

--
With Regards,
Amit Kapila.

On Tue, Mar 14, 2023 at 5:03 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Tue, 14 Mar 2023 at 12:37, Peter Smith <smithpb2250@gmail.com> wrote:
> >
>
> The same issue exists here too:
> 1)
> -       if (toptxn != NULL && !rbtxn_has_catalog_changes(toptxn))
> +       if (rbtxn_is_subtxn(txn))
>         {
> -               toptxn->txn_flags |= RBTXN_HAS_CATALOG_CHANGES;
> -               dclist_push_tail(&rb->catchange_txns, &toptxn->catchange_node);
> +               ReorderBufferTXN *toptxn = rbtxn_get_toptxn(txn);
>
> 2)
>  -       if (change->txn->toptxn)
> -               topxid = change->txn->toptxn->xid;
> +       if (rbtxn_is_subtxn(change->txn))
> +               topxid = rbtxn_get_toptxn(change->txn)->xid;
>
> If you plan to fix, bothe the above also should be handled.
>

I don't know if it would be any better to change the above two as
compared to what the proposed patch has.

--
With Regards,
Amit Kapila.

On Tue, Mar 14, 2023 at 10:43 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Mar 14, 2023 at 12:37 PM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > Thanks for the review!
> >
> > On Mon, Mar 13, 2023 at 6:19 PM vignesh C <vignesh21@gmail.com> wrote:
> > ...
> >
> > > 4) We check if txn->toptxn is not null twice here both in if condition
> > > and in the assignment, we could retain the assignment operation as
> > > earlier to remove the 2nd check:
> > > -       if (txn->toptxn)
> > > -               txn = txn->toptxn;
> > > +       if (isa_subtxn(txn))
> > > +               txn = get_toptxn(txn);
> > >
> > > We could avoid one check again by:
> > > +       if (isa_subtxn(txn))
> > > +               txn = txn->toptxn;
> > >
> >
> > Yeah, that is true, but I chose not to keep the original assignment in
> > this case mainly because then it is consistent with the other changed
> > code ---  e.g. Every other direct member assignment/access of the
> > 'toptxn' member now hides behind the macros so I did not want this
> > single place to be the odd one out. TBH, I don't think 1 extra check
> > is of any significance, but it is not a problem to change like you
> > suggested if other people also want it done that way.
> >
>
> Can't we directly use rbtxn_get_toptxn() for this case? I think that
> way code will look neat. I see that it is not exactly matching the
> existing check so you might be worried but I feel the new code will
> achieve the same purpose and will be easy to follow.
>

OK. Done as suggested.

PSA v3.

------
Kind Regards,
Peter Smith.
Fujitsu Australia

On Tue, Mar 14, 2023 at 10:33 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Tue, 14 Mar 2023 at 12:37, Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > Thanks for the review!
> >
> > On Mon, Mar 13, 2023 at 6:19 PM vignesh C <vignesh21@gmail.com> wrote:
> > ...
> > > Few comments:
> > > 1) Can we move the macros along with the other macros present in this
> > > file, just above this structure, similar to the macros added for
> > > txn_flags:
> > >         /* Toplevel transaction for this subxact (NULL for top-level). */
> > > +#define isa_toptxn(rbtxn) (rbtxn->toptxn == NULL)
> > > +#define isa_subtxn(rbtxn) (rbtxn->toptxn != NULL)
> > > +#define get_toptxn(rbtxn) (isa_subtxn(rbtxn) ? rbtxn->toptxn : rbtxn)
> > >
> > > 2) The macro name can be changed to rbtxn_is_toptxn, rbtxn_is_subtxn
> > > and rbtxn_get_toptxn to keep it consistent with others:
> > >         /* Toplevel transaction for this subxact (NULL for top-level). */
> > > +#define isa_toptxn(rbtxn) (rbtxn->toptxn == NULL)
> > > +#define isa_subtxn(rbtxn) (rbtxn->toptxn != NULL)
> > > +#define get_toptxn(rbtxn) (isa_subtxn(rbtxn) ? rbtxn->toptxn : rbtxn)
> > >
> > > 3) We could add separate comments for each of the macros:
> > >         /* Toplevel transaction for this subxact (NULL for top-level). */
> > > +#define isa_toptxn(rbtxn) (rbtxn->toptxn == NULL)
> > > +#define isa_subtxn(rbtxn) (rbtxn->toptxn != NULL)
> > > +#define get_toptxn(rbtxn) (isa_subtxn(rbtxn) ? rbtxn->toptxn : rbtxn)
> > >
> >
> > All the above are fixed as suggested.
> >
> > > 4) We check if txn->toptxn is not null twice here both in if condition
> > > and in the assignment, we could retain the assignment operation as
> > > earlier to remove the 2nd check:
> > > -       if (txn->toptxn)
> > > -               txn = txn->toptxn;
> > > +       if (isa_subtxn(txn))
> > > +               txn = get_toptxn(txn);
> > >
> > > We could avoid one check again by:
> > > +       if (isa_subtxn(txn))
> > > +               txn = txn->toptxn;
> > >
> >
> > Yeah, that is true, but I chose not to keep the original assignment in
> > this case mainly because then it is consistent with the other changed
> > code ---  e.g. Every other direct member assignment/access of the
> > 'toptxn' member now hides behind the macros so I did not want this
> > single place to be the odd one out. TBH, I don't think 1 extra check
> > is of any significance, but it is not a problem to change like you
> > suggested if other people also want it done that way.
>
> The same issue exists here too:
> 1)
> -       if (toptxn != NULL && !rbtxn_has_catalog_changes(toptxn))
> +       if (rbtxn_is_subtxn(txn))
>         {
> -               toptxn->txn_flags |= RBTXN_HAS_CATALOG_CHANGES;
> -               dclist_push_tail(&rb->catchange_txns, &toptxn->catchange_node);
> +               ReorderBufferTXN *toptxn = rbtxn_get_toptxn(txn);
>
> 2)
>  -       if (change->txn->toptxn)
> -               topxid = change->txn->toptxn->xid;
> +       if (rbtxn_is_subtxn(change->txn))
> +               topxid = rbtxn_get_toptxn(change->txn)->xid;
>
> If you plan to fix, bothe the above also should be handled.

OK, noted. Anyway, for now, I preferred the 'toptxn' member to be
consistently hidden in the code so I don't plan to remove those
macros.

Also, please see Amit's reply [1] to your suggestion.

>
> 3) The comment on top of rbtxn_get_toptxn could be kept similar in
> both the below places. I know it is not because of your change, but
> since it is a very small change probably we could include it along
> with this patch:
> @@ -717,10 +717,7 @@ ReorderBufferProcessPartialChange(ReorderBuffer
> *rb, ReorderBufferTXN *txn,
>                 return;
>
>         /* Get the top transaction. */
> -       if (txn->toptxn != NULL)
> -               toptxn = txn->toptxn;
> -       else
> -               toptxn = txn;
> +       toptxn = rbtxn_get_toptxn(txn);
>
>         /*
>          * Indicate a partial change for toast inserts.  The change will be
> @@ -812,10 +809,7 @@ ReorderBufferQueueChange(ReorderBuffer *rb,
> TransactionId xid, XLogRecPtr lsn,
>                 ReorderBufferTXN *toptxn;
>
>                 /* get the top transaction */
> -               if (txn->toptxn != NULL)
> -                       toptxn = txn->toptxn;
> -               else
> -                       toptxn = txn;
> +               toptxn = rbtxn_get_toptxn(txn);
>

IMO the comment ("/* get the top transaction */") was not really
saying anything useful that is not already obvious from the macro name
("rbtxn_get_toptxn"). So I've removed it entirely in your 2nd case.
This change is consistent with other parts of the patch where the
toptxn is just assigned in the declaration.

PSA v3. [2]

------
[1] Amit reply to your suggestion -
https://www.postgresql.org/message-id/CAA4eK1%2BoqfUSC3vpu6bJzgfnSmu_yaeoLS%3DRb3416GuS5iRP1Q%40mail.gmail.com
[2] v3 -
https://www.postgresql.org/message-id/CAHut%2BPtrD4xU4OPUB64ZK%2BDPDhfKn3zph%3DnDpEWUFFzUvMKo2w%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

Hi,

On Wed, Mar 15, 2023 at 8:55 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Tue, Mar 14, 2023 at 10:43 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Tue, Mar 14, 2023 at 12:37 PM Peter Smith <smithpb2250@gmail.com> wrote:
> > >
> > > Thanks for the review!
> > >
> > > On Mon, Mar 13, 2023 at 6:19 PM vignesh C <vignesh21@gmail.com> wrote:
> > > ...
> > >
> > > > 4) We check if txn->toptxn is not null twice here both in if condition
> > > > and in the assignment, we could retain the assignment operation as
> > > > earlier to remove the 2nd check:
> > > > -       if (txn->toptxn)
> > > > -               txn = txn->toptxn;
> > > > +       if (isa_subtxn(txn))
> > > > +               txn = get_toptxn(txn);
> > > >
> > > > We could avoid one check again by:
> > > > +       if (isa_subtxn(txn))
> > > > +               txn = txn->toptxn;
> > > >
> > >
> > > Yeah, that is true, but I chose not to keep the original assignment in
> > > this case mainly because then it is consistent with the other changed
> > > code ---  e.g. Every other direct member assignment/access of the
> > > 'toptxn' member now hides behind the macros so I did not want this
> > > single place to be the odd one out. TBH, I don't think 1 extra check
> > > is of any significance, but it is not a problem to change like you
> > > suggested if other people also want it done that way.
> > >
> >
> > Can't we directly use rbtxn_get_toptxn() for this case? I think that
> > way code will look neat. I see that it is not exactly matching the
> > existing check so you might be worried but I feel the new code will
> > achieve the same purpose and will be easy to follow.
> >
>
> OK. Done as suggested.
>

+1 to the idea. Here are some minor comments:

@@ -1667,7 +1658,7 @@ ReorderBufferTruncateTXN(ReorderBuffer *rb,
ReorderBufferTXN *txn, bool txn_prep
  * about the toplevel xact (we send the XID in all messages), but we never
  * stream XIDs of empty subxacts.
  */
- if ((!txn_prepared) && ((!txn->toptxn) || (txn->nentries_mem != 0)))
+ if ((!txn_prepared) && (rbtxn_is_toptxn(txn) || (txn->nentries_mem != 0)))
  txn->txn_flags |= RBTXN_IS_STREAMED;

Probably the following comment of the above lines also needs to be updated?

     * The toplevel transaction, identified by (toptxn==NULL), is marked as
     * streamed always,

---
+/* Is this a top-level transaction? */
+#define rbtxn_is_toptxn(txn)\
+(\
+ (txn)->toptxn == NULL\
+)
+
+/* Is this a subtransaction? */
+#define rbtxn_is_subtxn(txn)\
+(\
+ (txn)->toptxn != NULL\
+)
+
+/* Get the top-level transaction of this (sub)transaction. */
+#define rbtxn_get_toptxn(txn)\
+(\
+ rbtxn_is_subtxn(txn) ? (txn)->toptxn : (txn)\
+)

We need a whitespace before backslashes.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Wed, Mar 15, 2023 at 4:55 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> Hi,
>
...
> +1 to the idea. Here are some minor comments:
>
> @@ -1667,7 +1658,7 @@ ReorderBufferTruncateTXN(ReorderBuffer *rb,
> ReorderBufferTXN *txn, bool txn_prep
>   * about the toplevel xact (we send the XID in all messages), but we never
>   * stream XIDs of empty subxacts.
>   */
> - if ((!txn_prepared) && ((!txn->toptxn) || (txn->nentries_mem != 0)))
> + if ((!txn_prepared) && (rbtxn_is_toptxn(txn) || (txn->nentries_mem != 0)))
>   txn->txn_flags |= RBTXN_IS_STREAMED;
>
> Probably the following comment of the above lines also needs to be updated?
>
>      * The toplevel transaction, identified by (toptxn==NULL), is marked as
>      * streamed always,
>
> ---
> +/* Is this a top-level transaction? */
> +#define rbtxn_is_toptxn(txn)\
> +(\
> + (txn)->toptxn == NULL\
> +)
> +
> +/* Is this a subtransaction? */
> +#define rbtxn_is_subtxn(txn)\
> +(\
> + (txn)->toptxn != NULL\
> +)
> +
> +/* Get the top-level transaction of this (sub)transaction. */
> +#define rbtxn_get_toptxn(txn)\
> +(\
> + rbtxn_is_subtxn(txn) ? (txn)->toptxn : (txn)\
> +)
>
> We need a whitespace before backslashes.
>

Thanks for your interest in my patch.

PSA v4 which addresses both of your review comments.

------
Kind Regards,
Peter Smith.
Fujitsu Australia

On Thu, Mar 16, 2023 at 7:20 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> PSA v4 which addresses both of your review comments.

Looks like a reasonable change to me.

A nitpick: how about using rbtxn_get_toptxn instead of an explicit
variable toptxn for single use?

1.
Change
    ReorderBufferTXN *toptxn = rbtxn_get_toptxn(txn);
    TestDecodingTxnData *txndata = toptxn->output_plugin_private;

To
    TestDecodingTxnData *txndata = rbtxn_get_toptxn(txn)->output_plugin_private;

2.
Change
        ReorderBufferTXN *toptxn = rbtxn_get_toptxn(txn);
        toptxn->txn_flags |= RBTXN_HAS_STREAMABLE_CHANGE;

To
        rbtxn_get_toptxn(txn)->txn_flags |= RBTXN_HAS_STREAMABLE_CHANGE;

3.
Change
    /*
     * Update the total size in top level as well. This is later used to
     * compute the decoding stats.
     */
    toptxn = rbtxn_get_toptxn(txn);

    if (addition)
    {
        txn->size += sz;
        rb->size += sz;

        /* Update the total size in the top transaction. */
        toptxn->total_size += sz;
    }
    else
    {
        Assert((rb->size >= sz) && (txn->size >= sz));
        txn->size -= sz;
        rb->size -= sz;

        /* Update the total size in the top transaction. */
        toptxn->total_size -= sz;
    }

To

    /*
     * Update the total size in top level as well. This is later used to
     * compute the decoding stats.
     */
    if (addition)
    {
        txn->size += sz;
        rb->size += sz;
        rbtxn_get_toptxn(txn)->total_size += sz;
    }
    else
    {
        Assert((rb->size >= sz) && (txn->size >= sz));
        txn->size -= sz;
        rb->size -= sz;
        rbtxn_get_toptxn(txn)->total_size -= sz;
    }

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Thu, Mar 16, 2023 at 10:40 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Thu, Mar 16, 2023 at 7:20 AM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > PSA v4 which addresses both of your review comments.
>
> Looks like a reasonable change to me.
>
> A nitpick: how about using rbtxn_get_toptxn instead of an explicit
> variable toptxn for single use?
>

I find all three suggestions are similar. Personally, I think the
current code looks better. The v4 patch LGTM and I am planning to
commit it unless there are more comments or people find your
suggestions as an improvement.

--
With Regards,
Amit Kapila.

On Thu, Mar 16, 2023 at 7:20 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> PSA v4 which addresses both of your review comments.
>

Pushed.

--
With Regards,
Amit Kapila.

The build-farm was OK for the last 18hrs after this push, except there
was one error on mamba [1] in test-decoding-check.

This patch did change the test_decoding.c file, so it seems an
unlikely coincidence, but OTOH the change was very small and I don't
see yet how it could have caused a problem here but nowhere else.

Although, mamba has since passed again since that failure.

Any thoughts?

------
[1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=mamba&dt=2023-03-17%2005%3A36%3A10

Kind Regards,
Peter Smith.
Fujitsu Australia

On Sat, Mar 18, 2023 at 8:47 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> The build-farm was OK for the last 18hrs after this push, except there
> was one error on mamba [1] in test-decoding-check.
>
> This patch did change the test_decoding.c file, so it seems an
> unlikely coincidence, but OTOH the change was very small and I don't
> see yet how it could have caused a problem here but nowhere else.
>
> Although, mamba has since passed again since that failure.
>
> Any thoughts?
>
> ------
> [1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=mamba&dt=2023-03-17%2005%3A36%3A10
>

Subsequent testing with this "toptxn" patch reverted [1] was able to
reproduce the same error, so it seems this "toptxn" patch is unrelated
to the reported build-farm failure.

[1] https://www.postgresql.org/message-id/CAHut%2BPvVrjwJm_9ZqnXJk4x9k8dN0dYrV%2BT5_Rd30BSneDhv1A%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia