public inbox for [email protected]
help / color / mirror / Atom feedFrom: Nathan Bossart <[email protected]>
To: Chapman Flack <[email protected]>
Cc: [email protected]
Cc: David Steele <[email protected]>
Subject: Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file
Date: Sat, 26 Feb 2022 14:06:14 -0800
Message-ID: <20220226220614.GA673898@nathanxps13> (raw)
In-Reply-To: <164589413251.1035.2695761148079719188.pgcf@coridan.postgresql.org>
References: <CAHg+QDfiM+WU61tF6=nPZocMZvHDzCK47Kneyb0ZRULYzV5sKQ@mail.gmail.com>
<20220221172306.GA3698472@nathanxps13>
<164589413251.1035.2695761148079719188.pgcf@coridan.postgresql.org>
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
view thread (14+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: Postgres restart in the middle of exclusive backup and the presence of backup_label file
In-Reply-To: <20220226220614.GA673898@nathanxps13>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox