public inbox for [email protected]
help / color / mirror / Atom feedFrom: Peter Smith <[email protected]>
To: 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 10:54:26 +1100
Message-ID: <CAHut+PtrD4xU4OPUB64ZK+DPDhfKn3zph=nDpEWUFFzUvMKo2w@mail.gmail.com> (raw)
In-Reply-To: <CAA4eK1LGFhOsqLhZfiHOvgmzzO5MHJ7OgQMnOCbYp6A9Ks92WQ@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>
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.
PSA v3.
------
Kind Regards,
Peter Smith.
Fujitsu Australia
Attachments:
[application/octet-stream] v3-0001-Add-macros-for-ReorderBufferTXN-toptxn.patch (6.4K, ../CAHut+PtrD4xU4OPUB64ZK+DPDhfKn3zph=nDpEWUFFzUvMKo2w@mail.gmail.com/2-v3-0001-Add-macros-for-ReorderBufferTXN-toptxn.patch)
download | inline diff:
From 9b660e136afa6f820af3c3d27841ece4e021121e Mon Sep 17 00:00:00 2001
From: Peter Smith <[email protected]>
Date: Wed, 15 Mar 2023 09:37:20 +1100
Subject: [PATCH v3] 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 | 40 ++++++++++---------------
src/backend/replication/pgoutput/pgoutput.c | 6 ++--
src/include/replication/reorderbuffer.h | 18 +++++++++++
4 files changed, 38 insertions(+), 30 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..0f063b0 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
@@ -809,13 +806,7 @@ ReorderBufferQueueChange(ReorderBuffer *rb, TransactionId xid, XLogRecPtr lsn,
change->action == REORDER_BUFFER_CHANGE_TRUNCATE ||
change->action == REORDER_BUFFER_CHANGE_MESSAGE)
{
- ReorderBufferTXN *toptxn;
-
- /* get the top transaction */
- if (txn->toptxn != NULL)
- toptxn = txn->toptxn;
- else
- toptxn = txn;
+ ReorderBufferTXN *toptxn = rbtxn_get_toptxn(txn);
toptxn->txn_flags |= RBTXN_HAS_STREAMABLE_CHANGE;
}
@@ -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;
if (txn_prepared)
@@ -3207,10 +3198,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 +3283,7 @@ ReorderBufferAddInvalidations(ReorderBuffer *rb, TransactionId xid,
* so that we can execute them all together. See comments atop this
* function.
*/
- if (txn->toptxn)
- txn = txn->toptxn;
+ txn = rbtxn_get_toptxn(txn);
Assert(nmsgs > 0);
@@ -3354,7 +3341,6 @@ ReorderBufferXidSetCatalogChanges(ReorderBuffer *rb, TransactionId xid,
XLogRecPtr lsn)
{
ReorderBufferTXN *txn;
- ReorderBufferTXN *toptxn;
txn = ReorderBufferTXNByXid(rb, xid, true, NULL, lsn, true);
@@ -3370,11 +3356,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 +3609,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 +3997,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..08dc9ee 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 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)\
+)
+
typedef struct ReorderBufferTXN
{
/* See above */
--
1.8.3.1
view thread (3+ 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], [email protected]
Subject: Re: Add macros for ReorderBufferTXN toptxn
In-Reply-To: <CAHut+PtrD4xU4OPUB64ZK+DPDhfKn3zph=nDpEWUFFzUvMKo2w@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