Hiding undocumented enum values?

Started by Tom Laneover 17 years ago8 messages

tgl@sss.pgh.pa.us

over 17 years ago

There are several GUC enums that accept values that aren't documented
anywhere; the worst offender being backslash_quote, which has more
undocumented spellings than documented ones:

/*
* Although only "on", "off", and "safe_encoding" are documented, we
* accept all the likely variants of "on" and "off".
*/
static const struct config_enum_entry backslash_quote_options[] = {
{"safe_encoding", BACKSLASH_QUOTE_SAFE_ENCODING},
{"on", BACKSLASH_QUOTE_ON},
{"off", BACKSLASH_QUOTE_OFF},
{"true", BACKSLASH_QUOTE_ON},
{"false", BACKSLASH_QUOTE_OFF},
{"yes", BACKSLASH_QUOTE_ON},
{"no", BACKSLASH_QUOTE_OFF},
{"1", BACKSLASH_QUOTE_ON},
{"0", BACKSLASH_QUOTE_OFF},
{NULL, 0}
};

I am wondering if it's a good idea to hide the redundant entries
to reduce clutter in the pg_settings display. (We could do this
by adding a "hidden" boolean to struct config_enum_entry.)
Thoughts?

regards, tom lane

stark@enterprisedb.com

over 17 years ago

In reply to: Tom Lane (#1)

Re: Hiding undocumented enum values?

"Tom Lane" <tgl@sss.pgh.pa.us> writes:

I am wondering if it's a good idea to hide the redundant entries
to reduce clutter in the pg_settings display. (We could do this
by adding a "hidden" boolean to struct config_enum_entry.)
Thoughts?

Well it looks like it's trying to emulate the boolean guc behaviour but with
an extra value. So it would make sense for the output to look like boolean
variables. Do we list the valid values for boolean variables at all? Should
we?

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Ask me about EnterpriseDB's Slony Replication support!

tgl@sss.pgh.pa.us

over 17 years ago

In reply to: Gregory Stark (#2)

Re: Hiding undocumented enum values?

Gregory Stark <stark@enterprisedb.com> writes:

Well it looks like it's trying to emulate the boolean guc behaviour but with
an extra value.

Right, that's exactly what it's doing.

Do we list the valid values for boolean variables at all?

Very first thing in section 18.1 of TFM. The individual variable
descriptions don't repeat that, though (and shouldn't IMHO).

regards, tom lane

badalex@gmail.com

over 17 years ago

In reply to: Tom Lane (#1)

1 attachment(s)

Re: Hiding undocumented enum values?

On Tue, May 27, 2008 at 10:20 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I am wondering if it's a good idea to hide the redundant entries
to reduce clutter in the pg_settings display. (We could do this
by adding a "hidden" boolean to struct config_enum_entry.)
Thoughts?

+1

regards, tom lane

Maybe something like the attached patch?

I looked into just making it a string so we could use parse_bool...
because backslash_quote seems to be the exception not the rule. But I
decided having a hidden flag seems more useful anyway...

Attachments:

guc.patchapplication/octet-stream; name=guc.patchDownload

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 4c3e2c8..80d7d1a 100644
*** a/src/backend/utils/misc/guc.c
--- /bsrc/backend/utils/misc/guc.c
*************** static char *config_enum_get_options(str
*** 173,255 ****
   * Options for enum values defined in this module.
   */
  static const struct config_enum_entry message_level_options[] = {
! 	{"debug", DEBUG2},
! 	{"debug5", DEBUG5},
! 	{"debug4", DEBUG4},
! 	{"debug3", DEBUG3},
! 	{"debug2", DEBUG2},
! 	{"debug1", DEBUG1},
! 	{"log", LOG},
! 	{"info", INFO},
! 	{"notice", NOTICE},
! 	{"warning", WARNING},
! 	{"error", ERROR},
! 	{"fatal", FATAL},
! 	{"panic", PANIC},
! 	{NULL, 0}
  };
  
  static const struct config_enum_entry log_error_verbosity_options[] = {
! 	{"default", PGERROR_DEFAULT},
! 	{"terse", PGERROR_TERSE},
! 	{"verbose", PGERROR_VERBOSE},
! 	{NULL, 0}
  };
  
  static const struct config_enum_entry log_statement_options[] = {
! 	{"none", LOGSTMT_NONE},
! 	{"ddl", LOGSTMT_DDL},
! 	{"mod", LOGSTMT_MOD},
! 	{"all", LOGSTMT_ALL},
! 	{NULL, 0}
  };
  
  static const struct config_enum_entry regex_flavor_options[] = {
!     {"advanced", REG_ADVANCED},
!     {"extended", REG_EXTENDED},
!     {"basic", REG_BASIC},
!     {NULL, 0}
  };
  
  static const struct config_enum_entry isolation_level_options[] = {
! 	{"serializable", XACT_SERIALIZABLE},
! 	{"repeatable read", XACT_REPEATABLE_READ},
! 	{"read committed", XACT_READ_COMMITTED},
! 	{"read uncommitted", XACT_READ_UNCOMMITTED},
! 	{NULL, 0}
  };
  
  static const struct config_enum_entry session_replication_role_options[] = {
! 	{"origin", SESSION_REPLICATION_ROLE_ORIGIN},
! 	{"replica", SESSION_REPLICATION_ROLE_REPLICA},
! 	{"local", SESSION_REPLICATION_ROLE_LOCAL},
! 	{NULL, 0}
  };
  
  #ifdef HAVE_SYSLOG
  static const struct config_enum_entry syslog_facility_options[] = {
! 	{"local0", LOG_LOCAL0},
! 	{"local1", LOG_LOCAL1},
! 	{"local2", LOG_LOCAL2},
! 	{"local3", LOG_LOCAL3},
! 	{"local4", LOG_LOCAL4},
! 	{"local5", LOG_LOCAL5},
! 	{"local6", LOG_LOCAL6},
! 	{"local7", LOG_LOCAL7},
! 	{NULL, 0}
  };
  #endif
  
  static const struct config_enum_entry xmlbinary_options[] = {
! 	{"base64", XMLBINARY_BASE64},
! 	{"hex", XMLBINARY_HEX},
! 	{NULL, 0}
  };
  
  static const struct config_enum_entry xmloption_options[] = {
! 	{"content", XMLOPTION_CONTENT},
! 	{"document", XMLOPTION_DOCUMENT},
! 	{NULL, 0}
  };
  
  /*
--- 173,255 ----
   * 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},
! 	{"debug3", DEBUG3, false},
! 	{"debug2", DEBUG2, false},
! 	{"debug1", DEBUG1, false},
! 	{"log", LOG, false},
! 	{"info", INFO, false},
! 	{"notice", NOTICE, false},
! 	{"warning", WARNING, false},
! 	{"error", ERROR, false},
! 	{"fatal", FATAL, false},
! 	{"panic", PANIC, false},
! 	{NULL, 0, false}
  };
  
  static const struct config_enum_entry log_error_verbosity_options[] = {
! 	{"default", PGERROR_DEFAULT, false},
! 	{"terse", PGERROR_TERSE, false},
! 	{"verbose", PGERROR_VERBOSE, false},
! 	{NULL, 0, false}
  };
  
  static const struct config_enum_entry log_statement_options[] = {
! 	{"none", LOGSTMT_NONE, false},
! 	{"ddl", LOGSTMT_DDL, false},
! 	{"mod", LOGSTMT_MOD, false},
! 	{"all", LOGSTMT_ALL, false},
! 	{NULL, 0, false}
  };
  
  static const struct config_enum_entry regex_flavor_options[] = {
!     {"advanced", REG_ADVANCED, false},
!     {"extended", REG_EXTENDED, false},
!     {"basic", REG_BASIC, false},
!     {NULL, 0, false}
  };
  
  static const struct config_enum_entry isolation_level_options[] = {
! 	{"serializable", XACT_SERIALIZABLE, false},
! 	{"repeatable read", XACT_REPEATABLE_READ, false},
! 	{"read committed", XACT_READ_COMMITTED, false},
! 	{"read uncommitted", XACT_READ_UNCOMMITTED, false},
! 	{NULL, 0, false}
  };
  
  static const struct config_enum_entry session_replication_role_options[] = {
! 	{"origin", SESSION_REPLICATION_ROLE_ORIGIN, false},
! 	{"replica", SESSION_REPLICATION_ROLE_REPLICA, false},
! 	{"local", SESSION_REPLICATION_ROLE_LOCAL, false},
! 	{NULL, 0, false}
  };
  
  #ifdef HAVE_SYSLOG
  static const struct config_enum_entry syslog_facility_options[] = {
! 	{"local0", LOG_LOCAL0, false},
! 	{"local1", LOG_LOCAL1, false},
! 	{"local2", LOG_LOCAL2, false},
! 	{"local3", LOG_LOCAL3, false},
! 	{"local4", LOG_LOCAL4, false},
! 	{"local5", LOG_LOCAL5, false},
! 	{"local6", LOG_LOCAL6, false},
! 	{"local7", LOG_LOCAL7, false},
! 	{NULL, 0, false}
  };
  #endif
  
  static const struct config_enum_entry xmlbinary_options[] = {
! 	{"base64", XMLBINARY_BASE64, false},
! 	{"hex", XMLBINARY_HEX, false},
! 	{NULL, 0, false}
  };
  
  static const struct config_enum_entry xmloption_options[] = {
! 	{"content", XMLOPTION_CONTENT, false},
! 	{"document", XMLOPTION_DOCUMENT, false},
! 	{NULL, 0, false}
  };
  
  /*
*************** static const struct config_enum_entry xm
*** 257,272 ****
   * accept all the likely variants of "on" and "off".
   */
  static const struct config_enum_entry backslash_quote_options[] = {
! 	{"safe_encoding", BACKSLASH_QUOTE_SAFE_ENCODING},
! 	{"on", BACKSLASH_QUOTE_ON},
! 	{"off", BACKSLASH_QUOTE_OFF},
! 	{"true", BACKSLASH_QUOTE_ON},
! 	{"false", BACKSLASH_QUOTE_OFF},
! 	{"yes", BACKSLASH_QUOTE_ON},
! 	{"no", BACKSLASH_QUOTE_OFF},
! 	{"1", BACKSLASH_QUOTE_ON},
! 	{"0", BACKSLASH_QUOTE_OFF},
! 	{NULL, 0}
  };
  
  /*
--- 257,272 ----
   * accept all the likely variants of "on" and "off".
   */
  static const struct config_enum_entry backslash_quote_options[] = {
! 	{"safe_encoding", BACKSLASH_QUOTE_SAFE_ENCODING, false},
! 	{"on", BACKSLASH_QUOTE_ON, false},
! 	{"off", BACKSLASH_QUOTE_OFF, false},
! 	{"true", BACKSLASH_QUOTE_ON, true},
! 	{"false", BACKSLASH_QUOTE_OFF, true},
! 	{"yes", BACKSLASH_QUOTE_ON, true},
! 	{"no", BACKSLASH_QUOTE_OFF, true},
! 	{"1", BACKSLASH_QUOTE_ON, true},
! 	{"0", BACKSLASH_QUOTE_OFF, true},
! 	{NULL, 0, false}
  };
  
  /*
*************** config_enum_get_options(struct config_en
*** 4331,4340 ****
  
  	if (!entry || !entry->name)
  		return NULL;					/* Should not happen */
! 	
  	while (entry && entry->name)
  	{
! 		len += strlen(entry->name) + 2; /* string and ", " */
  		entry++;
  	}
  
--- 4331,4342 ----
  
  	if (!entry || !entry->name)
  		return NULL;					/* Should not happen */
! 
  	while (entry && entry->name)
  	{
! 		if (!entry->hidden)
! 			len += strlen(entry->name) + 2; /* string and ", " */
! 
  		entry++;
  	}
  
*************** config_enum_get_options(struct config_en
*** 4345,4352 ****
  	entry = record->options;
  	while (entry && entry->name)
  	{
! 		strcat(hintmsg, entry->name);
! 		strcat(hintmsg, ", ");
  		entry++;
  	}
  
--- 4347,4358 ----
  	entry = record->options;
  	while (entry && entry->name)
  	{
! 		if (!entry->hidden)
! 		{
! 			strcat(hintmsg, entry->name);
! 			strcat(hintmsg, ", ");
! 		}
! 
  		entry++;
  	}
  
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index b8dc84e..feda255 100644
*** a/src/include/utils/guc.h
--- /bsrc/include/utils/guc.h
*************** struct config_enum_entry
*** 100,105 ****
--- 100,107 ----
  {
  	const char *name;
  	int         val;
+ 	/* dont show in pg_settings */
+ 	bool hidden;
  };

Magnus Hagander

magnus@hagander.net

over 17 years ago

In reply to: Tom Lane (#1)

Re: Hiding undocumented enum values?

Tom Lane wrote:

There are several GUC enums that accept values that aren't documented
anywhere; the worst offender being backslash_quote, which has more
undocumented spellings than documented ones:

/*
* Although only "on", "off", and "safe_encoding" are documented, we
* accept all the likely variants of "on" and "off".
*/
static const struct config_enum_entry backslash_quote_options[] = {
{"safe_encoding", BACKSLASH_QUOTE_SAFE_ENCODING},
{"on", BACKSLASH_QUOTE_ON},
{"off", BACKSLASH_QUOTE_OFF},
{"true", BACKSLASH_QUOTE_ON},
{"false", BACKSLASH_QUOTE_OFF},
{"yes", BACKSLASH_QUOTE_ON},
{"no", BACKSLASH_QUOTE_OFF},
{"1", BACKSLASH_QUOTE_ON},
{"0", BACKSLASH_QUOTE_OFF},
{NULL, 0}
};

I am wondering if it's a good idea to hide the redundant entries
to reduce clutter in the pg_settings display. (We could do this
by adding a "hidden" boolean to struct config_enum_entry.)
Thoughts?

Seems reasonable. Another option would be to simply drop the
undocumented options, but then it wouldn't be "compatible" with pure
boolean variables so I think that would be a bad idea.

I can do this tomorrow if there are no people making good arguments for
dropping them completely.

//Magnus

Magnus Hagander

magnus@hagander.net

over 17 years ago

In reply to: Alex Hunsaker (#4)

Re: Hiding undocumented enum values?

Alex Hunsaker wrote:

On Tue, May 27, 2008 at 10:20 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I am wondering if it's a good idea to hide the redundant entries
to reduce clutter in the pg_settings display. (We could do this
by adding a "hidden" boolean to struct config_enum_entry.)
Thoughts?

+1

regards, tom lane

Maybe something like the attached patch?

Oops, missed that there was a patch posted already. Looks like the way
to do it (except I'd move the comment :-P) if that's the way we go.

I looked into just making it a string so we could use parse_bool...
because backslash_quote seems to be the exception not the rule. But I
decided having a hidden flag seems more useful anyway...

It used to be a string. We don't want that, because then we can't tell
the client which possible values are available. That's the whole reason
for the creation of the enum type gucs...

//Magnus

badalex@gmail.com

over 17 years ago

In reply to: Magnus Hagander (#6)

Re: Hiding undocumented enum values?

On Tue, May 27, 2008 at 12:05 PM, Magnus Hagander <magnus@hagander.net> wrote:

Alex Hunsaker wrote:

On Tue, May 27, 2008 at 10:20 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I am wondering if it's a good idea to hide the redundant entries
to reduce clutter in the pg_settings display. (We could do this
by adding a "hidden" boolean to struct config_enum_entry.)
Thoughts?

+1

regards, tom lane

Maybe something like the attached patch?

Oops, missed that there was a patch posted already. Looks like the way
to do it (except I'd move the comment :-P) if that's the way we go.

OK, the updated patch is on pg_patches under "guc config_enum_entry
add hidden field"

-moved the comment into config_enum_get_options()
-fixed a possible buffer underrun if every option was hidden
-updated against HEAD

I looked into just making it a string so we could use parse_bool...
because backslash_quote seems to be the exception not the rule. But I
decided having a hidden flag seems more useful anyway...

It used to be a string. We don't want that, because then we can't tell
the client which possible values are available. That's the whole reason
for the creation of the enum type gucs...

Well its good i did not go that route then :)

Show quoted text

//Magnus

Magnus Hagander

magnus@hagander.net

over 17 years ago

In reply to: Alex Hunsaker (#7)

Re: Hiding undocumented enum values?

Alex Hunsaker wrote:

On Tue, May 27, 2008 at 12:05 PM, Magnus Hagander
<magnus@hagander.net> wrote:

Alex Hunsaker wrote:

On Tue, May 27, 2008 at 10:20 AM, Tom Lane <tgl@sss.pgh.pa.us>
wrote:

I am wondering if it's a good idea to hide the redundant entries
to reduce clutter in the pg_settings display. (We could do this
by adding a "hidden" boolean to struct config_enum_entry.)
Thoughts?

+1

regards, tom lane

Maybe something like the attached patch?

Oops, missed that there was a patch posted already. Looks like the
way to do it (except I'd move the comment :-P) if that's the way we
go.

OK, the updated patch is on pg_patches under "guc config_enum_entry
add hidden field"

Thanks, I've reviewed and applied.

-moved the comment into config_enum_get_options()

I moved it again, to the header :-)

-fixed a possible buffer underrun if every option was hidden

That fix didn't take into account the possibility of having different
prefixes. Since it is a pretty stupid thing to have a GUC enum with
*only* hidden entries, I just made it do nothing in this case and
updated the comment. The buffer underrun check is still there.

I looked into just making it a string so we could use parse_bool...
because backslash_quote seems to be the exception not the rule.
But I decided having a hidden flag seems more useful anyway...

It used to be a string. We don't want that, because then we can't
tell the client which possible values are available. That's the
whole reason for the creation of the enum type gucs...

Well its good i did not go that route then :)

Yup :)

//Magnus