public inbox for [email protected]  

help / color / mirror / Atom feed

From: vignesh C <[email protected]>
To: Peter Smith <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Add macros for ReorderBufferTXN toptxn
Date: Mon, 13 Mar 2023 12:49:10 +0530
Message-ID: <CALDaNm1J-V06vNM1JBwP1OJgAQKWntJvfG8gf=3Kfnpm=bE-Yw@mail.gmail.com> (raw)
In-Reply-To: <CAHut+PuCznOyTqBQwjRUu-ibG-=KHyCv-0FTcWQtZUdR88umfg@mail.gmail.com>
References: <CAHut+PuCznOyTqBQwjRUu-ibG-=KHyCv-0FTcWQtZUdR88umfg@mail.gmail.com>

On Fri, 10 Mar 2023 at 04:36, Peter Smith <[email protected]> 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

view thread (2+ messages)

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected]
  Subject: Re: Add macros for ReorderBufferTXN toptxn
  In-Reply-To: <CALDaNm1J-V06vNM1JBwP1OJgAQKWntJvfG8gf=3Kfnpm=bE-Yw@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox