Re: Misleading comment in prologue of ReorderBufferQueueMessage
On Fri, Dec 18, 2020 at 3:37 PM Ashutosh Bapat wrote: > > On Wed, Dec 16, 2020 at 8:00 AM Amit Kapila 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.
Re: Misleading comment in prologue of ReorderBufferQueueMessage
On Wed, Dec 16, 2020 at 8:00 AM Amit Kapila wrote: > On Tue, Dec 15, 2020 at 11:25 AM Ashutosh Bapat > wrote: > > > > On Mon, Dec 14, 2020 at 3:14 PM Amit Kapila > wrote: > >> > >> On Mon, Dec 14, 2020 at 2:45 PM Ashutosh Bapat > >> 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. > Used this one. PFA patch. -- Best Wishes, Ashutosh From b23d080b10ebcb210cb186ab3d4832e2448d71b7 Mon Sep 17 00:00:00 2001 From: Ashutosh Bapat 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
Re: Misleading comment in prologue of ReorderBufferQueueMessage
On Tue, Dec 15, 2020 at 11:25 AM Ashutosh Bapat wrote: > > On Mon, Dec 14, 2020 at 3:14 PM Amit Kapila wrote: >> >> On Mon, Dec 14, 2020 at 2:45 PM Ashutosh Bapat >> 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.
Re: Misleading comment in prologue of ReorderBufferQueueMessage
On Mon, Dec 14, 2020 at 3:14 PM Amit Kapila wrote: > On Mon, Dec 14, 2020 at 2:45 PM Ashutosh Bapat > 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. -- -- Best Wishes, Ashutosh
Re: Misleading comment in prologue of ReorderBufferQueueMessage
On Mon, Dec 14, 2020 at 2:45 PM Ashutosh Bapat 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.
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
