public inbox for [email protected]
help / color / mirror / Atom feedFrom: Peter Smith <[email protected]>
To: vignesh C <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Add macros for ReorderBufferTXN toptxn
Date: Tue, 14 Mar 2023 18:06:44 +1100
Message-ID: <CAHut+PuRgxhk4GBK8Fm8hgDH=KLGvCjjOvcNXMapHgyN1bQfDA@mail.gmail.com> (raw)
In-Reply-To: <CALDaNm1J-V06vNM1JBwP1OJgAQKWntJvfG8gf=3Kfnpm=bE-Yw@mail.gmail.com>
References: <CAHut+PuCznOyTqBQwjRUu-ibG-=KHyCv-0FTcWQtZUdR88umfg@mail.gmail.com>
<CALDaNm1J-V06vNM1JBwP1OJgAQKWntJvfG8gf=3Kfnpm=bE-Yw@mail.gmail.com>
Thanks for the review!
On Mon, Mar 13, 2023 at 6:19 PM vignesh C <[email protected]> wrote:
...
> 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)
>
All the above are fixed as suggested.
> 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.
------
Kind Regards,
Peter Smith.
Fujitsu Australia.
Attachments:
[application/octet-stream] v2-0001-Add-macros-for-ReorderBufferTXN-toptxn.patch (6.3K, ../CAHut+PuRgxhk4GBK8Fm8hgDH=KLGvCjjOvcNXMapHgyN1bQfDA@mail.gmail.com/2-v2-0001-Add-macros-for-ReorderBufferTXN-toptxn.patch)
download | inline diff:
From 38de68f8ab7be9857e2c922db351f8d0d8c4a656 Mon Sep 17 00:00:00 2001
From: Peter Smith <[email protected]>
Date: Tue, 14 Mar 2023 16:33:24 +1100
Subject: [PATCH v2] Add macros for ReorderBufferTXN toptxn.
Make toptxn related code more readable by introducing some simple macros.
---
contrib/test_decoding/test_decoding.c | 4 +--
src/backend/replication/logical/reorderbuffer.c | 38 +++++++++++--------------
src/backend/replication/pgoutput/pgoutput.c | 6 ++--
src/include/replication/reorderbuffer.h | 18 ++++++++++++
4 files changed, 39 insertions(+), 27 deletions(-)
diff --git a/contrib/test_decoding/test_decoding.c b/contrib/test_decoding/test_decoding.c
index b7e6048..628c6a2 100644
--- a/contrib/test_decoding/test_decoding.c
+++ b/contrib/test_decoding/test_decoding.c
@@ -815,11 +815,11 @@ pg_decode_stream_abort(LogicalDecodingContext *ctx,
* maintain the output_plugin_private only under the toptxn so if this is
* not the toptxn then fetch the toptxn.
*/
- ReorderBufferTXN *toptxn = txn->toptxn ? txn->toptxn : txn;
+ ReorderBufferTXN *toptxn = rbtxn_get_toptxn(txn);
TestDecodingTxnData *txndata = toptxn->output_plugin_private;
bool xact_wrote_changes = txndata->xact_wrote_changes;
- if (txn->toptxn == NULL)
+ if (rbtxn_is_toptxn(txn))
{
Assert(txn->output_plugin_private != NULL);
pfree(txndata);
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 2d17c55..fe15762 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -717,10 +717,7 @@ ReorderBufferProcessPartialChange(ReorderBuffer *rb, ReorderBufferTXN *txn,
return;
/* Get the top transaction. */
- if (txn->toptxn != NULL)
- toptxn = txn->toptxn;
- else
- toptxn = txn;
+ toptxn = rbtxn_get_toptxn(txn);
/*
* Indicate a partial change for toast inserts. The change will be
@@ -812,10 +809,7 @@ ReorderBufferQueueChange(ReorderBuffer *rb, TransactionId xid, XLogRecPtr lsn,
ReorderBufferTXN *toptxn;
/* get the top transaction */
- if (txn->toptxn != NULL)
- toptxn = txn->toptxn;
- else
- toptxn = txn;
+ toptxn = rbtxn_get_toptxn(txn);
toptxn->txn_flags |= RBTXN_HAS_STREAMABLE_CHANGE;
}
@@ -1667,7 +1661,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;
if (txn_prepared)
@@ -3207,10 +3201,7 @@ ReorderBufferChangeMemoryUpdate(ReorderBuffer *rb,
* Update the total size in top level as well. This is later used to
* compute the decoding stats.
*/
- if (txn->toptxn != NULL)
- toptxn = txn->toptxn;
- else
- toptxn = txn;
+ toptxn = rbtxn_get_toptxn(txn);
if (addition)
{
@@ -3295,8 +3286,8 @@ ReorderBufferAddInvalidations(ReorderBuffer *rb, TransactionId xid,
* so that we can execute them all together. See comments atop this
* function.
*/
- if (txn->toptxn)
- txn = txn->toptxn;
+ if (rbtxn_is_subtxn(txn))
+ txn = rbtxn_get_toptxn(txn);
Assert(nmsgs > 0);
@@ -3354,7 +3345,6 @@ ReorderBufferXidSetCatalogChanges(ReorderBuffer *rb, TransactionId xid,
XLogRecPtr lsn)
{
ReorderBufferTXN *txn;
- ReorderBufferTXN *toptxn;
txn = ReorderBufferTXNByXid(rb, xid, true, NULL, lsn, true);
@@ -3370,11 +3360,15 @@ ReorderBufferXidSetCatalogChanges(ReorderBuffer *rb, TransactionId xid,
* conveniently check just top-level transaction and decide whether to
* build the hash table or not.
*/
- toptxn = txn->toptxn;
- if (toptxn != NULL && !rbtxn_has_catalog_changes(toptxn))
+ if (rbtxn_is_subtxn(txn))
{
- toptxn->txn_flags |= RBTXN_HAS_CATALOG_CHANGES;
- dclist_push_tail(&rb->catchange_txns, &toptxn->catchange_node);
+ ReorderBufferTXN *toptxn = rbtxn_get_toptxn(txn);
+
+ if (!rbtxn_has_catalog_changes(toptxn))
+ {
+ toptxn->txn_flags |= RBTXN_HAS_CATALOG_CHANGES;
+ dclist_push_tail(&rb->catchange_txns, &toptxn->catchange_node);
+ }
}
}
@@ -3619,7 +3613,7 @@ ReorderBufferCheckMemoryLimit(ReorderBuffer *rb)
(txn = ReorderBufferLargestStreamableTopTXN(rb)) != NULL)
{
/* we know there has to be one, because the size is not zero */
- Assert(txn && !txn->toptxn);
+ Assert(txn && rbtxn_is_toptxn(txn));
Assert(txn->total_size > 0);
Assert(rb->size >= txn->total_size);
@@ -4007,7 +4001,7 @@ ReorderBufferStreamTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
bool txn_is_streamed;
/* We can never reach here for a subtransaction. */
- Assert(txn->toptxn == NULL);
+ Assert(rbtxn_is_toptxn(txn));
/*
* We can't make any assumptions about base snapshot here, similar to what
diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c
index 00a2d73..3a2d2e3 100644
--- a/src/backend/replication/pgoutput/pgoutput.c
+++ b/src/backend/replication/pgoutput/pgoutput.c
@@ -694,8 +694,8 @@ maybe_send_schema(LogicalDecodingContext *ctx,
if (in_streaming)
xid = change->txn->xid;
- if (change->txn->toptxn)
- topxid = change->txn->toptxn->xid;
+ if (rbtxn_is_subtxn(change->txn))
+ topxid = rbtxn_get_toptxn(change->txn)->xid;
else
topxid = xid;
@@ -1879,7 +1879,7 @@ pgoutput_stream_abort(struct LogicalDecodingContext *ctx,
Assert(!in_streaming);
/* determine the toplevel transaction */
- toptxn = (txn->toptxn) ? txn->toptxn : txn;
+ toptxn = rbtxn_get_toptxn(txn);
Assert(rbtxn_is_streamed(toptxn));
diff --git a/src/include/replication/reorderbuffer.h b/src/include/replication/reorderbuffer.h
index 215d149..ea1e271 100644
--- a/src/include/replication/reorderbuffer.h
+++ b/src/include/replication/reorderbuffer.h
@@ -249,6 +249,24 @@ typedef struct ReorderBufferChange
((txn)->txn_flags & RBTXN_SKIPPED_PREPARE) != 0 \
)
+/* Is this a top-level transaction? */
+#define rbtxn_is_toptxn(txn)\
+(\
+ (txn)->toptxn == NULL\
+)
+
+/* Is this a sub-transaction? */
+#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)\
+)
+
typedef struct ReorderBufferTXN
{
/* See above */
--
1.8.3.1
view thread (5+ messages) latest in thread
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: <CAHut+PuRgxhk4GBK8Fm8hgDH=KLGvCjjOvcNXMapHgyN1bQfDA@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