Thread: pg_recvlogical --endpos
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
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
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
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
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
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
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
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
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
> 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
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