Memory unit GUC range checks

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

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