Re: Remove useless arguments in ReadCheckpointRecord().

I agree to removing the two parameters. And agree to let
ReadCheckpointRecord not conscious of the location source.

At Thu, 21 Jul 2022 11:45:23 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in 
> Agreed. Attached is the updated version of the patch.
> Thanks for the review!

-    (errmsg("could not locate required checkpoint record"),
+    (errmsg("could not locate a valid checkpoint record in backup_label file"),

"in backup_label" there looks *to me* need some verb..  By the way,
this looks like a good chance to remove the (now) extra parens around
errmsg() and friends.

For example:

-    (errmsg("could not locate a valid checkpoint record in backup_label file"),
+    errmsg("could not locate checkpoint record spcified in backup_label file"),

-    (errmsg("could not locate a valid checkpoint record in control file")));
+    errmsg("could not locate checkpoint record recorded in control file")));


+                (errmsg("invalid checkpoint record")));

Is it useful to show the specified LSN there?

+                (errmsg("invalid resource manager ID in checkpoint record")));

We have a similar message "invalid resource manager ID %u at
%X/%X". Since the caller explains that it is a checkpoint record, we
can share the message here.

+                (errmsg("invalid xl_info in checkpoint record")));

(It is not an issue of this patch, though) I don't think this is
appropriate for user-facing message. Counldn't we say "unexpected
record type: %x" or something like?

+                (errmsg("invalid length of checkpoint record")));

We have "record with invalid length at %X/%X" or "invalid record
length at %X/%X: wanted %u, got %u". Counld we reuse any of them?

> > 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.

I don't think we want to recommend pg_resetwal to those who don't
reach it by themselves by other means.  I know of a few instances of
people who had the tool (unrecoverably) break their own clusters.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center