Re: Add recovery to pg_control and remove backup_label

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: David Steele <david(at)pgmasters(dot)net>
Cc: Pg Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Add recovery to pg_control and remove backup_label
Date: 2023-11-07 08:20:27
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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

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

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message John Naylor 2023-11-07 08:27:53 Commitfest: older Waiting on Author entries
Previous Message Xiang Gao 2023-11-07 08:05:45 RE: CRC32C Parallel Computation Optimization on ARM