MIME-Version: 1.0
References: <CAHut+PuCznOyTqBQwjRUu-ibG-=KHyCv-0FTcWQtZUdR88umfg@mail.gmail.com>
In-Reply-To: <CAHut+PuCznOyTqBQwjRUu-ibG-=KHyCv-0FTcWQtZUdR88umfg@mail.gmail.com>
From: vignesh C <vignesh21@gmail.com>
Date: Mon, 13 Mar 2023 12:49:10 +0530
Message-ID: <CALDaNm1J-V06vNM1JBwP1OJgAQKWntJvfG8gf=3Kfnpm=bE-Yw@mail.gmail.com>
Subject: Re: Add macros for ReorderBufferTXN toptxn
To: Peter Smith <smithpb2250@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Archived-At: <https://www.postgresql.org/message-id/CALDaNm1J-V06vNM1JBwP1OJgAQKWntJvfG8gf%3D3Kfnpm%3DbE-Yw%40mail.gmail.com>
Precedence: bulk

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