Обсуждение: Remove redundant extra_desc info for enum GUC variables?
Most of the GUC variables that have been converted to enums have an
extra_desc string that lists the valid values --- in HEAD, try
SELECT name,extra_desc,enumvals from pg_settings where vartype = 'enum';
ISTM this is just about 100% redundant with the enumvals column and
should be removed to reduce translation and maintenance effort.
Any objections?
One point of interest is that for client_min_messages and
log_min_messages, the ordering of the values has significance, and it's
different for the two cases. The enum patch has lost that info by
trying to use the same auxiliary list for both variables. But having
two lists doesn't seem like an excessive amount of overhead.
regards, tom lane
Tom Lane wrote: > Most of the GUC variables that have been converted to enums have an > extra_desc string that lists the valid values --- in HEAD, try > SELECT name,extra_desc,enumvals from pg_settings where vartype = > 'enum'; > > ISTM this is just about 100% redundant with the enumvals column and > should be removed to reduce translation and maintenance effort. > Any objections? No, seems like the correct thing to do. > One point of interest is that for client_min_messages and > log_min_messages, the ordering of the values has significance, and > it's different for the two cases. The enum patch has lost that info > by trying to use the same auxiliary list for both variables. But > having two lists doesn't seem like an excessive amount of overhead. Is there any actual reason why they're supposed to be treated differently? //Magnus
Magnus Hagander <magnus@hagander.net> writes:
>> One point of interest is that for client_min_messages and
>> log_min_messages, the ordering of the values has significance, and
>> it's different for the two cases.
> Is there any actual reason why they're supposed to be treated
> differently?
Yeah: LOG level sorts differently in the two cases; it's fairly high
priority for server log output and much lower for client output.
regards, tom lane
Tom Lane wrote: > Magnus Hagander <magnus@hagander.net> writes: > >> One point of interest is that for client_min_messages and > >> log_min_messages, the ordering of the values has significance, and > >> it's different for the two cases. > > > Is there any actual reason why they're supposed to be treated > > differently? > > Yeah: LOG level sorts differently in the two cases; it's fairly high > priority for server log output and much lower for client output. Ok, easy fix if we break them apart. Should we continue to accept values that we're not going to care about, or should I change that at the same time? (for example, client_min_messages doesn't use INFO, but we do accept that in <= 8.3 anyway) //Magnus
Magnus Hagander <magnus@hagander.net> writes:
> Tom Lane wrote:
>> Yeah: LOG level sorts differently in the two cases; it's fairly high
>> priority for server log output and much lower for client output.
> Ok, easy fix if we break them apart. Should we continue to accept
> values that we're not going to care about, or should I change that at
> the same time? (for example, client_min_messages doesn't use INFO,
> but we do accept that in <= 8.3 anyway)
I'd be inclined to keep the actual behavior the same as it was.
We didn't document INFO for this variable, perhaps, but it's accepted
and has a well-defined behavior.
regards, tom lane
Tom Lane wrote:
> Magnus Hagander <magnus@hagander.net> writes:
>> Tom Lane wrote:
>>> Yeah: LOG level sorts differently in the two cases; it's fairly high
>>> priority for server log output and much lower for client output.
>
>> Ok, easy fix if we break them apart. Should we continue to accept
>> values that we're not going to care about, or should I change that at
>> the same time? (for example, client_min_messages doesn't use INFO,
>> but we do accept that in <= 8.3 anyway)
>
> I'd be inclined to keep the actual behavior the same as it was.
> We didn't document INFO for this variable, perhaps, but it's accepted
> and has a well-defined behavior.
Sorry for not getting back to this one sooner, it ended up in the wrong
end of the queue.
Does this patch look like what you meant? It should split them apart,
and it also hides the undocumented levels, but still accept it (now that
we have the ability to hide GUC vars)
//Magnus
Index: src/backend/utils/misc/guc.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/misc/guc.c,v
retrieving revision 1.457
diff -c -r1.457 guc.c
*** src/backend/utils/misc/guc.c 30 Jun 2008 10:58:47 -0000 1.457
--- src/backend/utils/misc/guc.c 30 Jun 2008 19:50:24 -0000
***************
*** 172,178 ****
/*
* Options for enum values defined in this module.
*/
! static const struct config_enum_entry message_level_options[] = {
{"debug", DEBUG2, false},
{"debug5", DEBUG5, false},
{"debug4", DEBUG4, false},
--- 172,183 ----
/*
* Options for enum values defined in this module.
*/
!
! /*
! * We have different sets for client and server message level options because
! * they sort slightly different (see "log" level)
! */
! static const struct config_enum_entry client_message_level_options[] = {
{"debug", DEBUG2, false},
{"debug5", DEBUG5, false},
{"debug4", DEBUG4, false},
***************
*** 180,189 ****
--- 185,211 ----
{"debug2", DEBUG2, false},
{"debug1", DEBUG1, false},
{"log", LOG, false},
+ {"info", INFO, true},
+ {"notice", NOTICE, false},
+ {"warning", WARNING, false},
+ {"error", ERROR, false},
+ {"fatal", FATAL, true},
+ {"panic", PANIC, true},
+ {NULL, 0, false}
+ };
+
+ static const struct config_enum_entry server_message_level_options[] = {
+ {"debug", DEBUG2, false},
+ {"debug5", DEBUG5, false},
+ {"debug4", DEBUG4, false},
+ {"debug3", DEBUG3, false},
+ {"debug2", DEBUG2, false},
+ {"debug1", DEBUG1, false},
{"info", INFO, false},
{"notice", NOTICE, false},
{"warning", WARNING, false},
{"error", ERROR, false},
+ {"log", LOG, false},
{"fatal", FATAL, false},
{"panic", PANIC, false},
{NULL, 0, false}
***************
*** 2449,2461 ****
{
{"client_min_messages", PGC_USERSET, LOGGING_WHEN,
gettext_noop("Sets the message levels that are sent to the client."),
! gettext_noop("Valid values are DEBUG5, DEBUG4, DEBUG3, DEBUG2, "
! "DEBUG1, LOG, NOTICE, WARNING, and ERROR. Each level includes all the "
! "levels that follow it. The later the level, the fewer messages are "
! "sent.")
},
&client_min_messages,
! NOTICE, message_level_options,NULL, NULL
},
{
--- 2471,2481 ----
{
{"client_min_messages", PGC_USERSET, LOGGING_WHEN,
gettext_noop("Sets the message levels that are sent to the client."),
! gettext_noop("Each level includes all the levels that follow it. The later"
! " the level, the fewer messages are sent.")
},
&client_min_messages,
! NOTICE, client_message_level_options,NULL, NULL
},
{
***************
*** 2480,2491 ****
{
{"log_min_messages", PGC_SUSET, LOGGING_WHEN,
gettext_noop("Sets the message levels that are logged."),
! gettext_noop("Valid values are DEBUG5, DEBUG4, DEBUG3, DEBUG2, DEBUG1, "
! "INFO, NOTICE, WARNING, ERROR, LOG, FATAL, and PANIC. Each level "
! "includes all the levels that follow it.")
},
&log_min_messages,
! WARNING, message_level_options, NULL, NULL
},
{
--- 2500,2509 ----
{
{"log_min_messages", PGC_SUSET, LOGGING_WHEN,
gettext_noop("Sets the message levels that are logged."),
! gettext_noop("Each level includes all levels that follow it.")
},
&log_min_messages,
! WARNING, server_message_level_options, NULL, NULL
},
{
***************
*** 2495,2501 ****
"specified level or a higher level are logged.")
},
&log_min_error_statement,
! ERROR, message_level_options, NULL, NULL
},
{
--- 2513,2519 ----
"specified level or a higher level are logged.")
},
&log_min_error_statement,
! ERROR, server_message_level_options, NULL, NULL
},
{
Magnus Hagander <magnus@hagander.net> writes:
> Does this patch look like what you meant? It should split them apart,
> and it also hides the undocumented levels, but still accept it (now that
> we have the ability to hide GUC vars)
Seems reasonable, although I'm still dissatisfied with the handling of
the "debug" alias for debug2. I think if it's not hidden then it has
to be placed in correct sort position. Since it's not documented in
config.sgml, I think marking it hidden would be fine.
regards, tom lane
Tom Lane wrote: > Magnus Hagander <magnus@hagander.net> writes: >> Does this patch look like what you meant? It should split them apart, >> and it also hides the undocumented levels, but still accept it (now that >> we have the ability to hide GUC vars) > > Seems reasonable, although I'm still dissatisfied with the handling of > the "debug" alias for debug2. I think if it's not hidden then it has > to be placed in correct sort position. Since it's not documented in > config.sgml, I think marking it hidden would be fine. Good point, and thanks for the quick review. Will fix and apply. //Magnus