Hi Shihao,
On 8/28/26 05:34, shihao zhong wrote:
>
> 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.
Thank you for having a look at the patch.
> 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.
Agreed, and I have addressed it in v13 by adding a conditional to the
relevant errors with a alternate message:
ControlFile->backupLabelRequired ?
errhint("Touch \"%s/recovery.signal\" or \"%s/standby.signal\" and add
required recovery options.\n"
"Do not remove \"%s/backup_label\"; it is required to recover this
backup.",
DataDir, DataDir, DataDir) :
> Smaller things:
> 1. errhint() should start with a capital letter ("Restore the ...").
> Fixed in v12, along with "(char *)controlFile" -> "(char *) controlFile".
pg_indent would catch this, but always better to have to source as clean
as possible. Added into v13.
> 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.
Changes to the commit message preserved in v13. Docs updated per below.
> 3. Assert(crc_ok && memcmp(...) == 0) combines two conditions; splitting
> them would say which one failed.
I left this as is from v11. I think it's a pretty normal thing to do. If
you are running with asserts then you are debugging anyway so resolving
the missed condition is pretty trivial, IMO.
> 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.
Yes, I think a dedicated test module makes sense here, so I kept that in
v13.
> 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.
Docs have been updated with each of these suggestions.
I also added a few additional tests to flesh out the coverage.
Regards,
-David
