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