Thread

Commits

  1. Add TAP test to automate the equivalent of check_guc, take two

  2. Retire src/backend/utils/misc/check_guc

  3. Add TAP test to automate the equivalent of check_guc

  4. Add PostgreSQL::Test::Cluster::config_data()

  5. Introduce pg_settings_get_flags() to find flags associated to a GUC

  6. Revert changes about warnings/errors for placeholders.

  7. Improve the description of various GUCs

  1. GUC flags

    Justin Pryzby <pryzby@telsasoft.com> — 2021-11-29T03:08:33Z

    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
    
  2. Re: GUC flags

    Michael Paquier <michael@paquier.xyz> — 2021-11-29T08:04:01Z

    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
    
  3. Re: GUC flags

    Michael Paquier <michael@paquier.xyz> — 2021-11-30T06:36:45Z

    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
    
  4. Re: GUC flags

    Justin Pryzby <pryzby@telsasoft.com> — 2021-12-01T07:59:05Z

    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
    
    
    
    
  5. Re: GUC flags

    Michael Paquier <michael@paquier.xyz> — 2021-12-01T11:58:00Z

    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
    
  6. Re: GUC flags

    Justin Pryzby <pryzby@telsasoft.com> — 2021-12-02T03:34:39Z

    > @@ -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
    
    
    
    
  7. Re: GUC flags

    Michael Paquier <michael@paquier.xyz> — 2021-12-02T05:11:38Z

    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
    
  8. Re: GUC flags

    Justin Pryzby <pryzby@telsasoft.com> — 2021-12-02T05:17:34Z

    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
    
    
    
    
  9. Re: GUC flags

    Michael Paquier <michael@paquier.xyz> — 2021-12-03T01:06:47Z

    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
    
  10. Re: GUC flags

    Justin Pryzby <pryzby@telsasoft.com> — 2021-12-06T05:38:05Z

    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
    
  11. Re: GUC flags

    Michael Paquier <michael@paquier.xyz> — 2021-12-06T06:58:39Z

    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
    
  12. Re: GUC flags

    Justin Pryzby <pryzby@telsasoft.com> — 2021-12-06T13:36:55Z

    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
    
    
    
    
  13. Re: GUC flags

    Michael Paquier <michael@paquier.xyz> — 2021-12-08T06:27:19Z

    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
    
  14. Re: GUC flags

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-12-08T12:23:51Z

    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.
    
    
    
    
  15. Re: GUC flags

    Michael Paquier <michael@paquier.xyz> — 2021-12-09T08:17:54Z

    On Wed, Dec 08, 2021 at 01:23:51PM +0100, Peter Eisentraut wrote:
    > 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.
    
    Okay.  If we do that, I am wondering whether it would be better to
    rewrite this script in perl then, so as there is no need to worry
    about the compatibility of grep.  And also, it would make sense to
    return a non-zero exit code if an incompatibility is found for the
    automation part.
    --
    Michael
    
  16. Re: GUC flags

    Justin Pryzby <pryzby@telsasoft.com> — 2021-12-09T15:53:23Z

    On Thu, Dec 09, 2021 at 05:17:54PM +0900, Michael Paquier wrote:
    > On Wed, Dec 08, 2021 at 01:23:51PM +0100, Peter Eisentraut wrote:
    > > 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.
    > 
    > Okay.  If we do that, I am wondering whether it would be better to
    > rewrite this script in perl then, so as there is no need to worry
    > about the compatibility of grep.  And also, it would make sense to
    > return a non-zero exit code if an incompatibility is found for the
    > automation part.
    
    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).
    
  17. Re: GUC flags

    Justin Pryzby <pryzby@telsasoft.com> — 2021-12-16T21:06:51Z

    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:
    > > On Wed, Dec 08, 2021 at 01:23:51PM +0100, Peter Eisentraut wrote:
    > > > 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.
    > > 
    > > Okay.  If we do that, I am wondering whether it would be better to
    > > rewrite this script in perl then, so as there is no need to worry
    > > about the compatibility of grep.  And also, it would make sense to
    > > return a non-zero exit code if an incompatibility is found for the
    > > automation part.
    > 
    > 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).
    
    Fixed regression tests caused by another patches.
    
  18. Re: GUC flags

    Justin Pryzby <pryzby@telsasoft.com> — 2021-12-29T02:32:40Z

    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:
    > > On Wed, Dec 08, 2021 at 01:23:51PM +0100, Peter Eisentraut wrote:
    > > > 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.
    > > 
    > > Okay.  If we do that, I am wondering whether it would be better to
    > > rewrite this script in perl then, so as there is no need to worry
    > > about the compatibility of grep.  And also, it would make sense to
    > > return a non-zero exit code if an incompatibility is found for the
    > > automation part.
    > 
    > 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.
    
    -- 
    Justin
    
  19. Re: GUC flags

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-01-05T02:47:57Z

    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
    
    
    
    
  20. Re: GUC flags

    Justin Pryzby <pryzby@telsasoft.com> — 2022-01-05T03:06:48Z

    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 
    > > 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
    
    It's a good idea.
    
    I suppose you intend that "A" means it's enabled and "a" means it's disabled ?
    
    	A => show all
    	R => reset all
    	S => not in sample
    	E => explain
    	C => computed
    
    Which is enough to support the tests that I came up with:
    
    +       (flags&4) != 0 AS no_show_all,
    +       (flags&8) != 0 AS no_reset_all,
    +       (flags&32) != 0 AS not_in_sample,
    +       (flags&1048576) != 0 AS guc_explain,
    +       (flags&2097152) != 0 AS guc_computed
    
    However, I think if we add a field to pg_stat_activity, it would be in a
    separate patch, expected to be independently useful.
    
    1) expose GUC flags to pg_stat_activity;
    2) rewrite check_guc as a sql regression test;
    
    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...
    
    I think pg_get_guc_flags() may be best, but I'm interested to hear other
    opinions.
    
    -- 
    Justin
    
    
    
    
  21. Re: GUC flags

    Michael Paquier <michael@paquier.xyz> — 2022-01-05T05:17:11Z

    On Tue, Jan 04, 2022 at 09:06:48PM -0600, Justin Pryzby wrote:
    > I think pg_get_guc_flags() may be best, but I'm interested to hear other
    > opinions.
    
    My opinion on this matter is rather close to what you have here with
    handling things through one extra attribute.  But I don't see the
    point of using an extra function where users would need to do a manual
    mapping of the flag bits back to a a text representation of them.  So
    I would suggest to just add one text[] to pg_show_all_settings, with
    values being the bit names themselves, without the prefix "GUC_", for
    the ones we care most about.  Sticking with one column for each one
    would require a catversion bump all the time, which could be
    cumbersome in the long run.
    --
    Michael
    
  22. Re: GUC flags

    Justin Pryzby <pryzby@telsasoft.com> — 2022-01-05T23:55:17Z

    On Wed, Jan 05, 2022 at 02:17:11PM +0900, Michael Paquier wrote:
    > On Tue, Jan 04, 2022 at 09:06:48PM -0600, Justin Pryzby wrote:
    > > I think pg_get_guc_flags() may be best, but I'm interested to hear other
    > > opinions.
    > 
    > My opinion on this matter is rather close to what you have here with
    > handling things through one extra attribute.  But I don't see the
    > point of using an extra function where users would need to do a manual
    > mapping of the flag bits back to a a text representation of them.
    
    If it were implemented as a function, this would be essentially internal and
    left undocumented.  Only exposed for the purpose of re-implementing check_guc.
    
    > I would suggest to just add one text[] to pg_show_all_settings
    
    Good idea to use the backing function without updating the view.
    
    pg_settings is currently defined with "SELECT *".  Is it fine to enumerate a
    list of columns instead ?
    
  23. Re: GUC flags

    Michael Paquier <michael@paquier.xyz> — 2022-01-06T05:19:08Z

    On Wed, Jan 05, 2022 at 05:55:17PM -0600, Justin Pryzby wrote:
    > pg_settings is currently defined with "SELECT *".  Is it fine to enumerate a
    > list of columns instead ?
    
    I'd like to think that this is a better practice when it comes
    documenting the columns, but I don't see an actual need for this extra
    complication here.
    
    > +	initStringInfo(&ret);
    > +	appendStringInfoChar(&ret, '{');
    > +
    > +	if (flags & GUC_NO_SHOW_ALL)
    > +		appendStringInfo(&ret, "NO_SHOW_ALL,");
    > +	if (flags & GUC_NO_RESET_ALL)
    > +		appendStringInfo(&ret, "NO_RESET_ALL,");
    > +	if (flags & GUC_NOT_IN_SAMPLE)
    > +		appendStringInfo(&ret, "NOT_IN_SAMPLE,");
    > +	if (flags & GUC_EXPLAIN)
    > +		appendStringInfo(&ret, "EXPLAIN,");
    > +	if (flags & GUC_RUNTIME_COMPUTED)
    > +		appendStringInfo(&ret, "RUNTIME_COMPUTED,");
    > +
    > +	/* Remove trailing comma, if any */
    > +	if (ret.len > 1)
    > +		ret.data[--ret.len] = '\0';
    
    The way of building the text array is incorrect here.  See
    heap_tuple_infomask_flags() in pageinspect as an example with all the
    HEAP_* flags.  I think that you should allocate an array of Datums,
    use CStringGetTextDatum() to assign each array element, wrapping the
    whole with construct_array() to build the final value for the
    parameter tuple.
    --
    Michael
    
  24. Re: GUC flags

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-01-06T05:35:54Z

    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
    
    
    
    
  25. Re: GUC flags

    Justin Pryzby <pryzby@telsasoft.com> — 2022-01-06T05:36:42Z

    On Thu, Jan 06, 2022 at 02:19:08PM +0900, Michael Paquier wrote:
    > On Wed, Jan 05, 2022 at 05:55:17PM -0600, Justin Pryzby wrote:
    > > pg_settings is currently defined with "SELECT *".  Is it fine to enumerate a
    > > list of columns instead ?
    > 
    > I'd like to think that this is a better practice when it comes
    > documenting the columns, but I don't see an actual need for this extra
    > complication here.
    
    The reason is to avoid showing the flags in the pg_settings view, which should
    not be bloated just so we can retire check_guc.
    
    > > +	initStringInfo(&ret);
    > > +	appendStringInfoChar(&ret, '{');
    > > +
    > > +	if (flags & GUC_NO_SHOW_ALL)
    > > +		appendStringInfo(&ret, "NO_SHOW_ALL,");
    > > +	if (flags & GUC_NO_RESET_ALL)
    > > +		appendStringInfo(&ret, "NO_RESET_ALL,");
    > > +	if (flags & GUC_NOT_IN_SAMPLE)
    > > +		appendStringInfo(&ret, "NOT_IN_SAMPLE,");
    > > +	if (flags & GUC_EXPLAIN)
    > > +		appendStringInfo(&ret, "EXPLAIN,");
    > > +	if (flags & GUC_RUNTIME_COMPUTED)
    > > +		appendStringInfo(&ret, "RUNTIME_COMPUTED,");
    > > +
    > > +	/* Remove trailing comma, if any */
    > > +	if (ret.len > 1)
    > > +		ret.data[--ret.len] = '\0';
    > 
    > The way of building the text array is incorrect here.  See
    > heap_tuple_infomask_flags() in pageinspect as an example with all the
    > HEAP_* flags.  I think that you should allocate an array of Datums,
    > use CStringGetTextDatum() to assign each array element, wrapping the
    > whole with construct_array() to build the final value for the
    > parameter tuple.
    
    I actually did it that way last night ... however GetConfigOptionByNum() is
    expecting it to return a text string, not an array.
    
    -- 
    Justin
    
    
    
    
  26. Re: GUC flags

    Justin Pryzby <pryzby@telsasoft.com> — 2022-01-25T01:07:29Z

    On Wed, Jan 05, 2022 at 11:36:41PM -0600, Justin Pryzby wrote:
    > On Thu, Jan 06, 2022 at 02:19:08PM +0900, Michael Paquier wrote:
    > > > +	initStringInfo(&ret);
    > > > +	appendStringInfoChar(&ret, '{');
    > > > +
    > > > +	if (flags & GUC_NO_SHOW_ALL)
    > > > +		appendStringInfo(&ret, "NO_SHOW_ALL,");
    > > > +	if (flags & GUC_NO_RESET_ALL)
    > > > +		appendStringInfo(&ret, "NO_RESET_ALL,");
    > > > +	if (flags & GUC_NOT_IN_SAMPLE)
    > > > +		appendStringInfo(&ret, "NOT_IN_SAMPLE,");
    > > > +	if (flags & GUC_EXPLAIN)
    > > > +		appendStringInfo(&ret, "EXPLAIN,");
    > > > +	if (flags & GUC_RUNTIME_COMPUTED)
    > > > +		appendStringInfo(&ret, "RUNTIME_COMPUTED,");
    > > > +
    > > > +	/* Remove trailing comma, if any */
    > > > +	if (ret.len > 1)
    > > > +		ret.data[--ret.len] = '\0';
    > > 
    > > The way of building the text array is incorrect here.  See
    
    I think you'll find that this is how it's done elsewhere in postgres.
    In the frontend, see appendPQExpBufferChar and appendPGArray and 3e6e86abc.
    On the backend, see: git grep -F "'{'" |grep -w appendStringInfoChar
    
    I updated the patch with a regex to accommodate GUCs without '=', as needed
    since f47ed79cc8.
    
    -- 
    Justin
    
  27. Re: GUC flags

    Michael Paquier <michael@paquier.xyz> — 2022-01-25T07:25:48Z

    On Mon, Jan 24, 2022 at 07:07:29PM -0600, Justin Pryzby wrote:
    > I think you'll find that this is how it's done elsewhere in postgres.
    > In the frontend, see appendPQExpBufferChar and appendPGArray and 3e6e86abc.
    > On the backend, see: git grep -F "'{'" |grep -w appendStringInfoChar
    
    Yeah, I was not careful enough to look after the uses of TEXTARRAYOID,
    and there is one in the same area, as of config_enum_get_options().
    At least things are consistent this way.
    
    > I updated the patch with a regex to accommodate GUCs without '=', as needed
    > since f47ed79cc8.
    
    Okay.  While looking at your proposal, I was thinking that we had
    better include the array with the flags by default in pg_settings, and
    not just pg_show_all_settings().
    
    +SELECT lower(name) FROM pg_settings_flags WHERE NOT not_in_sample EXCEPT
    +SELECT regexp_replace(ln, '^#?([_[:alpha:]]+) (= .*|[^ ]*$)', '\1') AS guc
    +FROM (SELECT regexp_split_to_table(pg_read_file('postgresql.conf'),
    '\n') AS ln) conf
    
    Tests reading postgresql.conf would break on instances started with a
    custom config_file provided by a command line, no?  You could change
    the patch to use the value provided by the GUC, instead, but I am not
    convinced that we need that at all, even if check_guc does so.
    
    Regarding the tests, I am not sure if we need to be this much
    extensive.  We could take is slow, and I am also wondering if this
    could not cause some issues with GUCs loaded via
    shared_preload_libraries if we are too picky about the requirements,
    as this could cause installcheck failures.
    
    The following things have been issues recently, though, and they look
    sensible enough to have checks for: 
    - GUC_NOT_IN_SAMPLE with developer options.
    - Query-tuning parameters with GUC_EXPLAIN, and we'd better add some
    comments in the test to explain why there are exceptions like
    default_statistics_target.
    - preset parameters marked as runtime-computed.
    - NO_SHOW_ALL and NOT_IN_SAMPLE.
    
    Thanks,
    --
    Michael
    
  28. Re: GUC flags

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-01-25T10:47:14Z

    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.
    
    
    
    
  29. Re: GUC flags

    Justin Pryzby <pryzby@telsasoft.com> — 2022-01-25T18:07:51Z

    On Tue, Jan 25, 2022 at 11:47:14AM +0100, Peter Eisentraut wrote:
    > 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.
    
    I want to avoid putting this in pg_settings.
    
    The two options discussed so far are:
     - to add an function to return the flags;
     - to add the flags to pg_show_all_settings(), but not show it in pg_settings view;
    
    I interpretted Michael's suggested as adding it to pg_get_all_settings(), but
    *not* including it in the pg_settings view.  Now it seems like I misunderstood,
    and Michael wants to add it to the view.
    
    But, even if we only handle the 5 flags we have an immediate use for, it makes
    the user-facing view too "wide", just to accommodate this internal use.
    
    If it were in the pg_settings view, I think it ought to have *all* the flags
    (not just the flags that help us to retire ./check_guc).  That's much too much.
    
    -- 
    Justin
    
    
    
    
  30. Re: GUC flags

    Michael Paquier <michael@paquier.xyz> — 2022-01-26T00:54:43Z

    On Tue, Jan 25, 2022 at 12:07:51PM -0600, Justin Pryzby wrote:
    > On Tue, Jan 25, 2022 at 11:47:14AM +0100, Peter Eisentraut wrote:
    >> 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.)
    
    EXPLAIN is useful to know which parameter could be part of an explain
    query, as that's not an information provided now, even if the category
    provides a hint.  COMPUTED is also useful for the purpose of postgres
    -C in my opinion.  I am reserved about the rest in terms of user
    experience, but the other ones are useful to automate the checks
    check_guc was doing, which is still the main goal of this patch if we
    remove this script.  And experience has proved lately that people
    forget a lot to mark GUCs correctly.
    
    > I interpretted Michael's suggested as adding it to pg_get_all_settings(), but
    > *not* including it in the pg_settings view.  Now it seems like I misunderstood,
    > and Michael wants to add it to the view.
    
    Yeah, I meant to add that in the view, as it is already wide.  I'd be
    fine with a separate SQL function at the end, but putting that in
    pg_show_all_settings() without considering pg_settings would not be
    consistent.  There is the argument that one could miss an update of
    system_views.sql if adding more data to pg_show_all_settings(), even
    if that's not really going to happen.
    
    > But, even if we only handle the 5 flags we have an immediate use for, it makes
    > the user-facing view too "wide", just to accommodate this internal use.
    
    short_desc and extra_desc count for most of the bloat already, so that
    would not change much, but I am fine to discard my point to not make
    things worse.
    --
    Michael
    
  31. Re: GUC flags

    Justin Pryzby <pryzby@telsasoft.com> — 2022-01-26T03:44:26Z

    On Wed, Jan 26, 2022 at 09:54:43AM +0900, Michael Paquier wrote:
    > On Tue, Jan 25, 2022 at 12:07:51PM -0600, Justin Pryzby wrote:
    > > On Tue, Jan 25, 2022 at 11:47:14AM +0100, Peter Eisentraut wrote:
    > >> 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.)
    > 
    > EXPLAIN is useful to know which parameter could be part of an explain
    > query, as that's not an information provided now, even if the category
    > provides a hint.  COMPUTED is also useful for the purpose of postgres
    > -C in my opinion.
    
    It seems like an arbitrary and short-sighted policy to expose a handful of
    flags in the view for the purpose of retiring ./check_guc, but not expose other
    flags, because we thought we knew that no user could ever want them.
    
    We should either expose all the flags, or should put them into an undocumented
    function.  Otherwise, how would we document the flags argument ?  "Shows some
    of the flags" ?  An undocumented function avoids this issue.
    
    Should I update the patch to put the function back ?
    Should I also make the function expose all of the flags ?
    
    -- 
    Justin
    
    
    
    
  32. Re: GUC flags

    Michael Paquier <michael@paquier.xyz> — 2022-01-26T06:29:29Z

    On Tue, Jan 25, 2022 at 09:44:26PM -0600, Justin Pryzby wrote:
    > It seems like an arbitrary and short-sighted policy to expose a handful of
    > flags in the view for the purpose of retiring ./check_guc, but not expose other
    > flags, because we thought we knew that no user could ever want them.
    > 
    > We should either expose all the flags, or should put them into an undocumented
    > function.  Otherwise, how would we document the flags argument ?  "Shows some
    > of the flags" ?  An undocumented function avoids this issue.
    
    My vote would be to have a documented function, with a minimal set of
    the flags exposed and documented, with the option to expand that in
    the future.  COMPUTED and EXPLAIN are useful, and allow some of the
    automated tests to happen.  NOT_IN_SAMPLE and GUC_NO_SHOW_ALL are less 
    useful for the user, and are more developer oriented, but are useful
    for the tests.  So having these four seem like a good first cut.
    --
    Michael
    
  33. Re: GUC flags

    Justin Pryzby <pryzby@telsasoft.com> — 2022-01-28T04:36:21Z

    On Wed, Jan 26, 2022 at 03:29:29PM +0900, Michael Paquier wrote:
    > On Tue, Jan 25, 2022 at 09:44:26PM -0600, Justin Pryzby wrote:
    > > It seems like an arbitrary and short-sighted policy to expose a handful of
    > > flags in the view for the purpose of retiring ./check_guc, but not expose other
    > > flags, because we thought we knew that no user could ever want them.
    > > 
    > > We should either expose all the flags, or should put them into an undocumented
    > > function.  Otherwise, how would we document the flags argument ?  "Shows some
    > > of the flags" ?  An undocumented function avoids this issue.
    > 
    > My vote would be to have a documented function, with a minimal set of
    > the flags exposed and documented, with the option to expand that in
    > the future.  COMPUTED and EXPLAIN are useful, and allow some of the
    > automated tests to happen.  NOT_IN_SAMPLE and GUC_NO_SHOW_ALL are less 
    > useful for the user, and are more developer oriented, but are useful
    > for the tests.  So having these four seem like a good first cut.
    
    I implemented that (But my own preference would still be for an *undocumented*
    function which returns whatever flags we find to be useful to include.  Or
    alternately, a documented function which exposes every flag).
    
    > +SELECT lower(name) FROM pg_settings_flags WHERE NOT not_in_sample EXCEPT
    > +SELECT regexp_replace(ln, '^#?([_[:alpha:]]+) (= .*|[^ ]*$)', '\1') AS guc
    > +FROM (SELECT regexp_split_to_table(pg_read_file('postgresql.conf'),
    > '\n') AS ln) conf
    > 
    > Tests reading postgresql.conf would break on instances started with a
    > custom config_file provided by a command line, no?
    
    Maybe you misunderstood - I'm not reading the file specified by
    current_setting('config_file').  Rather, I'm reading
    tmp_check/data/postgresql.conf, which is copied from the sample conf.
    Do you see an issue with that ?
    
    The regression tests are only intended run from a postgres source dir, and if
    someone runs the from somewhere else, and they "fail", I think that's because
    they violated their assumption, not because of a problem with the test.
    
    I wondered if it should chomp off anything added by pg_regress --temp-regress.
    However that's either going to be a valid guc (or else it would fail some other
    test).  Or an extention's guc (which this isn't testing), which has a dot, and
    which this regex doesn't match, so doesn't cause false positives.
    
    -- 
    Justin
    
  34. Re: GUC flags

    Michael Paquier <michael@paquier.xyz> — 2022-01-29T06:38:53Z

    On Thu, Jan 27, 2022 at 10:36:21PM -0600, Justin Pryzby wrote:
    > Maybe you misunderstood - I'm not reading the file specified by
    > current_setting('config_file').  Rather, I'm reading
    > tmp_check/data/postgresql.conf, which is copied from the sample conf.
    > Do you see an issue with that ?
    
    Yes, as of:
    mv $PGDATA/postgresql.conf $PGDATA/popo.conf
    pg_ctl start -D $PGDATA -o '-c config_file=popo.conf'
    make installcheck
    
    I have not checked, but I am pretty sure that a couple of
    distributions out there would pass down a custom path for the
    configuration file, while removing postgresql.conf from the data
    directory to avoid any confusion because if one finds out that some
    parameters are defined but not loaded.  Your patch fails on that.
    
    > The regression tests are only intended run from a postgres source dir, and if
    > someone runs the from somewhere else, and they "fail", I think that's because
    > they violated their assumption, not because of a problem with the test.
    
    The tests are able to work out on HEAD, I'd rather not break something
    that has worked this way for years.  Two other aspects that we may
    want to worry about are include_dir and include if we were to add
    tests for that, perhaps.  This last part is not really a strong
    requirement IMO, though. 
    
    > I wondered if it should chomp off anything added by pg_regress --temp-regress.
    > However that's either going to be a valid guc (or else it would fail some other
    > test).  Or an extention's guc (which this isn't testing), which has a dot, and
    > which this regex doesn't match, so doesn't cause false positives.
    
    I am not sure about those parts, being reserved about the parts that
    involve the format of postgresql.conf or any other configuration
    parts, but we could tackle that after, if necessary.
    
    For now, I have down a review of the patch, tweaking the docs, the
    code and the test to take care of all the inconsistencies I could
    find.  This looks like a good first cut to be able to remove check_guc
    (the attached removes it, but I think that we'd better treat that
    independently of the actual feature proposed, for clarity).
    --
    Michael
    
  35. Re: GUC flags

    Justin Pryzby <pryzby@telsasoft.com> — 2022-01-30T00:18:50Z

    On Sat, Jan 29, 2022 at 03:38:53PM +0900, Michael Paquier wrote:
    > +-- Three exceptions as of transaction_*
    > +SELECT name FROM pg_settings_flags
    > +  WHERE NOT no_show_all AND no_reset_all
    > +  ORDER BY 1;
    > +          name          
    > +------------------------
    > + transaction_deferrable
    > + transaction_isolation
    > + transaction_read_only
    > +(3 rows)
    
    I think "as of" is not the right phrase here.
    Maybe say: Exceptions are transaction_*
    
    > --- a/doc/src/sgml/func.sgml
    > +++ b/doc/src/sgml/func.sgml
    > @@ -23596,6 +23596,45 @@ SELECT pg_type_is_visible('myschema.widget'::regtype);
    > +       <para>
    > +        Returns an array of the flags associated with the given GUC, or
    > +        <literal>NULL</literal> if the  does not exist.  The result is
    
    I guess it should say "if the GUC does not exist".
    
    > +        an empty array if the GUC exists but there are no flags to show,
    > +        as supported by the list below.
    
    I'd say "...but none of the GUC's flags are exposed by this function."
    
    > +        The following flags are exposed (the most meaningful ones are
    > +        included):
    
    "The most meaningful ones are included" doesn't seem to add anything.
    Maybe it'd be useful to say "(Only the most useful flags are exposed)"
    
    > +          <literal>EXPLAIN</literal>, parameters included in
    > +          <command>EXPLAIN</command> commands.
    > +         </member>
    > +         <member>
    
    I think the description is wrong, or just copied from the others.
    EXPLAIN is for GUCs which are shown in EXPLAIN(SETTINGS).
    
    |EXPLAIN, parameters included in EXPLAIN commands.
    |NO_SHOW_ALL, parameters excluded from SHOW ALL commands.
    |NO_RESET_ALL, parameters excluded from RESET ALL commands.
    |NOT_IN_SAMPLE, parameters not included in postgresql.conf by default.
    |RUNTIME_COMPUTED, runtime-computed parameters. 
    
    Instead of a comma, these should use a colon, or something else?
    
    -- 
    Justin
    
    
    
    
  36. Re: GUC flags

    Michael Paquier <michael@paquier.xyz> — 2022-01-31T05:17:41Z

    On Sat, Jan 29, 2022 at 06:18:50PM -0600, Justin Pryzby wrote:
    > "The most meaningful ones are included" doesn't seem to add anything.
    > Maybe it'd be useful to say "(Only the most useful flags are exposed)"
    
    Yes, I have used something like that.
    
    > I think the description is wrong, or just copied from the others.
    > EXPLAIN is for GUCs which are shown in EXPLAIN(SETTINGS).
    
    Added some details here.
    
    > |EXPLAIN, parameters included in EXPLAIN commands.
    > |NO_SHOW_ALL, parameters excluded from SHOW ALL commands.
    > |NO_RESET_ALL, parameters excluded from RESET ALL commands.
    > |NOT_IN_SAMPLE, parameters not included in postgresql.conf by default.
    > |RUNTIME_COMPUTED, runtime-computed parameters. 
    > 
    > Instead of a comma, these should use a colon, or something else?
    
    And switched to a colon here.
    
    With all those doc fixes, applied after an extra round of review.  So
    this makes us rather covered with the checks on the flags.
    
    Now, what do we do with the rest of check_guc that involve a direct
    lookup at what's on disk.  We have the following:
    1) Check the format of the option lists in guc.c.
    2) Check the format of postgresql.conf.sample:
    -- Valid options preceded by a '#' character.
    -- Valid options followed by ' =', with at least one space before the
    equal sign.
    3) Check that options not marked as NOT_IN_SAMPLE are in the sample
    file.
    
    I have never seen 1) as a problem, and pgindent takes care of that at
    some degree.  2) is also mostly cosmetic, and committers are usually
    careful when adding a new GUC.  3) would be the most interesting
    piece, and would cover most cases if we consider that a default
    installation just copies postgresql.conf.sample over, as proposed
    upthread in 0002.
    
    Now, 3) has also the problem that it would fail installcheck as one
    can freely add a developer option in the configuration.  We could
    solve that by adding a check in a TAP test, by using pg_config
    --sharedir to find where the sample file is located.  I wonder if this
    would be a problem for some distributions, though, so adding such a
    dependency feels a bit scary even if it would mean that initdb is
    patched.
    
    As a whole, I'd like to think that we would not lose much if check_guc
    is removed.
    --
    Michael
    
  37. Re: GUC flags

    Justin Pryzby <pryzby@telsasoft.com> — 2022-01-31T22:56:45Z

    On Mon, Jan 31, 2022 at 02:17:41PM +0900, Michael Paquier wrote:
    > With all those doc fixes, applied after an extra round of review.  So
    > this makes us rather covered with the checks on the flags.
    
    Thanks
    
    > Now, what do we do with the rest of check_guc that involve a direct
    > lookup at what's on disk.  We have the following:
    > 1) Check the format of the option lists in guc.c.
    > 2) Check the format of postgresql.conf.sample:
    > -- Valid options preceded by a '#' character.
    > -- Valid options followed by ' =', with at least one space before the
    > equal sign.
    > 3) Check that options not marked as NOT_IN_SAMPLE are in the sample
    > file.
    > 
    > I have never seen 1) as a problem, and pgindent takes care of that at
    > some degree.  2) is also mostly cosmetic, and committers are usually
    > careful when adding a new GUC.  3) would be the most interesting
    > piece, and would cover most cases if we consider that a default
    > installation just copies postgresql.conf.sample over, as proposed
    > upthread in 0002.
    > 
    > Now, 3) has also the problem that it would fail installcheck as one
    > can freely add a developer option in the configuration.  We could
    
    I'm not clear on what things are required/prohibited to allow/expect
    "installcheck" to pass.  It's possible that postgresql.conf doesn't even exist
    in the data dir, right ?
    
    It's okay with me if the config_file-reading stuff isn't re-implemented.
    
    -- 
    Justin
    
    
    
    
  38. Re: GUC flags

    Michael Paquier <michael@paquier.xyz> — 2022-02-06T05:09:45Z

    On Mon, Jan 31, 2022 at 04:56:45PM -0600, Justin Pryzby wrote:
    > I'm not clear on what things are required/prohibited to allow/expect
    > "installcheck" to pass.  It's possible that postgresql.conf doesn't even exist
    > in the data dir, right ?
    
    There are no written instructions AFAIK, but I have as personal rule
    to not break the tests in configurations where they worked
    previously.
    
    > It's okay with me if the config_file-reading stuff isn't re-implemented.
    
    Actually, I am thinking that we should implement it before retiring
    completely check_guc, but not in the fashion you are suggesting.  I
    would be tempted to add something in the TAP tests as of
    src/test/misc/, where we initialize an instance to get the information
    about all the GUCs from SQL, and map that to the sample file located
    at pg_config --sharedir.  I actually have in my patch set for
    pg_upgrade's TAP a perl routine that could be used for this purpose,
    as of the following in Cluster.pm:
    
    +=item $node->config_data($option)
    +
    +Grab some data from pg_config, with $option being the command switch
    +used.
    +
    +=cut
    +
    +sub config_data
    +{
    +   my ($self, $option) = @_;
    +   local %ENV = $self->_get_env();
    +
    +   my ($stdout, $stderr);
    +   my $result =
    +     IPC::Run::run [ $self->installed_command('pg_config'), $option
    ],
    +     '>', \$stdout, '2>', \$stderr
    +     or die "could not execute pg_config";
    +   chomp($stdout);
    +   $stdout =~ s/\r$//;
    +
    +   return $stdout;
    +}
    
    What do you think?  (I was thinking about applying that separately
    anyway, to lower the load of the pg_upgrade patch a bit.)
    --
    Michael
    
  39. Re: GUC flags

    Michael Paquier <michael@paquier.xyz> — 2022-02-07T02:40:12Z

    On Sun, Feb 06, 2022 at 02:09:45PM +0900, Michael Paquier wrote:
    > Actually, I am thinking that we should implement it before retiring
    > completely check_guc, but not in the fashion you are suggesting.  I
    > would be tempted to add something in the TAP tests as of
    > src/test/misc/, where we initialize an instance to get the information
    > about all the GUCs from SQL, and map that to the sample file located
    > at pg_config --sharedir.  I actually have in my patch set for
    > pg_upgrade's TAP a perl routine that could be used for this purpose,
    > as of the following in Cluster.pm:
    
    I have been poking at that, and this is finishing to be pretty
    elegant as of the attached.  With this in place, we are able to
    cross-check GUCs marked as NOT_IN_SAMPLE (or not) with the contents of 
    postgresql.conf.sample, so as check_guc could get retired without us
    losing much.
    
    I am planning to apply the Cluster.pm part of the patch separately,
    for clarity, as I want this routine in place for some other patch.
    --
    Michael
    
  40. Re: GUC flags

    Justin Pryzby <pryzby@telsasoft.com> — 2022-02-07T03:04:14Z

    Thanks for working on it.
    
    Your test is checking that stuff in sample.conf is actually a GUC and not
    marked NOT_IN_SAMPLE.  But those are both unlikely mistakes to make.
    
    The important/interesting test is the opposite: that all GUCs are present in
    the sample file.  It's a lot easier for someone to forget to add a GUC to
    sample.conf than it is for someone to accidentally add something that isn't a
    GUC.
    
    I'd first parse the GUC-like lines in the file, making a list of gucs_in_file
    and then compare the two lists.
    
    -- 
    Justin
    
    
    
    
  41. Re: GUC flags

    Michael Paquier <michael@paquier.xyz> — 2022-02-08T01:44:07Z

    On Sun, Feb 06, 2022 at 09:04:14PM -0600, Justin Pryzby wrote:
    > Your test is checking that stuff in sample.conf is actually a GUC and not
    > marked NOT_IN_SAMPLE.  But those are both unlikely mistakes to make.
    
    Yeah, you are right.  Still, I don't see any reason to not include both.
    
    > I'd first parse the GUC-like lines in the file, making a list of gucs_in_file
    > and then compare the two lists.
    
    This is a good idea, and makes the tests faster because there is no
    need to test each GUC separately.  While testing a bit more, I got
    recalled by the fact that config_file is not marked as NOT_IN_SAMPLE
    and not in postgresql.conf.sample, so the new case you suggested was
    failing.
    
    What do you think about the updated version attached?  I have applied
    the addition of config_data() separately.
    --
    Michael
    
  42. Re: GUC flags

    Justin Pryzby <pryzby@telsasoft.com> — 2022-02-08T03:07:28Z

    On Tue, Feb 08, 2022 at 10:44:07AM +0900, Michael Paquier wrote:
    > What do you think about the updated version attached?  I have applied
    > the addition of config_data() separately.
    
    Looks fine
    
    > +	# Check if this line matches a GUC parameter.
    > +	if ($line =~ m/^#?([_[:alpha:]]+) (= .*|[^ ]*$)/)
    
    I think this is the regex I wrote to handle either "name = value" or "name
    value", which was needed between f47ed79cc..4d7c3e344.   See skip_equals.
    
    It's fine the way it is, but could also remove the 2nd half of the alternation
    (|), since GUCs shouldn't be added to sample.conf without '='.
    
    -- 
    Justin
    
    
    
    
  43. Re: GUC flags

    Michael Paquier <michael@paquier.xyz> — 2022-02-08T04:06:29Z

    On Mon, Feb 07, 2022 at 09:07:28PM -0600, Justin Pryzby wrote:
    > I think this is the regex I wrote to handle either "name = value" or "name
    > value", which was needed between f47ed79cc..4d7c3e344.   See skip_equals.
    
    Yes, I took it from there, noticing that it was working just fine for
    this purpose.
    
    > It's fine the way it is, but could also remove the 2nd half of the alternation
    > (|), since GUCs shouldn't be added to sample.conf without '='.
    
    Makes sense.  check_guc also checks after this pattern.
    --
    Michael
    
  44. Re: GUC flags

    Michael Paquier <michael@paquier.xyz> — 2022-02-09T01:19:09Z

    On Tue, Feb 08, 2022 at 01:06:29PM +0900, Michael Paquier wrote:
    > Makes sense.  check_guc also checks after this pattern.
    
    Okay, I have done all the adjustments you mentioned, added a couple of
    comments and applied the patch.  If the buildfarm is happy, I'll
    go retire check_guc.
    --
    Michael