Re: Add macros for ReorderBufferTXN toptxn

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