Re: [BUG] pg_basebackup from disconnected standby fails

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?

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

If you think that's not an improvement, feel free to discard it. Still
it seems to me that the comment as currently presented is confusing.
--
Michael