Re: Return pg_control from pg_backup_stop().

From: David Steele <david(at)pgbackrest(dot)org>
To: Andrey Borodin <x4mmm(at)yandex-team(dot)ru>
Cc: shihao zhong <zhong950419(at)gmail(dot)com>, Andy Pogrebnoi <andrew(dot)pogrebnoi(at)percona(dot)com>, Michael Paquier <michael(at)paquier(dot)xyz>, Haibo Yan <tristan(dot)yim(at)gmail(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>, Heikki Linnakangas <hlinnaka(at)iki(dot)fi>, Robert Haas <robertmhaas(at)gmail(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, Fujii Masao <masao(dot)fujii(at)gmail(dot)com>, Shaun Thomas <shaun(dot)thomas(at)pgedge(dot)com>
Subject: Re: Return pg_control from pg_backup_stop().
Date: 2026-09-15 18:45:38
Message-ID: d9372aa0-0990-445a-b928-0902cd7088a9@pgbackrest.org
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Peter Geoghegan 2026-09-15 18:47:38 Re: index prefetching
Previous Message Robert Treat 2026-09-15 18:30:34 Re: REPACK (CONCURRENTLY) doesn't handle invalid indexes