Обсуждение: Issue with past commit: Allow fractional input values for integer GUCs ...

Hi Hackers,

I may be wrong, but an issue seems to have been introduced by the
following commit (March 11, 2019):

    Allow fractional input values for integer GUCs, and improve rounding logic
    https://github.com/postgres/postgres/commit/1a83a80a2fe5b559f85ed4830acb92d5124b7a9a

The changes made allow fractional input for some cases where I believe
it shouldn't be allowed (i.e. when the setting does not accept a
unit).
For example,

log_file_mode = 384.234
max_connections = 1.0067e2
port = 5432.123

(Is it intentional - or indeed useful - to allow such settings, for
integer options?)

Also, the modified parse_int() function is used for parsing other
options, such as the integer storage parameters for CREATE TABLE and
CREATE INDEX. For example, the following integer parameter settings
are currently allowed but I don't believe that they should be:

CREATE TABLE ... WITH (fillfactor = 23.45);
CREATE TABLE ... WITH (parallel_workers = 5.4);


I have attached a patch with a proposed correction, keeping it a
simple change to the existing parse_int() function, rather than making
further changes for more optimal integer parsing code. The patch also
updates a couple of test cases (reverting one to its original state
before the commit mentioned above).

Let me know what you think.

Regards,
Greg Nancarrow
Fujitsu Australia

Greg Nancarrow <gregn4422@gmail.com> writes:
> The changes made allow fractional input for some cases where I believe
> it shouldn't be allowed (i.e. when the setting does not accept a
> unit).
> ...
> (Is it intentional - or indeed useful - to allow such settings, for
> integer options?)

Given that the commit included a test case exercising exactly that,
I'm not sure why you might think it was unintentional.  IIRC, the
reasoning was that we ought to hide whether any given GUC is int or
float underneath, in anticipation of future changes like caf626b2c.
Another argument is that in regular SQL, you can assign a fractional
value to an integer column and the system will let you do it; so
why not in SET?

In any case, we already shipped that behavior in v12, so I don't think
we can take it away now.  People don't appreciate formerly valid
settings suddenly not working any more.

            regards, tom lane

> Given that the commit included a test case exercising exactly that,
> I'm not sure why you might think it was unintentional.

Well, maybe not exercising exactly that. No positive test case was
added. The commit replaced a CREATE TABLE fillfactor test case testing
that "30.5" is invalid, with a test case testing that "-30.1" is
out-of-range. I guess that does indirectly test that "-30.1" is not an
improper value, though the out-of-range error means that test case
should really be put in the "-- Fail min/max values check" section and
not in the "-- Fail while setting improper values" section.

My point was that allowing the fractional input really only makes
sense if the "integer" option/GUC has an associated unit. That's why I
questioned whether allowing it in this case (when the integer
option/GUC has no associated unit, like "port" or "max_connections")
was intentional or useful.


>  IIRC, the
> reasoning was that we ought to hide whether any given GUC is int or
> float underneath, in anticipation of future changes like caf626b2c.
> Another argument is that in regular SQL, you can assign a fractional
> value to an integer column and the system will let you do it; so
> why not in SET?
>
> In any case, we already shipped that behavior in v12, so I don't think
> we can take it away now.  People don't appreciate formerly valid
> settings suddenly not working any more.
>

I guess we'll have to live with the current behaviour then.


Regards,
Greg Nancarrow
Fujitsu Australia

On Mon, Aug 24, 2020 at 5:32 AM Greg Nancarrow <gregn4422@gmail.com> wrote:
> For example,
>
> log_file_mode = 384.234
> max_connections = 1.0067e2
> port = 5432.123
> CREATE TABLE ... WITH (fillfactor = 23.45);
> CREATE TABLE ... WITH (parallel_workers = 5.4);

I don't think any of these cases should be allowed. Surely if we
allowed 384.234 to be inserted into an integer column, everyone would
say that we'd lost our minds. These cases seem no different. The
discussion to which the commit links is mainly about allowing 0.2s to
work like 200ms, or something of that sort, when the value is
specified as a fraction but works out to an integer when converted to
the base unit. That is a completely different thing from letting
people configure 5.4 parallel workers.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> I don't think any of these cases should be allowed. Surely if we
> allowed 384.234 to be inserted into an integer column, everyone would
> say that we'd lost our minds.

regression=# create table itable (f1 int);
CREATE TABLE
regression=# insert into itable values (384.234);
INSERT 0 1
regression=# table itable;
 f1  
-----
 384
(1 row)

It's always worked like that, and nobody's complained about it.
I suspect, in fact, that one could find chapter and verse in the
SQL spec that requires it, just like "numeric" values should get
rounded if you write too many digits.

            regards, tom lane

On Wed, Aug 26, 2020 at 4:47 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> regression=# create table itable (f1 int);
> CREATE TABLE
> regression=# insert into itable values (384.234);
> INSERT 0 1
> regression=# table itable;
>  f1
> -----
>  384
> (1 row)
>
> It's always worked like that, and nobody's complained about it.
> I suspect, in fact, that one could find chapter and verse in the
> SQL spec that requires it, just like "numeric" values should get
> rounded if you write too many digits.

That is a bit different from what I had in mind, because it does not
involve a call to int4in(). Instead, it involves a cast. I was
imagining that you would put quotation marks around 384.234, which
does indeed fail:

ERROR:  invalid input syntax for type integer: "384.234"

So the question is whether users who supply values for integer-valued
reloptions, or integer-valued GUCs, expect that they will be parsed as
integers, or whether they expect that they will be parsed as float
values and the cast to integers.

While the new behavior seems fine -- and indeed convenient -- for GUCs
that are numeric with a unit, it does not seem very nice at all for
GUCs that are unitless integers. Why do you think that anyone would be
pleased to discover that they can set port = 543.2? We must decide
whether it is more likely that such a setting is the result of a user
error about which we should issue some complaint, or on the other hand
whether the user is hoping that we will be good enough to round the
value off so as to spare them the trouble. My own view is that the
former is vastly more probably than the latter.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> While the new behavior seems fine -- and indeed convenient -- for GUCs
> that are numeric with a unit, it does not seem very nice at all for
> GUCs that are unitless integers.

I find that distinction to be entirely without merit; not least because
we also have unitless float GUCs.  I think the fact that we have some
float and some integer GUCs is an implementation detail more than a
fundamental property --- especially since SQL considers integers
to be just the scale-zero subset of numerics.  I recognize that your
opinion is different, but to me it seems fine as-is.

            regards, tom lane