Re: Inaccurate error message when set fdw batch_size to 0 - Mailing list pgsql-hackers

On Mon, May 10, 2021 at 7:39 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> writes:
> > On Mon, May 10, 2021 at 12:00 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> Yeah, this error message seems outright buggy.  However, it's a minor
> >> matter.  Also, some people think "positive" is the same thing as
> >> "non-negative", so maybe we need less ambiguous wording?
>
> > Since value 0 can't be considered as either a positive or negative
> > integer, I think we can do as following(roughly):
>
> > if (value < 0) "requires a zero or positive integer value"
> > if (value <= 0) "requires a positive integer value"
>
> I was thinking of avoiding the passive voice and writing
>
>         "foo must be greater than zero"

+1 for "foo must be greater than zero" if (foo <= 0) kind of errors.
But, we also have some values for which zero is accepted, see below
error messages. How about the error message "foo must be greater than
or equal to zero"?

remainder for hash partition must be a non-negative integer
parallel vacuum degree must be a non-negative integer
repeat count size must be a non-negative integer
number of workers must be a non-negative integer
distance in phrase operator should be non-negative and less than %d

> which removes all doubt.  It's not necessary to keep the "integer"
> aspect of the existing text, because if someone had supplied a
> non-integer value, that would not have gotten this far anyway.

This led me to have a look at two postgres_fdw options: fetch_size and
batch_size, whether they accept positive non-integers like '123.456',
'789.123' and some unsound strings such as '100$%$#$#', '9,223,372,'.
It looks like yes, the truncated values 123. 789, 100, 9
(respectively) are accepted. This is because the way strtol is used to
fetch the integers from string. I'm not sure if that's intentional.
fetch_size = strtol(defGetString(def), NULL, 10);
batch_size = strtol(defGetString(def), NULL, 10);

I know that fetch_size and batch_size are "number of rows", so no
sensible users may specify values with fractional part or non integer
characters, but still we can fix this with the endptr parameter of
the strtol. Note that for the options fdw_startup_cost and
fdw_tuple_cost it's already fixed.

I'm thinking of starting a separate thread to discuss this, if this
thread is not the right place.

> > I'm not sure whether we should consider changing these messages:
> > remainder for hash partition must be a non-negative integer
> > parallel vacuum degree must be a non-negative integer
> > repeat count size must be a non-negative integer
> > number of workers must be a non-negative integer
> > %s requires a non-negative numeric value
> > distance in phrase operator should be non-negative and less than %d
>
> I think for consistency it'd be good to change 'em all.

+1.

> I'm almost tempted to put this matter into our message style guide too.

+1.

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