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

On Sat, Oct 14, 2023 at 03:45:33PM -0400, David Steele wrote:
> On 9/28/23 19:59, Michael Paquier wrote:
> 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.

Thanks for the review.  Yes, I am wondering if other people would
chime in here.  It doesn't feel like this has gathered enough
opinions.  Now this thread has been around for many months, and we've
done quite a few changes in the backup APIs in the last few years with
few users complaining back about them..

> 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.

The main point is that there is no meaning in setting the latch until
the backup_label file is read because if ArchiveRecoveryRequested is
*not* set the startup process would outright fail as of the lack of
[recovery|standby].signal.

>> 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.

Looking at the streaming APIs of pg_basebackup, it looks like this
would be a matter of using bbstreamer_inject_file() to inject an empty
file into the stream.  Still something seems to be off once
compression methods are involved..  Hmm.  I am not sure.  Well, this
could always be done as a patch independant of this one, under a
separate discussion.  There are extra arguments about whether it would
be a good idea to add a recovery.signal even when taking a backup from
a standby, and do that only in 17~.

>> 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.

It wouldn't be the first time we break compatibility in this area, so
perhaps you are right and keeping this requirement is fine, even if it
requires one extra step when recovering a self-contained backup
generated by pg_basebackup.  At least this forces users to look at
their setup and check if something is wrong.  We'd likely finish with
a few "bug" reports, as well :D
--
Michael