Re: Remove useless arguments in ReadCheckpointRecord().

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