Обсуждение: slightly misleading Error message in guc.c

Поиск
Список
Период
Сортировка

slightly misleading Error message in guc.c

От
jian he
Дата:
hi.
minor issue in guc.c.

set work_mem to '1kB';
ERROR:  1 kB is outside the valid range for parameter "work_mem" (64
.. 2147483647)
should it be
ERROR:  1 kB is outside the valid range for parameter "work_mem" (64
kB .. 2147483647 kB)
?
since the units for work_mem are { "B", "kB", "MB", "GB", and "TB"}

search `outside the valid range for parameter`,
there are two occurrences in guc.c.



Re: slightly misleading Error message in guc.c

От
Nazir Bilal Yavuz
Дата:
Hi,

On Mon, 22 Apr 2024 at 11:44, jian he <jian.universality@gmail.com> wrote:
>
> hi.
> minor issue in guc.c.
>
> set work_mem to '1kB';
> ERROR:  1 kB is outside the valid range for parameter "work_mem" (64
> .. 2147483647)
> should it be
> ERROR:  1 kB is outside the valid range for parameter "work_mem" (64
> kB .. 2147483647 kB)
> ?
> since the units for work_mem are { "B", "kB", "MB", "GB", and "TB"}
>
> search `outside the valid range for parameter`,
> there are two occurrences in guc.c.

Nice find. I agree it could cause confusion.

-- 
Regards,
Nazir Bilal Yavuz
Microsoft



Re: slightly misleading Error message in guc.c

От
Tom Lane
Дата:
jian he <jian.universality@gmail.com> writes:
> set work_mem to '1kB';
> ERROR:  1 kB is outside the valid range for parameter "work_mem" (64
> .. 2147483647)
> should it be
> ERROR:  1 kB is outside the valid range for parameter "work_mem" (64
> kB .. 2147483647 kB)
> ?
> since the units for work_mem are { "B", "kB", "MB", "GB", and "TB"}

Seems like a useful change ... about like this?

            regards, tom lane

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index f51b3e0b50..3fb6803998 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -3173,15 +3173,20 @@ parse_and_validate_value(struct config_generic *record,
                 if (newval->intval < conf->min || newval->intval > conf->max)
                 {
                     const char *unit = get_config_unit_name(conf->gen.flags);
+                    const char *unitspace;
+
+                    if (unit)
+                        unitspace = " ";
+                    else
+                        unit = unitspace = "";

                     ereport(elevel,
                             (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-                             errmsg("%d%s%s is outside the valid range for parameter \"%s\" (%d .. %d)",
-                                    newval->intval,
-                                    unit ? " " : "",
-                                    unit ? unit : "",
+                             errmsg("%d%s%s is outside the valid range for parameter \"%s\" (%d%s%s .. %d%s%s)",
+                                    newval->intval, unitspace, unit,
                                     name,
-                                    conf->min, conf->max)));
+                                    conf->min, unitspace, unit,
+                                    conf->max, unitspace, unit)));
                     return false;
                 }

@@ -3209,15 +3214,20 @@ parse_and_validate_value(struct config_generic *record,
                 if (newval->realval < conf->min || newval->realval > conf->max)
                 {
                     const char *unit = get_config_unit_name(conf->gen.flags);
+                    const char *unitspace;
+
+                    if (unit)
+                        unitspace = " ";
+                    else
+                        unit = unitspace = "";

                     ereport(elevel,
                             (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-                             errmsg("%g%s%s is outside the valid range for parameter \"%s\" (%g .. %g)",
-                                    newval->realval,
-                                    unit ? " " : "",
-                                    unit ? unit : "",
+                             errmsg("%g%s%s is outside the valid range for parameter \"%s\" (%g%s%s .. %g%s%s)",
+                                    newval->realval, unitspace, unit,
                                     name,
-                                    conf->min, conf->max)));
+                                    conf->min, unitspace, unit,
+                                    conf->max, unitspace, unit)));
                     return false;
                 }

diff --git a/src/test/regress/expected/guc.out b/src/test/regress/expected/guc.out
index 127c953297..455b6d6c0c 100644
--- a/src/test/regress/expected/guc.out
+++ b/src/test/regress/expected/guc.out
@@ -510,7 +510,7 @@ SELECT '2006-08-13 12:34:56'::timestamptz;
 SET seq_page_cost TO 'NaN';
 ERROR:  invalid value for parameter "seq_page_cost": "NaN"
 SET vacuum_cost_delay TO '10s';
-ERROR:  10000 ms is outside the valid range for parameter "vacuum_cost_delay" (0 .. 100)
+ERROR:  10000 ms is outside the valid range for parameter "vacuum_cost_delay" (0 ms .. 100 ms)
 SET no_such_variable TO 42;
 ERROR:  unrecognized configuration parameter "no_such_variable"
 -- Test "custom" GUCs created on the fly (which aren't really an

Re: slightly misleading Error message in guc.c

От
Daniel Gustafsson
Дата:
> On 22 Apr 2024, at 18:04, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Seems like a useful change

Agreed.

> ... about like this?

Patch LGTM.

--
Daniel Gustafsson




Re: slightly misleading Error message in guc.c

От
Tom Lane
Дата:
Daniel Gustafsson <daniel@yesql.se> writes:
>> On 22 Apr 2024, at 18:04, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Seems like a useful change

> Agreed.

>> ... about like this?

> Patch LGTM.

Pushed.

            regards, tom lane