Обсуждение: GUC flags

This isn't flagged with GUC_EXPLAIN:
enable_incremental_sort

Here's some more candidates:
shared_buffers - it seems unfortunate this is not included; actually, it seems
    like maybe this should be always included - not just if it's set to a
    non-default, but especially if it's left at the default..
    I suppose it's more important for DML than SELECT.
temp_tablespaces isn't, but temp_buffers is;
autovacuum - if it's off, that can be the cause of the issue (same as force_parallel_mode, which has GUC_EXPLAIN);
max_worker_processes - isn't, but these are: max_parallel_workers, max_parallel_workers_per_gather;
track_io_timing - it can have high overhead;
session_preload_libraries, shared_preload_libraries, local_preload_libraries;
debug_assertions - it's be kind of nice to show this whenever it's true (even thought it's not "changed")
debug_discard_caches
lc_collate and lc_ctype ?
server_version_num - I asked about this in the past, but Tom thought it should
    not be included, and I kind of agree, but I'm including it here for
    completeness sake

This isn't marked GUC_NOT_IN_SAMPLE, like all other DEVELOPER_OPTIONS:
trace_recovery_messages

I'm not sure jit_tuple_deforming should be marked GUC_NOT_IN_SAMPLE.
I disable this one because it's slow for tables with many attributes.
Same for jit_expressions ?

bgwriter_lru_maxpages should have GUC_UNIT_BLOCKS

max_identifier_length should have BYTES (like log_parameter_max_length and
track_activity_query_size).

block_size and wal_block_size should say BYTES (like wal_segment_size)
And all three descriptions should say ".. in [prefix]bytes" (but see below).

Maybe these should have COMPAT_OPTIONS_PREVIOUS:
integer_datetimes
ssl_renegotiation_limit

autovacuum_freeze_max_age has a comment about pg_resetwal which is obsolete
since 74cf7d46a.

checkpoint_warning refers to "checkpoint segments", which is obsolete since
88e982302.

The attached does the least-disputable, lowest hanging fruit.

More ideas:

Maybe maintenance_io_concurrency should not be GUC_EXPLAIN.  But it's used not
only by ANALYZE but also heap_index_delete_tuples.

Should these be GUC_RUNTIME_COMPUTED?
in_hot_standby, data_directory_mode

Since GUC_DISALLOW_IN_FILE effectively implies GUC_NOT_IN_SAMPLE in
src/backend/utils/misc/help_config.c:displayStruct(), many of the redundant
GUC_NOT_IN_SAMPLE could be removed.

I think several things with COMPAT_OPTIONS_PREVIOUS could have 
GUC_NO_SHOW_ALL and/or GUC_NOT_IN_SAMPLE.

The GUC descriptions are a hodge podge of full sentences and telegrams.
There's no consistency whether the long description can be read independently
from the short description.  For these GUCs, the short description reads more
like a "DETAIL" message:
|trace_recovery_messages, log_min_error_statement, log_min_messages, client_min_messages
|log_transaction_sample_rate, log_statement_sample_rate
|data_directory_mode, log_file_mode, unix_socket_permissions
|log_directory, log_destination, log_line_prefix
|unix_socket_group, default_tablespace, DateStyle, maintenance_work_mem, geqo_generations

For integer/real GUCs, the long description is being used just to describe the
"special" values:
|jit_inline_above_cost, jit_optimize_above_cost, jit_above_cost, log_startup_progress_interval,
|tcp_user_timeout, tcp_keepalives_interval, tcp_keepalives_idle, log_temp_files, old_snapshot_threshold,
|log_parameter_max_length_on_error, log_parameter_max_length, log_autovacuum_min_duration, log_min_duration_sample,
|idle_session_timeout, idle_in_transaction_session_timeout, lock_timeout,
|statement_timeout, shared_memory_size_in_huge_pages

Descriptions of some GUCs describe their default units, but other's don't.
The units are not very important for input, since a non-default unit can be
specified, like SET statement_timeout='1h'.  It's important for output, and
SHOW already includes a unit, which may not be the default unit.  So I think 
the default units should be removed from the descriptions.

This cleanup is similar to GUC categories fixed in a55a98477.
Tom was of the impression that there's more loose ends on that thread.
https://www.postgresql.org/message-id/flat/16997-ff16127f6e0d1390@postgresql.org

-- 
Justin

On Sun, Nov 28, 2021 at 09:08:33PM -0600, Justin Pryzby wrote:
> This isn't flagged with GUC_EXPLAIN:
> enable_incremental_sort

Yeah, that's inconsistent.

> This isn't marked GUC_NOT_IN_SAMPLE, like all other DEVELOPER_OPTIONS:
> trace_recovery_messages

Indeed.

> I'm not sure jit_tuple_deforming should be marked GUC_NOT_IN_SAMPLE.
> I disable this one because it's slow for tables with many attributes.
> Same for jit_expressions ?

That would be consistent.  Both are not in postgresql.conf.sample.

> bgwriter_lru_maxpages should have GUC_UNIT_BLOCKS
>
> max_identifier_length should have BYTES (like log_parameter_max_length and
> track_activity_query_size).
>
> block_size and wal_block_size should say BYTES (like wal_segment_size)
> And all three descriptions should say ".. in [prefix]bytes" (but see below).

Okay for these.

> Maybe these should have COMPAT_OPTIONS_PREVIOUS:
> integer_datetimes
> ssl_renegotiation_limit

Hmm.  Okay as well for integer_datetimes.

> autovacuum_freeze_max_age has a comment about pg_resetwal which is obsolete
> since 74cf7d46a.
>
> checkpoint_warning refers to "checkpoint segments", which is obsolete since
> 88e982302.

That's part of 0002.  That's a bit weird to use now, so I'd agree with
your suggestion to use "WAL segments" instead.

0001, to adjust the units, and 0003, to make the GUC descriptions less
unit-dependent, are good ideas.

-      gettext_noop("Use of huge pages on Linux or Windows."),
+      gettext_noop("Enable use of huge pages on Linux or Windows."),
This should be "Enables use of".

    {"compute_query_id", PGC_SUSET, STATS_MONITORING,
-    gettext_noop("Compute query identifiers."),
+    gettext_noop("Enables in-core computation of a query identifier."),
This could just be "Computes"?

I am not completely sure that all the contents of 0002 are
improvements, but the suggestions done for huge_pages,
ssl_passphrase_command_supports_reload, checkpoint_warning and
commit_siblings seem fine to me.
--
Michael

On Mon, Nov 29, 2021 at 05:04:01PM +0900, Michael Paquier wrote:
> 0001, to adjust the units, and 0003, to make the GUC descriptions less
> unit-dependent, are good ideas.

Actually, after sleeping on it and doing some digging in
codesearch.debian.org, changing the units of max_identifier_length,
block_size and wal_block_size could induce some breakages for anything
using a SHOW command, something that becomes particularly worse now
that SHOW is supported in replication connections, and it would force
clients to know and parse the units of a value.  Perhaps I am being
too careful here, but that could harm a lot of users.  It is worth
noting that I have found some driver code making use of pg_settings,
which would not be influenced by such a change, but it is unsafe to
assume that everybody does that.

The addition of GUC_EXPLAIN for enable_incremental_sort, the comment
fix for autovacuum_freeze_max_age, the use of COMPAT_OPTIONS_PREVIOUS
for ssl_renegotiation_limit and the addition of GUC_NOT_IN_SAMPLE for
trace_recovery_messages are fine, though.

> I am not completely sure that all the contents of 0002 are
> improvements, but the suggestions done for huge_pages,
> ssl_passphrase_command_supports_reload, checkpoint_warning and
> commit_siblings seem fine to me.

Hmm, I think the patched description of checkpoint_warning is not that
much an improvement compared to the current one.  While the current
description uses the term "checkpoint segments", which is, I agree,
weird.  The new one would lose the term "checkpoint", making the short
description of the parameter lose some of its context.

I have done a full review of the patch set, and applied the obvious
fixes/improvements as of be54551.  Attached is an extra patch based on
the contents of the whole set sent upthread:
- Improvement of the description of checkpoint_segments.
- Reworded the description of all parameters using "N units", rather
than just switching to "this period of units".  I have been using
something more generic.

Thoughts?
--
Michael

On Tue, Nov 30, 2021 at 03:36:45PM +0900, Michael Paquier wrote:
> -            gettext_noop("Forces a switch to the next WAL file if a "
> -                         "new file has not been started within N seconds."),
> +            gettext_noop("Sets the amount of time to wait before forcing a "
> +                         "switch to the next WAL file."),

..

> -            gettext_noop("Waits N seconds on connection startup after authentication."),
> +            gettext_noop("Sets the amount of seconds to wait on connection "
> +                         "startup after authentication."),

"amount of time", like above.

> -            gettext_noop("Waits N seconds on connection startup before authentication."),
> +            gettext_noop("Sets the amount of seconds to wait on connection "
> +                         "startup before authentication."),

same

>      {
>          {"checkpoint_warning", PGC_SIGHUP, WAL_CHECKPOINTS,
> -            gettext_noop("Enables warnings if checkpoint segments are filled more "
> -                         "frequently than this."),
> +            gettext_noop("Sets the maximum time before warning if checkpoints "
> +                         "triggered by WAL volume happen too frequently."),
>              gettext_noop("Write a message to the server log if checkpoints "
> -                         "caused by the filling of checkpoint segment files happens more "
> +                         "caused by the filling of WAL segment files happens more "

It should say "happen" , since it's referring to "checkpoints".
That was a pre-existing issue.

>          {"log_parameter_max_length", PGC_SUSET, LOGGING_WHAT,
> -            gettext_noop("When logging statements, limit logged parameter values to first N bytes."),
> +            gettext_noop("Sets the maximum amount of data logged for bind "
> +                         "parameter values when logging statements."),

I think this one should actually say "in bytes" or at least say "maximum
length".  It seems unlikely that someone is going to specify this in other
units, and it's confusing to everyone else to refer to "amount of data" instead
of "length in bytes".


>          {"log_parameter_max_length_on_error", PGC_USERSET, LOGGING_WHAT,
> -            gettext_noop("When reporting an error, limit logged parameter values to first N bytes."),
> +            gettext_noop("Sets the maximum amount of data logged for bind "
> +                         "parameter values when logging statements, on error."),

same

> -            gettext_noop("Automatic log file rotation will occur after N minutes."),
> +            gettext_noop("Sets the maximum amount of time to wait before "
> +                         "forcing log file rotation."),

Should it say "maximum" ?  Does that mean anything ?

-- 
Justin

On Wed, Dec 01, 2021 at 01:59:05AM -0600, Justin Pryzby wrote:
> On Tue, Nov 30, 2021 at 03:36:45PM +0900, Michael Paquier wrote:
>> -            gettext_noop("Waits N seconds on connection startup before authentication."),
>> +            gettext_noop("Sets the amount of seconds to wait on connection "
>> +                         "startup before authentication."),
>
> same

Thanks.  This makes things more consistent.

>>      {
>>          {"checkpoint_warning", PGC_SIGHUP, WAL_CHECKPOINTS,
>> -            gettext_noop("Enables warnings if checkpoint segments are filled more "
>> -                         "frequently than this."),
>> +            gettext_noop("Sets the maximum time before warning if checkpoints "
>> +                         "triggered by WAL volume happen too frequently."),
>>              gettext_noop("Write a message to the server log if checkpoints "
>> -                         "caused by the filling of checkpoint segment files happens more "
>> +                         "caused by the filling of WAL segment files happens more "
>
> It should say "happen" , since it's referring to "checkpoints".
> That was a pre-existing issue.

Indeed.

>>          {"log_parameter_max_length", PGC_SUSET, LOGGING_WHAT,
>> -            gettext_noop("When logging statements, limit logged parameter values to first N bytes."),
>> +            gettext_noop("Sets the maximum amount of data logged for bind "
>> +                         "parameter values when logging statements."),
>
> I think this one should actually say "in bytes" or at least say "maximum
> length".  It seems unlikely that someone is going to specify this in other
> units, and it's confusing to everyone else to refer to "amount of data" instead
> of "length in bytes".

Okay.  Do you like the updated version attached?

>> -            gettext_noop("Automatic log file rotation will occur after N minutes."),
>> +            gettext_noop("Sets the maximum amount of time to wait before "
>> +                         "forcing log file rotation."),
>
> Should it say "maximum" ?  Does that mean anything ?

To be consistent with the rest of your suggestions, we could use here:
"Sets the amount of time to wait before forcing log file rotation"

Thanks,
--
Michael

> @@ -2142,7 +2142,8 @@ static struct config_int ConfigureNamesInt[] =
>          {"post_auth_delay", PGC_BACKEND, DEVELOPER_OPTIONS,
> -            gettext_noop("Waits N seconds on connection startup after authentication."),
> +            gettext_noop("Sets the amount of time to wait on connection "
> +                         "startup after authentication."),

> @@ -2762,7 +2763,8 @@ static struct config_int ConfigureNamesInt[] =
>          {"pre_auth_delay", PGC_SIGHUP, DEVELOPER_OPTIONS,
> -            gettext_noop("Waits N seconds on connection startup before authentication."),
> +            gettext_noop("Sets the amount of time to wait on connection "
> +                         "startup before authentication."),
>              gettext_noop("This allows attaching a debugger to the process."),

I wonder if these should say "Sets the amount of time to wait [before]
authentication during connection startup"

>          {"checkpoint_warning", PGC_SIGHUP, WAL_CHECKPOINTS,
> -            gettext_noop("Enables warnings if checkpoint segments are filled more "
> -                         "frequently than this."),
> +            gettext_noop("Sets the maximum time before warning if checkpoints "
> +                         "triggered by WAL volume happen too frequently."),
>              gettext_noop("Write a message to the server log if checkpoints "
> -                         "caused by the filling of checkpoint segment files happens more "
> +                         "caused by the filling of WAL segment files happen more "
>                           "frequently than this number of seconds. Zero turns off the warning."),

Should this still say "seconds" ?
Or change it to "this amount of time"?
I'm not sure.

>          {"log_rotation_age", PGC_SIGHUP, LOGGING_WHERE,
> -            gettext_noop("Automatic log file rotation will occur after N minutes."),
> +            gettext_noop("Sets the amount of time to wait before forcing "
> +                         "log file rotation."),
>              NULL,
>              GUC_UNIT_MIN
>          },
> @@ -3154,7 +3159,8 @@ static struct config_int ConfigureNamesInt[] =
>  
>      {
>          {"log_rotation_size", PGC_SIGHUP, LOGGING_WHERE,
> -            gettext_noop("Automatic log file rotation will occur after N kilobytes."),
> +            gettext_noop("Sets the maximum size of log file to reach before "
> +                         "forcing log file rotation."),

Actually, I think that for log_rotation_size, it should not say "forcing".

"Sets the maximum size a log file can reach before being rotated"

BTW the EXPLAIN flag for enable_incremental_sort could be backpatched to v13.

-- 
Justin

On Wed, Dec 01, 2021 at 09:34:39PM -0600, Justin Pryzby wrote:
>> @@ -2762,7 +2763,8 @@ static struct config_int ConfigureNamesInt[] =
>>          {"pre_auth_delay", PGC_SIGHUP, DEVELOPER_OPTIONS,
>> -            gettext_noop("Waits N seconds on connection startup before authentication."),
>> +            gettext_noop("Sets the amount of time to wait on connection "
>> +                         "startup before authentication."),
>>              gettext_noop("This allows attaching a debugger to the process."),
>
> I wonder if these should say "Sets the amount of time to wait [before]
> authentication during connection startup"

Hmm.  I don't see much a difference between both of wordings in this
context.

>>              gettext_noop("Write a message to the server log if checkpoints "
>> -                         "caused by the filling of checkpoint segment files happens more "
>> +                         "caused by the filling of WAL segment files happen more "
>>                           "frequently than this number of seconds. Zero turns off the warning."),
>
> Should this still say "seconds" ?
> Or change it to "this amount of time"?
> I'm not sure.

Either way would be fine by me, though I'd agree to be consistent and
use "this amount of time" here.

>>          {"log_rotation_size", PGC_SIGHUP, LOGGING_WHERE,
>> -            gettext_noop("Automatic log file rotation will occur after N kilobytes."),
>> +            gettext_noop("Sets the maximum size of log file to reach before "
>> +                         "forcing log file rotation."),
>
> Actually, I think that for log_rotation_size, it should not say "forcing".
>
> "Sets the maximum size a log file can reach before being rotated"

Okay.  Fine by me.

> BTW the EXPLAIN flag for enable_incremental_sort could be backpatched to v13.

This could cause small diffs in EXPLAIN outputs, which could be
surprising.  This is not worth taking any risks.
--
Michael

On Thu, Dec 02, 2021 at 02:11:38PM +0900, Michael Paquier wrote:
> On Wed, Dec 01, 2021 at 09:34:39PM -0600, Justin Pryzby wrote:
> >> @@ -2762,7 +2763,8 @@ static struct config_int ConfigureNamesInt[] =
> >>          {"pre_auth_delay", PGC_SIGHUP, DEVELOPER_OPTIONS,
> >> -            gettext_noop("Waits N seconds on connection startup before authentication."),
> >> +            gettext_noop("Sets the amount of time to wait on connection "
> >> +                         "startup before authentication."),
> >>              gettext_noop("This allows attaching a debugger to the process."),
> > 
> > I wonder if these should say "Sets the amount of time to wait [before]
> > authentication during connection startup"
> 
> Hmm.  I don't see much a difference between both of wordings in this
> context.

I find it easier to read "wait before authentication ..." than "wait ... before
authentication".

> > BTW the EXPLAIN flag for enable_incremental_sort could be backpatched to v13.
> 
> This could cause small diffs in EXPLAIN outputs, which could be
> surprising.  This is not worth taking any risks.

Only if one specifies explain(SETTINGS).
It's fine either way ;)

-- 
Justin

On Wed, Dec 01, 2021 at 11:17:34PM -0600, Justin Pryzby wrote:
> I find it easier to read "wait before authentication ..." than "wait ... before
> authentication".

I have a hard time seeing a strong difference here.  At the end, I
have used what you suggested, adjusted the rest based on your set of
comments, and applied the patch.
--
Michael

On Fri, Dec 03, 2021 at 10:06:47AM +0900, Michael Paquier wrote:
> On Wed, Dec 01, 2021 at 11:17:34PM -0600, Justin Pryzby wrote:
> > I find it easier to read "wait before authentication ..." than "wait ... before
> > authentication".
> 
> I have a hard time seeing a strong difference here.  At the end, I
> have used what you suggested, adjusted the rest based on your set of
> comments, and applied the patch.

Thanks.  One more item.  The check_guc script currently outputs 68 false
positives - even though it includes a list of 20 exceptions.  This is not
useful.

$ (cd ./src/backend/utils/misc/; ./check_guc) |wc -l
68

With the attached:

$ (cd ./src/backend/utils/misc/; ./check_guc)   
config_file seems to be missing from postgresql.conf.sample

That has a defacto exception for the "include" directive, which seems
reasonable.

This requires GNU awk.  I'm not sure if that's a limitation of any
significance.

-- 
Justin

On Sun, Dec 05, 2021 at 11:38:05PM -0600, Justin Pryzby wrote:
> Thanks.  One more item.  The check_guc script currently outputs 68 false
> positives - even though it includes a list of 20 exceptions.  This is not
> useful.

Indeed.  Hmm.  This script does a couple of things:
1) Check the format of the options defined in the various lists of
guc.c, which is something people format well, and pgindent also does
a part of this job.
2) Check that options in the hardcoded list of GUCs in
INTENTIONALLY_NOT_INCLUDED are not included in
postgresql.conf.sample
3) Check that nothing considered as a parameter in
postgresql.conf.sample is listed in guc.c.

Your patch removes 1) and 2), but keeps 3) to check for dead
parameter references in postgresql.conf.sample.

Is check_guc actually run on a periodic basis by somebody?  Based on
the amount of false positives that has accumulated over the years, and
what `git grep` can already do for 3), it seems to me that we have
more arguments in favor of just removing it entirely.
--
Michael

On Mon, Dec 06, 2021 at 03:58:39PM +0900, Michael Paquier wrote:
> On Sun, Dec 05, 2021 at 11:38:05PM -0600, Justin Pryzby wrote:
> > Thanks.  One more item.  The check_guc script currently outputs 68 false
> > positives - even though it includes a list of 20 exceptions.  This is not
> > useful.
> 
> Indeed.  Hmm.  This script does a couple of things:
> 1) Check the format of the options defined in the various lists of
> guc.c, which is something people format well, and pgindent also does 
> a part of this job.
> 2) Check that options in the hardcoded list of GUCs in
> INTENTIONALLY_NOT_INCLUDED are not included in
> postgresql.conf.sample
> 3) Check that nothing considered as a parameter in
> postgresql.conf.sample is listed in guc.c.
> 
> Your patch removes 1) and 2), but keeps 3) to check for dead
> parameter references in postgresql.conf.sample.

The script checks that guc.c and sample config are consistent.

I think your undertanding of INTENTIONALLY_NOT_INCLUDED is not right.
That's a list of stuff it "avoids reporting" as an suspected error, not an
additional list of stuff to checks.  INTENTIONALLY_NOT_INCLUDED is a list of
stuff like NOT_IN_SAMPLE, which is better done by parsing /NOT_IN_SAMPLE/.

> Is check_guc actually run on a periodic basis by somebody?  Based on
> the amount of false positives that has accumulated over the years, and
> what `git grep` can already do for 3), it seems to me that we have
> more arguments in favor of just removing it entirely.

I saw that Tom updated it within the last 12 months, which I took to mean that
it was still being maintained. But I'm okay with removing it.

-- 
Justin

On Mon, Dec 06, 2021 at 07:36:55AM -0600, Justin Pryzby wrote:
> The script checks that guc.c and sample config are consistent.
>
> I think your undertanding of INTENTIONALLY_NOT_INCLUDED is not right.
> That's a list of stuff it "avoids reporting" as an suspected error, not an
> additional list of stuff to checks.  INTENTIONALLY_NOT_INCLUDED is a list of
> stuff like NOT_IN_SAMPLE, which is better done by parsing /NOT_IN_SAMPLE/.

Indeed.  I got that wrong, thanks for clarifying.

> I saw that Tom updated it within the last 12 months, which I took to mean that
> it was still being maintained. But I'm okay with removing it.

Yes, I saw that as of bf8a662.  With 42 incorrect reports, I still see
more evidence with removing it.  Before doing anything, let's wait for
and gather some opinions.  I am adding Bruce (as the original author)
and Tom in CC as they are the ones who have updated this script the
most in the last ~15 years.
--
Michael

On 08.12.21 07:27, Michael Paquier wrote:
>> I saw that Tom updated it within the last 12 months, which I took to mean that
>> it was still being maintained. But I'm okay with removing it.
> Yes, I saw that as of bf8a662.  With 42 incorrect reports, I still see
> more evidence with removing it.  Before doing anything, let's wait for
> and gather some opinions.  I am adding Bruce (as the original author)
> and Tom in CC as they are the ones who have updated this script the
> most in the last ~15 years.

I wasn't really aware of this script either.  But I think it's a good 
idea to have it.  But only if it's run automatically as part of a test 
suite run.

At Tue, 28 Dec 2021 20:32:40 -0600, Justin Pryzby <pryzby@telsasoft.com> wrote in 
> On Thu, Dec 09, 2021 at 09:53:23AM -0600, Justin Pryzby wrote:
> > On Thu, Dec 09, 2021 at 05:17:54PM +0900, Michael Paquier wrote:
> > One option is to expose the GUC flags in pg_settings, so this can all be done
> > in SQL regression tests.
> > 
> > Maybe the flags should be text strings, so it's a nicer user-facing interface.
> > But then the field would be pretty wide, even though we're only adding it for
> > regression tests.  The only other alternative I can think of is to make a
> > sql-callable function like pg_get_guc_flags(text guc).
> 
> Rebased on cab5b9ab2c066ba904f13de2681872dcda31e207.
> 
> And added 0003, which changes to instead exposes the flags as a function, to
> avoid changing pg_settings and exposing internally-defined integer flags in
> that somewhat prominent view.

Just an idea but couldn't we use flags in a series of one-letter flag
representations?  It is more user-friendly than integers but shorter
than full-text representation.

+SELECT name, flags FROM pg_settings;
          name          |  flags             
------------------------+--------
 application_name       |  ARsec
 transaction_deferrable |  Arsec
 transaction_isolation  |  Arsec
 transaction_read_only  |  Arsec

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

At Tue, 4 Jan 2022 21:06:48 -0600, Justin Pryzby <pryzby@telsasoft.com> wrote in 
> On Wed, Jan 05, 2022 at 11:47:57AM +0900, Kyotaro Horiguchi wrote:
> > At Tue, 28 Dec 2021 20:32:40 -0600, Justin Pryzby <pryzby@telsasoft.com> wrote in 
> In that case, *all* the flags should be exposed.  There's currently 20, which
> means it may not work well after all - it's already too long, and could get
> longer, and/or overflow the alphabet...

Yeah, if we show all 20 properties, the string is too long as well as
all properties cannot have a sensible abbreviation character..

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On 25.01.22 02:07, Justin Pryzby wrote:
> +CREATE TABLE pg_settings_flags AS SELECT name, category,
> +    'NO_SHOW_ALL'    =ANY(flags) AS no_show_all,
> +    'NO_RESET_ALL'    =ANY(flags) AS no_reset_all,
> +    'NOT_IN_SAMPLE'    =ANY(flags) AS not_in_sample,
> +    'EXPLAIN'    =ANY(flags) AS guc_explain,
> +    'COMPUTED'    =ANY(flags) AS guc_computed
> +    FROM pg_show_all_settings();

Does this stuff have any value for users?  I'm worried we are exposing a 
bunch of stuff that is really just for internal purposes.  Like, what 
value does showing "not_in_sample" have?  On the other hand, 
"guc_explain" might be genuinely useful, since that is part of a 
user-facing feature.  (I don't like the "guc_*" naming though.)

Your patch doesn't contain a documentation change, so I don't know how 
and to what extend this is supposed to be presented to users.