Thread
Commits
-
Add TAP test to automate the equivalent of check_guc, take two
- 7265dbffad7f 15.0 landed
-
Retire src/backend/utils/misc/check_guc
- cf29a11ef646 15.0 landed
-
Add TAP test to automate the equivalent of check_guc
- b0a55f4d4ad5 15.0 landed
-
Add PostgreSQL::Test::Cluster::config_data()
- ba15f16107be 15.0 landed
-
Introduce pg_settings_get_flags() to find flags associated to a GUC
- d10e41d4238e 15.0 landed
-
Revert changes about warnings/errors for placeholders.
- cab5b9ab2c06 15.0 cited
-
Improve the description of various GUCs
- 03774f9bb304 15.0 landed
-
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
-
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 -
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
-
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 -
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 -
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 -
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 -
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 -
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
-
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
-
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
-
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
-
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
-
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.
-
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
-
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).
-
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.
-
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
-
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 -
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
-
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
-
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 ?
-
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 -
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
-
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 -
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 -
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 -
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.
-
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
-
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
-
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
-
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
-
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 -
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 -
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 -
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
-
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
-
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 -
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
-
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
-
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
-
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
-
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
-
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