public inbox for [email protected]  

help / color / mirror / Atom feed

From: David Steele <[email protected]>
To: Michael Paquier <[email protected]>
Cc: Kyotaro Horiguchi <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Subject: Re: Requiring recovery.signal or standby.signal when recovering with a backup_label
Date: Sat, 14 Oct 2023 15:45:33 -0400
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>

On 9/28/23 19:59, Michael Paquier wrote:
> On Thu, Sep 28, 2023 at 04:23:42PM -0400, David Steele wrote:
>>
>> So overall, +1 for Michael's patch, though I have only read through it and
>> not tested it yet.
> 
> Reviews, thoughts and opinions are welcome.

OK, I have now reviewed and tested the patch and it looks good to me. I 
stopped short of marking this RfC since there are other reviewers in the 
mix.

I dislike that we need to repeat:

OwnLatch(&XLogRecoveryCtl->recoveryWakeupLatch);

But I see the logic behind why you did it and there's no better way to 
do it as far as I can see.

>> One comment, though, if we are going to require recovery.signal when
>> backup_label is present, should it just be implied? Why error and force the
>> user to create it?
> 
> That's one thing I was considering, but I also cannot convince myself
> that this is the best option because the presence of recovery.signal
> or standby.standby (if both, standby.signal takes priority) makes it
> clear what type of recovery is wanted at disk level.  I'd be OK if
> folks think that this is a sensible consensus, as well, even if I
> don't really agree with it.

I'm OK with keeping it as required for now.

> Another idea I had was to force the creation of recovery.signal by
> pg_basebackup even if -R is not used.  All the reports we've seen with
> people getting confused came from pg_basebackup that enforces no
> configuration.

This change makes it more obvious if configuration is missing (since 
you'll get an error), however +1 for adding this to pg_basebackup.

> A last thing, that had better be covered in a separate thread and
> patch, is about validateRecoveryParameters().  These days, I'd like to
> think that it may be OK to lift at least the restriction on
> restore_command being required if we are doing recovery to ease the
> case of self-contained backups (aka the case where all the WAL needed
> to reach a consistent point is in pg_wal/ or its tarball)

Hmmm, I'm not sure about this. I'd prefer users set 
restore_command=/bin/false explicitly to fetch WAL from pg_wal by 
default if that's what they really intend.

Regards,
-David

view thread (3+ messages)

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], [email protected], [email protected]
  Subject: Re: Requiring recovery.signal or standby.signal when recovering with a backup_label
  In-Reply-To: <[email protected]>

* 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