Hi Michael,
Thanks for the detailed feedback.
> For clarity's sake, we are talking about lowering this one in
> xlogrecovery.c, which relates to the code path where these is no
> backup_label file:
> ereport(PANIC,
> errmsg("could not locate a valid checkpoint record at %X/%08X",
> LSN_FORMAT_ARGS(CheckPointLoc)));
I agree that case (1) is sufficient for the purpose of this change. I
mentioned the scenarios where a backup_label file exists mainly to
consider additional coverage in this area, but I agree those would
only be bonuses, as you note later.
> For the sake of the change from the PANIC to FATAL mentioned at the
> top of this message, (1) would be enough.
>
> The two cases of (2) I'm mentioning would be nice bonuses. I would
> recommend to double-check first if we trigger these errors in some
> tests of the existing tests, actually, perhaps we don't need to add
> anything except a check in some node's logs for the error string
> patterns wanted.
I agree with your assessment. Case (1) is enough for this change, and
the cases in (2) would be nice bonuses. I’m fine with dropping cases
(3), (4) for now.
I had a quick look at the existing recovery TAP tests and didn’t
immediately find a case where simply adding log checks would cover
these error paths, but I’ll double‑check once more before sending the
patch. I’ll work on this and share the patch soon.
Best Regards,
Nitin Jadhav
Azure Database for PostgreSQL
Microsoft
