Date: Mon, 28 Feb 2022 21:51:00 -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: <20220301055100.GA1002387@nathanxps13>
References: 
 <CAHg+QDfiM+WU61tF6=nPZocMZvHDzCK47Kneyb0ZRULYzV5sKQ@mail.gmail.com>
 <20220221172306.GA3698472@nathanxps13>
 <164589413251.1035.2695761148079719188.pgcf@coridan.postgresql.org>
 <20220226220614.GA673898@nathanxps13>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220226220614.GA673898@nathanxps13>
Archived-At: 
 <https://www.postgresql.org/message-id/20220301055100.GA1002387%40nathanxps13>
Precedence: bulk

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

I've spent some time looking into this, and I haven't found a clean way to
emit a WARNING only if the "exclusive" parameter is supplied (and set to
false).  AFAICT flinfo->fn_expr doesn't tell us whether the parameter was
supplied or the default value was used.  I was able to get it working by
splitting pg_start_backup() into 3 separate internal functions (i.e.,
pg_start_backup_1arg(), pg_start_backup_2arg(), and
pg_start_backup_3arg()), but this breaks calls such as
pg_start_backup('mylabel', exclusive => false), and it might complicate
privilege management for users.

Without a WARNING, I think it will be difficult to justify removing the
"exclusive" parameter in the future.  We would either need to leave it
around forever, or we would have to risk unnecessarily breaking some
working backup scripts.  I wonder if we should just remove it now and make
sure that this change is well-documented in the release notes.

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