Thread: 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

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 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