From: Tom Lane <tgl@sss.pgh.pa.us>
To: Julien Rouhaud <rjuju123@gmail.com>
Cc: Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
Cc: David Rowley <david.rowley@2ndquadrant.com>
Cc: Jeff Janes <jeff.janes@gmail.com>
Cc: Jeremy Schneider <schnjere@amazon.com>
Cc: Joe Conway <mail@joeconway.com>
Cc: Peter Geoghegan <pg@bowt.ie>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Should we increase the default vacuum_cost_limit?
Date: Sun, 10 Mar 2019 16:58:39 -0400
Message-ID: <20054.1552251519@sss.pgh.pa.us> (raw)
In-Reply-To: <CAOBaU_Z8k1RAB8u761qg-L9EKZzywwiJde=hiuBRN6hvE8+7vQ@mail.gmail.com>
References: <CAKJS1f_YbXC2qTMPyCbmsPiKvZYwpuQNQMohiRXLj1r=8_rYvw@mail.gmail.com>
<CAH2-Wz=WQ9ju6sjPF7RcW0g6NPXAenZeC5qVme6uj+eMRw5q2g@mail.gmail.com>
<241b80ab-7af0-c606-7b2c-b0a5d6be696a@joeconway.com>
<CAKJS1f_5xofbg0K16MkhQPeCXV6oGCuD2SVcV76OW7ydFDHDkw@mail.gmail.com>
<df548503-d261-d8c4-97d0-61be6617df41@2ndQuadrant.com>
<4a068b09-5a6f-e975-07c9-2333fcef0a8a@amazon.com>
<30411a5b-ef11-71ef-e9ee-4d26ef3fd0c0@2ndQuadrant.com>
<CAMkU=1zM7sK9Xo2jjwCLj-t+GHR5nQJnPyFUp+puBiFpOQ7aBA@mail.gmail.com>
<21034.1552068630@sss.pgh.pa.us>
<CAKJS1f9wbS+SzEDUXyMuLCsNgwMH=1Ztj3QE3WxuKdJtbqrOEA@mail.gmail.com>
<13d143dd-0a44-0c16-218c-bc924b5922f0@2ndQuadrant.com>
<26656.1552096451@sss.pgh.pa.us>
<28720.1552101086@sss.pgh.pa.us>
<CAKJS1f8s5Cxat65_-uE97oUpoonO3p_2rFMf-cKA7kpAaS1-OA@mail.gmail.com>
<203719fc-1970-4a49-8013-eba3fd109b19@2ndQuadrant.com>
<24300.1552154154@sss.pgh.pa.us>
<c88dc1b3-9d2c-5a87-717c-0fb2497b1f22@2ndQuadrant.com>
<CAOBaU_Y8n9pmuRKeXUzUb_pPZmL-Nm601p_MdEMHBcBGp4W0UQ@! mail.gmail.com>
<1798.1552165479@sss.pgh.pa.us>
<CAOBaU_Z8k1RAB8u761qg-L9EKZzywwiJde=hiuBRN6hvE8+7vQ@mail.gmail.com>
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
Attachments:
[text/x-diff] integer-gucs-with-fractions.patch (6.8K, ../20054.1552251519@sss.pgh.pa.us/2-integer-gucs-with-fractions.patch)
download | inline diff:diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgmlindex 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.cindex 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.outindex 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.sqlindex 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);
[text/x-diff] roundoff-fractions-harder.patch (817B, ../20054.1552251519@sss.pgh.pa.us/3-roundoff-fractions-harder.patch)
download | inline diff:diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.cindex 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;
}
}
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, rjuju123@gmail.com, andrew.dunstan@2ndquadrant.com, david.rowley@2ndquadrant.com, jeff.janes@gmail.com, schnjere@amazon.com, mail@joeconway.com, pg@bowt.ie, pgsql-hackers@lists.postgresql.org
Subject: Re: Should we increase the default vacuum_cost_limit?
In-Reply-To: <20054.1552251519@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox