On 2022/07/21 14:54, Kyotaro Horiguchi wrote:
> I agree to removing the two parameters. And agree to let
> ReadCheckpointRecord not conscious of the location source.
Thanks for the review!
> 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..
Sorry, I failed to understand your point. Could you clarify your point?
> 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")));
Because it's recommended not to put parenthesis just before errmsg(), you mean? I'm ok to remove such parenthesis, but
I'dlike understand why before doing that. I was thinking that either having or not having parenthesis in front of
errmsg()is ok, so there are many calls to errmsg() with parenthesis, in xlogrecovery.c.
> + (errmsg("invalid checkpoint record")));
>
> Is it useful to show the specified LSN there?
Yes, LSN info would be helpful also for debugging.
I separated the patch into two; one is to remove useless arguments in ReadCheckpointRecord(), another is to improve log
messages.I added LSN info in log messages in the second patch.
> + (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.
+1
> + (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?
The proposed log message doesn't look like an improvement. But I have no better one. So I left the message as it is, in
thepatch, for now.
>
> + (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?
+1
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION