Memory unit GUC range checks
Hi,
I played around with the GUC memory units, specifically to test the new
GUC_UNIT_BYTES flag (commit 6e7baa32):
$ postmaster -c track_activity_query_size=1024kB
FATAL: 1048576 is outside the valid range for parameter
"track_activity_query_size" (100 .. 102400)
$ postmaster -c track_activity_query_size=1024MB
FATAL: 1073741824 is outside the valid range for parameter
"track_activity_query_size" (100 .. 102400)
$ postmaster -c track_activity_query_size=1024GB
FATAL: invalid value for parameter "track_activity_query_size": "1024GB"
HINT: Value exceeds integer range.
$ postmaster -c track_activity_query_size=1024TB
FATAL: invalid value for parameter "track_activity_query_size": "1024TB"
HINT: Valid units for this parameter are "kB", "MB", "GB", and "TB".
The first two look OK, but the last two cases seem a bit weird. With
1024 GB, it would be nice to print the valid range, like in the first
two cases.
The HINT in the last message seems wrong: the hint claims that "TB" is
accepted, yet "1024 TB" was not accepted. And shouldn't the hint also
mention "B", since we accept that now?
Testing a setting with GUC_UNIT_KB:
$ postmaster -c work_mem=102400B
FATAL: invalid value for parameter "work_mem": "100000B"
HINT: Valid units for this parameter are "kB", "MB", "GB", and "TB".
This time the hint is accurate, but why is "B" not accepted here? Seems
inconsistent.
- Heikki
On 16/05/18 15:19, Heikki Linnakangas wrote:
$ postmaster -c track_activity_query_size=1024TB
FATAL: invalid value for parameter "track_activity_query_size": "1024TB"
HINT: Valid units for this parameter are "kB", "MB", "GB", and "TB"....
The HINT in the last message seems wrong: the hint claims that "TB" is
accepted, yet "1024 TB" was not accepted. And shouldn't the hint also
mention "B", since we accept that now?Testing a setting with GUC_UNIT_KB:
$ postmaster -c work_mem=102400B
FATAL: invalid value for parameter "work_mem": "100000B"
HINT: Valid units for this parameter are "kB", "MB", "GB", and "TB".This time the hint is accurate, but why is "B" not accepted here? Seems
inconsistent.
Here's a pretty straightforward fix for these two issues. Any objections
or better ideas?
- Heikki
Attachments:
byte-unit-fixes-1.patchtext/x-patch; name=byte-unit-fixes-1.patchDownload
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 7cd2d2d80e..93402030f7 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -705,7 +705,7 @@ typedef struct
char unit[MAX_UNIT_LEN + 1]; /* unit, as a string, like "kB" or
* "min" */
int base_unit; /* GUC_UNIT_XXX */
- int multiplier; /* If positive, multiply the value with this
+ int64 multiplier; /* If positive, multiply the value with this
* for unit -> base_unit conversion. If
* negative, divide (with the absolute value) */
} unit_conversion;
@@ -718,10 +718,16 @@ typedef struct
#error XLOG_BLCKSZ must be between 1KB and 1MB
#endif
-static const char *memory_units_hint = gettext_noop("Valid units for this parameter are \"kB\", \"MB\", \"GB\", and \"TB\".");
+static const char *memory_units_hint = gettext_noop("Valid units for this parameter are \"B\", \"kB\", \"MB\", \"GB\", and \"TB\".");
static const unit_conversion memory_unit_conversion_table[] =
{
+ /*
+ * TB -> bytes conversion always overflows 32-bit integer, so this
+ * always produces an error. Include it for completeness, and so that
+ * you get an "out of range" error, rather than "invalid unit".
+ */
+ {"TB", GUC_UNIT_BYTE, INT64CONST(1024) * 1024 * 1024 * 1024},
{"GB", GUC_UNIT_BYTE, 1024 * 1024 * 1024},
{"MB", GUC_UNIT_BYTE, 1024 * 1024},
{"kB", GUC_UNIT_BYTE, 1024},
@@ -731,21 +737,25 @@ static const unit_conversion memory_unit_conversion_table[] =
{"GB", GUC_UNIT_KB, 1024 * 1024},
{"MB", GUC_UNIT_KB, 1024},
{"kB", GUC_UNIT_KB, 1},
+ {"B", GUC_UNIT_KB, -1024},
{"TB", GUC_UNIT_MB, 1024 * 1024},
{"GB", GUC_UNIT_MB, 1024},
{"MB", GUC_UNIT_MB, 1},
{"kB", GUC_UNIT_MB, -1024},
+ {"B", GUC_UNIT_MB, -(1024 * 1024)},
{"TB", GUC_UNIT_BLOCKS, (1024 * 1024 * 1024) / (BLCKSZ / 1024)},
{"GB", GUC_UNIT_BLOCKS, (1024 * 1024) / (BLCKSZ / 1024)},
{"MB", GUC_UNIT_BLOCKS, 1024 / (BLCKSZ / 1024)},
{"kB", GUC_UNIT_BLOCKS, -(BLCKSZ / 1024)},
+ {"B", GUC_UNIT_BLOCKS, -(BLCKSZ / (1024 * 1024))},
{"TB", GUC_UNIT_XBLOCKS, (1024 * 1024 * 1024) / (XLOG_BLCKSZ / 1024)},
{"GB", GUC_UNIT_XBLOCKS, (1024 * 1024) / (XLOG_BLCKSZ / 1024)},
{"MB", GUC_UNIT_XBLOCKS, 1024 / (XLOG_BLCKSZ / 1024)},
{"kB", GUC_UNIT_XBLOCKS, -(XLOG_BLCKSZ / 1024)},
+ {"B", GUC_UNIT_XBLOCKS, -(XLOG_BLCKSZ / (1024 * 1024))},
{""} /* end of table marker */
};
Hi!
On Wed, May 16, 2018 at 3:19 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
I played around with the GUC memory units, specifically to test the new
GUC_UNIT_BYTES flag (commit 6e7baa32):$ postmaster -c track_activity_query_size=1024kB
FATAL: 1048576 is outside the valid range for parameter
"track_activity_query_size" (100 .. 102400)$ postmaster -c track_activity_query_size=1024MB
FATAL: 1073741824 is outside the valid range for parameter
"track_activity_query_size" (100 .. 102400)$ postmaster -c track_activity_query_size=1024GB
FATAL: invalid value for parameter "track_activity_query_size": "1024GB"
HINT: Value exceeds integer range.$ postmaster -c track_activity_query_size=1024TB
FATAL: invalid value for parameter "track_activity_query_size": "1024TB"
HINT: Valid units for this parameter are "kB", "MB", "GB", and "TB".The first two look OK, but the last two cases seem a bit weird. With 1024
GB, it would be nice to print the valid range, like in the first two cases.
+1,
I also think it would be nice to show units in the valid range. I image
that if I would see such message at the first time, then I would try to
reverse-engineer units from input value representation in the error
message. Displaying units in the valid range would be clarifying for me.
The HINT in the last message seems wrong: the hint claims that "TB" is
accepted, yet "1024 TB" was not accepted.
Yes, this seems like a bug. I think it should be fixed.
And shouldn't the hint also mention "B", since we accept that now?
Yes, I think "B" should be mentioned in hint.
Testing a setting with GUC_UNIT_KB:
$ postmaster -c work_mem=102400B
FATAL: invalid value for parameter "work_mem": "100000B"
HINT: Valid units for this parameter are "kB", "MB", "GB", and "TB".This time the hint is accurate, but why is "B" not accepted here? Seems
inconsistent.
+1
------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
On Wed, May 16, 2018 at 3:49 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
On 16/05/18 15:19, Heikki Linnakangas wrote:
$ postmaster -c track_activity_query_size=1024TB
FATAL: invalid value for parameter "track_activity_query_size": "1024TB"
HINT: Valid units for this parameter are "kB", "MB", "GB", and "TB"....
The HINT in the last message seems wrong: the hint claims that "TB" is
accepted, yet "1024 TB" was not accepted. And shouldn't the hint also
mention "B", since we accept that now?Testing a setting with GUC_UNIT_KB:
$ postmaster -c work_mem=102400B
FATAL: invalid value for parameter "work_mem": "100000B"
HINT: Valid units for this parameter are "kB", "MB", "GB", and "TB".This time the hint is accurate, but why is "B" not accepted here? Seems
inconsistent.Here's a pretty straightforward fix for these two issues. Any objections
or better ideas?
This patch looks good for me.
But I would also like to see units in valid range message (as I wrote in
previous email).
------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
On 16/05/18 16:35, Alexander Korotkov wrote:
I also think it would be nice to show units in the valid range. I image
that if I would see such message at the first time, then I would try to
reverse-engineer units from input value representation in the error
message. Displaying units in the valid range would be clarifying for me.
Totally agreed. Users shouldn't need to know whether the base unit for a
setting is bytes or kilobytes or megabytes. But that sounds like a
feature rather than a bug, so, not now.
- Heikki
Hi,
On 2018-05-16 15:49:29 +0300, Heikki Linnakangas wrote:
Here's a pretty straightforward fix for these two issues. Any objections or
better ideas?
Generally ok, two minor points:
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 7cd2d2d80e..93402030f7 100644 {"TB", GUC_UNIT_BLOCKS, (1024 * 1024 * 1024) / (BLCKSZ / 1024)}, {"GB", GUC_UNIT_BLOCKS, (1024 * 1024) / (BLCKSZ / 1024)}, {"MB", GUC_UNIT_BLOCKS, 1024 / (BLCKSZ / 1024)}, {"kB", GUC_UNIT_BLOCKS, -(BLCKSZ / 1024)}, + {"B", GUC_UNIT_BLOCKS, -(BLCKSZ / (1024 * 1024))},
Isn't this 0 in the common case of 8k pages?
{"TB", GUC_UNIT_XBLOCKS, (1024 * 1024 * 1024) / (XLOG_BLCKSZ / 1024)},
{"GB", GUC_UNIT_XBLOCKS, (1024 * 1024) / (XLOG_BLCKSZ / 1024)},
{"MB", GUC_UNIT_XBLOCKS, 1024 / (XLOG_BLCKSZ / 1024)},
{"kB", GUC_UNIT_XBLOCKS, -(XLOG_BLCKSZ / 1024)},
+ {"B", GUC_UNIT_XBLOCKS, -(XLOG_BLCKSZ / (1024 * 1024))},
Same?
Greetings,
Andres Freund
On Wed, May 16, 2018 at 10:41 PM, Andres Freund <andres@anarazel.de> wrote:
On 2018-05-16 15:49:29 +0300, Heikki Linnakangas wrote:
Here's a pretty straightforward fix for these two issues. Any objections
or
better ideas?
Generally ok, two minor points:
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 7cd2d2d80e..93402030f7 100644 {"TB", GUC_UNIT_BLOCKS, (1024 * 1024 * 1024) / (BLCKSZ / 1024)}, {"GB", GUC_UNIT_BLOCKS, (1024 * 1024) / (BLCKSZ / 1024)}, {"MB", GUC_UNIT_BLOCKS, 1024 / (BLCKSZ / 1024)}, {"kB", GUC_UNIT_BLOCKS, -(BLCKSZ / 1024)}, + {"B", GUC_UNIT_BLOCKS, -(BLCKSZ / (1024 * 1024))},Isn't this 0 in the common case of 8k pages?
{"TB", GUC_UNIT_XBLOCKS, (1024 * 1024 * 1024) / (XLOG_BLCKSZ /
1024)},
{"GB", GUC_UNIT_XBLOCKS, (1024 * 1024) / (XLOG_BLCKSZ / 1024)},
{"MB", GUC_UNIT_XBLOCKS, 1024 / (XLOG_BLCKSZ / 1024)},
{"kB", GUC_UNIT_XBLOCKS, -(XLOG_BLCKSZ / 1024)},
+ {"B", GUC_UNIT_XBLOCKS, -(XLOG_BLCKSZ / (1024 * 1024))},Same?
As I understand, in these cases multiplier should be just -BLCKSZ and
-XLOG_BLCKSZ correspondingly.
------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
On 17/05/18 00:56, Alexander Korotkov wrote:
On Wed, May 16, 2018 at 10:41 PM, Andres Freund <andres@anarazel.de> wrote:
Generally ok, two minor points:
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 7cd2d2d80e..93402030f7 100644 {"TB", GUC_UNIT_BLOCKS, (1024 * 1024 * 1024) / (BLCKSZ / 1024)}, {"GB", GUC_UNIT_BLOCKS, (1024 * 1024) / (BLCKSZ / 1024)}, {"MB", GUC_UNIT_BLOCKS, 1024 / (BLCKSZ / 1024)}, {"kB", GUC_UNIT_BLOCKS, -(BLCKSZ / 1024)}, + {"B", GUC_UNIT_BLOCKS, -(BLCKSZ / (1024 * 1024))},Isn't this 0 in the common case of 8k pages?
{"TB", GUC_UNIT_XBLOCKS, (1024 * 1024 * 1024) / (XLOG_BLCKSZ /
1024)},
{"GB", GUC_UNIT_XBLOCKS, (1024 * 1024) / (XLOG_BLCKSZ / 1024)},
{"MB", GUC_UNIT_XBLOCKS, 1024 / (XLOG_BLCKSZ / 1024)},
{"kB", GUC_UNIT_XBLOCKS, -(XLOG_BLCKSZ / 1024)},
+ {"B", GUC_UNIT_XBLOCKS, -(XLOG_BLCKSZ / (1024 * 1024))},Same?
As I understand, in these cases multiplier should be just -BLCKSZ and
-XLOG_BLCKSZ correspondingly.
Yep, quite right. Fixed and committed, thanks!
- Heikki