Re: Add missing error codes to PANIC/FATAL error reports in xlogrecovery

Hi,

On 2023-11-30 10:54:12 -0800, Krishnakumar R wrote:
> diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
> index c61566666a..2f50928e7e 100644
> --- a/src/backend/access/transam/xlogrecovery.c
> +++ b/src/backend/access/transam/xlogrecovery.c
> @@ -630,7 +630,8 @@ InitWalRecovery(ControlFileData *ControlFile, bool *wasShutdown_ptr,
>                  if (!ReadRecord(xlogprefetcher, LOG, false,
>                                  checkPoint.ThisTimeLineID))
>                      ereport(FATAL,
> -                            (errmsg("could not find redo location referenced by checkpoint record"),
> +                            (errcode(ERRCODE_DATA_CORRUPTED),
> +                             errmsg("could not find redo location referenced by checkpoint record"),
>                               errhint("If you are restoring from a backup, touch \"%s/recovery.signal\" or
\"%s/standby.signal\"and add required recovery options.\n"
 
>                                       "If you are not restoring from a backup, try removing the file
\"%s/backup_label\".\n"
>                                       "Be careful: removing \"%s/backup_label\" will result in a corrupt cluster if
restoringfrom a backup.",
 

Wondering if we should add a ERRCODE_CLUSTER_CORRUPTED for cases like this. We
have ERRCODE_DATA_CORRUPTED and ERRCODE_INDEX_CORRUPTED, which make
ERRCODE_DATA_CORRUPTED feel a bit too specific in this kind of situation?

OTOH, just having anything other than ERRCODE_INTERNAL_ERROR is better.


> @@ -640,7 +641,8 @@ InitWalRecovery(ControlFileData *ControlFile, bool *wasShutdown_ptr,
>          else
>          {
>              ereport(FATAL,
> -                    (errmsg("could not locate required checkpoint record"),
> +                    (errcode(ERRCODE_DATA_CORRUPTED),
> +                     errmsg("could not locate required checkpoint record"),
>                       errhint("If you are restoring from a backup, touch \"%s/recovery.signal\" or
\"%s/standby.signal\"and add required recovery options.\n"
 
>                               "If you are not restoring from a backup, try removing the file \"%s/backup_label\".\n"
>                               "Be careful: removing \"%s/backup_label\" will result in a corrupt cluster if restoring
froma backup.",
 

Another aside: Isn't the hint here obsolete since we've removed exclusive
backups? I can't think of any scenario now where removing backup_label would
be correct in a non-exclusive backup.


> @@ -817,7 +820,8 @@ InitWalRecovery(ControlFileData *ControlFile, bool *wasShutdown_ptr,
>           */
>          switchpoint = tliSwitchPoint(ControlFile->checkPointCopy.ThisTimeLineID, expectedTLEs, NULL);
>          ereport(FATAL,
> -                (errmsg("requested timeline %u is not a child of this server's history",
> +                (errcode(ERRCODE_DATA_CORRUPTED),
> +                 errmsg("requested timeline %u is not a child of this server's history",
>                          recoveryTargetTLI),
>                   errdetail("Latest checkpoint is at %X/%X on timeline %u, but in the history of the requested
timeline,the server forked off from that timeline at %X/%X.",
 
>                             LSN_FORMAT_ARGS(ControlFile->checkPoint),

Hm, this one arguably is not corruption, but we still cannot
continue. ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE or maybe a new error code?

Greetings,

Andres Freund