Date: Sat, 26 Feb 2022 14:06:14 -0800
From: Nathan Bossart <nathandbossart@gmail.com>
To: Chapman Flack <chap@anastigmatix.net>
Cc: pgsql-hackers@lists.postgresql.org, David Steele <david@pgmasters.net>
Subject: Re: Postgres restart in the middle of exclusive backup and the
 presence of backup_label file
Message-ID: <20220226220614.GA673898@nathanxps13>
References: <CAHg+QDfiM+WU61tF6=nPZocMZvHDzCK47Kneyb0ZRULYzV5sKQ@mail.gmail.com>
 <20220221172306.GA3698472@nathanxps13>
 <164589413251.1035.2695761148079719188.pgcf@coridan.postgresql.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <164589413251.1035.2695761148079719188.pgcf@coridan.postgresql.org>
Archived-At: <https://www.postgresql.org/message-id/20220226220614.GA673898%40nathanxps13>
Precedence: bulk

On Sat, Feb 26, 2022 at 04:48:52PM +0000, Chapman Flack wrote:
> 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.

This is a good point.  I think I agree with your proposed changes.  I
believe it is possible to add a deprecation warning only when 'exclusive'
is specified.  If anything, we can create a separate function that accepts
the 'exclusive' parameter and that always emits a NOTICE or WARNING.

> 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.)

Agreed.  I will look into updating this one, too.  I think the 'exclusive'
parameter should remain documented for now for both pg_start_backup() and
pg_stop_backup(), but this documentation will just note that it is there
for backward compatibility and must be set to false.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com