Re: Should we increase the default vacuum_cost_limit? - Mailing list pgsql-hackers

Julien Rouhaud <rjuju123@gmail.com> writes:
> On Sat, Mar 9, 2019 at 10:04 PM Tom Lane <tgl@sss.pgh.pa.us> 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

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 single
        quotes within the value.  Quotes can usually be omitted if the value
        is a simple number or identifier, however.
+       (Values that match a SQL keyword require quoting in some contexts.)
       </para>
      </listitem>

      <listitem>
       <para>
        <emphasis>Numeric (integer and floating point):</emphasis>
-       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 nearest
+       integer if the parameter is of type integer.  Integer parameters
+       additionally accept hexadecimal input (beginning
+       with <literal>0x</literal>) and octal input (beginning
+       with <literal>0</literal>), but these formats cannot have a fraction.
+       Do not use thousands separators.
+       Quotes are not required, except for hexadecimal input.
       </para>
      </listitem>

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-point
+ * formats (which will be rounded to integer after any units conversion).
+ * Optionally, the value can be followed by a unit name if "flags" indicates
+ * 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 flags, const char **hintmsg)
     if (hintmsg)
         *hintmsg = 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 starting
+     * 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 = 0;
     val = strtol(value, &endptr, 0);
-
-    if (endptr == value)
-        return false;            /* no HINT for integer syntax error */
-
-    if (errno == ERANGE || val != (int64) ((int32) val))
+    if (*endptr == '.' || *endptr == 'e' || *endptr == 'E' ||
+        errno == ERANGE)
     {
-        if (hintmsg)
-            *hintmsg = gettext_noop("Value exceeds integer range.");
-        return false;
+        errno = 0;
+        val = strtod(value, &endptr);
     }

-    /* allow whitespace between integer and unit */
+    if (endptr == value || errno == 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 != '\0')
     {
-        double        cval;
-
         if ((flags & GUC_UNIT) == 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 flags, const char **hintmsg)
             }
             return false;
         }
+    }

-        /* Round to int, then check for overflow due to units conversion */
-        cval = rint(cval);
-        if (cval > INT_MAX || cval < INT_MIN)
-        {
-            if (hintmsg)
-                *hintmsg = gettext_noop("Value exceeds integer range.");
-            return false;
-        }
-        val = (int64) cval;
+    /* Round to int, then check for overflow */
+    val = rint(val);
+
+    if (val > INT_MAX || val < INT_MIN)
+    {
+        if (hintmsg)
+            *hintmsg = gettext_noop("Value exceeds integer range.");
+        return false;
     }

     if (result)
@@ -6218,10 +6231,10 @@ parse_int(const char *value, int *result, int flags, 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" indicates
+ * 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/expected/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.fillfactor=2);
 ERROR:  unrecognized parameter namespace "not_existing_namespace"
 -- Fail while setting improper values
-CREATE TABLE reloptions_test2(i INT) WITH (fillfactor=30.5);
-ERROR:  invalid value for integer option "fillfactor": 30.5
+CREATE TABLE reloptions_test2(i INT) WITH (fillfactor=-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='string');
 ERROR:  invalid value for integer option "fillfactor": string
 CREATE TABLE reloptions_test2(i INT) WITH (fillfactor=true);
diff --git a/src/test/regress/sql/reloptions.sql b/src/test/regress/sql/reloptions.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=2);
 CREATE TABLE reloptions_test2(i INT) WITH (not_existing_namespace.fillfactor=2);

 -- Fail while setting improper values
-CREATE TABLE reloptions_test2(i INT) WITH (fillfactor=30.5);
+CREATE TABLE reloptions_test2(i INT) WITH (fillfactor=-30.1);
 CREATE TABLE reloptions_test2(i INT) WITH (fillfactor='string');
 CREATE TABLE reloptions_test2(i INT) WITH (fillfactor=true);
 CREATE TABLE reloptions_test2(i INT) WITH (autovacuum_enabled=12);
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;
         }
     }