Message-ID: <e6657efe-f7b1-0894-9807-453c01bad81a@pgmasters.net>
Date: Tue, 1 Mar 2022 08:44:51 -0600
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.6.1
Subject: Re: Postgres restart in the middle of exclusive backup and the
 presence of backup_label file
Content-Language: en-US
To: Nathan Bossart <nathandbossart@gmail.com>,
 Chapman Flack <chap@anastigmatix.net>
Cc: pgsql-hackers@lists.postgresql.org
References: <CAHg+QDfiM+WU61tF6=nPZocMZvHDzCK47Kneyb0ZRULYzV5sKQ@mail.gmail.com>
 <20220221172306.GA3698472@nathanxps13>
 <164589413251.1035.2695761148079719188.pgcf@coridan.postgresql.org>
 <20220226220614.GA673898@nathanxps13> <20220301055100.GA1002387@nathanxps13>
From: David Steele <david@pgmasters.net>
In-Reply-To: <20220301055100.GA1002387@nathanxps13>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/e6657efe-f7b1-0894-9807-453c01bad81a%40pgmasters.net>
Precedence: bulk

On 2/28/22 23:51, Nathan Bossart wrote:
> 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.

Personally, I am in favor of removing it. We change/rename 
functions/tables/views when we need to, and this happens in almost every 
release.

What we need to do is make sure that an older installation won't 
silently work in a broken way, i.e. if we remove the exclusive flag 
somebody expecting the pre-9.6 behavior might not receive an error and 
think everything is OK. That would not be good.

One option might be to rename the functions. Something like 
pg_backup_start/stop.

Regards,
-David