Subject: Re: Add macros for ReorderBufferTXN toptxn

From: Peter Smith <[email-masked]>
Date: Thu, 16 Mar 2023 12:49:34 +1100

On Wed, Mar 15, 2023 at 4:55 PM Masahiko Sawada <[email-masked]> 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

---

From: Bharath Rupireddy <[email-masked]>
Date: Thu, 16 Mar 2023 10:40:05 +0530

> 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

---

From: Amit Kapila <[email-masked]>
Date: Thu, 16 Mar 2023 10:50:10 +0530

> 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.