Memory unit GUC range checks

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

Memory unit GUC range checks

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

Reply | Threaded
Open this post in threaded view
|

Re: Memory unit GUC range checks

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

byte-unit-fixes-1.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Memory unit GUC range checks

Alexander Korotkov
In reply to this post by Heikki Linnakangas
Hi!

On Wed, May 16, 2018 at 3:19 PM, Heikki Linnakangas <[hidden email]> 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 
Reply | Threaded
Open this post in threaded view
|

Re: Memory unit GUC range checks

Alexander Korotkov
In reply to this post by Heikki Linnakangas
On Wed, May 16, 2018 at 3:49 PM, Heikki Linnakangas <[hidden email]> 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 
Reply | Threaded
Open this post in threaded view
|

Re: Memory unit GUC range checks

Heikki Linnakangas
In reply to this post by Alexander Korotkov
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

Reply | Threaded
Open this post in threaded view
|

Re: Memory unit GUC range checks

Andres Freund
In reply to this post by Heikki Linnakangas
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

Reply | Threaded
Open this post in threaded view
|

Re: Memory unit GUC range checks

Alexander Korotkov
On Wed, May 16, 2018 at 10:41 PM, Andres Freund <[hidden email]> 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 
Previous Thread Next Thread