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

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