Thread: pg_recvlogical --endpos

pg_recvlogical --endpos

From
Craig Ringer
Date:
Hi all

Here's a rebased version of my pg_recvlogical --endpos patch from the
9.5 series, updated to incoroprate Álvaro's changes.

This will be mainly useful for recovery tests as we start adding more
logical decoding testing.

See original post for more detail:

https://www.postgresql.org/message-id/CAMsr+YHBm3mUtXb2_RD=QsnUpdT0dR8K-+GTbBgpRdYuZFmXtw@mail.gmail.com

--
 Craig Ringer                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Attachment

Re: pg_recvlogical --endpos

From
Euler Taveira
Date:
On 01-09-2016 01:41, Craig Ringer wrote:
> Here's a rebased version of my pg_recvlogical --endpos patch from the
> 9.5 series, updated to incoroprate Álvaro's changes.
> 
I should review this patch in the other commitfest but was swamped with
work. The patch is almost ready but I have some points.

* We usually don't include itemlist into binary options. I suggest you
to add a new paragraph for the first item and the second one you could
add a note;
* I think you should add a small note explaining the 'endpos' behavior.
Also, I think endpos could be inclusive (since recovery also has this
behavior by default);
* I think there is a typo in those pieces of code:

+           if (endpos != InvalidXLogRecPtr && walEnd >= endpos)

+       if (endpos != InvalidXLogRecPtr && cur_record_lsn > endpos)

If you decide to be inclusive, it should be > otherwise >=.

There could be TAP tests for pg_recvlogical but it is material for
another patch.

I'll mark this patch waiting on author for your considerations.


--   Euler Taveira                   Timbira - http://www.timbira.com.br/  PostgreSQL: Consultoria, Desenvolvimento,
Suporte24x7 e Treinamento
 



Re: pg_recvlogical --endpos

From
Craig Ringer
Date:
On 19 November 2016 at 10:04, Euler Taveira <euler@timbira.com.br> wrote:
> On 01-09-2016 01:41, Craig Ringer wrote:
>> Here's a rebased version of my pg_recvlogical --endpos patch from the
>> 9.5 series, updated to incoroprate Álvaro's changes.
>>
> I should review this patch in the other commitfest but was swamped with
> work. The patch is almost ready but I have some points.
>
> * We usually don't include itemlist into binary options. I suggest you
> to add a new paragraph for the first item and the second one you could
> add a note;
> * I think you should add a small note explaining the 'endpos' behavior.
> Also, I think endpos could be inclusive (since recovery also has this
> behavior by default);
> * I think there is a typo in those pieces of code:
>
> +           if (endpos != InvalidXLogRecPtr && walEnd >= endpos)
>
> +       if (endpos != InvalidXLogRecPtr && cur_record_lsn > endpos)
>
> If you decide to be inclusive, it should be > otherwise >=.
>
> There could be TAP tests for pg_recvlogical but it is material for
> another patch.
>
> I'll mark this patch waiting on author for your considerations.

Thanks.

I've updated the patch for this. It's already posted on the logical
decoding timeline following thread, so I'll avoid repeating it here.

https://www.postgresql.org/message-id/CAMsr%2BYGd5dv3zPNch6BU4UXX49NJDC9m3-Y%3DV5q%3DTNcE9QgSaQ%40mail.gmail.com

I addressed the docs formatting and made endpos inclusive, and added tests.

You're right about the keepalive boundary, it should've been > not >=
. In the updated patch it's >= because endpos is now inclusive.



-- Craig Ringer                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services



Re: pg_recvlogical --endpos

From
"Okano, Naoki"
Date:
On Monday, November 21, 2016 1:08 PM Craig Ringer wrote:
> I've updated the patch for this. It's already posted on the logical
> decoding timeline following thread, so I'll avoid repeating it here.
> 
> https://www.postgresql.org/message-id/CAMsr%2BYGd5dv3zPNch6BU4UXX49NJDC9m3-Y%3DV5q%3DTNcE9QgSaQ%40mail.gmail.com
I checked the latest patch.
I think that the error message shown below is a typo. 

> +    if (endpos != InvalidXLogRecPtr && !do_start_slot)
> +    {
> +        fprintf(stderr,
> +                _("%s: cannot use --create-slot or --drop-slot together with --endpos\n"),
The condition '!do_start_slot' is not reflected in the error message.
The patch should allow --endpos to work with --create-slot.

Also, the document explains as follows.
> +        specified LSN.  If specified when not in <option>--start</option>
> +        mode, an error is raised.
So, it is better to output an error message that matches the document when changing the error message.

Regards,
Okano Naoki
Fujitsu

Re: pg_recvlogical --endpos

From
Craig Ringer
Date:
On 22 November 2016 at 16:52, Okano, Naoki <okano.naoki@jp.fujitsu.com> wrote:
> On Monday, November 21, 2016 1:08 PM Craig Ringer wrote:
>> I've updated the patch for this. It's already posted on the logical
>> decoding timeline following thread, so I'll avoid repeating it here.
>>
>> https://www.postgresql.org/message-id/CAMsr%2BYGd5dv3zPNch6BU4UXX49NJDC9m3-Y%3DV5q%3DTNcE9QgSaQ%40mail.gmail.com
> I checked the latest patch.
> I think that the error message shown below is a typo.
>
>> +     if (endpos != InvalidXLogRecPtr && !do_start_slot)
>> +     {
>> +             fprintf(stderr,
>> +                             _("%s: cannot use --create-slot or --drop-slot together with --endpos\n"),
> The condition '!do_start_slot' is not reflected in the error message.

Would it be better rephrased as "--endpos can only be used with --start" ?

> The patch should allow --endpos to work with --create-slot.

How? It doesn't make sense with --create-slot .

> Also, the document explains as follows.
>> +        specified LSN.  If specified when not in <option>--start</option>
>> +        mode, an error is raised.
> So, it is better to output an error message that matches the document when changing the error message.

Not sure I understand this.


-- Craig Ringer                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services



Re: pg_recvlogical --endpos

From
"Okano, Naoki"
Date:
On November 29, 2016 at 5:03 PM Craig Ringer wrote:
> Would it be better rephrased as "--endpos can only be used with --start" ?
OK. I think this phrase is better than the previous phrase. 

>> The patch should allow --endpos to work with --create-slot.
>
> How? It doesn't make sense with --create-slot .
This patch is updated to incorporate Alvaro's changes shown below, isn't it? 
https://www.postgresql.org/message-id/20160503180658.GA59498%40alvherre.pgsql
I thought that the consent in community was taken with this specification...
I could not find any mention that it did not make sense with --create-slot.
But if exists, would you let me know?

Regards,
Okano Naoki
Fujitsu

Re: pg_recvlogical --endpos

From
Craig Ringer
Date:
On 30 November 2016 at 09:18, Okano, Naoki <okano.naoki@jp.fujitsu.com> wrote:
>
> On November 29, 2016 at 5:03 PM Craig Ringer wrote:
>> Would it be better rephrased as "--endpos can only be used with --start" ?
> OK. I think this phrase is better than the previous phrase.
>
>>> The patch should allow --endpos to work with --create-slot.
>>
>> How? It doesn't make sense with --create-slot .
> This patch is updated to incorporate Alvaro's changes shown below, isn't it?
> https://www.postgresql.org/message-id/20160503180658.GA59498%40alvherre.pgsql
> I thought that the consent in community was taken with this specification...
> I could not find any mention that it did not make sense with --create-slot.

What would --endpos with --create-slot do?

-- Craig Ringer                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services



Re: pg_recvlogical --endpos

From
"Okano, Naoki"
Date:
On Wednesday, November 30, 2016 10:34 AM Craig Ringer wrote:
>On 30 November 2016 at 09:18, Okano, Naoki <okano(dot)naoki(at)jp(dot)fujitsu(dot)com> wrote:
>>
>> On November 29, 2016 at 5:03 PM Craig Ringer wrote:
>>> Would it be better rephrased as "--endpos can only be used with --start" ?
>> OK. I think this phrase is better than the previous phrase.
>>
>>>> The patch should allow --endpos to work with --create-slot.
>>>
>>> How? It doesn't make sense with --create-slot .
>> This patch is updated to incorporate Alvaro's changes shown below, isn't it?
>> https://www.postgresql.org/message-id/20160503180658.GA59498%40alvherre.pgsql
>> I thought that the consent in community was taken with this specification...
>>> I could not find any mention that it did not make sense with --create-slot.

> What would --endpos with --create-slot do?

Sorry, I was misunderstanding. you are right.
I have noticed that --endpos doesn't make sense unless it is with --start.
I checked --endpos works with --create-slot and --start.
So, there is no problem with this patch.

Regards,
Okano Naoki
Fujitsu

Re: pg_recvlogical --endpos

From
Haribabu Kommi
Date:


On Wed, Nov 30, 2016 at 4:26 PM, Okano, Naoki <okano.naoki@jp.fujitsu.com> wrote:

On Wednesday, November 30, 2016 10:34 AM Craig Ringer wrote:
>On 30 November 2016 at 09:18, Okano, Naoki <okano(dot)naoki(at)jp(dot)fujitsu(dot)com> wrote:
>>
>> On November 29, 2016 at 5:03 PM Craig Ringer wrote:
>>> Would it be better rephrased as "--endpos can only be used with --start" ?
>> OK. I think this phrase is better than the previous phrase.
>>
>>>> The patch should allow --endpos to work with --create-slot.
>>>
>>> How? It doesn't make sense with --create-slot .
>> This patch is updated to incorporate Alvaro's changes shown below, isn't it?
>> https://www.postgresql.org/message-id/20160503180658.GA59498%40alvherre.pgsql
>> I thought that the consent in community was taken with this specification...
>>> I could not find any mention that it did not make sense with --create-slot.

> What would --endpos with --create-slot do?

Sorry, I was misunderstanding. you are right.
I have noticed that --endpos doesn't make sense unless it is with --start.
I checked --endpos works with --create-slot and --start.
So, there is no problem with this patch.


Moved to next CF with "needs review" state.


Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] pg_recvlogical --endpos

From
Craig Ringer
Date:
> Moved to next CF with "needs review" state.

Here's an updated series. It's on top of the entry
https://commitfest.postgresql.org/12/883/ for PostgresNode TAP test
enhancements.

It corresponds exactly to patches [2,3,4] in the logical decoding on
standby post at
https://www.postgresql.org/message-id/CAMsr+YEzC=-+eV09A=ra150FjtkmTqT5Q70PiqBwytbOR3cshg@mail.gmail.com

If this is committed, the remaining decoding on standby patches will
just be the meat of the feature.

-- 
 Craig Ringer                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Attachment

Re: [HACKERS] pg_recvlogical --endpos

From
Simon Riggs
Date:
On 4 January 2017 at 13:37, Craig Ringer <craig@2ndquadrant.com> wrote:
>> Moved to next CF with "needs review" state.
>
> Here's an updated series. It's on top of the entry
> https://commitfest.postgresql.org/12/883/ for PostgresNode TAP test
> enhancements.
>
> It corresponds exactly to patches [2,3,4] in the logical decoding on
> standby post at
> https://www.postgresql.org/message-id/CAMsr+YEzC=-+eV09A=ra150FjtkmTqT5Q70PiqBwytbOR3cshg@mail.gmail.com
>
> If this is committed, the remaining decoding on standby patches will
> just be the meat of the feature.

Patches 2 and 3 committed for now. Please do everything else on the
logical decoding on standby posts. Thanks.

-- 
Simon Riggs                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services