Re: [BUG] pg_basebackup from disconnected standby fails

On Wed, Oct 26, 2016 at 1:53 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Mon, Oct 24, 2016 at 12:25 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>> On Mon, Oct 24, 2016 at 1:39 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>>>
>>> I think what you are saying is not completely right, because we do
>>> update minRecoveryPoint when we don't perform a new restart point.
>>> When we perform restart point, then it assumes that flushing the
>>> buffers will take care of updating minRecoveryPoint.
>>
>> Yep, minRecoveryPoint still gets updated when the last checkpoint
>> record is the last restart point to avoid a hot standby to allow
>> read-only connections at a LSN-point earlier than the last shutdown.
>> Anyway, we can clearly reject 1. in the light of
>> https://www.postgresql.org/message-id/CAA4eK1KmjtsXqF0cav7Cs4d4vwv2H_pc8d8q1BUCqDzAF+7EzQ@mail.gmail.com
>> when playing with different stop locations at recovery.
>>
>
> That point holds good only for cases when we try to update minimum
> recovery point beyond what is required (like earlier we were thinking
> to update it unconditionally), however what is being discussed here is
> to update only if it is not updated by flush of buffers. I think that
> is okay, but I feel Kyotaro-San's fix is a good fix for the problem
> and we don't need to add some more code (additional update of control
> file) to fix the problem.

Reading ten times my last paragraph and staring at my screen for 15
long weird minutes, I cannot recall what I had in mind when I wrote
those lines in particular..

[...scratches head...]

But yes, thinking *harder*, I agree that updating minRecoveryPoint
just after the checkpoint record would be fine and removes the need to
have more WAL than necessary in for a backup taken from a standby.
That will also prevent cases where minRecoveryPoint is older than the
recovery start point. On top of that the cost of an extra call to
UpdateControlFile() looks cheap considering that CreateRestartPoint()
is called only by the checkpointer or at shutdown.

Just coding things this solution gives roughtly the attached? The TAP
test passes btw.
--
Michael