Hi Andrey,
Thank you for having a look at the patch.
On 9/15/26 21:13, Andrey Borodin wrote:
>
> On 15 Sep 2026, David Steele wrote:
>> I left this as is from v11. I think it's a pretty normal thing to do.
>
> I found a case where the assertion itself does not hold:
>
>> +Assert(crc_ok &&
>> + memcmp(dataDisk, &controlData, sizeof(ControlFileData)) == 0);
>
> During XLOG_CHECKPOINT_ONLINE replay, xlog_redo() updates
> ControlFile->checkPointCopy.nextXid under ControlFileLock without
> calling UpdateControlFile(). The in-memory copy can therefore differ
> from the on-disk file even while we hold the lock.
>
> I can trigger this with the v13 patch on current master: pause a
> standby's base backup at basebackup-before-send-files, allocate an XID
> on the primary, run CHECKPOINT and wait for replay. Resuming the backup
> hits this assertion in backup_control_file().
>
> IIUC, this mismatch does not mean that pg_control is torn. Perhaps we
> could drop the whole-struct equality assertion?
The mismatch is not an issue as far as I can see. I'll leave that
determination up to Michael, though, since he added the assert.
> On the motivation, we have had many WAL-G support requests from users
> who removed backup_label to get a restored cluster to start. Here is
> one example [0], and a pgBackRest case prompted by the startup hint [1].
> I think protecting against this mistake is worthwhile.
I've seen this now more times than I can count, even from people that I
would expect to know better. The hint from Postgres to delete
backup_label certainly does not help the situation, though even with
this patch we can't remove it completely. I'm hoping to at least deal
with the common case.
> One concern about describing pg_resetwal as "the only way to clear it
> otherwise", even with the corruption warning: a WAL-G user actually
> removed backup_label and ran pg_resetwal, thinking this only meant
> giving up PITR [2]. Could we frame this as bypassing the protection,
> not as a way to clear the flag? It cannot replace the WAL replay needed
> to make a backup consistent.
Yeah, I have also seen pg_resetwal abused in various ways. I added that
language based on feedback from Shihao and I think it should be
documented but perhaps we need more cautionary language there. Let's see
what people think.
Regards,
-David
