public inbox for [email protected]
help / color / mirror / Atom feedFrom: Michael Paquier <[email protected]>
To: David Steele <[email protected]>
Cc: Pg Hackers <[email protected]>
Subject: Re: Add recovery to pg_control and remove backup_label
Date: Tue, 7 Nov 2023 17:20:27 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
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
Attachments:
[application/pgp-signature] signature.asc (833B, ../[email protected]/2-signature.asc)
download
view thread (5+ messages)
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected]
Subject: Re: Add recovery to pg_control and remove backup_label
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox