Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file - Mailing list pgsql-hackers

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           not tested
Documentation:            not tested

This patch applies cleanly for me and passes installcheck-world.
I have not yet studied all of the changes in detail.

Some proofreading nits in the documentation: the pg_stop_backup
with arguments has lost the 'exclusive' argument, but still shows a comma
before the 'wait_for_archive' argument. And the niladic pg_stop_backup
is still documented, though it no longer exists.

My biggest concerns are the changes to the SQL-visible pg_start_backup
and pg_stop_backup functions. When the non-exclusive API was implemented
(in 7117685), that was done with care (with a new optional argument to
pg_start_backup, and a new overload of pg_stop_backup) to avoid immediate
breakage of working backup scripts.

With this patch, even scripts that were dutifully migrated to that new API and
now invoke pg_start_backup(label, false) or (label, exclusive => false) will
immediately and unnecessarily break. What I would suggest for this patch
would be to change the exclusive default from true to false, and have the
function report an ERROR if true is passed.

Otherwise, for sites using a third-party backup solution, there will be an
unnecessary requirement to synchronize a PostgreSQL upgrade with an
upgrade of the backup solution that won't be broken by the change. For
a site with their backup procedures scripted in-house, there will be an
unnecessarily urgent need for the current admin team to study and patch
the currently-working scripts.

That can be avoided by just changing the default to false and rejecting calls
where true is passed. That will break only scripts that never got the memo
about moving to non-exclusive backup, available for six years now.

Assuming the value is false, so no error is thrown, is it practical to determine
from flinfo->fn_expr whether the value was defaulted or supplied? If so, I would
further suggest reporting a deprecation WARNING if it was explicitly supplied,
with a HINT that the argument can simply be removed at the call site, and will
become unrecognized in some future release.

pg_stop_backup needs thought, because 7117685 added a new overload for that
function, rather than just an optional argument. This patch removes the old
niladic version that returned pg_lsn, leaving just one version, with an optional
argument, that returns a record.

Here again, the old niladic one was only suitable for exclusive backups, so there
can't be any script existing in 2022 that still calls that unless it has never been
updated in six years to nonexclusive backups, and that breakage can't be
helped.

Any scripts that did get dutifully updated over the last six years will be calling the
record-returning version, passing false, or exclusive => false. This patch as it
stands will unnecessarily break those, but here again I think that can be avoided
just by making the exclusive parameter optional with default false, and reporting
an error if true is passed.

Here again, I would consider also issuing a deprecation warning if the argument
is explicitly supplied, if it is practical to determine that from fn_expr. (I haven't
looked yet to see how practical that is.)

Regards,
-Chap