Thread: Re: How about a psql backslash command to show GUCs?
On 2022-Apr-06, Tom Lane wrote: > However, I very often find myself resorting to the much more tedious > > select * from pg_settings where name like '%foo%'; > > when I want to see some related parameters, or when I'm a bit > fuzzy on the exact name of the parameter. Been there many times, so +1 for the general idea. > In the discussion about adding privileges for GUCs [1], there > was a proposal to add a new psql backslash command to show GUCs, > which could reduce this problem to something like > > \dcp *foo* +1. As for command name, I like \show as proposed by Pavel. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
On 4/6/22 13:58, Alvaro Herrera wrote: > On 2022-Apr-06, Tom Lane wrote: > >> However, I very often find myself resorting to the much more tedious >> >> select * from pg_settings where name like '%foo%'; >> >> when I want to see some related parameters, or when I'm a bit >> fuzzy on the exact name of the parameter. > > Been there many times, so +1 for the general idea. > >> In the discussion about adding privileges for GUCs [1], there >> was a proposal to add a new psql backslash command to show GUCs, >> which could reduce this problem to something like >> >> \dcp *foo* > > +1. As for command name, I like \show as proposed by Pavel. +1 I'd love something for the same reasons. No as sure about \show though. That seems like it could be confused with showing other stuff. Maybe consistent with \sf[+] and \sv[+] we could add \sc[+]? Joe
Joe Conway <mail@joeconway.com> writes:
> No as sure about \show though. That seems like it could be confused with
> showing other stuff. Maybe consistent with \sf[+] and \sv[+] we could
> add \sc[+]?
Hmm ... my first reaction to that was "no, it should be \sp for
'parameter'". But with the neighboring \sf for 'function', it'd
be easy to think that maybe 'p' means 'procedure'.
I do agree that \show might be a bad choice, the reason being that
the adjacent \set command is for psql variables not GUCs; if we
had a \show I'd sort of expect it to be a variant spelling of
"\echo :variable".
"\sc" isn't awful perhaps.
Ah, naming ... the hardest problem in computer science.
regards, tom lane
On 4/6/22 2:40 PM, Tom Lane wrote: > Joe Conway <mail@joeconway.com> writes: >> No as sure about \show though. That seems like it could be confused with >> showing other stuff. Maybe consistent with \sf[+] and \sv[+] we could >> add \sc[+]? > > Hmm ... my first reaction to that was "no, it should be \sp for > 'parameter'". But with the neighboring \sf for 'function', it'd > be easy to think that maybe 'p' means 'procedure'. > > I do agree that \show might be a bad choice, the reason being that > the adjacent \set command is for psql variables not GUCs; if we > had a \show I'd sort of expect it to be a variant spelling of > "\echo :variable". > > "\sc" isn't awful perhaps. > > Ah, naming ... the hardest problem in computer science. (but the easiest thing to have an opinion on ;) +1 on the feature proposal. I am a bit torn between "\dcp" (or \dsetting / \dconfig? we don't necessarily need for it to be super short) and "\sc". Certainly with pattern matching the interface for the "\d" commands would fit that pattern. "\sc" would make sense for a thorough introspection of what is in the GUC. That said, we get that with SHOW today. So I'm leaning towards something in the "\d" family. Thanks, Jonathan
Attachment
> On Apr 6, 2022, at 2:34 PM, Jonathan S. Katz <jkatz@postgresql.org> wrote: > > "\sc" would make sense I originally wrote the command as \dcp (describe configuration parameter) because \dp (describe parameter) wasn't available. The thing we're showing is a "parameter", not a "config". If we're going to use a single letter, I'd prefer /p/. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
"Jonathan S. Katz" <jkatz@postgresql.org> writes:
> I am a bit torn between "\dcp" (or \dsetting / \dconfig? we don't
> necessarily need for it to be super short) and "\sc". Certainly with
> pattern matching the interface for the "\d" commands would fit that
> pattern. "\sc" would make sense for a thorough introspection of what is
> in the GUC. That said, we get that with SHOW today.
> So I'm leaning towards something in the "\d" family.
I agree that \d-something makes the most sense from a functionality
standpoint. But I don't want to make the name too long, even if we
do have tab completion to help.
\dconf maybe?
regards, tom lane
On Wed, Apr 6, 2022 at 6:16 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
"Jonathan S. Katz" <jkatz@postgresql.org> writes:
> I am a bit torn between "\dcp" (or \dsetting / \dconfig? we don't
> necessarily need for it to be super short) and "\sc". Certainly with
> pattern matching the interface for the "\d" commands would fit that
> pattern. "\sc" would make sense for a thorough introspection of what is
> in the GUC. That said, we get that with SHOW today.
> So I'm leaning towards something in the "\d" family.
I agree that \d-something makes the most sense from a functionality
standpoint. But I don't want to make the name too long, even if we
do have tab completion to help.
\dconf maybe?
I don't have a strong preference, but just tossing it out there; maybe embrace the novelty of GUC?
\dguc
David J.
On 4/6/22 9:18 PM, David G. Johnston wrote: > On Wed, Apr 6, 2022 at 6:16 PM Tom Lane <tgl@sss.pgh.pa.us > I agree that \d-something makes the most sense from a functionality > standpoint. But I don't want to make the name too long, even if we > do have tab completion to help. > > \dconf maybe? > > > I don't have a strong preference, but just tossing it out there; maybe > embrace the novelty of GUC? > > \dguc +1 for \dconf Jonathan
Attachment
"Jonathan S. Katz" <jkatz@postgresql.org> writes:
> +1 for \dconf
Here's a draft patch using \dconf. No tests or docs yet.
regards, tom lane
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 079f4a1a76..bbf4a5a44e 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -780,7 +780,14 @@ exec_command_d(PsqlScanState scan_state, bool active_branch, const char *cmd)
success = describeTablespaces(pattern, show_verbose);
break;
case 'c':
- success = listConversions(pattern, show_verbose, show_system);
+ if (strncmp(cmd, "dconf", 5) == 0)
+ success = describeConfigurationParameters(pattern,
+ show_verbose,
+ show_system);
+ else
+ success = listConversions(pattern,
+ show_verbose,
+ show_system);
break;
case 'C':
success = listCasts(pattern, show_verbose);
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 4dddf08789..e55cfcb2f3 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -4430,6 +4430,68 @@ listConversions(const char *pattern, bool verbose, bool showSystem)
return true;
}
+/*
+ * \dconf
+ *
+ * Describes configuration parameters.
+ */
+bool
+describeConfigurationParameters(const char *pattern, bool verbose,
+ bool showSystem)
+{
+ PQExpBufferData buf;
+ PGresult *res;
+ printQueryOpt myopt = pset.popt;
+
+ initPQExpBuffer(&buf);
+ printfPQExpBuffer(&buf,
+ "SELECT s.name AS \"%s\", s.setting AS \"%s\", "
+ "s.unit AS \"%s\", s.vartype AS \"%s\"",
+ gettext_noop("Parameter"),
+ gettext_noop("Setting"),
+ gettext_noop("Unit"),
+ gettext_noop("Type"));
+
+ if (verbose)
+ {
+ appendPQExpBuffer(&buf, ", s.context AS \"%s\", ",
+ gettext_noop("Context"));
+ if (pset.sversion >= 150000)
+ printACLColumn(&buf, "p.paracl");
+ else
+ appendPQExpBuffer(&buf, "NULL AS \"%s\"",
+ gettext_noop("Access privileges"));
+ }
+
+ appendPQExpBufferStr(&buf, "\nFROM pg_catalog.pg_settings s\n");
+
+ if (verbose && pset.sversion >= 150000)
+ appendPQExpBufferStr(&buf,
+ " LEFT JOIN pg_catalog.pg_parameter_acl p\n"
+ " ON pg_catalog.lower(s.name) = p.parname\n");
+
+ processSQLNamePattern(pset.db, &buf, pattern,
+ false, false,
+ NULL, "pg_catalog.lower(s.name)", NULL,
+ NULL);
+
+ appendPQExpBufferStr(&buf, "ORDER BY 1;");
+
+ res = PSQLexec(buf.data);
+ termPQExpBuffer(&buf);
+ if (!res)
+ return false;
+
+ myopt.nullPrint = NULL;
+ myopt.title = _("List of configuration parameters");
+ myopt.translate_header = true;
+
+ printQuery(res, &myopt, pset.queryFout, false, pset.logfile);
+
+ PQclear(res);
+ return true;
+}
+
/*
* \dy
*
diff --git a/src/bin/psql/describe.h b/src/bin/psql/describe.h
index fd6079679c..4eb2710e27 100644
--- a/src/bin/psql/describe.h
+++ b/src/bin/psql/describe.h
@@ -76,6 +76,10 @@ extern bool listDomains(const char *pattern, bool verbose, bool showSystem);
/* \dc */
extern bool listConversions(const char *pattern, bool verbose, bool showSystem);
+/* \dconf */
+extern bool describeConfigurationParameters(const char *pattern, bool verbose,
+ bool showSystem);
+
/* \dC */
extern bool listCasts(const char *pattern, bool verbose);
diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index b3971bce64..eff9d1a16e 100644
--- a/src/bin/psql/help.c
+++ b/src/bin/psql/help.c
@@ -166,7 +166,7 @@ slashUsage(unsigned short int pager)
* Use "psql --help=commands | wc" to count correctly. It's okay to count
* the USE_READLINE line even in builds without that.
*/
- output = PageOutput(137, pager ? &(pset.popt.topt) : NULL);
+ output = PageOutput(138, pager ? &(pset.popt.topt) : NULL);
fprintf(output, _("General\n"));
fprintf(output, _(" \\copyright show PostgreSQL usage and distribution terms\n"));
@@ -231,6 +231,7 @@ slashUsage(unsigned short int pager)
fprintf(output, _(" \\dAp[+] [AMPTRN [OPFPTRN]] list support functions of operator families\n"));
fprintf(output, _(" \\db[+] [PATTERN] list tablespaces\n"));
fprintf(output, _(" \\dc[S+] [PATTERN] list conversions\n"));
+ fprintf(output, _(" \\dconf[+] [PATTERN] list configuration parameters\n"));
fprintf(output, _(" \\dC[+] [PATTERN] list casts\n"));
fprintf(output, _(" \\dd[S] [PATTERN] show object descriptions not displayed elsewhere\n"));
fprintf(output, _(" \\dD[S+] [PATTERN] list domains\n"));
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 32d0b4855f..015037d626 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1690,7 +1690,7 @@ psql_completion(const char *text, int start, int end)
"\\connect", "\\conninfo", "\\C", "\\cd", "\\copy",
"\\copyright", "\\crosstabview",
"\\d", "\\da", "\\dA", "\\dAc", "\\dAf", "\\dAo", "\\dAp",
- "\\db", "\\dc", "\\dC", "\\dd", "\\ddp", "\\dD",
+ "\\db", "\\dc", "\\dconf", "\\dC", "\\dd", "\\ddp", "\\dD",
"\\des", "\\det", "\\deu", "\\dew", "\\dE", "\\df",
"\\dF", "\\dFd", "\\dFp", "\\dFt", "\\dg", "\\di", "\\dl", "\\dL",
"\\dm", "\\dn", "\\do", "\\dO", "\\dp", "\\dP", "\\dPi", "\\dPt",
@@ -4539,6 +4539,8 @@ psql_completion(const char *text, int start, int end)
COMPLETE_WITH_QUERY(Query_for_list_of_access_methods);
else if (TailMatchesCS("\\db*"))
COMPLETE_WITH_QUERY(Query_for_list_of_tablespaces);
+ else if (TailMatchesCS("\\dconf*"))
+ COMPLETE_WITH_QUERY(Query_for_list_of_show_vars);
else if (TailMatchesCS("\\dD*"))
COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_domains);
else if (TailMatchesCS("\\des*"))
I also find myself querying pg_settings all too often. More typing than I'd like. On Thu, 7 Apr 2022 at 06:40, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I do agree that \show might be a bad choice, the reason being that > the adjacent \set command is for psql variables not GUCs; if we > had a \show I'd sort of expect it to be a variant spelling of > "\echo :variable". I also think \show is not a great choice. I'd rather see us follow the \d pattern for showing information about objects in the database. > "\sc" isn't awful perhaps. I think \dG is pretty good. G for GUC. David
On 4/6/22 23:02, Tom Lane wrote: > "Jonathan S. Katz" <jkatz@postgresql.org> writes: >> +1 for \dconf > > Here's a draft patch using \dconf. No tests or docs yet. WFM -- using some form of \d<something> makes more sense than \s<something>, and I can't think of anything better that \dconf. I will say that I care about context far more often than unit or type though, so from my point of view I would switch them around with respect to which is only shown with verbose. Joe
On 4/6/22 23:25, David Rowley wrote: > I also find myself querying pg_settings all too often. More typing > than I'd like. > > On Thu, 7 Apr 2022 at 06:40, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> I do agree that \show might be a bad choice, the reason being that >> the adjacent \set command is for psql variables not GUCs; if we >> had a \show I'd sort of expect it to be a variant spelling of >> "\echo :variable". > I also think \show is not a great choice. I'd rather see us follow the > \d pattern for showing information about objects in the database. > >> "\sc" isn't awful perhaps. > I think \dG is pretty good. G for GUC. > -1 on anything that is based on "GUC", an ancient and now largely irrelevant acronym. How many developers, let alone users, know what it stands for? \dconf seems fine to me cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
On 4/7/22 8:36 AM, Joe Conway wrote:
> On 4/6/22 23:02, Tom Lane wrote:
>> "Jonathan S. Katz" <jkatz@postgresql.org> writes:
>>> +1 for \dconf
>>
>> Here's a draft patch using \dconf. No tests or docs yet.
>
> WFM -- using some form of \d<something> makes more sense than
> \s<something>, and I can't think of anything better that \dconf.
>
> I will say that I care about context far more often than unit or type
> though, so from my point of view I would switch them around with respect
> to which is only shown with verbose.
I disagree somewhat -- I agree the context should be in the regular
view, but unit and type are also important. If I had to choose to drop
one, I'd choose type as it could be inferred, but I would say better to
keep them all.
The downside is that by including context, the standard list appears to
push past my 99px width terminal in non-enhanced view, but that may be OK.
A couple of minor things:
+ appendPQExpBufferStr(&buf, "ORDER BY 1;");
I don't know how much we do positional ordering in our queries, but it
may be better to explicitly order by "s.name". I doubt this column name
is likely to change, and if for some reason someone shuffles the output
order of \dconf, it makes it less likely to break someone's view.
I did not test this via an extension, but we do allow for mixed case in
custom GUCs:
postgres=# SHOW jkatz.test;
JKATZ.test
------------
abc
I don't know if we want to throw a "LOWER(s.name)" on at least the
ordering, given we allow for "SHOW" itself to load these case-insensitively.
+ fprintf(output, _(" \\dconf[+] [PATTERN] list configuration
parameters\n"));
Maybe to appeal to all crowds, we say "list configuration parameters
(GUCs)"?
Thanks,
Jonathan
Attachment
"Jonathan S. Katz" <jkatz@postgresql.org> writes:
> On 4/7/22 8:36 AM, Joe Conway wrote:
>> I will say that I care about context far more often than unit or type
>> though, so from my point of view I would switch them around with respect
>> to which is only shown with verbose.
> I disagree somewhat -- I agree the context should be in the regular
> view, but unit and type are also important. If I had to choose to drop
> one, I'd choose type as it could be inferred, but I would say better to
> keep them all.
Given the new ability to grant privileges on GUCs, context alone is not
sufficient to know when something can be set. So the context and the
privileges seem like they should go together, and that's why I have them
both under "+".
I can see the argument for relegating type to the "+" format, in hopes of
keeping the default display narrow enough for ordinary terminal windows.
> A couple of minor things:
> + appendPQExpBufferStr(&buf, "ORDER BY 1;");
> I don't know how much we do positional ordering in our queries, but it
> may be better to explicitly order by "s.name".
"ORDER BY n" seems to be the de facto standard in describe.c. Perhaps
there's an argument for changing it throughout that file, but I don't
think this one function should be out of step with the rest.
> I don't know if we want to throw a "LOWER(s.name)" on at least the
> ordering, given we allow for "SHOW" itself to load these case-insensitively.
Yeah, I went back and forth on that myself --- I was looking at the
example of DateStyle, and noticing that although you see it in mixed
case in the command's output, tab completion isn't happy unless you
enter it in lower case (ie, date<TAB> works, Date<TAB> not so much).
Forcibly lowercasing the command output would fix that inconsistency,
but on the other hand it introduces an inconsistency with what the
pg_settings view shows. Not sure what's the least bad. We might be
able to fix the tab completion behavior, if we don't mind complicating
tab-complete.c even more; so probably that's the thing to look at
before changing the output.
> Maybe to appeal to all crowds, we say "list configuration parameters
> (GUCs)"?
I'm in the camp that says that GUC is not an acronym we wish to expose
to end users.
regards, tom lane
On Thu, Apr 7, 2022 at 7:56 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
"Jonathan S. Katz" <jkatz@postgresql.org> writes:
> Maybe to appeal to all crowds, we say "list configuration parameters
> (GUCs)"?
I'm in the camp that says that GUC is not an acronym we wish to expose
to end users.
I am too. In any case, either go all-in with GUC (i.e., \dG or \dguc) or pretend it doesn't exist - an in-between position is unappealing.
David J.
On Wed, Apr 06, 2022 at 11:02:54PM -0400, Tom Lane wrote:
> "Jonathan S. Katz" <jkatz@postgresql.org> writes:
> > +1 for \dconf
>
> Here's a draft patch using \dconf. No tests or docs yet.
The patch as written is a thin layer around pg_settings.
SHOW and current_setting() translate to human units, which is particularly
useful for some settings, like those with units of 8k pages.
Is it better to use that "cooked" version for display in the backslash command
instead of the raw view from pg_settings ?
Otherwise, I see myself first using tab completion or, failing that,
SELECT * FROM pg_settings WHERE name~'something', followed by SHOW, to
avoid messing up counting digits, multiplication or unit conversions.
> + printfPQExpBuffer(&buf,
> + "SELECT s.name AS \"%s\", s.setting AS \"%s\", "
> + "s.unit AS \"%s\", s.vartype AS \"%s\"",
> + gettext_noop("Parameter"),
> + gettext_noop("Setting"),
> + gettext_noop("Unit"),
> + gettext_noop("Type"));
> +
> + appendPQExpBufferStr(&buf, "\nFROM pg_catalog.pg_settings s\n");
On 4/7/22 11:10 AM, David G. Johnston wrote: > On Thu, Apr 7, 2022 at 7:56 AM Tom Lane <tgl@sss.pgh.pa.us > <mailto:tgl@sss.pgh.pa.us>> wrote: > > "Jonathan S. Katz" <jkatz@postgresql.org > <mailto:jkatz@postgresql.org>> writes: > > > Maybe to appeal to all crowds, we say "list configuration parameters > > (GUCs)"? > > I'm in the camp that says that GUC is not an acronym we wish to expose > to end users. > > > I am too. WFM. Jonathan
Attachment
> On Apr 7, 2022, at 6:21 AM, Andrew Dunstan <andrew@dunslane.net> wrote: > > \dconf seems fine to me We have too many synonyms for configuration parameters. "config", "guc", "parameter", and "setting" are already in use. I thought we agreed on the other thread that "setting" means the value, and "parameter" is the thing being set. It'strue that "config" refers to parameters in the name of pg_catalog.set_config, which is a pretty strong precedent, butsadly "config" also refers to configuration files, the build configuration (as in the "pg_config" tool), text search configuration,etc. While grep'ing through doc/src/sgml, I see no instances of "conf" ever referring to configuration parameters. It only everrefers to configuration files. I'd prefer not adding it to the list of synonyms. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Justin Pryzby <pryzby@telsasoft.com> writes:
> SHOW and current_setting() translate to human units, which is particularly
> useful for some settings, like those with units of 8k pages.
> Is it better to use that "cooked" version for display in the backslash command
> instead of the raw view from pg_settings ?
Oh, that's a good idea --- lets us drop the units column entirely.
The attached revision does that and moves the "type" column to
secondary status, as discussed upthread. I also added docs and
simple regression tests, and fixed two problems that were preventing
completion of custom (qualified) GUC names (we need to use the
VERBATIM option for those queries). There remains the issue that
tab completion for GUC names ought to be case-insensitive, but
that's a pre-existing bug in tab-complete.c's other GUC name
completions too; I'll tackle it later.
As for the name, \dconf has a slight plurality in votes so far,
so I'm sticking with that.
I think this is ready to go unless someone has a significantly
better idea.
regards, tom lane
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 492a960247..4dc6628add 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -2205,7 +2205,7 @@ REVOKE ALL ON accounts FROM PUBLIC;
<entry><literal>PARAMETER</literal></entry>
<entry><literal>sA</literal></entry>
<entry>none</entry>
- <entry>none</entry>
+ <entry><literal>\dconf+</literal></entry>
</row>
<row>
<entry><literal>SCHEMA</literal></entry>
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index f01adb1fd2..864be97808 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1380,6 +1380,23 @@ testdb=>
</varlistentry>
+ <varlistentry>
+ <term><literal>\dconf[+] [ <link linkend="app-psql-patterns"><replaceable
class="parameter">pattern</replaceable></link>]</literal></term>
+ <listitem>
+ <para>
+ Lists server configuration parameters and their values.
+ If <replaceable class="parameter">pattern</replaceable>
+ is specified, only parameters whose names match the pattern are
+ listed.
+ If <literal>+</literal> is appended to the command name, each
+ parameter is listed with its data type, context in which the
+ parameter can be set, and access privileges (if non-default access
+ privileges have been granted).
+ </para>
+ </listitem>
+ </varlistentry>
+
+
<varlistentry>
<term><literal>\dC[+] [ <link linkend="app-psql-patterns"><replaceable
class="parameter">pattern</replaceable></link>]</literal></term>
<listitem>
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 079f4a1a76..bbf4a5a44e 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -780,7 +780,14 @@ exec_command_d(PsqlScanState scan_state, bool active_branch, const char *cmd)
success = describeTablespaces(pattern, show_verbose);
break;
case 'c':
- success = listConversions(pattern, show_verbose, show_system);
+ if (strncmp(cmd, "dconf", 5) == 0)
+ success = describeConfigurationParameters(pattern,
+ show_verbose,
+ show_system);
+ else
+ success = listConversions(pattern,
+ show_verbose,
+ show_system);
break;
case 'C':
success = listCasts(pattern, show_verbose);
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 73bbbe2eb4..47590a1302 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -4434,6 +4434,68 @@ listConversions(const char *pattern, bool verbose, bool showSystem)
return true;
}
+/*
+ * \dconf
+ *
+ * Describes configuration parameters.
+ */
+bool
+describeConfigurationParameters(const char *pattern, bool verbose,
+ bool showSystem)
+{
+ PQExpBufferData buf;
+ PGresult *res;
+ printQueryOpt myopt = pset.popt;
+
+ initPQExpBuffer(&buf);
+ printfPQExpBuffer(&buf,
+ "SELECT s.name AS \"%s\", "
+ "pg_catalog.current_setting(s.name) AS \"%s\"",
+ gettext_noop("Parameter"),
+ gettext_noop("Value"));
+
+ if (verbose)
+ {
+ appendPQExpBuffer(&buf,
+ ", s.vartype AS \"%s\", s.context AS \"%s\", ",
+ gettext_noop("Type"),
+ gettext_noop("Context"));
+ if (pset.sversion >= 150000)
+ printACLColumn(&buf, "p.paracl");
+ else
+ appendPQExpBuffer(&buf, "NULL AS \"%s\"",
+ gettext_noop("Access privileges"));
+ }
+
+ appendPQExpBufferStr(&buf, "\nFROM pg_catalog.pg_settings s\n");
+
+ if (verbose && pset.sversion >= 150000)
+ appendPQExpBufferStr(&buf,
+ " LEFT JOIN pg_catalog.pg_parameter_acl p\n"
+ " ON pg_catalog.lower(s.name) = p.parname\n");
+
+ processSQLNamePattern(pset.db, &buf, pattern,
+ false, false,
+ NULL, "pg_catalog.lower(s.name)", NULL,
+ NULL);
+
+ appendPQExpBufferStr(&buf, "ORDER BY 1;");
+
+ res = PSQLexec(buf.data);
+ termPQExpBuffer(&buf);
+ if (!res)
+ return false;
+
+ myopt.nullPrint = NULL;
+ myopt.title = _("List of configuration parameters");
+ myopt.translate_header = true;
+
+ printQuery(res, &myopt, pset.queryFout, false, pset.logfile);
+
+ PQclear(res);
+ return true;
+}
+
/*
* \dy
*
diff --git a/src/bin/psql/describe.h b/src/bin/psql/describe.h
index fd6079679c..4eb2710e27 100644
--- a/src/bin/psql/describe.h
+++ b/src/bin/psql/describe.h
@@ -76,6 +76,10 @@ extern bool listDomains(const char *pattern, bool verbose, bool showSystem);
/* \dc */
extern bool listConversions(const char *pattern, bool verbose, bool showSystem);
+/* \dconf */
+extern bool describeConfigurationParameters(const char *pattern, bool verbose,
+ bool showSystem);
+
/* \dC */
extern bool listCasts(const char *pattern, bool verbose);
diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index b3971bce64..eff9d1a16e 100644
--- a/src/bin/psql/help.c
+++ b/src/bin/psql/help.c
@@ -166,7 +166,7 @@ slashUsage(unsigned short int pager)
* Use "psql --help=commands | wc" to count correctly. It's okay to count
* the USE_READLINE line even in builds without that.
*/
- output = PageOutput(137, pager ? &(pset.popt.topt) : NULL);
+ output = PageOutput(138, pager ? &(pset.popt.topt) : NULL);
fprintf(output, _("General\n"));
fprintf(output, _(" \\copyright show PostgreSQL usage and distribution terms\n"));
@@ -231,6 +231,7 @@ slashUsage(unsigned short int pager)
fprintf(output, _(" \\dAp[+] [AMPTRN [OPFPTRN]] list support functions of operator families\n"));
fprintf(output, _(" \\db[+] [PATTERN] list tablespaces\n"));
fprintf(output, _(" \\dc[S+] [PATTERN] list conversions\n"));
+ fprintf(output, _(" \\dconf[+] [PATTERN] list configuration parameters\n"));
fprintf(output, _(" \\dC[+] [PATTERN] list casts\n"));
fprintf(output, _(" \\dd[S] [PATTERN] show object descriptions not displayed elsewhere\n"));
fprintf(output, _(" \\dD[S+] [PATTERN] list domains\n"));
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 025d3f71a1..f47c5acd50 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1690,7 +1690,7 @@ psql_completion(const char *text, int start, int end)
"\\connect", "\\conninfo", "\\C", "\\cd", "\\copy",
"\\copyright", "\\crosstabview",
"\\d", "\\da", "\\dA", "\\dAc", "\\dAf", "\\dAo", "\\dAp",
- "\\db", "\\dc", "\\dC", "\\dd", "\\ddp", "\\dD",
+ "\\db", "\\dc", "\\dconf", "\\dC", "\\dd", "\\ddp", "\\dD",
"\\des", "\\det", "\\deu", "\\dew", "\\dE", "\\df",
"\\dF", "\\dFd", "\\dFp", "\\dFt", "\\dg", "\\di", "\\dl", "\\dL",
"\\dm", "\\dn", "\\do", "\\dO", "\\dp", "\\dP", "\\dPi", "\\dPt",
@@ -3790,7 +3790,7 @@ psql_completion(const char *text, int start, int end)
TailMatches("GRANT|REVOKE", MatchAny, MatchAny, "ON", "PARAMETER") ||
TailMatches("REVOKE", "GRANT", "OPTION", "FOR", MatchAny, "ON", "PARAMETER") ||
TailMatches("REVOKE", "GRANT", "OPTION", "FOR", MatchAny, MatchAny, "ON", "PARAMETER"))
- COMPLETE_WITH_QUERY(Query_for_list_of_alter_system_set_vars);
+ COMPLETE_WITH_QUERY_VERBATIM(Query_for_list_of_alter_system_set_vars);
else if (TailMatches("GRANT", MatchAny, "ON", "PARAMETER", MatchAny) ||
TailMatches("GRANT", MatchAny, MatchAny, "ON", "PARAMETER", MatchAny))
@@ -4542,6 +4542,8 @@ psql_completion(const char *text, int start, int end)
COMPLETE_WITH_QUERY(Query_for_list_of_access_methods);
else if (TailMatchesCS("\\db*"))
COMPLETE_WITH_QUERY(Query_for_list_of_tablespaces);
+ else if (TailMatchesCS("\\dconf*"))
+ COMPLETE_WITH_QUERY_VERBATIM(Query_for_list_of_show_vars);
else if (TailMatchesCS("\\dD*"))
COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_domains);
else if (TailMatchesCS("\\des*"))
diff --git a/src/fe_utils/string_utils.c b/src/fe_utils/string_utils.c
index bca50ec6de..1c61840462 100644
--- a/src/fe_utils/string_utils.c
+++ b/src/fe_utils/string_utils.c
@@ -918,8 +918,12 @@ processSQLNamePattern(PGconn *conn, PQExpBuffer buf, const char *pattern,
* Convert shell-style 'pattern' into the regular expression(s) we want to
* execute. Quoting/escaping into SQL literal format will be done below
* using appendStringLiteralConn().
+ *
+ * If the caller provided a schemavar, we want to split the pattern on
+ * ".", otherwise not.
*/
- patternToSQLRegex(PQclientEncoding(conn), NULL, &schemabuf, &namebuf,
+ patternToSQLRegex(PQclientEncoding(conn), NULL,
+ (schemavar ? &schemabuf : NULL), &namebuf,
pattern, force_escape);
/*
diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out
index 185c505312..79bdf78971 100644
--- a/src/test/regress/expected/psql.out
+++ b/src/test/regress/expected/psql.out
@@ -5090,6 +5090,22 @@ List of access methods
hash | uuid_ops | uuid | uuid | 2 | uuid_hash_extended
(5 rows)
+-- check \dconf
+\dconf enable_seq*
+List of configuration parameters
+ Parameter | Value
+----------------+-------
+ enable_seqscan | on
+(1 row)
+
+\dconf+ enable_index*
+ List of configuration parameters
+ Parameter | Value | Type | Context | Access privileges
+----------------------+-------+------+---------+-------------------
+ enable_indexonlyscan | on | bool | user |
+ enable_indexscan | on | bool | user |
+(2 rows)
+
-- check \df, \do with argument specifications
\df *sqrt
List of functions
diff --git a/src/test/regress/sql/psql.sql b/src/test/regress/sql/psql.sql
index 8f49a5f347..0ea00cf8db 100644
--- a/src/test/regress/sql/psql.sql
+++ b/src/test/regress/sql/psql.sql
@@ -1241,6 +1241,10 @@ drop role regress_partitioning_role;
\dAp+ btree float_ops
\dAp * pg_catalog.uuid_ops
+-- check \dconf
+\dconf enable_seq*
+\dconf+ enable_index*
+
-- check \df, \do with argument specifications
\df *sqrt
\df *sqrt num*
Mark Dilger <mark.dilger@enterprisedb.com> writes:
> We have too many synonyms for configuration parameters. "config", "guc", "parameter", and "setting" are already in
use. I thought we agreed on the other thread that "setting" means the value, and "parameter" is the thing being set.
Right, so the suggestion of \dsetting seems a tad off-kilter.
> It's true that "config" refers to parameters in the name of pg_catalog.set_config, which is a pretty strong
precedent,but sadly "config" also refers to configuration files, the build configuration (as in the "pg_config" tool),
textsearch configuration, etc.
I'd also thought briefly about \dpar or \dparam, but I'm not sure that
that's much of an improvement. \dconf is at least in line with the
docs' terminology of "configuration parameter". (Note that bare
"parameter" has other meanings too, eg function parameter.) I wouldn't
fight too hard if people want to lengthen it to \dconfig for consistency
with set_config().
regards, tom lane
On 4/7/22 12:22 PM, Tom Lane wrote: > Justin Pryzby <pryzby@telsasoft.com> writes: >> SHOW and current_setting() translate to human units, which is particularly >> useful for some settings, like those with units of 8k pages. >> Is it better to use that "cooked" version for display in the backslash command >> instead of the raw view from pg_settings ? > > Oh, that's a good idea --- lets us drop the units column entirely. +1 > The attached revision does that and moves the "type" column to > secondary status, as discussed upthread. I also added docs and > simple regression tests, and fixed two problems that were preventing > completion of custom (qualified) GUC names (we need to use the > VERBATIM option for those queries). There remains the issue that > tab completion for GUC names ought to be case-insensitive, but > that's a pre-existing bug in tab-complete.c's other GUC name > completions too; I'll tackle it later. > > As for the name, \dconf has a slight plurality in votes so far, > so I'm sticking with that. > > I think this is ready to go unless someone has a significantly > better idea. I ran the equivalent SQL locally and it LGTM. Docs read well to me. Code looks as good as it can to me. +1 Jonathan
Attachment
> On Apr 7, 2022, at 9:29 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > I wouldn't > fight too hard if people want to lengthen it to \dconfig for consistency > with set_config(). I'd prefer \dconfig, but if the majority on this list view that as pedantically forcing them to type more, I'm not goingto kick up a fuss about \dconf. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Mark Dilger <mark.dilger@enterprisedb.com> writes:
>> On Apr 7, 2022, at 9:29 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I wouldn't
>> fight too hard if people want to lengthen it to \dconfig for consistency
>> with set_config().
> I'd prefer \dconfig, but if the majority on this list view that as pedantically forcing them to type more, I'm not
goingto kick up a fuss about \dconf.
Maybe I'm atypical, but I'm probably going to use tab completion
either way, so it's not really more keystrokes. The consistency
point is a good one that I'd not considered before.
regards, tom lane
> On Apr 7, 2022, at 9:37 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Maybe I'm atypical, but I'm probably going to use tab completion > either way, so it's not really more keystrokes. Same here, because after tab-completing \dcon\t\t into \dconfig, I'm likely to also tab-complete to get the list of parameters. I frequently can't recall the exact spelling of them. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 4/7/22 12:42 PM, Mark Dilger wrote:
>
>
>> On Apr 7, 2022, at 9:37 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>
>> Maybe I'm atypical, but I'm probably going to use tab completion
>> either way, so it's not really more keystrokes.
>
> Same here, because after tab-completing \dcon\t\t into \dconfig, I'm likely to also tab-complete to get the list of
parameters. I frequently can't recall the exact spelling of them.
This seems like the only "\d" command that would require tab completion,
given all the others are far less descriptive (\dt, \dv, etc.) And at
least from my user perspective, if I ever need anything other than \dt,
I typically have to go to \? to look it up.
I'm generally in favor of consistency, though in case skewing towards
what we expose to the user. If "\dconfig" gives a bit more across the
board, I'm OK with that. "\dparam" could be a bit confusing to end users
("parameter for what?") so I'm -1 on that.
Jonathan
Attachment
On 4/7/22 12:37, Tom Lane wrote: > Mark Dilger <mark.dilger@enterprisedb.com> writes: >>> On Apr 7, 2022, at 9:29 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> I wouldn't >>> fight too hard if people want to lengthen it to \dconfig for consistency >>> with set_config(). > >> I'd prefer \dconfig, but if the majority on this list view that as pedantically forcing them to type more, I'm not goingto kick up a fuss about \dconf. > > Maybe I'm atypical, but I'm probably going to use tab completion > either way, so it's not really more keystrokes. The consistency > point is a good one that I'd not considered before. Yeah I had thought about \dconfig too -- +1 to that, although I am fine with \dconf too. Joe
On Thu, Apr 7, 2022 at 9:58 AM Joe Conway <mail@joeconway.com> wrote:
On 4/7/22 12:37, Tom Lane wrote:
> Mark Dilger <mark.dilger@enterprisedb.com> writes:
>>> On Apr 7, 2022, at 9:29 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> I wouldn't
>>> fight too hard if people want to lengthen it to \dconfig for consistency
>>> with set_config().
>
>> I'd prefer \dconfig, but if the majority on this list view that as pedantically forcing them to type more, I'm not going to kick up a fuss about \dconf.
>
> Maybe I'm atypical, but I'm probably going to use tab completion
> either way, so it's not really more keystrokes. The consistency
> point is a good one that I'd not considered before.
Yeah I had thought about \dconfig too -- +1 to that, although I am fine
with \dconf too.
\dconfig[+] gets my vote. I was going to say "conf" just isn't common jargon to say or write; but the one place it is - file extensions - is relevant and common. But still, I would go with the non-jargon form.
David J.
čt 7. 4. 2022 v 19:04 odesílatel David G. Johnston <david.g.johnston@gmail.com> napsal:
On Thu, Apr 7, 2022 at 9:58 AM Joe Conway <mail@joeconway.com> wrote:On 4/7/22 12:37, Tom Lane wrote:
> Mark Dilger <mark.dilger@enterprisedb.com> writes:
>>> On Apr 7, 2022, at 9:29 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> I wouldn't
>>> fight too hard if people want to lengthen it to \dconfig for consistency
>>> with set_config().
>
>> I'd prefer \dconfig, but if the majority on this list view that as pedantically forcing them to type more, I'm not going to kick up a fuss about \dconf.
>
> Maybe I'm atypical, but I'm probably going to use tab completion
> either way, so it's not really more keystrokes. The consistency
> point is a good one that I'd not considered before.
Yeah I had thought about \dconfig too -- +1 to that, although I am fine
with \dconf too.\dconfig[+] gets my vote. I was going to say "conf" just isn't common jargon to say or write; but the one place it is - file extensions - is relevant and common. But still, I would go with the non-jargon form.
dconfig is better, because google can be confused - dconf is known project https://en.wikipedia.org/wiki/Dconf
The length is not too important when we have tab complete
Regards
Pavel
David J.
Pavel Stehule <pavel.stehule@gmail.com> writes: > čt 7. 4. 2022 v 19:04 odesílatel David G. Johnston < > david.g.johnston@gmail.com> napsal: >> \dconfig[+] gets my vote. I was going to say "conf" just isn't common >> jargon to say or write; but the one place it is - file extensions - is >> relevant and common. But still, I would go with the non-jargon form. > dconfig is better, because google can be confused - dconf is known project > https://en.wikipedia.org/wiki/Dconf Looks like the consensus has shifted to \dconfig. I'll do it like that. regards, tom lane
Re: Tom Lane > Looks like the consensus has shifted to \dconfig. I'll do it like that. A bit late to the party, but two more ideas: The name has evolved from \dcp over various longer \d-things to the more verbose \dconfig. How about we evolve it even more and just call it \config? That would be much easier to remember - in fact after I seeing the patch the other day, I wanted to try it today and I was confused when \config didn't work, and had to read the git log to see how it's actually called. It also doesn't conflict with tab completion too much, \conf<tab> would work. The other bit is hiding non-default values. "\dconfig" by itself is very long and not very interesting. I have this in my .psqlrc that I use very often on servers I'm visiting: \set config 'SELECT name, current_setting(name), CASE source WHEN $$configuration file$$ THEN regexp_replace(sourcefile,$$^/.*/$$, $$$$)||$$:$$||sourceline ELSE source END FROM pg_settings WHERE source <> $$default$$;' I would think that if \dconfig showed the non-default settings only, it would be much more useful; the full list would still be available with "\dconfig *". This is in line with \dt only showing tables on the search_path, and "\dt *.*" showing all. Christoph
Christoph Berg <myon@debian.org> writes:
> The name has evolved from \dcp over various longer \d-things to the
> more verbose \dconfig. How about we evolve it even more and just call
> it \config?
I think people felt that it should be part of the \d family.
Also, because we have \connect and \conninfo, you'd need to
type at least five characters before you could tab-complete,
whereas \dconfig is unique at four (you just need \dco).
> I would think that if \dconfig showed the non-default settings only,
> it would be much more useful; the full list would still be available
> with "\dconfig *". This is in line with \dt only showing tables on the
> search_path, and "\dt *.*" showing all.
Hm, I could get on board with that -- any other opinions?
(Perhaps there's an argument for omitting "override"
settings as well?)
regards, tom lane
On Sat, Apr 09, 2022 at 10:31:17AM -0400, Tom Lane wrote: > Christoph Berg <myon@debian.org> writes: > > > I would think that if \dconfig showed the non-default settings only, > > it would be much more useful; the full list would still be available > > with "\dconfig *". This is in line with \dt only showing tables on the > > search_path, and "\dt *.*" showing all. > > Hm, I could get on board with that -- any other opinions? +1 for it, that's often what I'm interested in when looking at the GUCs in general. > (Perhaps there's an argument for omitting "override" > settings as well?) -0.1. Most are usually not useful, but I can see at least data_checksums and wal_buffers that are still interesting.
On 4/9/22 11:58 AM, Julien Rouhaud wrote: > On Sat, Apr 09, 2022 at 10:31:17AM -0400, Tom Lane wrote: >> Christoph Berg <myon@debian.org> writes: >> >>> I would think that if \dconfig showed the non-default settings only, >>> it would be much more useful; the full list would still be available >>> with "\dconfig *". This is in line with \dt only showing tables on the >>> search_path, and "\dt *.*" showing all. >> >> Hm, I could get on board with that -- any other opinions? > > +1 for it, that's often what I'm interested in when looking at the GUCs in > general. -1, at least for the moment. Sometimes a user doesn't know what they're looking for coupled with being unaware of what the default value is. If a setting is set to a default value and that value is the problematic setting, a user should be able to see that even in a full list. (The \dt searching only tables "search_path" vs. the database has also bitten me too. I did ultimately learn about "\dt *.*", but this makes the user have to unpack more layers to do simple things). Jonathan
Attachment
"Jonathan S. Katz" <jkatz@postgresql.org> writes:
> -1, at least for the moment. Sometimes a user doesn't know what they're
> looking for coupled with being unaware of what the default value is. If
> a setting is set to a default value and that value is the problematic
> setting, a user should be able to see that even in a full list.
Sure, but then you do "\dconfig *". With there being several hundred
GUCs (and no doubt more coming), I'm not sure that "show me every GUC"
is a common use-case at all, let alone so common as to deserve being
the default behavior.
One thing we could perhaps do to reduce confusion is to change the
table heading when doing this, say from "List of configuration parameters"
to "List of non-default configuration parameters".
regards, tom lane
On Sat, Apr 9, 2022 at 9:27 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
"Jonathan S. Katz" <jkatz@postgresql.org> writes:
> -1, at least for the moment. Sometimes a user doesn't know what they're
> looking for coupled with being unaware of what the default value is. If
> a setting is set to a default value and that value is the problematic
> setting, a user should be able to see that even in a full list.
Sure, but then you do "\dconfig *". With there being several hundred
GUCs (and no doubt more coming), I'm not sure that "show me every GUC"
is a common use-case at all, let alone so common as to deserve being
the default behavior.
I'm for having a default that doesn't mean "show everything".
I'm also wondering whether we can invent GUC namespaces for the different contexts, so I can use a pattern like: context.internal.*
A similar ability for category would be nice but we'd have to invent labels to make it practical.
\dconfig [pattern [mode]]
mode: all, overridden
So mode is overridden if pattern is absent but all if pattern is present, with the ability to specify overridden.
pattern: [[{context.{context label}}|{category.{category label}}.]...]{parameter name pattern}
parameter name pattern: [{two part name prefix}.]{base parameter pattern}
One thing we could perhaps do to reduce confusion is to change the
table heading when doing this, say from "List of configuration parameters"
to "List of non-default configuration parameters".
I'd be inclined to echo a note after the output table that says that not all configuration parameters are displayed - possibly even providing a count [all - overridden]. The header is likely to be ignored even if it still ends up on screen after scrolling.
David J.
On Sat, Apr 9, 2022 at 10:31 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Christoph Berg <myon@debian.org> writes: > > The name has evolved from \dcp over various longer \d-things to the > > more verbose \dconfig. How about we evolve it even more and just call > > it \config? > > I think people felt that it should be part of the \d family. > Also, because we have \connect and \conninfo, you'd need to > type at least five characters before you could tab-complete, > whereas \dconfig is unique at four (you just need \dco). Regarding this point, I kind of think that \dconfig is a break with established precedent, but in a good way. Previous additions have generally tried to pick some vaguely mnemonic sequence of letters that somehow corresponds to what's being listed, but you have to be pretty hard-core to remember what \db and \dAc and \drds do. \dconfig is probably easier to remember. -- Robert Haas EDB: http://www.enterprisedb.com
On 4/9/22 16:31, Tom Lane wrote: > Christoph Berg <myon@debian.org> writes: > >> I would think that if \dconfig showed the non-default settings only, >> it would be much more useful; the full list would still be available >> with "\dconfig *". This is in line with \dt only showing tables on the >> search_path, and "\dt *.*" showing all. > > Hm, I could get on board with that -- any other opinions? +1 -- Vik Fearing
On 4/9/22 12:27 PM, Tom Lane wrote: > "Jonathan S. Katz" <jkatz@postgresql.org> writes: >> -1, at least for the moment. Sometimes a user doesn't know what they're >> looking for coupled with being unaware of what the default value is. If >> a setting is set to a default value and that value is the problematic >> setting, a user should be able to see that even in a full list. > > Sure, but then you do "\dconfig *". With there being several hundred > GUCs (and no doubt more coming), I'm not sure that "show me every GUC" > is a common use-case at all, let alone so common as to deserve being > the default behavior. > > One thing we could perhaps do to reduce confusion is to change the > table heading when doing this, say from "List of configuration parameters" > to "List of non-default configuration parameters". Reasonable points. I don't have any objections to this proposal. Jonathan
Attachment
"Jonathan S. Katz" <jkatz@postgresql.org> writes:
> On 4/9/22 12:27 PM, Tom Lane wrote:
>> Sure, but then you do "\dconfig *". With there being several hundred
>> GUCs (and no doubt more coming), I'm not sure that "show me every GUC"
>> is a common use-case at all, let alone so common as to deserve being
>> the default behavior.
>>
>> One thing we could perhaps do to reduce confusion is to change the
>> table heading when doing this, say from "List of configuration parameters"
>> to "List of non-default configuration parameters".
> Reasonable points. I don't have any objections to this proposal.
Hearing no further comments, done like that.
regards, tom lane
On 4/11/22 3:12 PM, Tom Lane wrote: > "Jonathan S. Katz" <jkatz@postgresql.org> writes: >> On 4/9/22 12:27 PM, Tom Lane wrote: >>> Sure, but then you do "\dconfig *". With there being several hundred >>> GUCs (and no doubt more coming), I'm not sure that "show me every GUC" >>> is a common use-case at all, let alone so common as to deserve being >>> the default behavior. >>> >>> One thing we could perhaps do to reduce confusion is to change the >>> table heading when doing this, say from "List of configuration parameters" >>> to "List of non-default configuration parameters". > >> Reasonable points. I don't have any objections to this proposal. > > Hearing no further comments, done like that. Thanks! I have a usability comment based on my testing. I ran "\dconfig" and one of the settings that came up was "max_connections" which was set to 100, or the default. I had noticed that "max_connections" was uncommented in my postgresql.conf file, which I believe was from the autogenerated provided by initdb. Similarly, min_wal_size/max_wal_size were in the list and set to their default values. These were also uncommented in my postgresql.conf from the autogeneration. My question is if we're only going to list out the settings that are customized, are we going to: 1. Hide a setting if it matches a default value, even if a user set it to be the default value? OR 2. Comment out all of the settings in a generated postgresql.conf file? Thanks, Jonathan
Attachment
"Jonathan S. Katz" <jkatz@postgresql.org> writes:
> My question is if we're only going to list out the settings that are
> customized, are we going to:
> 1. Hide a setting if it matches a default value, even if a user set it
> to be the default value? OR
> 2. Comment out all of the settings in a generated postgresql.conf file?
As committed, it prints anything that's shown as "source != 'default'"
in pg_settings, which means anything for which the value wasn't
taken from the wired-in default. I suppose an alternative definition
could be "setting != boot_val". Not really sure if that's better.
This idea does somewhat address my unhappiness upthread about printing
values with source = 'internal', but I see that it gets confused by
some GUCs with custom show hooks, like unix_socket_permissions.
Maybe it needs to be "source != 'default' AND setting != boot_val"?
regards, tom lane
Plus maybe making initdb not set values to their default if the auto probing ends up at that values.
Christoph
Christoph
Christoph Berg <cb@df7cb.de> writes:
> Plus maybe making initdb not set values to their default if the auto probing ends up at that values.
Seems a bit fragile: we'd have to make sure that initdb knew what the
boot_val is. IIRC, some of those are not necessarily immutable constants,
so there'd be room for error.
regards, tom lane
On 4/11/22 4:11 PM, Tom Lane wrote: > "Jonathan S. Katz" <jkatz@postgresql.org> writes: >> My question is if we're only going to list out the settings that are >> customized, are we going to: > >> 1. Hide a setting if it matches a default value, even if a user set it >> to be the default value? OR >> 2. Comment out all of the settings in a generated postgresql.conf file? > > As committed, it prints anything that's shown as "source != 'default'" > in pg_settings, which means anything for which the value wasn't > taken from the wired-in default. I suppose an alternative definition > could be "setting != boot_val". Not really sure if that's better. > > This idea does somewhat address my unhappiness upthread about printing > values with source = 'internal', but I see that it gets confused by > some GUCs with custom show hooks, like unix_socket_permissions. > Maybe it needs to be "source != 'default' AND setting != boot_val"? Running through a few GUCs, that seems reasonable. Happy to test the patch out prior to commit to see if it renders better. Jonathan
Attachment
"Jonathan S. Katz" <jkatz@postgresql.org> writes:
> On 4/11/22 4:11 PM, Tom Lane wrote:
>> This idea does somewhat address my unhappiness upthread about printing
>> values with source = 'internal', but I see that it gets confused by
>> some GUCs with custom show hooks, like unix_socket_permissions.
>> Maybe it needs to be "source != 'default' AND setting != boot_val"?
> Running through a few GUCs, that seems reasonable. Happy to test the
> patch out prior to commit to see if it renders better.
It'd just look like this, I think. I see from looking at guc.c that
boot_val can be NULL, so we'd better use IS DISTINCT FROM.
regards, tom lane
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index e7377d4583..17790bd8a4 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -4410,7 +4410,8 @@ describeConfigurationParameters(const char *pattern, bool verbose,
NULL, "pg_catalog.lower(s.name)", NULL,
NULL);
else
- appendPQExpBufferStr(&buf, "WHERE s.source <> 'default'\n");
+ appendPQExpBufferStr(&buf, "WHERE s.source <> 'default' AND\n"
+ " s.setting IS DISTINCT FROM s.boot_val\n");
appendPQExpBufferStr(&buf, "ORDER BY 1;");
On 4/12/22 11:19 AM, Tom Lane wrote: > "Jonathan S. Katz" <jkatz@postgresql.org> writes: >> On 4/11/22 4:11 PM, Tom Lane wrote: >>> This idea does somewhat address my unhappiness upthread about printing >>> values with source = 'internal', but I see that it gets confused by >>> some GUCs with custom show hooks, like unix_socket_permissions. >>> Maybe it needs to be "source != 'default' AND setting != boot_val"? > >> Running through a few GUCs, that seems reasonable. Happy to test the >> patch out prior to commit to see if it renders better. > > It'd just look like this, I think. I see from looking at guc.c that > boot_val can be NULL, so we'd better use IS DISTINCT FROM. (IS DISTINCT FROM is pretty handy :) I tested it and I like this a lot better, at least it's much more consolidated. They all seem to be generated (directories, timezones, collations/encodings). The one exception to this seems to be "max_stack_depth", which is rendering on my "\dconfig" though I didn't change it, an it's showing it's default value of 2MB. "boot_val" says 100, "reset_val" says 2048, and it's commented out in my postgresql.conf. Do we want to align that? That said, the patch itself LGTM. Jonathan
Attachment
"Jonathan S. Katz" <jkatz@postgresql.org> writes:
> On 4/12/22 11:19 AM, Tom Lane wrote:
>> It'd just look like this, I think. I see from looking at guc.c that
>> boot_val can be NULL, so we'd better use IS DISTINCT FROM.
> I tested it and I like this a lot better, at least it's much more
> consolidated. They all seem to be generated (directories, timezones,
> collations/encodings).
Yeah, most of what shows up in a minimally-configured installation is
postmaster-computed settings like config_file, rather than things
that were actually set by the DBA. Personally I'd rather hide the
ones that have source = 'override', but that didn't seem to be the
consensus.
> The one exception to this seems to be "max_stack_depth", which is
> rendering on my "\dconfig" though I didn't change it, an it's showing
> it's default value of 2MB. "boot_val" says 100, "reset_val" says 2048,
> and it's commented out in my postgresql.conf. Do we want to align that?
I don't think there's any principled thing we could do about that in
psql. The boot_val is a conservatively small 100kB, but we crank
that up automatically based on getrlimit(RLIMIT_STACK), so on any
reasonable platform it's going to show as not being default.
regards, tom lane
On 4/12/22 1:00 PM, Tom Lane wrote: > "Jonathan S. Katz" <jkatz@postgresql.org> writes: >> On 4/12/22 11:19 AM, Tom Lane wrote: >>> It'd just look like this, I think. I see from looking at guc.c that >>> boot_val can be NULL, so we'd better use IS DISTINCT FROM. > >> I tested it and I like this a lot better, at least it's much more >> consolidated. They all seem to be generated (directories, timezones, >> collations/encodings). > > Yeah, most of what shows up in a minimally-configured installation is > postmaster-computed settings like config_file, rather than things > that were actually set by the DBA. Personally I'd rather hide the > ones that have source = 'override', but that didn't seem to be the > consensus. The list seems more reasonable now, though now that I'm fully in the "less is more" camp based on the "non-defaults" description, I think anything we can do to further prune is good. >> The one exception to this seems to be "max_stack_depth", which is >> rendering on my "\dconfig" though I didn't change it, an it's showing >> it's default value of 2MB. "boot_val" says 100, "reset_val" says 2048, >> and it's commented out in my postgresql.conf. Do we want to align that? > > I don't think there's any principled thing we could do about that in > psql. The boot_val is a conservatively small 100kB, but we crank > that up automatically based on getrlimit(RLIMIT_STACK), so on any > reasonable platform it's going to show as not being default. Got it. We may be at a point where it's "good enough" and let more people chime in during beta. Thanks, Jonathan
Attachment
"Jonathan S. Katz" <jkatz@postgresql.org> writes:
> On 4/12/22 1:00 PM, Tom Lane wrote:
>> Yeah, most of what shows up in a minimally-configured installation is
>> postmaster-computed settings like config_file, rather than things
>> that were actually set by the DBA. Personally I'd rather hide the
>> ones that have source = 'override', but that didn't seem to be the
>> consensus.
> The list seems more reasonable now, though now that I'm fully in the
> "less is more" camp based on the "non-defaults" description, I think
> anything we can do to further prune is good.
Hearing no further comments, I pushed this version. There didn't seem
to be a need to adjust the docs.
> We may be at a point where it's "good enough" and let more people chime
> in during beta.
Right, lots of time yet for bikeshedding in beta.
regards, tom lane
On Tue, Apr 12, 2022 at 11:19:40AM -0400, Tom Lane wrote: > "Jonathan S. Katz" <jkatz@postgresql.org> writes: > > On 4/11/22 4:11 PM, Tom Lane wrote: > >> This idea does somewhat address my unhappiness upthread about printing > >> values with source = 'internal', but I see that it gets confused by > >> some GUCs with custom show hooks, like unix_socket_permissions. > >> Maybe it needs to be "source != 'default' AND setting != boot_val"? > > > Running through a few GUCs, that seems reasonable. Happy to test the > > patch out prior to commit to see if it renders better. > > It'd just look like this, I think. I see from looking at guc.c that > boot_val can be NULL, so we'd better use IS DISTINCT FROM. I noticed this is showing "pre-computed" gucs, like: shared_memory_size | 149MB shared_memory_size_in_huge_pages | 75 I'm not opposed to that, but I wonder if that's what's intended / best. -- Justin
Justin Pryzby <pryzby@telsasoft.com> writes:
> I noticed this is showing "pre-computed" gucs, like:
> shared_memory_size | 149MB
> shared_memory_size_in_huge_pages | 75
> I'm not opposed to that, but I wonder if that's what's intended / best.
I had suggested upthread that we might want to hide items with
source = 'override', but that idea didn't seem to be getting traction.
A different idea is to hide items with context = 'internal'.
Looking at the items selected by the current rule in a default
installation:
postgres=# SELECT s.name, source, context FROM pg_catalog.pg_settings s
WHERE s.source <> 'default' AND
s.setting IS DISTINCT FROM s.boot_val
ORDER BY 1;
name | source | context
----------------------------------+----------------------+------------
TimeZone | configuration file | user
application_name | client | user
client_encoding | client | user
config_file | override | postmaster
data_directory | override | postmaster
default_text_search_config | configuration file | user
hba_file | override | postmaster
ident_file | override | postmaster
lc_messages | configuration file | superuser
log_timezone | configuration file | sighup
max_stack_depth | environment variable | superuser
shared_memory_size | override | internal
shared_memory_size_in_huge_pages | override | internal
wal_buffers | override | postmaster
(14 rows)
So hiding internal-context items would hit exactly the two you mention,
but hiding override-source items would hit several more.
(I'm kind of wondering why wal_buffers is showing as "override";
that seems like a quirk.)
Thoughts?
regards, tom lane
On Mon, Jun 6, 2022 at 12:02 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Thoughts? This all seems pretty subjective. As someone who sometimes supports customers, I usually kind of want the customer to give me all the settings, just in case something that didn't seem important to them (or to whoever coded up the \dconfig command) turns out to be relevant. It's easier to ask once and then look for the information you need than to go back and forth asking for more data, and I don't want to have to try to infer things based on what version they are running or how a certain set of binaries was built. But if I already know that the configuration on a given system is basically sane, I probably only care about the parameters with non-default values, and a computed parameter can't be set, so I guess it has a default value by definition. So if the charter for this command is to show only non-default GUCs, then it seems reasonable to leave these out. I think part of the problem here, though, is that one can imagine a variety of charters that might be useful. A user could want to see the parameters that have values in their session that differ from the system defaults, or parameters that have values which differ from the compiled-in defaults, or parameters that can be changed without a restart, or all the pre-computed parameters, or all the parameters that contain "vacuum" in the name, or all the query-planner-related parameters, or all the parameters on which any privileges have been granted. And it's just a judgement call which of those things we ought to try to accommodate in the psql syntax and which ones ought to be done by writing an ad-hoc query against pg_settings. -- Robert Haas EDB: http://www.enterprisedb.com
> On Jun 6, 2022, at 9:02 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Thoughts? I think it depends on your mental model of what \dconfig is showing you. Is it showing you the list of configs that youcan SET, or just the list of all configs?
Robert Haas <robertmhaas@gmail.com> writes:
> I think part of the problem here, though, is that one can imagine a
> variety of charters that might be useful. A user could want to see the
> parameters that have values in their session that differ from the
> system defaults, or parameters that have values which differ from the
> compiled-in defaults, or parameters that can be changed without a
> restart, or all the pre-computed parameters, or all the parameters
> that contain "vacuum" in the name, or all the query-planner-related
> parameters, or all the parameters on which any privileges have been
> granted. And it's just a judgement call which of those things we ought
> to try to accommodate in the psql syntax and which ones ought to be
> done by writing an ad-hoc query against pg_settings.
Sure. Nonetheless, having decided to introduce this command, we have
to make that judgement call.
psql-ref.sgml currently explains that
If <replaceable class="parameter">pattern</replaceable> is specified,
only parameters whose names match the pattern are listed. Without
a <replaceable class="parameter">pattern</replaceable>, only
parameters that are set to non-default values are listed.
(Use <literal>\dconfig *</literal> to see all parameters.)
so we have the "all of them" and "ones whose names match a pattern"
cases covered. And the definition of the default behavior as
"only ones that are set to non-default values" seems reasonable enough,
but the question is what counts as a "non-default value", or for that
matter what counts as "setting".
I think a reasonable case can be made for excluding "internal" GUCs
on the grounds that (a) they cannot be set, and therefore (b) whatever
value they have might as well be considered the default.
regards, tom lane
On Mon, Jun 6, 2022 at 5:02 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > I think a reasonable case can be made for excluding "internal" GUCs > on the grounds that (a) they cannot be set, and therefore (b) whatever > value they have might as well be considered the default. I agree. -- Robert Haas EDB: http://www.enterprisedb.com
On 6/7/22 10:26 AM, Robert Haas wrote: > On Mon, Jun 6, 2022 at 5:02 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: >> I think a reasonable case can be made for excluding "internal" GUCs >> on the grounds that (a) they cannot be set, and therefore (b) whatever >> value they have might as well be considered the default. > > I agree. I think some of these could be interesting if they deviate from the default (e.g. "in_hot_standby") as it will give the user context on the current state of the system. However, something like that is still fairly easy to determine (e.g. `pg_catalog.pg_is_in_recovery()`). And looking through the settings marked "internal" showing the non-defaults may not provide much additional context to a user. +1 for excluding them. Jonathan
Attachment
Re: Jonathan S. Katz > On 6/7/22 10:26 AM, Robert Haas wrote: > I think some of these could be interesting if they deviate from the default > (e.g. "in_hot_standby") as it will give the user context on the current > state of the system. > > However, something like that is still fairly easy to determine (e.g. > `pg_catalog.pg_is_in_recovery()`). And looking through the settings marked > "internal" showing the non-defaults may not provide much additional context > to a user. in_hot_standby sounds very useful to have in that list. Christoph
Christoph Berg <myon@debian.org> writes:
> in_hot_standby sounds very useful to have in that list.
I thought about this some more and concluded that we're blaming the
messenger. There's nothing wrong with \dconfig's filtering rule;
rather, it's the fault of the backend for mislabeling a lot of
run-time-computed values as source=PGC_S_OVERRIDE when they really
are dynamic defaults. Now in fairness, I think a lot of that code
predates the invention of PGC_S_DYNAMIC_DEFAULT, so there was not
a better way when it was written ... but there is now.
The attached draft patch makes the following changes:
* All run-time calculations of PGC_INTERNAL variables are relabeled
as PGC_S_DYNAMIC_DEFAULT. There is not any higher source value
that we'd allow to set such a variable, so this is sufficient.
(I didn't do anything about in_hot_standby, which is set through
a hack rather than via set_config_option; not sure whether we want
to do anything there, or what it should be if we do.) The net
effect of this compared to upthread examples is to hide
shared_memory_size and shared_memory_size_in_huge_pages from \dconfig.
* The auto-tune value of wal_buffers is relabeled as PGC_S_DYNAMIC_DEFAULT
if possible (but due to pre-existing hacks, we might have to force it).
This will hide that one too as long as you didn't manually set it.
* The rlimit-derived value of max_stack_depth is likewise relabeled
as PGC_S_DYNAMIC_DEFAULT, resolving the complaint Jonathan had upthread.
But now that we have a way to hide this, I'm having second thoughts
about whether we should. If you are on a platform that's forcing an
unreasonably small stack size, it'd be good if \dconfig told you so.
Could it be sane to label that value as PGC_S_DYNAMIC_DEFAULT only when
it's the limit value (2MB) and PGC_S_ENV_VAR when it's smaller?
In any case, I expect that we'd apply this patch only to HEAD, which
means that when using psql's \dconfig against a pre-v15 server,
you'd still see these settings that we're trying to hide.
That doesn't bother me too much, but maybe some would find it
confusing.
Thoughts?
regards, tom lane
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 71136b11a2..2461317ee4 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -4167,7 +4167,7 @@ ReadControlFile(void)
snprintf(wal_segsz_str, sizeof(wal_segsz_str), "%d", wal_segment_size);
SetConfigOption("wal_segment_size", wal_segsz_str, PGC_INTERNAL,
- PGC_S_OVERRIDE);
+ PGC_S_DYNAMIC_DEFAULT);
/* check and update variables dependent on wal_segment_size */
if (ConvertToXSegs(min_wal_size_mb, wal_segment_size) < 2)
@@ -4186,7 +4186,7 @@ ReadControlFile(void)
/* Make the initdb settings visible as GUC variables, too */
SetConfigOption("data_checksums", DataChecksumsEnabled() ? "yes" : "no",
- PGC_INTERNAL, PGC_S_OVERRIDE);
+ PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
}
/*
@@ -4343,13 +4343,22 @@ XLOGShmemSize(void)
* This isn't an amazingly clean place to do this, but we must wait till
* NBuffers has received its final value, and must do it before using the
* value of XLOGbuffers to do anything important.
+ *
+ * We prefer to report this value as PGC_S_DYNAMIC_DEFAULT. However, if
+ * the DBA explicitly set wal_buffers = -1 in the config file, then
+ * PGC_S_DYNAMIC_DEFAULT will fail to override that and we must force the
+ * matter with PGC_S_OVERRIDE.
*/
if (XLOGbuffers == -1)
{
char buf[32];
snprintf(buf, sizeof(buf), "%d", XLOGChooseNumBuffers());
- SetConfigOption("wal_buffers", buf, PGC_POSTMASTER, PGC_S_OVERRIDE);
+ SetConfigOption("wal_buffers", buf, PGC_POSTMASTER,
+ PGC_S_DYNAMIC_DEFAULT);
+ if (XLOGbuffers == -1) /* failed to apply it? */
+ SetConfigOption("wal_buffers", buf, PGC_POSTMASTER,
+ PGC_S_OVERRIDE);
}
Assert(XLOGbuffers > 0);
diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c
index 9fa8fdd4cf..9a610d41ad 100644
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -262,7 +262,7 @@ BootstrapModeMain(int argc, char *argv[], bool check_only)
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("-X requires a power of two value between 1 MB and 1 GB")));
SetConfigOption("wal_segment_size", optarg, PGC_INTERNAL,
- PGC_S_OVERRIDE);
+ PGC_S_DYNAMIC_DEFAULT);
}
break;
case 'c':
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 3b73e26956..dde4bc25b1 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -921,7 +921,7 @@ PostmasterMain(int argc, char *argv[])
* useful to show, even if one would only expect at least PANIC. LOG
* entries are hidden.
*/
- SetConfigOption("log_min_messages", "FATAL", PGC_INTERNAL,
+ SetConfigOption("log_min_messages", "FATAL", PGC_SUSET,
PGC_S_OVERRIDE);
}
diff --git a/src/backend/storage/ipc/ipci.c b/src/backend/storage/ipc/ipci.c
index 26372d95b3..1a6f527051 100644
--- a/src/backend/storage/ipc/ipci.c
+++ b/src/backend/storage/ipc/ipci.c
@@ -334,7 +334,8 @@ InitializeShmemGUCs(void)
size_b = CalculateShmemSize(NULL);
size_mb = add_size(size_b, (1024 * 1024) - 1) / (1024 * 1024);
sprintf(buf, "%zu", size_mb);
- SetConfigOption("shared_memory_size", buf, PGC_INTERNAL, PGC_S_OVERRIDE);
+ SetConfigOption("shared_memory_size", buf,
+ PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
/*
* Calculate the number of huge pages required.
@@ -346,6 +347,7 @@ InitializeShmemGUCs(void)
hp_required = add_size(size_b / hp_size, 1);
sprintf(buf, "%zu", hp_required);
- SetConfigOption("shared_memory_size_in_huge_pages", buf, PGC_INTERNAL, PGC_S_OVERRIDE);
+ SetConfigOption("shared_memory_size_in_huge_pages", buf,
+ PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
}
}
diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
index ec6a61594a..b25bd0e583 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -787,7 +787,7 @@ InitializeSessionUserId(const char *rolename, Oid roleid)
PGC_BACKEND, PGC_S_OVERRIDE);
SetConfigOption("is_superuser",
AuthenticatedUserIsSuperuser ? "on" : "off",
- PGC_INTERNAL, PGC_S_OVERRIDE);
+ PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
ReleaseSysCache(roleTup);
}
@@ -844,7 +844,7 @@ SetSessionAuthorization(Oid userid, bool is_superuser)
SetConfigOption("is_superuser",
is_superuser ? "on" : "off",
- PGC_INTERNAL, PGC_S_OVERRIDE);
+ PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
}
/*
@@ -901,7 +901,7 @@ SetCurrentRoleId(Oid roleid, bool is_superuser)
SetConfigOption("is_superuser",
is_superuser ? "on" : "off",
- PGC_INTERNAL, PGC_S_OVERRIDE);
+ PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
}
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index fa701daa26..6b9082604f 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -391,7 +391,7 @@ CheckMyDatabase(const char *name, bool am_superuser, bool override_allow_connect
SetDatabaseEncoding(dbform->encoding);
/* Record it as a GUC internal option, too */
SetConfigOption("server_encoding", GetDatabaseEncodingName(),
- PGC_INTERNAL, PGC_S_OVERRIDE);
+ PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
/* If we have no other source of client_encoding, use server encoding */
SetConfigOption("client_encoding", GetDatabaseEncodingName(),
PGC_BACKEND, PGC_S_DYNAMIC_DEFAULT);
@@ -470,8 +470,8 @@ CheckMyDatabase(const char *name, bool am_superuser, bool override_allow_connect
}
/* Make the locale settings visible as GUC variables, too */
- SetConfigOption("lc_collate", collate, PGC_INTERNAL, PGC_S_OVERRIDE);
- SetConfigOption("lc_ctype", ctype, PGC_INTERNAL, PGC_S_OVERRIDE);
+ SetConfigOption("lc_collate", collate, PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
+ SetConfigOption("lc_ctype", ctype, PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
check_strxfrm_bug();
diff --git a/src/backend/utils/misc/guc-file.l b/src/backend/utils/misc/guc-file.l
index 4360c461df..ce5633844c 100644
--- a/src/backend/utils/misc/guc-file.l
+++ b/src/backend/utils/misc/guc-file.l
@@ -401,9 +401,8 @@ ProcessConfigFileInternal(GucContext context, bool applySettings, int elevel)
* be run only after initialization is complete.
*
* XXX this is an unmaintainable crock, because we have to know how to set
- * (or at least what to call to set) every variable that could potentially
- * have PGC_S_DYNAMIC_DEFAULT or PGC_S_ENV_VAR source. However, there's no
- * time to redesign it for 9.1.
+ * (or at least what to call to set) every non-PGC_INTERNAL variable that
+ * could potentially have PGC_S_DYNAMIC_DEFAULT or PGC_S_ENV_VAR source.
*/
if (context == PGC_SIGHUP && applySettings)
{
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 55d41ae7d6..2e21bf0338 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -5939,6 +5939,8 @@ InitializeGUCOptionsFromEnvironment(void)
* rlimit isn't exactly an "environment variable", but it behaves about
* the same. If we can identify the platform stack depth rlimit, increase
* default stack depth setting up to whatever is safe (but at most 2MB).
+ * (But since this isn't really an environment variable, report the
+ * setting as PGC_S_DYNAMIC_DEFAULT not PGC_S_ENV_VAR.)
*/
stack_rlimit = get_stack_depth_rlimit();
if (stack_rlimit > 0)
@@ -5952,7 +5954,7 @@ InitializeGUCOptionsFromEnvironment(void)
new_limit = Min(new_limit, 2048);
sprintf(limbuf, "%ld", new_limit);
SetConfigOption("max_stack_depth", limbuf,
- PGC_POSTMASTER, PGC_S_ENV_VAR);
+ PGC_POSTMASTER, PGC_S_DYNAMIC_DEFAULT);
}
}
}
@@ -7618,6 +7620,8 @@ set_config_option(const char *name, const char *value,
name)));
return 0;
}
+ /* The source should be a boot_val or a dynamic default. */
+ Assert(source <= PGC_S_DYNAMIC_DEFAULT);
break;
case PGC_POSTMASTER:
if (context == PGC_SIGHUP)
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index efcbad7842..385c97fc76 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -83,8 +83,8 @@ typedef enum
* override the postmaster command line.) Tracking the source allows us
* to process sources in any convenient order without affecting results.
* Sources <= PGC_S_OVERRIDE will set the default used by RESET, as well
- * as the current value. Note that source == PGC_S_OVERRIDE should be
- * used when setting a PGC_INTERNAL option.
+ * as the current value. Note that source == PGC_S_DYNAMIC_DEFAULT should
+ * be used when setting a non-compile-time-constant PGC_INTERNAL option.
*
* PGC_S_INTERACTIVE isn't actually a source value, but is the
* dividing line between "interactive" and "non-interactive" sources for
I wrote:
> The attached draft patch makes the following changes:
Here's a v2 that polishes the loose ends:
> (I didn't do anything about in_hot_standby, which is set through
> a hack rather than via set_config_option; not sure whether we want
> to do anything there, or what it should be if we do.)
I concluded that directly assigning to in_hot_standby was a fairly
horrid idea and we should just change it with SetConfigOption.
With this coding, as long as in_hot_standby is TRUE it will show
as having a non-default setting in \dconfig. I had to remove the
assertion I'd added about PGC_INTERNAL variables only receiving
"default" values, but this just shows that was too inflexible anyway.
> * The rlimit-derived value of max_stack_depth is likewise relabeled
> as PGC_S_DYNAMIC_DEFAULT, resolving the complaint Jonathan had upthread.
> But now that we have a way to hide this, I'm having second thoughts
> about whether we should. If you are on a platform that's forcing an
> unreasonably small stack size, it'd be good if \dconfig told you so.
> Could it be sane to label that value as PGC_S_DYNAMIC_DEFAULT only when
> it's the limit value (2MB) and PGC_S_ENV_VAR when it's smaller?
I concluded that was just fine and did it.
regards, tom lane
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 71136b11a2..8764084e21 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -4167,7 +4167,7 @@ ReadControlFile(void)
snprintf(wal_segsz_str, sizeof(wal_segsz_str), "%d", wal_segment_size);
SetConfigOption("wal_segment_size", wal_segsz_str, PGC_INTERNAL,
- PGC_S_OVERRIDE);
+ PGC_S_DYNAMIC_DEFAULT);
/* check and update variables dependent on wal_segment_size */
if (ConvertToXSegs(min_wal_size_mb, wal_segment_size) < 2)
@@ -4186,7 +4186,7 @@ ReadControlFile(void)
/* Make the initdb settings visible as GUC variables, too */
SetConfigOption("data_checksums", DataChecksumsEnabled() ? "yes" : "no",
- PGC_INTERNAL, PGC_S_OVERRIDE);
+ PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
}
/*
@@ -4343,13 +4343,22 @@ XLOGShmemSize(void)
* This isn't an amazingly clean place to do this, but we must wait till
* NBuffers has received its final value, and must do it before using the
* value of XLOGbuffers to do anything important.
+ *
+ * We prefer to report this value's source as PGC_S_DYNAMIC_DEFAULT.
+ * However, if the DBA explicitly set wal_buffers = -1 in the config file,
+ * then PGC_S_DYNAMIC_DEFAULT will fail to override that and we must force
+ * the matter with PGC_S_OVERRIDE.
*/
if (XLOGbuffers == -1)
{
char buf[32];
snprintf(buf, sizeof(buf), "%d", XLOGChooseNumBuffers());
- SetConfigOption("wal_buffers", buf, PGC_POSTMASTER, PGC_S_OVERRIDE);
+ SetConfigOption("wal_buffers", buf, PGC_POSTMASTER,
+ PGC_S_DYNAMIC_DEFAULT);
+ if (XLOGbuffers == -1) /* failed to apply it? */
+ SetConfigOption("wal_buffers", buf, PGC_POSTMASTER,
+ PGC_S_OVERRIDE);
}
Assert(XLOGbuffers > 0);
diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c
index 9fa8fdd4cf..9a610d41ad 100644
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -262,7 +262,7 @@ BootstrapModeMain(int argc, char *argv[], bool check_only)
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("-X requires a power of two value between 1 MB and 1 GB")));
SetConfigOption("wal_segment_size", optarg, PGC_INTERNAL,
- PGC_S_OVERRIDE);
+ PGC_S_DYNAMIC_DEFAULT);
}
break;
case 'c':
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 3b73e26956..dde4bc25b1 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -921,7 +921,7 @@ PostmasterMain(int argc, char *argv[])
* useful to show, even if one would only expect at least PANIC. LOG
* entries are hidden.
*/
- SetConfigOption("log_min_messages", "FATAL", PGC_INTERNAL,
+ SetConfigOption("log_min_messages", "FATAL", PGC_SUSET,
PGC_S_OVERRIDE);
}
diff --git a/src/backend/storage/ipc/ipci.c b/src/backend/storage/ipc/ipci.c
index 26372d95b3..1a6f527051 100644
--- a/src/backend/storage/ipc/ipci.c
+++ b/src/backend/storage/ipc/ipci.c
@@ -334,7 +334,8 @@ InitializeShmemGUCs(void)
size_b = CalculateShmemSize(NULL);
size_mb = add_size(size_b, (1024 * 1024) - 1) / (1024 * 1024);
sprintf(buf, "%zu", size_mb);
- SetConfigOption("shared_memory_size", buf, PGC_INTERNAL, PGC_S_OVERRIDE);
+ SetConfigOption("shared_memory_size", buf,
+ PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
/*
* Calculate the number of huge pages required.
@@ -346,6 +347,7 @@ InitializeShmemGUCs(void)
hp_required = add_size(size_b / hp_size, 1);
sprintf(buf, "%zu", hp_required);
- SetConfigOption("shared_memory_size_in_huge_pages", buf, PGC_INTERNAL, PGC_S_OVERRIDE);
+ SetConfigOption("shared_memory_size_in_huge_pages", buf,
+ PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
}
}
diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
index ec6a61594a..b25bd0e583 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -787,7 +787,7 @@ InitializeSessionUserId(const char *rolename, Oid roleid)
PGC_BACKEND, PGC_S_OVERRIDE);
SetConfigOption("is_superuser",
AuthenticatedUserIsSuperuser ? "on" : "off",
- PGC_INTERNAL, PGC_S_OVERRIDE);
+ PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
ReleaseSysCache(roleTup);
}
@@ -844,7 +844,7 @@ SetSessionAuthorization(Oid userid, bool is_superuser)
SetConfigOption("is_superuser",
is_superuser ? "on" : "off",
- PGC_INTERNAL, PGC_S_OVERRIDE);
+ PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
}
/*
@@ -901,7 +901,7 @@ SetCurrentRoleId(Oid roleid, bool is_superuser)
SetConfigOption("is_superuser",
is_superuser ? "on" : "off",
- PGC_INTERNAL, PGC_S_OVERRIDE);
+ PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
}
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index fa701daa26..6b9082604f 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -391,7 +391,7 @@ CheckMyDatabase(const char *name, bool am_superuser, bool override_allow_connect
SetDatabaseEncoding(dbform->encoding);
/* Record it as a GUC internal option, too */
SetConfigOption("server_encoding", GetDatabaseEncodingName(),
- PGC_INTERNAL, PGC_S_OVERRIDE);
+ PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
/* If we have no other source of client_encoding, use server encoding */
SetConfigOption("client_encoding", GetDatabaseEncodingName(),
PGC_BACKEND, PGC_S_DYNAMIC_DEFAULT);
@@ -470,8 +470,8 @@ CheckMyDatabase(const char *name, bool am_superuser, bool override_allow_connect
}
/* Make the locale settings visible as GUC variables, too */
- SetConfigOption("lc_collate", collate, PGC_INTERNAL, PGC_S_OVERRIDE);
- SetConfigOption("lc_ctype", ctype, PGC_INTERNAL, PGC_S_OVERRIDE);
+ SetConfigOption("lc_collate", collate, PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
+ SetConfigOption("lc_ctype", ctype, PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
check_strxfrm_bug();
diff --git a/src/backend/utils/misc/guc-file.l b/src/backend/utils/misc/guc-file.l
index 4360c461df..ce5633844c 100644
--- a/src/backend/utils/misc/guc-file.l
+++ b/src/backend/utils/misc/guc-file.l
@@ -401,9 +401,8 @@ ProcessConfigFileInternal(GucContext context, bool applySettings, int elevel)
* be run only after initialization is complete.
*
* XXX this is an unmaintainable crock, because we have to know how to set
- * (or at least what to call to set) every variable that could potentially
- * have PGC_S_DYNAMIC_DEFAULT or PGC_S_ENV_VAR source. However, there's no
- * time to redesign it for 9.1.
+ * (or at least what to call to set) every non-PGC_INTERNAL variable that
+ * could potentially have PGC_S_DYNAMIC_DEFAULT or PGC_S_ENV_VAR source.
*/
if (context == PGC_SIGHUP && applySettings)
{
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 55d41ae7d6..a7cc49898b 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -5939,6 +5939,8 @@ InitializeGUCOptionsFromEnvironment(void)
* rlimit isn't exactly an "environment variable", but it behaves about
* the same. If we can identify the platform stack depth rlimit, increase
* default stack depth setting up to whatever is safe (but at most 2MB).
+ * Report the value's source as PGC_S_DYNAMIC_DEFAULT if it's 2MB, or as
+ * PGC_S_ENV_VAR if it's reflecting the rlimit limit.
*/
stack_rlimit = get_stack_depth_rlimit();
if (stack_rlimit > 0)
@@ -5947,12 +5949,19 @@ InitializeGUCOptionsFromEnvironment(void)
if (new_limit > 100)
{
+ GucSource source;
char limbuf[16];
- new_limit = Min(new_limit, 2048);
- sprintf(limbuf, "%ld", new_limit);
+ if (new_limit < 2048)
+ source = PGC_S_ENV_VAR;
+ else
+ {
+ new_limit = 2048;
+ source = PGC_S_DYNAMIC_DEFAULT;
+ }
+ snprintf(limbuf, sizeof(limbuf), "%ld", new_limit);
SetConfigOption("max_stack_depth", limbuf,
- PGC_POSTMASTER, PGC_S_ENV_VAR);
+ PGC_POSTMASTER, source);
}
}
}
@@ -6776,12 +6785,16 @@ BeginReportingGUCOptions(void)
reporting_enabled = true;
/*
- * Hack for in_hot_standby: initialize with the value we're about to send.
+ * Hack for in_hot_standby: set the GUC value true if appropriate. This
+ * is kind of an ugly place to do it, but there's few better options.
+ *
* (This could be out of date by the time we actually send it, in which
* case the next ReportChangedGUCOptions call will send a duplicate
* report.)
*/
- in_hot_standby = RecoveryInProgress();
+ if (RecoveryInProgress())
+ SetConfigOption("in_hot_standby", "true",
+ PGC_INTERNAL, PGC_S_OVERRIDE);
/* Transmit initial values of interesting variables */
for (i = 0; i < num_guc_variables; i++)
@@ -6822,15 +6835,8 @@ ReportChangedGUCOptions(void)
* transition from false to true.
*/
if (in_hot_standby && !RecoveryInProgress())
- {
- struct config_generic *record;
-
- record = find_option("in_hot_standby", false, false, ERROR);
- Assert(record != NULL);
- record->status |= GUC_NEEDS_REPORT;
- report_needed = true;
- in_hot_standby = false;
- }
+ SetConfigOption("in_hot_standby", "false",
+ PGC_INTERNAL, PGC_S_OVERRIDE);
/* Quick exit if no values have been changed */
if (!report_needed)
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index efcbad7842..d5976ecbfb 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -83,8 +83,7 @@ typedef enum
* override the postmaster command line.) Tracking the source allows us
* to process sources in any convenient order without affecting results.
* Sources <= PGC_S_OVERRIDE will set the default used by RESET, as well
- * as the current value. Note that source == PGC_S_OVERRIDE should be
- * used when setting a PGC_INTERNAL option.
+ * as the current value.
*
* PGC_S_INTERACTIVE isn't actually a source value, but is the
* dividing line between "interactive" and "non-interactive" sources for
@@ -99,6 +98,11 @@ typedef enum
* shouldn't throw hard errors in this case, at most NOTICEs, since the
* objects might exist by the time the setting is used for real.
*
+ * When setting the value of a non-compile-time-constant PGC_INTERNAL option,
+ * source == PGC_S_DYNAMIC_DEFAULT should typically be used so that the value
+ * will show as "default" in pg_settings. If there is a specific reason not
+ * to want that, use source == PGC_S_OVERRIDE.
+ *
* NB: see GucSource_Names in guc.c if you change this.
*/
typedef enum
On 6/7/22 1:02 PM, Tom Lane wrote: > In any case, I expect that we'd apply this patch only to HEAD, which > means that when using psql's \dconfig against a pre-v15 server, > you'd still see these settings that we're trying to hide. > That doesn't bother me too much, but maybe some would find it > confusing. Well, "\dconfig" is a v15 feature, and though it's in the client, the best compatibility for it will be with v15. I think it's OK to have the behavior different in v15 vs. older versions. Jonathan
Attachment
On 6/7/22 7:58 PM, Tom Lane wrote: > I wrote: >> The attached draft patch makes the following changes: > > Here's a v2 that polishes the loose ends: Thanks! I reviewed and did some basic testing locally. I did not see any of the generated defaults. >> (I didn't do anything about in_hot_standby, which is set through >> a hack rather than via set_config_option; not sure whether we want >> to do anything there, or what it should be if we do.) The comment diff showed that it went from "hack" to "hack" :) > I concluded that directly assigning to in_hot_standby was a fairly > horrid idea and we should just change it with SetConfigOption. > With this coding, as long as in_hot_standby is TRUE it will show > as having a non-default setting in \dconfig. I had to remove the > assertion I'd added about PGC_INTERNAL variables only receiving > "default" values, but this just shows that was too inflexible anyway. I tested this and the server correctly rendered "in_hot_standby" in \dconfig. I also tested setting "hot_standby to "on" while the server was not in recovery, and \dconfig correctly did not render "in_hot_standby". >> * The rlimit-derived value of max_stack_depth is likewise relabeled >> as PGC_S_DYNAMIC_DEFAULT, resolving the complaint Jonathan had upthread. >> But now that we have a way to hide this, I'm having second thoughts >> about whether we should. If you are on a platform that's forcing an >> unreasonably small stack size, it'd be good if \dconfig told you so. >> Could it be sane to label that value as PGC_S_DYNAMIC_DEFAULT only when >> it's the limit value (2MB) and PGC_S_ENV_VAR when it's smaller? > > I concluded that was just fine and did it. Reading the docs, I think this is OK to do. We already say that "2MB" is a very conservative setting. And even if the value can be computed to be larger, we don't allow the server to set it higher than "2MB". I don't know how frequently issues around "max_stack_depth" being too small are reported -- I'd be curious to know that -- but I don't have any strong arguments against allowing the behavior you describe based on our current docs. Jonathan
Attachment
"Jonathan S. Katz" <jkatz@postgresql.org> writes:
> I don't know how frequently issues around "max_stack_depth" being too
> small are reported -- I'd be curious to know that -- but I don't have
> any strong arguments against allowing the behavior you describe based on
> our current docs.
I can't recall any recent gripes on our own lists, but the issue was
top-of-mind for me after discovering that NetBSD defaults "ulimit -s"
to 2MB on at least some platforms. That would leave us setting
max_stack_depth to something less than that, probably about 1.5MB.
regards, tom lane
On 6/7/22 10:57 PM, Tom Lane wrote: > "Jonathan S. Katz" <jkatz@postgresql.org> writes: >> I don't know how frequently issues around "max_stack_depth" being too >> small are reported -- I'd be curious to know that -- but I don't have >> any strong arguments against allowing the behavior you describe based on >> our current docs. > > I can't recall any recent gripes on our own lists, but the issue was > top-of-mind for me after discovering that NetBSD defaults "ulimit -s" > to 2MB on at least some platforms. That would leave us setting > max_stack_depth to something less than that, probably about 1.5MB. Interesting. OK, I'd say let's keep the behavior that's in the patch. Jonathan
Attachment
"Jonathan S. Katz" <jkatz@postgresql.org> writes:
> Interesting. OK, I'd say let's keep the behavior that's in the patch.
Pushed then.
regards, tom lane
On 6/8/22 1:26 PM, Tom Lane wrote: > "Jonathan S. Katz" <jkatz@postgresql.org> writes: >> Interesting. OK, I'd say let's keep the behavior that's in the patch. > > Pushed then. Excellent -- thank you! Jonathan