Horton

Thread

Commits

  1. postgres_fdw: Tighten up allowed values for batch_size, fetch_size options.

  2. doc: Add type information for postgres_fdw parameters.

  1. postgres_fdw - should we tighten up batch_size, fetch_size options against non-numeric values?

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-05-17T09:58:52Z

    Hi,

It looks like the values such as '123.456', '789.123' '100$%$#$#',
'9,223,372,' are accepted and treated as valid integers for
postgres_fdw options batch_size and fetch_size. Whereas this is not
the case with fdw_startup_cost and fdw_tuple_cost options for which an
error is thrown. Attaching a patch to fix that.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

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

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2021-05-17T12:47:20Z

    On Mon, May 17, 2021 at 3:29 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> Hi,
>
> It looks like the values such as '123.456', '789.123' '100$%$#$#',
> '9,223,372,' are accepted and treated as valid integers for
> postgres_fdw options batch_size and fetch_size. Whereas this is not
> the case with fdw_startup_cost and fdw_tuple_cost options for which an
> error is thrown. Attaching a patch to fix that.

This looks like a definite improvement. I wonder if we should modify
defGetInt variants to convert strings into integers, so that there's
consistent error message for such errors. We could define defGetUInt
so that we could mention non-negative in the error message. Whether or
not we do that, this looks good.




-- 
Best Wishes,
Ashutosh Bapat

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

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-05-17T14:20:07Z

    On Mon, May 17, 2021 at 6:17 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Mon, May 17, 2021 at 3:29 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > Hi,
> >
> > It looks like the values such as '123.456', '789.123' '100$%$#$#',
> > '9,223,372,' are accepted and treated as valid integers for
> > postgres_fdw options batch_size and fetch_size. Whereas this is not
> > the case with fdw_startup_cost and fdw_tuple_cost options for which an
> > error is thrown. Attaching a patch to fix that.
>
> This looks like a definite improvement. I wonder if we should modify
> defGetInt variants to convert strings into integers, so that there's
> consistent error message for such errors. We could define defGetUInt
> so that we could mention non-negative in the error message.

If we do that, then the options that are only accepting unquoted
integers (i.e. 123, 456 etc.) and throwing errors for the quoted
integers ('123', '456' etc.) will then start to accept the quoted
integers. Other hackers might not agree to this change.

Another way is to have new API like
defGetInt32_2/defGetInt64_2/defGetNumeric_2 (or some other better
names) which basically accept both quoted and unquoted strings, see
[1] for a rough sketch of the function. These API can be useful if an
option needs to be parsed in both quoted and unquoted form. Or we can
also have these functions as [2] for only parsing quoted options. I
prefer [2] so that these API can be used without any code duplication.
Thoughts?

> Whether or not we do that, this looks good.

I'm also okay if we can just fix the fetch_size and back_size options
for now as it's done in the patch attached with the first mail. Note
that I have not added any test case as this change is a trivial thing.
If required, I can add one.

[1] -
int32
defGetInt32_2(DefElem *def)
{
    if (def->arg == NULL)
        ereport(ERROR,
                (errcode(ERRCODE_SYNTAX_ERROR),
                 errmsg("%s requires an integer value",
                             def->defname)));

    switch (nodeTag(def->arg))
    {
        case T_Integer:
            return (int32) intVal(def->arg);
        default:
        {
            char    *sval;
            int32   val;

            sval = defGetString(def);
            val = strtol(sval, &endp, 10);

            if (*endp == '\0')
                return val;
        }
    }

    ereport(ERROR,
            (errcode(ERRCODE_SYNTAX_ERROR),
            errmsg("%s requires an integer value",
                    def->defname)));

    return 0;
}

[2] -
int32
defGetInt32_2(DefElem *def)
{
    char    *sval;
    int32   val;

    sval = defGetString(def);
    val = strtol(sval, &endp, 10);

    if (*endp)
        ereport(ERROR,
                (errcode(ERRCODE_SYNTAX_ERROR),
                errmsg("%s requires an integer value",
                        def->defname)));
    return val;

}

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

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

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2021-05-18T13:22:25Z

    On Mon, May 17, 2021 at 7:50 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
> If we do that, then the options that are only accepting unquoted
> integers (i.e. 123, 456 etc.) and throwing errors for the quoted
> integers ('123', '456' etc.) will then start to accept the quoted
> integers. Other hackers might not agree to this change.

I guess the options which weren't accepting quoted strings, will catch
these errors at the time of parsing itself. Even if that's not true, I
would see that as an improvement. Anyway, I won't stretch this
further.

>
> Another way is to have new API like
> defGetInt32_2/defGetInt64_2/defGetNumeric_2 (or some other better
> names) which basically accept both quoted and unquoted strings, see
> [1] for a rough sketch of the function. These API can be useful if an
> option needs to be parsed in both quoted and unquoted form. Or we can
> also have these functions as [2] for only parsing quoted options. I
> prefer [2] so that these API can be used without any code duplication.
> Thoughts?

I am not sure whether we want to maintain two copies. In that case
your first patch is fine.

> Note
> that I have not added any test case as this change is a trivial thing.
> If required, I can add one.

That will help to make sure that we preserve the behaviour even
through code changes.

-- 
Best Wishes,
Ashutosh Bapat

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

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2021-05-18T13:45:15Z

    
On 2021/05/17 18:58, Bharath Rupireddy wrote:
> Hi,
> 
> It looks like the values such as '123.456', '789.123' '100$%$#$#',
> '9,223,372,' are accepted and treated as valid integers for
> postgres_fdw options batch_size and fetch_size. Whereas this is not
> the case with fdw_startup_cost and fdw_tuple_cost options for which an
> error is thrown. Attaching a patch to fix that.

This looks an improvement. But one issue is that the restore of
dump file taken by pg_dump from v13 may fail for v14 with this patch
if it contains invalid setting of fetch_size, e.g., "fetch_size '123.456'".
OTOH, since batch_size was added in v14, it has no such issue.

Regards,

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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-05-18T13:49:22Z

    Fujii Masao <masao.fujii@oss.nttdata.com> writes:
> On 2021/05/17 18:58, Bharath Rupireddy wrote:
>> It looks like the values such as '123.456', '789.123' '100$%$#$#',
>> '9,223,372,' are accepted and treated as valid integers for
>> postgres_fdw options batch_size and fetch_size. Whereas this is not
>> the case with fdw_startup_cost and fdw_tuple_cost options for which an
>> error is thrown. Attaching a patch to fix that.

> This looks an improvement. But one issue is that the restore of
> dump file taken by pg_dump from v13 may fail for v14 with this patch
> if it contains invalid setting of fetch_size, e.g., "fetch_size '123.456'".
> OTOH, since batch_size was added in v14, it has no such issue.

Maybe better to just silently round to integer?  I think that's
what we generally do with integer GUCs these days, eg

regression=# set work_mem = 102.9;
SET
regression=# show work_mem;
 work_mem 
----------
 103kB
(1 row)

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.

			regards, tom lane

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

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-05-18T14:16:39Z

    On Tue, May 18, 2021 at 7:19 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Fujii Masao <masao.fujii@oss.nttdata.com> writes:
> > On 2021/05/17 18:58, Bharath Rupireddy wrote:
> >> It looks like the values such as '123.456', '789.123' '100$%$#$#',
> >> '9,223,372,' are accepted and treated as valid integers for
> >> postgres_fdw options batch_size and fetch_size. Whereas this is not
> >> the case with fdw_startup_cost and fdw_tuple_cost options for which an
> >> error is thrown. Attaching a patch to fix that.
>
> > This looks an improvement. But one issue is that the restore of
> > dump file taken by pg_dump from v13 may fail for v14 with this patch
> > if it contains invalid setting of fetch_size, e.g., "fetch_size '123.456'".
> > OTOH, since batch_size was added in v14, it has no such issue.
>
> Maybe better to just silently round to integer?  I think that's
> what we generally do with integer GUCs these days, eg
>
> regression=# set work_mem = 102.9;
> SET
> regression=# show work_mem;
>  work_mem
> ----------
>  103kB
> (1 row)

I think we can use parse_int to parse the fetch_size and batch_size as
integers, which also rounds off decimals to integers and returns false
for non-numeric junk. But, it accepts both quoted and unquoted
integers, something like batch_size 100 and batch_size '100', which I
feel is okay because the reloptions also accept both.

While on this, we can also use parse_real for fdw_startup_cost and
fdw_tuple_cost options but with that they will accept both quoted and
unquoted real values.

Thoughts?

> 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.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

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

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-05-18T14:41:28Z

    On Tue, May 18, 2021 at 7:46 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Tue, May 18, 2021 at 7:19 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > Fujii Masao <masao.fujii@oss.nttdata.com> writes:
> > > On 2021/05/17 18:58, Bharath Rupireddy wrote:
> > >> It looks like the values such as '123.456', '789.123' '100$%$#$#',
> > >> '9,223,372,' are accepted and treated as valid integers for
> > >> postgres_fdw options batch_size and fetch_size. Whereas this is not
> > >> the case with fdw_startup_cost and fdw_tuple_cost options for which an
> > >> error is thrown. Attaching a patch to fix that.
> >
> > > This looks an improvement. But one issue is that the restore of
> > > dump file taken by pg_dump from v13 may fail for v14 with this patch
> > > if it contains invalid setting of fetch_size, e.g., "fetch_size '123.456'".
> > > OTOH, since batch_size was added in v14, it has no such issue.
> >
> > Maybe better to just silently round to integer?  I think that's
> > what we generally do with integer GUCs these days, eg
> >
> > regression=# set work_mem = 102.9;
> > SET
> > regression=# show work_mem;
> >  work_mem
> > ----------
> >  103kB
> > (1 row)
>
> I think we can use parse_int to parse the fetch_size and batch_size as
> integers, which also rounds off decimals to integers and returns false
> for non-numeric junk. But, it accepts both quoted and unquoted
> integers, something like batch_size 100 and batch_size '100', which I
> feel is okay because the reloptions also accept both.
>
> While on this, we can also use parse_real for fdw_startup_cost and
> fdw_tuple_cost options but with that they will accept both quoted and
> unquoted real values.

I'm sorry about saying that the unquoted integers are accepted with
batch_size, fetch_size, but actually the parser is throwing the syntax
error.

So, we can safely use parse_int for batch_size and fetch_size,
parse_real for fdw_tuple_cost and fdw_startup_cost without changing
any behaviour.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

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

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-05-19T02:36:57Z

    At Tue, 18 May 2021 19:46:39 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in 
> On Tue, May 18, 2021 at 7:19 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > Fujii Masao <masao.fujii@oss.nttdata.com> writes:
> > > On 2021/05/17 18:58, Bharath Rupireddy wrote:
> > >> It looks like the values such as '123.456', '789.123' '100$%$#$#',
> > >> '9,223,372,' are accepted and treated as valid integers for
> > >> postgres_fdw options batch_size and fetch_size. Whereas this is not
> > >> the case with fdw_startup_cost and fdw_tuple_cost options for which an
> > >> error is thrown. Attaching a patch to fix that.
> >
> > > This looks an improvement. But one issue is that the restore of
> > > dump file taken by pg_dump from v13 may fail for v14 with this patch
> > > if it contains invalid setting of fetch_size, e.g., "fetch_size '123.456'".
> > > OTOH, since batch_size was added in v14, it has no such issue.
> >
> > Maybe better to just silently round to integer?  I think that's
> > what we generally do with integer GUCs these days, eg
> >
> > regression=# set work_mem = 102.9;
> > SET
> > regression=# show work_mem;
> >  work_mem
> > ----------
> >  103kB
> > (1 row)
> 
> I think we can use parse_int to parse the fetch_size and batch_size as
> integers, which also rounds off decimals to integers and returns false
> for non-numeric junk. But, it accepts both quoted and unquoted
> integers, something like batch_size 100 and batch_size '100', which I
> feel is okay because the reloptions also accept both.
> 
> While on this, we can also use parse_real for fdw_startup_cost and
> fdw_tuple_cost options but with that they will accept both quoted and
> unquoted real values.
> 
> Thoughts?

They are more or less a kind of reloptions. So I think it is
reasonable to treat the option same way with RELOPT_TYPE_INT.  That
is, it would be better to use our standard functions rather than
random codes using bare libc functions for input from users. The same
can be said for parameters with real numbers, regardless of the
"quoted" discussion.

> > 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.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

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

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2021-05-19T02:58:24Z

    
On 2021/05/19 11:36, Kyotaro Horiguchi wrote:
> At Tue, 18 May 2021 19:46:39 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in
>> On Tue, May 18, 2021 at 7:19 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>
>>> Fujii Masao <masao.fujii@oss.nttdata.com> writes:
>>>> On 2021/05/17 18:58, Bharath Rupireddy wrote:
>>>>> It looks like the values such as '123.456', '789.123' '100$%$#$#',
>>>>> '9,223,372,' are accepted and treated as valid integers for
>>>>> postgres_fdw options batch_size and fetch_size. Whereas this is not
>>>>> the case with fdw_startup_cost and fdw_tuple_cost options for which an
>>>>> error is thrown. Attaching a patch to fix that.
>>>
>>>> This looks an improvement. But one issue is that the restore of
>>>> dump file taken by pg_dump from v13 may fail for v14 with this patch
>>>> if it contains invalid setting of fetch_size, e.g., "fetch_size '123.456'".
>>>> OTOH, since batch_size was added in v14, it has no such issue.
>>>
>>> Maybe better to just silently round to integer?  I think that's
>>> what we generally do with integer GUCs these days, eg
>>>
>>> regression=# set work_mem = 102.9;
>>> SET
>>> regression=# show work_mem;
>>>   work_mem
>>> ----------
>>>   103kB
>>> (1 row)
>>
>> I think we can use parse_int to parse the fetch_size and batch_size as
>> integers, which also rounds off decimals to integers and returns false
>> for non-numeric junk. But, it accepts both quoted and unquoted
>> integers, something like batch_size 100 and batch_size '100', which I
>> feel is okay because the reloptions also accept both.
>>
>> While on this, we can also use parse_real for fdw_startup_cost and
>> fdw_tuple_cost options but with that they will accept both quoted and
>> unquoted real values.
>>
>> Thoughts?
> 
> They are more or less a kind of reloptions. So I think it is
> reasonable to treat the option same way with RELOPT_TYPE_INT.  That
> is, it would be better to use our standard functions rather than
> random codes using bare libc functions for input from users. The same
> can be said for parameters with real numbers, regardless of the
> "quoted" discussion.

Sounds reasonable. Since parse_int() is used to parse RELOPT_TYPE_INT value
in reloptions.c, your idea seems to be almost the same as Bharath's one.
That is, use parse_int() and parse_real() to parse integer and real options
values, respectively.

> 
>>> 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

Regards,

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

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

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-05-19T05:34:31Z

    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.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

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

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2021-05-19T11:32:04Z

    
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

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

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-05-19T16:01:24Z

    On Wed, May 19, 2021 at 5:02 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> 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?

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.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

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

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2021-06-30T12:23:52Z

    
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?

-						(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?

Regards,

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

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

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-06-30T14:31:55Z

    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. 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.

Regards,
Bharath Rupireddy.

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

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2021-07-01T02:53:56Z

    
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

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

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-07-01T04:16:35Z

    On Thu, Jul 1, 2021 at 8:23 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> 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?

Agreed. PSA v5 patch.

Regards,
Bharath Rupireddy.

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

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2021-07-01T12:37:34Z

    
On 2021/07/01 13:16, Bharath Rupireddy wrote:
> On Thu, Jul 1, 2021 at 8:23 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>> 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?
> 
> Agreed. PSA v5 patch.

Thanks for updating the patch! LGTM.
Barring any objection, I will commit this patch.

One question is; should we back-patch this? This is not a bug fix,
so I'm not sure if it's worth back-patching that to already-released versions.
But it may be better to do that to v14.

Regards,

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

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

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-07-01T12:41:10Z

    On Thu, Jul 1, 2021 at 6:07 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>
> On 2021/07/01 13:16, Bharath Rupireddy wrote:
> > On Thu, Jul 1, 2021 at 8:23 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> >> 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?
> >
> > Agreed. PSA v5 patch.
>
> Thanks for updating the patch! LGTM.
> Barring any objection, I will commit this patch.

Thanks.

> One question is; should we back-patch this? This is not a bug fix,
> so I'm not sure if it's worth back-patching that to already-released versions.
> But it may be better to do that to v14.

IMO, it's a good-to-have fix in v14. But, -1 for backpatching to v13
and lower branches.

Regards,
Bharath Rupireddy.

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

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2021-07-07T02:17:34Z

    
On 2021/07/01 21:41, Bharath Rupireddy wrote:
> On Thu, Jul 1, 2021 at 6:07 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>
>> On 2021/07/01 13:16, Bharath Rupireddy wrote:
>>> On Thu, Jul 1, 2021 at 8:23 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>>> 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?
>>>
>>> Agreed. PSA v5 patch.
>>
>> Thanks for updating the patch! LGTM.
>> Barring any objection, I will commit this patch.
> 
> Thanks.
> 
>> One question is; should we back-patch this? This is not a bug fix,
>> so I'm not sure if it's worth back-patching that to already-released versions.
>> But it may be better to do that to v14.
> 
> IMO, it's a good-to-have fix in v14. But, -1 for backpatching to v13
> and lower branches.

Agreed. So I pushed the patch to master and v14. Thanks!

Regards,

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