Thread: Hiding undocumented enum values?
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
"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!
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
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...
Attachment
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
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
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 :) > //Magnus >
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