Horton

Thread

Commits

  1. Update comment atop of ReorderBufferQueueMessage().

  1. Misleading comment in prologue of ReorderBufferQueueMessage

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2020-12-14T09:14:39Z

    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

  2. Re: Misleading comment in prologue of ReorderBufferQueueMessage

    Amit Kapila <amit.kapila16@gmail.com> — 2020-12-14T09:46:08Z

    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.

  3. Re: Misleading comment in prologue of ReorderBufferQueueMessage

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2020-12-15T05:55:00Z

    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.
-- 
--
Best Wishes,
Ashutosh

  4. Re: Misleading comment in prologue of ReorderBufferQueueMessage

    Amit Kapila <amit.kapila16@gmail.com> — 2020-12-16T02:31:46Z

    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.

  5. Re: Misleading comment in prologue of ReorderBufferQueueMessage

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2020-12-18T10:07:47Z

    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 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

  6. Re: Misleading comment in prologue of ReorderBufferQueueMessage

    Amit Kapila <amit.kapila16@gmail.com> — 2020-12-19T06:42:16Z

    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.