Re: [BUG] pg_basebackup from disconnected standby fails

On Fri, Oct 28, 2016 at 5:08 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Fri, Oct 28, 2016 at 1:16 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Thu, Oct 27, 2016 at 9:05 AM, Michael Paquier
>> <michael(dot)paquier(at)gmail(dot)com> wrote:
>>> On Thu, Oct 27, 2016 at 7:16 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>>>> This can create problem if the checkpoint record spans across multiple
>>>> segments, because you are updating minRecoveryPoint to start of
>>>> checkpoint record. We need to update it to end+1 of checkpoint
>>>> record. Please find attached patch which takes care of same.
>>>
>>> I gave up counting my mistakes on this thread, thanks. You should
>>> update the comments of XLogCtlData for the new field
>>> lastCheckPointEndPtr so as it is not used by the background writer but
>>> when creating a new restart point to define the minimum recovery
>>> point.
>>
>> I committed and back-patched this with some additional work on the
>> comments, but I don't understand this remark. That comment seems like
>> it should refer to the checkpointer in modern branches, but isn't that
>> point independent of this patch?
>

Yeah, I also think so.

> * During recovery, we keep a copy of the latest checkpoint record here.
> - * Used by the background writer when it wants to create a restartpoint.
> + * lastCheckPointRecPtr points to start of checkpoint record and
> + * lastCheckPointEndPtr points to end+1 of checkpoint record. Used by the
> + * background writer when it wants to create a restartpoint.
>
> The patch committed introduces lastCheckPointEndPtr, which is not used
> to decide if a restart point should be created or not. Only
> lastCheckPointRecPtr fits with this purpose. lastCheckPointEndPtr is
> used just to check if minRecoveryPoint should be pushed up or not. So
> my point here would be to refine a bit this comment, in the shape of
> that for example:
> lastCheckPointRecPtr points to start of checkpoint record and
> lastCheckPointEndPtr points to end+1 of checkpoint record. Both are
> used by the checkpointer. lastCheckPointRecPtr is used when it wants
> to create a restart point, and lastCheckPointEndPtr is used to
> determine if the minimum recovery point should be updated or not.
>

lastCheckPointEndPtr is used *not* only to *determine* if the minimum
recovery point, but also used to *update* it. Actually, if you see
the comment is somewhat generic "Used by the background writer when it
wants to create a restartpoint." and lastCheckPointEndPtr gets used
when we create a restartpoint. I think there is no harm in further
improving the comment, but I see nothing wrong as it is.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com