Re: [BUG] pg_basebackup from disconnected standby fails

Hello,

At Thu, 21 Jul 2016 22:32:08 +0900, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote in <CAB7nPqSQgt280LtSW9EgX7CvU4JwVraMmOQTM42NM-qyXAkFOw(at)mail(dot)gmail(dot)com>
> On Thu, Jul 21, 2016 at 5:19 PM, Kyotaro HORIGUCHI
> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> > + * minRecoveryPoint can go behind the last checkpoint's redo location when
> > + * the checkpoint writes out no buffer. This does no harm to performing a
> > + * recovery but such inversion seems inconsistent from the view of the
> > + * callers and prevents them from knowing WAL segments needed by sane
> > + * calcuation. For the reason we return the last replayed point as the
> > + * backup end location. Note that it can be greater than the exact backup
> > + * end location or even the minimum recovery point of pg_control at the
> > + * time. This is harmless for current uses.
>
> It does not seem an improvement to me to mention a comment regarding
> minRecoveryPoint in do_pg_stop_backup, especially knowing that the
> patch that we have here uses the last replayed LSN and TLI and does
> not care about that anymore.

Recovery still uses the minRecoveryPoint stored in pg_control
value. The patch makes pg_stop_backup return replayEndRecPtr but
the previous comment does not mention why pg_stop_backup returns
replay end but recovery. So at least we should edit it so that it
describes the correct thing.

- * We return the current minimum recovery point as the backup end
- * location.
+ * We return the current replay end point as the backup end
+ * location.

Does this make sense? Next,

- * Note that it can be greater than the exact backup end
- * location if the minimum recovery point is updated after the backup of
- * pg_control. This is harmless for current uses.
+ * Note that it can be greater than the exact backup end
+ * location if the replay end point is updated after the backup
+ * is finished. This is harmless for current uses.

I think this is still correct. But this lacks a mention about
minRecoveryPoint. It is now separated from the return value of
this function.

+ * Recovery from a backup taken from a standby regards the
+ * minimum recovery point stored in pg_control as consistency
+ * point. It also can be greater than the exact backup end
+ * location if it is updated after the backup of
+ * pg_control. This is harmless, too.

It seems not good.

> > AFAICS pg_stop_backup returns a single value of LSN. I don't know
> > where this comes from, but the attached patch fixes this and adds
> > a mention on the "gap". The original description mentioned
> > backup_label and tablespace_map but it seems not necessary. The
> > following is the new content rewritten by the patch.
>
> No, this second patch is wrong. This part of the docs refers to
> non-exclusive backups, where 3 fields are returned. So the docs are
> correct.

Ah! I see. Thanks. pg_stop_backup has two signatures
'pg_stop_backup()' and 'pg_stop_backup(exclusive boolean)' and
this mentions the latter.

https://www.postgresql.org/docs/9.6/static/functions-admin.html

This page doesn't explain the components of the return value of
the second form. It is mentioned here

https://www.postgresql.org/docs/9.6/static/continuous-archiving.html

| The pg_stop_backup will return one row with three values. The
| second of these fields should be written to a file named
| backup_label in the root directory of the backup.

| The file identified by pg_stop_backup's first return value is the
| last segment that is required to form a complete set of backup
| files.

It returns an LSN, not a segment. But it won't be such a matter.

Don't we need a brief explanation about the second form of
pg_stop_backup, especially about non-exclusive use in the admin
function page?

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center