Hi,
Thanks for the review!
On Thu, 22 Feb 2024 at 16:55, Daniel Gustafsson <daniel@yesql.se> wrote:
>
> > On 6 Dec 2023, at 14:03, Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
>
> > There is an ongoing thread [1] for adding missing SQL error codes to
> > PANIC and FATAL error reports in xlogrecovery.c file. I did the same
> > but for xlog.c and relcache.c files.
>
> - elog(PANIC, "space reserved for WAL record does not match what was written");
> + ereport(PANIC,
> + (errcode(ERRCODE_DATA_CORRUPTED),
> + errmsg("space reserved for WAL record does not match what was written")));
>
> elogs turned into ereports should use errmsg_internal() to keep the strings
> from being translated.
Does errmsg_internal() need to be used all the time when turning elogs
into ereports? errmsg_internal()'s comment says that "This should be
used for "can't happen" cases that are probably not worth spending
translation effort on.". Is it enough to check if the error message
has a translation, and then decide the use of errmsg_internal() or
errmsg()?
> - elog(FATAL, "could not write init file");
> + ereport(FATAL,
> + (errcode_for_file_access(),
> + errmsg("could not write init file")));
>
> Is it worthwhile adding %m on these to get a little more help when debugging
> errors that shouldn't happen?
I believe it is worthwhile, so I will add.
> - elog(FATAL, "could not write init file");
> + ereport(FATAL,
> + (errcode_for_file_access(),
>
> The extra parenthesis are no longer needed, I don't know if we have a policy to
> remove them when changing an ereport call but we should at least not introduce
> new ones.
>
> - elog(FATAL, "cannot read pg_class without having selected a database");
> + ereport(FATAL,
> + (errcode(ERRCODE_INTERNAL_ERROR),
>
> ereport (and thus elog) already defaults to ERRCODE_INTERNAL_ERROR for ERROR or
> higher, so unless there is a better errcode an elog() call if preferrable here.
I did not know these, thanks.
> > I couldn't find a suitable error code for the "cache lookup failed for
> > relation" error in relcache.c and this error comes up in many places.
> > Would it be reasonable to create a new error code specifically for
> > this?
>
> We use ERRCODE_UNDEFINED_OBJECT for similar errors elsewhere, perhaps we can
> use that for these as well?
It looks okay to me, ERRCODE_UNDEFINED_OBJECT is mostly used for the
'not exist' errors and it seems the main reason for the 'cache lookup
failed for relation' error is because heap tuple does not exist
anymore.
--
Regards,
Nazir Bilal Yavuz
Microsoft