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

On 2021/05/19 14:34, Bharath Rupireddy wrote:
> On Wed, May 19, 2021 at 8:28 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>>>> I agree with throwing an error for non-numeric junk though.
>>>>> Allowing that on the grounds of backwards compatibility
>>>>> seems like too much of a stretch.
>>>>
>>>> +1.
>>>
>>> +1.
>>
>> +1
> 
> Thanks all for your inputs. PSA which uses parse_int for
> batch_size/fech_size and parse_real for fdw_startup_cost and
> fdw_tuple_cost.

Thanks for updating the patch! It basically looks good to me.

-			val = strtod(defGetString(def), &endp);
-			if (*endp || val < 0)
+			is_parsed = parse_real(defGetString(def), &val, 0, NULL);
+			if (!is_parsed || val < 0)
  				ereport(ERROR,
  						(errcode(ERRCODE_SYNTAX_ERROR),
  						 errmsg("%s requires a non-negative numeric value",

Isn't it better to check "!is_parsed" and "val < 0" separately like
reloptions.c does? That is, we should throw different error messages
for them?

ERRCODE_SYNTAX_ERROR seems strange for this type of error?
ERRCODE_INVALID_PARAMETER_VALUE is better and more proper?

Regards,

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