On Sat, 28 Jun 2025 00:33:45 +0900
Fujii Masao <masao.fu...@oss.nttdata.com> wrote:
>
>
> On 2025/06/27 13:09, Yugo Nagata wrote:
> > On Fri, 27 Jun 2025 12:22:17 +0900
> > Fujii Masao <masao.fu...@oss.nttdata.com> wrote:
> >
> >>> However, your patch is clear, and it seems we don't need to worry about
> >>> this concern.
> >>> Your patch looks good to me.
> >>
> >> Thanks for reviewing the patch! I've marked it as ready for committer.
> >
> > I have a few minor comment on the patch.
> >
> > + if (ival < 0)
> > + ereport(ERROR,
> > +
> > (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> > + errmsg("%s requires a
> > Boolean value, a non-negative "
> > +
> > "integer, or the string \"match\"",
> > +
> > def->defname)));
> >
> > ereport(ERROR,
> > (errcode(ERRCODE_SYNTAX_ERROR),
> > - errmsg("%s requires a Boolean value or \"match\"",
> > + errmsg("%s requires a Boolean value, a non-negative
> > integer, "
> > + "or the string \"match\"",
> > def->defname)));
> >
> > These two pieces of code raise the same error, but with different error
> > codes:
> > one returns ERRCODE_INVALID_PARAMETER_VALUE, while the other returns
> > ERRCODE_SYNTAX_ERROR.
> >
> > I believe the former is more appropriate, although the existing code uses
> > the
> > latter. In any case, the error codes should be consistent.
>
> I'm not sure there's an actual rule like "the error code must match
> if the error message is the same." But if using the same message with
> a different error code is confusing, I'm fine with changing
> the earlier message. For example:
>
>
> (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> - errmsg("%s requires a
> Boolean value, a non-negative "
> -
> "integer, or the string \"match\"",
> + errmsg("a negative
> integer value cannot be "
> +
> "specified for %s",
>
> def->defname)));
Fair point. There might not be any explicit rule.
However, I feel it somewhat confusing.
After looking into the code, it seems that ERRCODE_SYNTAX_ERROR tends to be used
for invalid keywords or parameter types, while ERRCODE_INVALID_PARAMETER_VALUE
is typically used for values that are out of the acceptable range.
So, the proposed change seems reasonable to me.
Regards,
Yugo Nagata
--
Yugo Nagata <nag...@sraoss.co.jp>
