Re: Add recovery to pg_control and remove backup_label

On Mon, Nov 06, 2023 at 05:39:02PM -0400, David Steele wrote:
> On 11/6/23 02:35, Michael Paquier wrote:
>> The patch is failing the recovery test 039_end_of_wal.pl. Could you
>> look at the failure?
>
> I'm not seeing this failure, and CI seems happy [1]. Can you give details of
> the error message?

I've retested today, and miss the failure. I'll let you know if I see
this again.

>> + /* Clear fields used to initialize recovery */
>> + ControlFile->backupCheckPoint = InvalidXLogRecPtr;
>> + ControlFile->backupStartPointTLI = 0;
>> + ControlFile->backupRecoveryRequired = false;
>> + ControlFile->backupFromStandby = false;
>>
>> These variables in the control file are cleaned up when the
>> backup_label file was read previously, but backup_label is renamed to
>> backup_label.old a bit later than that. Your logic looks correct seen
>> from here, but shouldn't these variables be set much later, aka just
>> *after* UpdateControlFile(). This gap between the initialization of
>> the control file and the in-memory reset makes the code quite brittle,
>> IMO.

Yeah, sorry, there's a think from me here. I meant to reset these
variables just before the UpdateControlFile() after InitWalRecovery()
in UpdateControlFile(), much closer to it.

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

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