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
signature.asc
Description: PGP signature