On 12/11/18 5:24 PM, David Steele wrote:
>
> Attached is the patch.
>
> I was a bit surprised by how much code went away. There was a lot of
> code dedicated to making sure that backup_label got renamed on shutdown,
> that there was not an exclusive backup running, etc.
>
> There were no tests for exclusive backup so the test changes were minimal.
>
> I did have to replace one "hot backup" in
> 010_logical_decoding_timelines.pl. I'm not sure why the backup was
> being done this way, or why the standby needs a copy of pg_replslot
> (which is not copied by pg_basebackup). It might be worth looking at
> but it seemed out of scope for this patch.
Rebased patch is attached.
I also fixed the issue that Robert pointed out with regard to changing
the function signatures. It's a bit weird that there are optional
parameters when the last parameter is really not optional (i.e. the
default will lead to an error). Any suggestions are appreciated.
Reading back through the thread, the major objection to the original
patch was that it was submitted too soon. Hopefully the elapsed time
addresses that concern.
Another common objection is that the new API is hard to use from bash
scripts. This is arguably still true, but Laurenz has demonstrated that
bash is capable of this feat:
https://github.com/cybertec-postgresql/safe-backup
Lastly, there is some concern about getting the backup label too late
when doing snapshots or using traditional backup software. It would
certainly be possible to return the backup_label and tablespace_map from
pg_start_backup() and let the user make the choice about what to do with
them. Any of these methods will require some scripting so I don't see
why the files couldn't be written as, for example, backup_label.snap and
tablespace_map.snap and then renamed by the restore script. The patch
does not currently make this change, but it could be added pretty easily
if that overcomes this objection.
Regards,
--
-David
david@pgmasters.net
