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(dot)fujii(at)oss(dot)nttdata(dot)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'd like 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 the patch, 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