public inbox for [email protected]
help / color / mirror / Atom feedFrom: Peter Smith <[email protected]>
To: Masahiko Sawada <[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: Thu, 16 Mar 2023 12:49:34 +1100
Message-ID: <CAHut+PsDvFvwBiSzB0bCZgMwDUfOD4dackL0ZcTHXtLuewdobA@mail.gmail.com> (raw)
In-Reply-To: <CAD21AoBT+brpQxN4MvYzWmq=25ac=3QEwwJXfCfqaV9-_YZNYQ@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>
<CAD21AoBT+brpQxN4MvYzWmq=25ac=3QEwwJXfCfqaV9-_YZNYQ@mail.gmail.com>
On Wed, Mar 15, 2023 at 4:55 PM Masahiko Sawada <[email protected]> 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
Attachments:
[application/octet-stream] v4-0001-Add-macros-for-ReorderBufferTXN-toptxn.patch (7.0K, ../CAHut+PsDvFvwBiSzB0bCZgMwDUfOD4dackL0ZcTHXtLuewdobA@mail.gmail.com/2-v4-0001-Add-macros-for-ReorderBufferTXN-toptxn.patch)
download | inline diff:
From 8318ccb4c2cea70280ef1142f76773cf40267427 Mon Sep 17 00:00:00 2001
From: Peter Smith <[email protected]>
Date: Thu, 16 Mar 2023 12:00:50 +1100
Subject: [PATCH v4] 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 | 46 ++++++++++---------------
src/backend/replication/pgoutput/pgoutput.c | 6 ++--
src/include/replication/reorderbuffer.h | 18 ++++++++++
4 files changed, 41 insertions(+), 33 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..6714258 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;
}
@@ -1655,9 +1646,9 @@ ReorderBufferTruncateTXN(ReorderBuffer *rb, ReorderBufferTXN *txn, bool txn_prep
/*
* Mark the transaction as streamed.
*
- * The toplevel transaction, identified by (toptxn==NULL), is marked as
- * streamed always, even if it does not contain any changes (that is, when
- * all the changes are in subtransactions).
+ * The top-level transaction, is marked as streamed always, even if it does
+ * not contain any changes (that is, when all the changes are in
+ * subtransactions).
*
* For subtransactions, we only mark them as streamed when there are
* changes in them.
@@ -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..e37f512 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 (4+ 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], [email protected]
Subject: Re: Add macros for ReorderBufferTXN toptxn
In-Reply-To: <CAHut+PsDvFvwBiSzB0bCZgMwDUfOD4dackL0ZcTHXtLuewdobA@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