public inbox for [email protected]  

help / color / mirror / Atom feed

From: Masahiko Sawada <[email protected]>
To: Peter Smith <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: vignesh C <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Add macros for ReorderBufferTXN toptxn
Date: Wed, 15 Mar 2023 14:54:32 +0900
Message-ID: <CAD21AoBT+brpQxN4MvYzWmq=25ac=3QEwwJXfCfqaV9-_YZNYQ@mail.gmail.com> (raw)
In-Reply-To: <CAHut+PtrD4xU4OPUB64ZK+DPDhfKn3zph=nDpEWUFFzUvMKo2w@mail.gmail.com>
References: <CAHut+PuCznOyTqBQwjRUu-ibG-=KHyCv-0FTcWQtZUdR88umfg@mail.gmail.com>
	<CALDaNm1J-V06vNM1JBwP1OJgAQKWntJvfG8gf=3Kfnpm=bE-Yw@mail.gmail.com>
	<CAHut+PuRgxhk4GBK8Fm8hgDH=KLGvCjjOvcNXMapHgyN1bQfDA@mail.gmail.com>
	<CAA4eK1LGFhOsqLhZfiHOvgmzzO5MHJ7OgQMnOCbYp6A9Ks92WQ@mail.gmail.com>
	<CAHut+PtrD4xU4OPUB64ZK+DPDhfKn3zph=nDpEWUFFzUvMKo2w@mail.gmail.com>

Hi,

On Wed, Mar 15, 2023 at 8:55 AM Peter Smith <[email protected]> wrote:
>
> On Tue, Mar 14, 2023 at 10:43 PM Amit Kapila <[email protected]> wrote:
> >
> > On Tue, Mar 14, 2023 at 12:37 PM Peter Smith <[email protected]> wrote:
> > >
> > > Thanks for the review!
> > >
> > > On Mon, Mar 13, 2023 at 6:19 PM vignesh C <[email protected]> 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

view thread (3+ 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], [email protected], [email protected]
  Subject: Re: Add macros for ReorderBufferTXN toptxn
  In-Reply-To: <CAD21AoBT+brpQxN4MvYzWmq=25ac=3QEwwJXfCfqaV9-_YZNYQ@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