BUG #16997: parameter server_encoding's category problem

On Fri, May 7, 2021 at 3:11 PM PG Bug reporting form
<noreply@postgresql.org> wrote:

The following bug has been logged on the website:

Bug reference: 16997
Logged by: yanliang lei
Email address: leiyanliang@highgo.com
PostgreSQL version: 13.1
Operating system: CentOS7.6
Description:

In the following sql statement， parameter server_encoding's category is
“Client Connection Defaults / Locale and Formatting”：

postgres=# select category from pg_settings where name='server_encoding';
-[ RECORD 1 ]------------------------------------------------
category | Client Connection Defaults / Locale and Formatting

postgres=#

In the document
https://www.postgresql.org/docs/current/runtime-config-client.html#RUNTIME-CONFIG-CLIENT-FORMAT，
there is no entry about parameter server_encoding.

and in the document
https://www.postgresql.org/docs/current/runtime-config-preset.html,there is
a entry about parameter server_encoding.

so, What is the parameter server_encoding's category?

Thanks for reporting. All the internal parameters that can't be set by
the users are under the "Preset Options" category. Likewise,
server_encoding, lc_collate and lc_ctype should also be listed under
the "Preset Options" category, even though there is a "Client
Connection Defaults / Locale and Formatting" category, which has user
configurable parameters. For instance, since the in_hot_standby is an
internal parameter, it is specified under the "Preset Options" even
though there is a "Replication / Standby Servers" category which
mentions all the user configurable parameters.

Therefore, I would say that the documentation is correct but the code
is not. Attached patch should fix this.

While on this, I also adjusted some of the wordings for the other
"Preset Options".

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> writes:

Thanks for reporting. All the internal parameters that can't be set by
the users are under the "Preset Options" category. Likewise,
server_encoding, lc_collate and lc_ctype should also be listed under
the "Preset Options" category, even though there is a "Client
Connection Defaults / Locale and Formatting" category, which has user
configurable parameters. For instance, since the in_hot_standby is an
internal parameter, it is specified under the "Preset Options" even
though there is a "Replication / Standby Servers" category which
mentions all the user configurable parameters.

Therefore, I would say that the documentation is correct but the code
is not. Attached patch should fix this.

Hm. I did some more careful checks (by comparing the pg_settings
view's output to the SGML docs) and found a boatload of additional
misclassifications. The most egregious being that somebody had
decided to invent a new sub-heading Write Ahead Log / Recovery,
but didn't bother to add a corresponding enum value; so the variables
in that sub-section were all misclassified (and not even all in
the same way).

I also discovered that postgresql.conf.sample wasn't all that well
in sync with the docs. It mostly agreed with the docs as to
classifications, but the ordering of individual options didn't
agree so well. I think this is a consequence of various people
having arbitrarily alphabetized the doc entries without making
postgresql.conf.sample match. I can see no excuse for them not
to match, though.

Using the same assumption that the docs represent our best ideas
in this area, the attached 0001 makes the GUC code and the sample
file agree with the docs.

I also think we should do 0002, which removes the group-level
enum values for those categories where we have sub-categories.
The group-level enum values seem to me to just be an attractive
bug-encouraging nuisance (there is indeed one place in 0001 that
changes a GUC that was misclassified at the top level). Plus
they result in translators having to do extra work.

There are a couple of loose ends yet:

* I notice that these GUCs appear in the view, but if they're
documented anywhere it's not in config.sgml:

transaction_deferrable | Client Connection Defaults / Statement Behavior
transaction_isolation | Client Connection Defaults / Statement Behavior
transaction_read_only | Client Connection Defaults / Statement Behavior

Should we add entries for them? If not, should we maybe move them
to the UNGROUPED category (and then make them GUC_NO_SHOW_ALL),
like other special GUCs such as "role"? I'm a bit inclined to the
latter, since they aren't supposed to be used in the same way as
ordinary GUCs.

* The code's names for the categories do not match up all that
well with the section headings in config.sgml, eg we have
Query Tuning in guc.c and Query Planning in the docs. Should we
try to sync that?

While on this, I also adjusted some of the wordings for the other
"Preset Options".

I agree that the preset options should uniformly use the phrasing
"Shows ...", but I thought your proposals here were mostly too
verbose.

regards, tom lane

On Sat, May 8, 2021 at 2:28 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I also discovered that postgresql.conf.sample wasn't all that well
in sync with the docs. It mostly agreed with the docs as to
classifications, but the ordering of individual options didn't
agree so well. I think this is a consequence of various people
having arbitrarily alphabetized the doc entries without making
postgresql.conf.sample match. I can see no excuse for them not
to match, though.

If the arrangement is to be in alphabetical order, then
max_parallel_maintenance_workers should come before
max_parallel_workers_per_gather.
 #max_worker_processes = 8        # (change requires restart)
-#max_parallel_maintenance_workers = 2    # taken from max_parallel_workers
 #max_parallel_workers_per_gather = 2    # taken from max_parallel_workers
-#parallel_leader_participation = on
+#max_parallel_maintenance_workers = 2    # taken from max_parallel_workers

Other changes look good to me.

Using the same assumption that the docs represent our best ideas
in this area, the attached 0001 makes the GUC code and the sample
file agree with the docs.

+1 to have the code and the docs in sync.

I also think we should do 0002, which removes the group-level
enum values for those categories where we have sub-categories.
The group-level enum values seem to me to just be an attractive
bug-encouraging nuisance (there is indeed one place in 0001 that
changes a GUC that was misclassified at the top level). Plus
they result in translators having to do extra work.

Yeah, removing the unused group level enums CONN_AUTH, RESOURCES, WAL,
REPLICATION and so on makes sense, since all the related parameters
are categorized under respective subcategories such as CONN_AUTH_XXX,
RESOURCES_XXX, WAL_XXX, REPLICATION_XXX. If at all, any new parameters
are added and if they don't fall in any of the subcategories, a new
subcategory can be added.

There are a couple of loose ends yet:

* I notice that these GUCs appear in the view, but if they're
documented anywhere it's not in config.sgml:

transaction_deferrable | Client Connection Defaults / Statement Behavior
transaction_isolation | Client Connection Defaults / Statement Behavior
transaction_read_only | Client Connection Defaults / Statement Behavior

Should we add entries for them? If not, should we maybe move them
to the UNGROUPED category (and then make them GUC_NO_SHOW_ALL),
like other special GUCs such as "role"? I'm a bit inclined to the
latter, since they aren't supposed to be used in the same way as
ordinary GUCs.

It looks like we are using GUC_NO_SHOW_ALL for unused, deprecated, or
the GUCs being used by other commands. Since the above 3 GUCs are
being used by START TRANSACTION ISOLATION LEVEL command, it makes
sense to make them GUC_NO_SHOW_ALL, but let's keep them under
CLIENT_CONN_STATEMENT. Anyways, they will not show up in the
pg_settings and in the docs. Also, it will be good if we can add
comment before them in guc.c /* Not for general use --- used by START
TRANSACTION ISOLATION LEVEL */

* The code's names for the categories do not match up all that
well with the section headings in config.sgml, eg we have
Query Tuning in guc.c and Query Planning in the docs. Should we
try to sync that?

Yeah, there are differences. (in guc.c, in docs) --> (Resource Usage,
Resource Consumption), (Write-Ahead Log, Write Ahead Log), (Query
Tuning, Query Planning), (Reporting and Logging, Error Reporting and
Logging), (Statistics, Run-time Statistics), (Autovacuum, Automatic
Vacuuming).

I think we can change the docs to match the guc.c code wordings. Thoughts?

While on this, I also adjusted some of the wordings for the other
"Preset Options".

I agree that the preset options should uniformly use the phrasing
"Shows ...", but I thought your proposals here were mostly too
verbose.

Thanks! The changes in 0001 are fine to me.

There is another thread at [1]/messages/by-id/20210413123139.GE6091@telsasoft.com that discusses cleaning up the unused
enums or modifying the GUC categories. And, there are some bugs being
raised for the inconsistency between what pg_settings show and what
docs say. Therefore, the patches proposed here will fix all of the
concerns.

It looks like there are more changes needed for docs clean up and for
making transaction_XXXX GUC_NO_SHOW_ALL, if agreed I can post separate
patches for these things on top of 0001 and 0002.

[1]: /messages/by-id/20210413123139.GE6091@telsasoft.com

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> writes:

On Sat, May 8, 2021 at 2:28 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I also discovered that postgresql.conf.sample wasn't all that well
in sync with the docs. It mostly agreed with the docs as to
classifications, but the ordering of individual options didn't
agree so well. I think this is a consequence of various people
having arbitrarily alphabetized the doc entries without making
postgresql.conf.sample match. I can see no excuse for them not
to match, though.

If the arrangement is to be in alphabetical order, then
max_parallel_maintenance_workers should come before
max_parallel_workers_per_gather.

No, the idea is to have postgresql.conf.sample match what's in
config.sgml, which is not at all alphabetical in that section.

We regularly have religious wars over whether strict alphabetical
order is the best way to present things. I'm generally in the
camp that finds that to be overly pedantic, so I'm not in a hurry
to change config.sgml here.

I've pushed those patches with a bit of additional work on the
comments around enum config_group. The other points remain
to be dealt with:

* I notice that these GUCs appear in the view, but if they're
documented anywhere it's not in config.sgml:
transaction_deferrable | Client Connection Defaults / Statement Behavior
transaction_isolation | Client Connection Defaults / Statement Behavior
transaction_read_only | Client Connection Defaults / Statement Behavior
Should we add entries for them? If not, should we maybe move them
to the UNGROUPED category (and then make them GUC_NO_SHOW_ALL),
like other special GUCs such as "role"? I'm a bit inclined to the
latter, since they aren't supposed to be used in the same way as
ordinary GUCs.

It looks like we are using GUC_NO_SHOW_ALL for unused, deprecated, or
the GUCs being used by other commands. Since the above 3 GUCs are
being used by START TRANSACTION ISOLATION LEVEL command, it makes
sense to make them GUC_NO_SHOW_ALL, but let's keep them under
CLIENT_CONN_STATEMENT.

I think that GUCs that aren't listed in config.sgml need to be
UNGROUPED; it's just confusing to show them as part of a group
when they're not in that group according to the docs. Also,
it's almost true that presence of GUC_NO_SHOW_ALL correlates
exactly with being UNGROUPED. The couple of exceptions are
legacy entries that perhaps should have been changed when they
were de-documented.

* The code's names for the categories do not match up all that
well with the section headings in config.sgml, eg we have
Query Tuning in guc.c and Query Planning in the docs. Should we
try to sync that?

I think we can change the docs to match the guc.c code wordings. Thoughts?

Meh... not sure why we'd take the docs as gospel up to this point
and then suddenly reverse course.

After thinking about it, I think it's fine if the guc.c string is a
shortened form of the config.sgml section title. For instance,
I don't feel a need to make "Reporting and Logging" match the docs'
"Error Reporting and Logging". But places where it's just randomly
different, like "Query Tuning" vs. "Query Planning", probably
should be fixed to avoid confusion.

There is another thread at [1] that discusses cleaning up the unused
enums or modifying the GUC categories.

Ah, I'd forgotten that thread already. I included Justin as part
author on this commit.

regards, tom lane

On Sat, May 8, 2021 at 9:55 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

It looks like we are using GUC_NO_SHOW_ALL for unused, deprecated, or
the GUCs being used by other commands. Since the above 3 GUCs are
being used by START TRANSACTION ISOLATION LEVEL command, it makes
sense to make them GUC_NO_SHOW_ALL, but let's keep them under
CLIENT_CONN_STATEMENT.

I think that GUCs that aren't listed in config.sgml need to be
UNGROUPED; it's just confusing to show them as part of a group
when they're not in that group according to the docs. Also,
it's almost true that presence of GUC_NO_SHOW_ALL correlates
exactly with being UNGROUPED. The couple of exceptions are
legacy entries that perhaps should have been changed when they
were de-documented.

I think the GUCs that have the GUC_NO_SHOW_ALL flag in guc.c, are not
showing up in pg_settings output along with show all and they are also
not mentioned in the config.sgml. Even if they aren't categorized
under UNGROUPED in guc.c that's not visible to the users. Therefore,
IMO we can retain the transaction_deferrable, transaction_isolation,
transaction_read_only category as is i.e. CLIENT_CONN_STATEMENT, just
add GUC_NO_SHOW_ALL flag and maybe a comment "/* Not for general use
--- used by START TRANSACTION ISOLATION LEVEL */.

* The code's names for the categories do not match up all that
well with the section headings in config.sgml, eg we have
Query Tuning in guc.c and Query Planning in the docs. Should we
try to sync that?

I think we can change the docs to match the guc.c code wordings. Thoughts?

Meh... not sure why we'd take the docs as gospel up to this point
and then suddenly reverse course.

After thinking about it, I think it's fine if the guc.c string is a
shortened form of the config.sgml section title. For instance,
I don't feel a need to make "Reporting and Logging" match the docs'
"Error Reporting and Logging". But places where it's just randomly
different, like "Query Tuning" vs. "Query Planning", probably
should be fixed to avoid confusion.

+1 to change Query Planning in docs to Query Tuning to match in guc.c.

Remaining seems fine: (in guc.c, in docs) --> (Resource Usage,
Resource Consumption), (Write-Ahead Log, Write Ahead Log), (Query
Tuning, Query Planning), (Reporting and Logging, Error Reporting and
Logging), (Statistics, Run-time Statistics), (Autovacuum, Automatic
Vacuuming)

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Sun, May 9, 2021 at 6:30 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

I think that GUCs that aren't listed in config.sgml need to be
UNGROUPED; it's just confusing to show them as part of a group
when they're not in that group according to the docs. Also,
it's almost true that presence of GUC_NO_SHOW_ALL correlates
exactly with being UNGROUPED. The couple of exceptions are
legacy entries that perhaps should have been changed when they
were de-documented.

I think the GUCs that have the GUC_NO_SHOW_ALL flag in guc.c, are not
showing up in pg_settings output along with show all and they are also
not mentioned in the config.sgml. Even if they aren't categorized
under UNGROUPED in guc.c that's not visible to the users. Therefore,
IMO we can retain the transaction_deferrable, transaction_isolation,
transaction_read_only category as is i.e. CLIENT_CONN_STATEMENT, just
add GUC_NO_SHOW_ALL flag and maybe a comment "/* Not for general use
--- used by START TRANSACTION ISOLATION LEVEL */.

Added GUC_NO_SHOW_ALL flag to transaction_deferrable,
transaction_isolation and transaction_read_only.

After thinking about it, I think it's fine if the guc.c string is a
shortened form of the config.sgml section title. For instance,
I don't feel a need to make "Reporting and Logging" match the docs'
"Error Reporting and Logging". But places where it's just randomly
different, like "Query Tuning" vs. "Query Planning", probably
should be fixed to avoid confusion.

+1 to change Query Planning in docs to Query Tuning to match in guc.c.

Remaining seems fine: (in guc.c, in docs) --> (Resource Usage,
Resource Consumption), (Write-Ahead Log, Write Ahead Log), (Query
Tuning, Query Planning), (Reporting and Logging, Error Reporting and
Logging), (Statistics, Run-time Statistics), (Autovacuum, Automatic
Vacuuming)

Changed "Query Planning" in the config.sgml to "Query Tuning".

Attaching a small patch with the above changes.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com