Re: Add recovery to pg_control and remove backup_label

On Tue, Nov 07, 2023 at 05:20:27PM +0900, Michael Paquier wrote:
> On Mon, Nov 06, 2023 at 05:39:02PM -0400, David Steele wrote:
> I've retested today, and miss the failure. I'll let you know if I see
> this again.

I've done a few more dozen runs, and still nothing. I am wondering
what this disturbance was.

>> If we set these fields where backup_label was renamed, the logic would not
>> be exactly the same since pg_control won't be updated until the next time
>> through the loop. Since the fields should be updated before
>> UpdateControlFile() I thought it made sense to keep all the updates
>> together.
>>
>> Overall I think it is simpler, and we don't need to acquire a lock on
>> ControlFile.
>
> What you are proposing is the same as what we already do for
> backupEndRequired or backupStartPoint in the control file when
> initializing recovery, so objection withdrawn.
>
>> Thomas included this change in his pg_basebackup changes so I did the same.
>> Maybe wait a bit before we split this out? Seems like a pretty small
>> change...
>
> Seems like a pretty good argument for refactoring that now, and let
> any other patches rely on it. Would you like to send a separate
> patch?

The split still looks worth doing seen from here, so I am switching
the patch as WoA for now.

>>> Actually, grouping backupStartPointTLI and minRecoveryPointTLI should
>>> reduce more the size with some alignment magic, no?
>>
>> I thought about this, but it seemed to me that existing fields had been
>> positioned to make the grouping logical rather than to optimize alignment,
>> e.g. minRecoveryPointTLI. Ideally that would have been placed near
>> backupEndRequired (or vice versa). But if the general opinion is to
>> rearrange for alignment, I'm OK with that.
>
> I've not tested, but it looks like moving backupStartPointTLI after
> backupEndPoint should shave 8 bytes, if you want to maintain a more
> coherent group for the LSNs.

+ * backupFromStandby indicates that the backup was taken on a standby. It is
+ * require to initialize recovery and set to false afterwards.
s/require/required/.

The term "backup recovery", that we've never used in the tree until
now as far as I know. Perhaps this recovery method should just be
referred as "recovery from backup"?

By the way, there is another thing that this patch has forgotten: the
SQL functions that display data from the control file. Shouldn't
pg_control_recovery() be extended with the new fields? These fields
may be less critical than the other ones related to recovery, but I
suspect that showing them can become handy at least for debugging and
monitoring purposes.

Something in this area is that backupRecoveryRequired is the switch
controlling if the fields set by the recovery initialization. Could
it be actually useful to leave the other fields as they are and only
reset backupRecoveryRequired before the first control file update?
This would leave a trace of the backup history directly in the control
file.

What about pg_resetwal and RewriteControlFile()? Shouldn't these
recovery fields be reset as well?

git diff --check is complaining a bit.
--
Michael