Re: [BUG] pg_basebackup from disconnected standby fails

On Thu, Jun 23, 2016 at 12:20 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Wed, Jun 15, 2016 at 4:52 PM, Kyotaro HORIGUCHI
> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> Hello,
>
> Sorry for the late reply, Horiguchi-san. I have finally been able to
> put some mind power into that.
>
>> This is somewhat artificial but the same situation could be made
>> also in the nature. The exact condition for this is replaying a
>> checkpoint record having no buffer modification since the
>> preceding checkpoint in the previous WAL segments.
>
> After thinking about that more, I am seeing your point.
> CreateRestartPoint is clearly missing the shot for the update of
> minRecoveryPoint even when a restart point can be created.
>

I think updating minRecoveryPoint unconditionally can change it's
purpose in some cases. Refer below comments in code:

* minRecoveryPoint is updated to the latest replayed LSN whenever we
* flush a data change during archive recovery. That guards against
* starting archive recovery, aborting it, and restarting with an earlier
* stop location. If we've already flushed data changes from WAL record X
* to disk, we mustn't start up until we reach X again.

Now, as per above rule, the value of minRecoveryPoint can be much
smaller than XLogCtl->replayEndRecPtr. I think this can change the
rules when we can allow read-only connections.

Another point to note is that we are updating checkpoint location
during restart points, when the database state is
DB_IN_ARCHIVE_RECOVERY and updating minRecoveryPoint unconditionally
doesn't look in sync with that as well.

I think your and Kyotaro-san's point that minRecoveryPoint should be
updated to support back-ups on standby has merits, but I think doing
it unconditionally might lead to change in behaviour in some cases.

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