Misleading comment in prologue of ReorderBufferQueueMessage
The name of the function suggests that the given message will be queued in
ReorderBuffer. The prologue of the function says so too
776 /*
777 * Queue message into a transaction so it can be processed upon commit.
778 */
It led me to think that a non-transactional message is processed along with
the surrounding transaction, esp. when it has an associated xid.
But in reality, the function queues only a transactional message and
decoders a non-transactional message immediately without waiting for a
commit.
We should modify the prologue to say
"Queue a transactional message into a transaction so that it can be
processed upon commit. A non-transactional message is processed
immediately." and also change the name of the function
to ReorderBufferProcessMessage(), but the later may break API compatibility.
--
Best Wishes,
Ashutosh
On Mon, Dec 14, 2020 at 2:45 PM Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
The name of the function suggests that the given message will be queued in ReorderBuffer. The prologue of the function says so too
776 /*
777 * Queue message into a transaction so it can be processed upon commit.
778 */
It led me to think that a non-transactional message is processed along with the surrounding transaction, esp. when it has an associated xid.But in reality, the function queues only a transactional message and decoders a non-transactional message immediately without waiting for a commit.
We should modify the prologue to say
"Queue a transactional message into a transaction so that it can be processed upon commit. A non-transactional message is processed immediately." and also change the name of the function to ReorderBufferProcessMessage(), but the later may break API compatibility.
+1 for the comment change but I am not sure if it is a good idea to
change the API name.
--
With Regards,
Amit Kapila.
On Mon, Dec 14, 2020 at 3:14 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Mon, Dec 14, 2020 at 2:45 PM Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:The name of the function suggests that the given message will be queued
in ReorderBuffer. The prologue of the function says so too
776 /*
777 * Queue message into a transaction so it can be processed uponcommit.
778 */
It led me to think that a non-transactional message is processed alongwith the surrounding transaction, esp. when it has an associated xid.
But in reality, the function queues only a transactional message and
decoders a non-transactional message immediately without waiting for a
commit.We should modify the prologue to say
"Queue a transactional message into a transaction so that it can beprocessed upon commit. A non-transactional message is processed
immediately." and also change the name of the function to
ReorderBufferProcessMessage(), but the later may break API compatibility.+1 for the comment change but I am not sure if it is a good idea to
change the API name.Can you please review wording? I will create a patch with updated wording.
--
--
Best Wishes,
Ashutosh
On Tue, Dec 15, 2020 at 11:25 AM Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
On Mon, Dec 14, 2020 at 3:14 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Mon, Dec 14, 2020 at 2:45 PM Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:The name of the function suggests that the given message will be queued in ReorderBuffer. The prologue of the function says so too
776 /*
777 * Queue message into a transaction so it can be processed upon commit.
778 */
It led me to think that a non-transactional message is processed along with the surrounding transaction, esp. when it has an associated xid.But in reality, the function queues only a transactional message and decoders a non-transactional message immediately without waiting for a commit.
We should modify the prologue to say
"Queue a transactional message into a transaction so that it can be processed upon commit. A non-transactional message is processed immediately." and also change the name of the function to ReorderBufferProcessMessage(), but the later may break API compatibility.+1 for the comment change but I am not sure if it is a good idea to
change the API name.Can you please review wording? I will create a patch with updated wording.
How about something like below:
A transactional message is queued to be processed upon commit and a
non-transactional message gets processed immediately.
OR
A transactional message is queued so it can be processed upon commit
and a non-transactional message gets processed immediately.
--
With Regards,
Amit Kapila.
On Wed, Dec 16, 2020 at 8:00 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Tue, Dec 15, 2020 at 11:25 AM Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:On Mon, Dec 14, 2020 at 3:14 PM Amit Kapila <amit.kapila16@gmail.com>
wrote:
On Mon, Dec 14, 2020 at 2:45 PM Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:The name of the function suggests that the given message will be
queued in ReorderBuffer. The prologue of the function says so too
776 /*
777 * Queue message into a transaction so it can be processed uponcommit.
778 */
It led me to think that a non-transactional message is processedalong with the surrounding transaction, esp. when it has an associated xid.
But in reality, the function queues only a transactional message and
decoders a non-transactional message immediately without waiting for a
commit.We should modify the prologue to say
"Queue a transactional message into a transaction so that it can beprocessed upon commit. A non-transactional message is processed
immediately." and also change the name of the function to
ReorderBufferProcessMessage(), but the later may break API compatibility.+1 for the comment change but I am not sure if it is a good idea to
change the API name.Can you please review wording? I will create a patch with updated
wording.
How about something like below:
A transactional message is queued to be processed upon commit and a
non-transactional message gets processed immediately.
Used this one. PFA patch.
--
Best Wishes,
Ashutosh
Attachments:
0001-Update-prologue-of-ReorderBufferQueueMessage.patchtext/x-patch; charset=US-ASCII; name=0001-Update-prologue-of-ReorderBufferQueueMessage.patchDownload
From b23d080b10ebcb210cb186ab3d4832e2448d71b7 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <ashutosh.bapat@2ndquadrant.com>
Date: Fri, 18 Dec 2020 15:33:48 +0530
Subject: [PATCH] Update prologue of ReorderBufferQueueMessage()
The prologue of this function describes behaviour in case of a
transactional WAL message only, but it accepts both transactional and
non-transactional WAL messages. Update the prologue to describe
behaviour in case of non-transactional WAL message as well.
Ashutosh Bapat, rephrased by Amit Kapila
---
src/backend/replication/logical/reorderbuffer.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 301baff244..85e78328b2 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -783,7 +783,8 @@ ReorderBufferQueueChange(ReorderBuffer *rb, TransactionId xid, XLogRecPtr lsn,
}
/*
- * Queue message into a transaction so it can be processed upon commit.
+ * A transactional message is queued to be processed upon commit and a
+ * non-transactional message gets processed immediately.
*/
void
ReorderBufferQueueMessage(ReorderBuffer *rb, TransactionId xid,
--
2.17.1
On Fri, Dec 18, 2020 at 3:37 PM Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
On Wed, Dec 16, 2020 at 8:00 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
How about something like below:
A transactional message is queued to be processed upon commit and a
non-transactional message gets processed immediately.Used this one. PFA patch.
Pushed!
--
With Regards,
Amit Kapila.