Re: Remove useless arguments in ReadCheckpointRecord().

At Fri, 22 Jul 2022 11:50:14 +0900, Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote in
> Sorry, I failed to understand your point. Could you clarify your
> point?

Wrote as a reply to Tom's message.

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

I believed that it is recommended to move to the style not having the
outmost parens. That style has been introduced by e3a87b4991.

> * The extra parentheses around the auxiliary function calls are now
> optional. Aside from being a bit less ugly, this removes a common
> gotcha for new contributors, because in some cases the compiler errors
> you got from forgetting them were unintelligible.
...
> While new code can be written either way, code intended to be
> back-patched will need to use extra parens for awhile yet. It seems
> worth back-patching this change into v12, so as to reduce the window
> where we have to be careful about that by one year. Hence, this patch
> is careful to preserve ABI compatibility; a followup HEAD-only patch
> will make some additional simplifications.

So I thought that if we modify an error message, its ererpot can be
rewritten.

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

Thanks!

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

Understood.

regards
--
Kyotaro Horiguchi
NTT Open Source Software Center