Re: Remove useless arguments in ReadCheckpointRecord().

On 2022/07/21 0:29, Bharath Rupireddy wrote:
> How about we transform the following messages into something like below?
>
> (errmsg("could not locate a valid checkpoint record"))); after
> ReadCheckpointRecord() for control file cases to "could not locate
> valid checkpoint record in control file"
> (errmsg("could not locate required checkpoint record"), after
> ReadCheckpointRecord() for backup_label case to "could not locate
> valid checkpoint record in backup_label file"
>
> The above messages give more meaningful and unique info to the users.

Agreed. Attached is the updated version of the patch.
Thanks for the review!

> May be unrelated, IIRC, for the errors like ereport(PANIC,
> (errmsg("could not locate a valid checkpoint record"))); we wanted to
> add a hint asking users to consider running pg_resetwal to fix the
> issue. The error for ReadCheckpointRecord() in backup_label file
> cases, already gives a hint errhint("If you are restoring from a
> backup, touch \"%s/recovery.signal\" ...... Adding the hint of running
> pg_resetwal (of course with a caution that it can cause inconsistency
> in the data and use it as a last resort as described in the docs)
> helps users and support engineers a lot to mitigate the server down
> cases quickly.

+1 to add some hint messages. But I'm not sure if it's good to hint the use of pg_resetwal because, as you're saying, it should be used as a last resort and has some big risks like data loss, corruption, etc. That is, I'm concerned about that some users might quickly run pg_resetwal because hint message says that, without reading the docs nor understanding those risks.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION