Re: Requiring recovery.signal or standby.signal when recovering with a backup_label

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