Re: postgres_fdw - should we tighten up batch_size, fetch_size options against non-numeric values?

On 2021/06/30 23:31, Bharath Rupireddy wrote:
> On Wed, Jun 30, 2021 at 5:53 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>
>> On 2021/05/20 1:01, Bharath Rupireddy wrote:
>>> Thanks for the comments. I added separate messages, changed the error
>>> code from ERRCODE_SYNTAX_ERROR to ERRCODE_INVALID_PARAMETER_VALUE and
>>> also quoted the option name in the error message. PSA v3 patch.
>>
>> Thanks for updating the patch!
>>
>> +                                               (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>> +                                                errmsg("invalid numeric value for option \"%s\"",
>> +                                                               def->defname)));
>>
>> In reloptions.c, when parse_real() fails to parse the input, the error message
>> "invalid value for floating point option..." is output.
>> For the sake of consistency, we should use the same error message here?
> 
> Actually, there's an existing error message errmsg("%s requires a
> non-negative numeric value" that used "numeric value". If we were to
> change errmsg("invalid numeric value for option \"%s\"", to
> errmsg("invalid value for floating point option \"%s\"",, then we
> might have to change the existing message. And also, the docs use
> "numeric value" for fdw_startup_cost and fdw_tuple_cost.

The recent commit 61d599ede7 documented that the type of those options is
floating point. But the docs still use "is a numeric value" in the descriptions
of them. Probably it should be replaced with "is a floating point value" there.
If we do this, isn't it better to use "floating point" even in the error message?


> IMO, let's go
> with errmsg("invalid value for numeric option \"%s\": %s",.
> 
>> -                                               (errcode(ERRCODE_SYNTAX_ERROR),
>> -                                                errmsg("%s requires a non-negative integer value",
>> +                                               (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>> +                                                errmsg("invalid integer value for option \"%s\"",
>>
>> IMO the error message should be "invalid value for integer option..." here
>> because of the same reason I told above. Thought?
> 
> Changed.
> 
> PSA v4.

Thanks!

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION