On 6/30/26 05:15, David Steele wrote: > The patch now implements only the new flag for pgcontrol and wires this > logic into basebackup.
Hi David,
Thanks for working on this -- it fills a gap that I think Postgres has
needed for a long time.
I reviewed and tested v11 on master and it looks all good.
Attached is v12: your patch plus a TAP test, two one-token fixes and a
commit message tweak, and the same as a delta against v11.
One thing I think needs a decision, which I left alone. xlogrecovery.c
still suggests removing backup_label in two places. Restore a
"pg_basebackup -X none" backup without recovery.signal and you get
"could not locate required checkpoint record", hinting "try removing the
file backup_label". Do that, and you get "could not find backup_label
required for recovery" telling you to put it back.
Blocking startup is right -- that second attempt used to succeed and
silently corrupt the cluster. But the patch exists to stop people from
removing backup_label, so the hint telling them to do so probably should
not survive it. Either suppress those lines when backupLabelRequired is
set, or have the new hint mention recovery.signal, which is usually the
real problem. Say which you prefer and I will write it.
Smaller things:
1. errhint() should start with a capital letter ("Restore the ...").
Fixed in v12, along with "(char *)controlFile" -> "(char *) controlFile".
2. The commit message says "limited to pg_basebackup", but the patch also
sets the flag in pg_rewind. The logic is fine; v12 just says so, and
it probably deserves a line in pg_rewind.sgml.
3. Assert(crc_ok && memcmp(...) == 0) combines two conditions; splitting
them would say which one failed.
4. v12 moves the test out of 002_archiving.pl, which is about
archive_cleanup_command, into 057_backup_label_required.pl. Your
assertions are kept, plus checks that the flag is set in the backup,
cleared after recovery, and the same for a backup from a standby.
5. Docs are untouched beyond the pg_control_recovery() table. At least
pg_basebackup.sgml needs a mention, and something in backup.sgml about
low-level backups not getting this protection now that the SQL part is
dropped. Worth saying that pg_resetwal is the only way to clear the
flag, too.
Thanks,
Shihao
pgcontrol-basebackup-flag-v12.patch
Description: Binary data
