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
