Re: [HACKERS] Creating backup history files for backups taken fromstandbys

On Fri, Mar 02, 2018 at 03:41:57PM -0500, David Steele wrote:
> On 3/2/18 1:03 PM, Fujii Masao wrote:
>> On Fri, Mar 2, 2018 at 1:07 PM, Michael Paquier <michael@paquier.xyz> wrote:
>>> We would talk about two backups running
>>> simultaneously on a standby, which would overlap with each other to
>>> generate a file aimed only at being helpful for debugging purposes, and
>>> we provide no information now for backups taken from standbys.  We could
>>> of course make that logic a bit smarter by checking if there is an
>>> extsing file with the same name and create a new file with a different
>>> name.  But is that worth the complication? That's where I am not
>>> convinced, and that's the reason why this patch is doing things this
>>> way.

Sorry for my late reply, I was thinking more about the whole thing.

>> What about including the backup history file in the base backup instead of
>> creating it in the standby's pg_wal and archiving it? As the good side effect
>> of this approach, we can use the backup history file for debugging purpose
>> even when WAL archiving is not enabled.

That's not going to be much helpful for tar'ed base backups as the
backup history file would be at the end of the stream :(  For a debug
file, that's not really helpful.

> I don't think this is a good idea, even if it does solve the race
> condition.  First, it would be different from the way primary-style
> backups generate this file.  Second, these files would not be excluded
> from future restore / backup cycles so they could build up after a while
> and cause confusion.  I guess we could teach PG to delete them or
> pg_basebackup to ignore them, but that doesn't seem worth the effort.

Something I did not consider though is that this causes archive commands
to choke on those identical file names, so any archive command which is
not able to handle that would cause WAL to bloat on the standby.  For
this reason I would rather drop the current approach taken.  Remember
that the docs tell to use a plain "cp" for example, so this would cause
standbys to go silently down.

Something that I would find way more portable though is the addition of
the backup history file in the output of pg_stop_backup(), which would
map as well with what Fujii-san's idea to add this file to the backup
data itself, and do the same for backups taken on a primary.  However
this would mean moving the 'c' marker marking the end of the tar stream
within do_pg_stop_backup(), and I am in no way a fan of that: the
handling of the tar stream should be out of reach of the start and stop
phases.

Another idea that I have here as well would be to change a bit the
history backup file name so as the backup name is added to it, but that
does not fly high as pg_basebackup uses its own name with spaces
(Yeah!), or even better the PID of the backend taking the backup.

The renaming is more appealing, still not perfect.  So my mood of the
day would be to just drop the patch entirely.
--
Michael