Julien Rouhaud <rjuju...@gmail.com> writes:
> On Sat, Mar 9, 2019 at 10:04 PM Tom Lane <t...@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;
}
}
