On 2025/06/27 13:09, Yugo Nagata wrote:
> On Fri, 27 Jun 2025 12:22:17 +0900
> Fujii Masao <masao.fujii@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)));
Regards,
--
Fujii Masao
NTT DATA Japan Corporation
