Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1h35X3-0000Qc-AY for pgsql-hackers@arkaria.postgresql.org; Sun, 10 Mar 2019 20:58:53 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1h35X0-0003Qp-Jc for pgsql-hackers@arkaria.postgresql.org; Sun, 10 Mar 2019 20:58:50 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1h35X0-0003Ir-9M for pgsql-hackers@lists.postgresql.org; Sun, 10 Mar 2019 20:58:50 +0000 Received: from sss.pgh.pa.us ([66.207.139.130]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1h35Ww-0007V0-KO for pgsql-hackers@lists.postgresql.org; Sun, 10 Mar 2019 20:58:48 +0000 Received: from sss1.sss.pgh.pa.us (localhost [127.0.0.1]) by sss.pgh.pa.us (8.14.4/8.14.4) with ESMTP id x2AKwd2F020055; Sun, 10 Mar 2019 16:58:39 -0400 From: Tom Lane To: Julien Rouhaud cc: Andrew Dunstan , David Rowley , Jeff Janes , Jeremy Schneider , Joe Conway , Peter Geoghegan , PostgreSQL Hackers Subject: Re: Should we increase the default vacuum_cost_limit? In-reply-to: References: <241b80ab-7af0-c606-7b2c-b0a5d6be696a@joeconway.com> <4a068b09-5a6f-e975-07c9-2333fcef0a8a@amazon.com> <30411a5b-ef11-71ef-e9ee-4d26ef3fd0c0@2ndQuadrant.com> <21034.1552068630@sss.pgh.pa.us> <13d143dd-0a44-0c16-218c-bc924b5922f0@2ndQuadrant.com> <26656.1552096451@sss.pgh.pa.us> <28720.1552101086@sss.pgh.pa.us> <203719fc-1970-4a49-8013-eba3fd109b19@2ndQuadrant.com> <24300.1552154154@sss.pgh.pa.us> <1798.1552165479@sss.pgh.pa.us> Comments: In-reply-to Julien Rouhaud message dated "Sun, 10 Mar 2019 14:10:43 +0100" MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0" Content-ID: <19966.1552251298.0@sss.pgh.pa.us> Date: Sun, 10 Mar 2019 16:58:39 -0400 Message-ID: <20054.1552251519@sss.pgh.pa.us> List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk ------- =_aaaaaaaaaa0 Content-Type: text/plain; charset="us-ascii" Content-ID: <19966.1552251298.1@sss.pgh.pa.us> Julien Rouhaud writes: > On Sat, Mar 9, 2019 at 10:04 PM Tom Lane wrote: >> 2. It's always bugged me that we don't allow fractional unit >> specifications, say "0.1GB", even for GUCs that are integers underneath. >> That would be a simple additional change on top of this, but I didn't >> do it here. > It annoyed me multiple times, so +1 for making that happen. The first patch below does that, but I noticed that if we just do it without any subtlety, you get results like this: regression=# set work_mem = '30.1GB'; SET regression=# show work_mem; work_mem ------------ 31562138kB (1 row) The second patch is a delta that rounds off to the next smaller unit if there is one, producing a less noisy result: regression=# set work_mem = '30.1GB'; SET regression=# show work_mem; work_mem ---------- 30822MB (1 row) I'm not sure if that's a good idea or just overthinking the problem. Thoughts? regards, tom lane ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="integer-gucs-with-fractions.patch"; charset="us-ascii" Content-ID: <19966.1552251298.2@sss.pgh.pa.us> Content-Description: integer-gucs-with-fractions.patch Content-Transfer-Encoding: quoted-printable diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index fe17357..3b59565 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -51,14 +51,21 @@ In general, enclose the value in single quotes, doubling any singl= e quotes within the value. Quotes can usually be omitted if the val= ue is a simple number or identifier, however. + (Values that match a SQL keyword require quoting in some contexts.= ) = Numeric (integer and floating point): - A decimal point is permitted only for floating-point parameters. - Do not use thousands separators. Quotes are not required. + Numeric parameters can be specified in the customary integer and + floating-point formats; fractional values are rounded to the neare= st + integer if the parameter is of type integer. Integer parameters + additionally accept hexadecimal input (beginning + with 0x) and octal input (beginning + with 0), but these formats cannot have a fracti= on. + Do not use thousands separators. + Quotes are not required, except for hexadecimal input. = diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index fe6c6f8..d374f53 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -6141,8 +6141,10 @@ get_config_unit_name(int flags) = /* * Try to parse value as an integer. The accepted formats are the - * usual decimal, octal, or hexadecimal formats, optionally followed by - * a unit name if "flags" indicates a unit is allowed. + * usual decimal, octal, or hexadecimal formats, as well as floating-poin= t + * formats (which will be rounded to integer after any units conversion). + * Optionally, the value can be followed by a unit name if "flags" indica= tes + * a unit is allowed. * * If the string parses okay, return true, else false. * If okay and result is not NULL, return the value in *result. @@ -6152,7 +6154,11 @@ get_config_unit_name(int flags) bool parse_int(const char *value, int *result, int flags, const char **hintmsg= ) { - int64 val; + /* + * We assume here that double is wide enough to represent any integer + * value with adequate precision. + */ + double val; char *endptr; = /* To suppress compiler warnings, always set output params */ @@ -6161,35 +6167,42 @@ parse_int(const char *value, int *result, int flag= s, const char **hintmsg) if (hintmsg) *hintmsg =3D NULL; = - /* We assume here that int64 is at least as wide as long */ + /* + * Try to parse as an integer (allowing octal or hex input). If the + * conversion stops at a decimal point or 'e', or overflows, re-parse as + * float. This should work fine as long as we have no unit names starti= ng + * with 'e'. If we ever do, the test could be extended to check for a + * sign or digit after 'e', but for now that's unnecessary. + */ errno =3D 0; val =3D strtol(value, &endptr, 0); - - if (endptr =3D=3D value) - return false; /* no HINT for integer syntax error */ - - if (errno =3D=3D ERANGE || val !=3D (int64) ((int32) val)) + if (*endptr =3D=3D '.' || *endptr =3D=3D 'e' || *endptr =3D=3D 'E' || + errno =3D=3D ERANGE) { - if (hintmsg) - *hintmsg =3D gettext_noop("Value exceeds integer range."); - return false; + errno =3D 0; + val =3D strtod(value, &endptr); } = - /* allow whitespace between integer and unit */ + if (endptr =3D=3D value || errno =3D=3D ERANGE) + return false; /* no HINT for these cases */ + + /* reject NaN (infinities will fail range check below) */ + if (isnan(val)) + return false; /* treat same as syntax error; no HINT */ + + /* allow whitespace between number and unit */ while (isspace((unsigned char) *endptr)) endptr++; = /* Handle possible unit */ if (*endptr !=3D '\0') { - double cval; - if ((flags & GUC_UNIT) =3D=3D 0) return false; /* this setting does not accept a unit */ = - if (!convert_to_base_unit((double) val, + if (!convert_to_base_unit(val, endptr, (flags & GUC_UNIT), - &cval)) + &val)) { /* invalid unit, or garbage after the unit; set hint and fail. */ if (hintmsg) @@ -6201,16 +6214,16 @@ parse_int(const char *value, int *result, int flag= s, const char **hintmsg) } return false; } + } = - /* Round to int, then check for overflow due to units conversion */ - cval =3D rint(cval); - if (cval > INT_MAX || cval < INT_MIN) - { - if (hintmsg) - *hintmsg =3D gettext_noop("Value exceeds integer range."); - return false; - } - val =3D (int64) cval; + /* Round to int, then check for overflow */ + val =3D rint(val); + + if (val > INT_MAX || val < INT_MIN) + { + if (hintmsg) + *hintmsg =3D gettext_noop("Value exceeds integer range."); + return false; } = if (result) @@ -6218,10 +6231,10 @@ parse_int(const char *value, int *result, int flag= s, const char **hintmsg) return true; } = - - /* * Try to parse value as a floating point number in the usual format. + * Optionally, the value can be followed by a unit name if "flags" indica= tes + * a unit is allowed. * * If the string parses okay, return true, else false. * If okay and result is not NULL, return the value in *result. diff --git a/src/test/regress/expected/reloptions.out b/src/test/regress/e= xpected/reloptions.out index f90c267..5266490 100644 --- a/src/test/regress/expected/reloptions.out +++ b/src/test/regress/expected/reloptions.out @@ -26,8 +26,9 @@ ERROR: unrecognized parameter "not_existing_option" CREATE TABLE reloptions_test2(i INT) WITH (not_existing_namespace.fillfac= tor=3D2); ERROR: unrecognized parameter namespace "not_existing_namespace" -- Fail while setting improper values -CREATE TABLE reloptions_test2(i INT) WITH (fillfactor=3D30.5); -ERROR: invalid value for integer option "fillfactor": 30.5 +CREATE TABLE reloptions_test2(i INT) WITH (fillfactor=3D-30.1); +ERROR: value -30.1 out of bounds for option "fillfactor" +DETAIL: Valid values are between "10" and "100". CREATE TABLE reloptions_test2(i INT) WITH (fillfactor=3D'string'); ERROR: invalid value for integer option "fillfactor": string CREATE TABLE reloptions_test2(i INT) WITH (fillfactor=3Dtrue); diff --git a/src/test/regress/sql/reloptions.sql b/src/test/regress/sql/re= loptions.sql index 44fcd8c..8551851 100644 --- a/src/test/regress/sql/reloptions.sql +++ b/src/test/regress/sql/reloptions.sql @@ -15,7 +15,7 @@ CREATE TABLE reloptions_test2(i INT) WITH (not_existing_= option=3D2); CREATE TABLE reloptions_test2(i INT) WITH (not_existing_namespace.fillfac= tor=3D2); = -- Fail while setting improper values -CREATE TABLE reloptions_test2(i INT) WITH (fillfactor=3D30.5); +CREATE TABLE reloptions_test2(i INT) WITH (fillfactor=3D-30.1); CREATE TABLE reloptions_test2(i INT) WITH (fillfactor=3D'string'); CREATE TABLE reloptions_test2(i INT) WITH (fillfactor=3Dtrue); CREATE TABLE reloptions_test2(i INT) WITH (autovacuum_enabled=3D12); ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="roundoff-fractions-harder.patch"; charset="us-ascii" Content-ID: <19966.1552251298.3@sss.pgh.pa.us> Content-Description: roundoff-fractions-harder.patch diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index d374f53..cdb6a61 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -5995,7 +5995,19 @@ convert_to_base_unit(double value, const char *unit, if (base_unit == table[i].base_unit && strcmp(unitstr, table[i].unit) == 0) { - *base_value = value * table[i].multiplier; + double cvalue = value * table[i].multiplier; + + /* + * If the user gave a fractional value such as "30.1GB", round it + * off to the nearest multiple of the next smaller unit, if there + * is one. + */ + if (*table[i + 1].unit && + base_unit == table[i + 1].base_unit) + cvalue = rint(cvalue / table[i + 1].multiplier) * + table[i + 1].multiplier; + + *base_value = cvalue; return true; } } ------- =_aaaaaaaaaa0--