Thread: fix stats_fetch_consistency value in postgresql.conf.sample
In postgresql.conf.sample, stats_fetch_consistency is set to "none," but the default appears to be "cache." Should these be consistent? I've attached a patch to change the entry in the sample. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
On Tue, May 24, 2022 at 03:01:47PM -0700, Nathan Bossart wrote: > In postgresql.conf.sample, stats_fetch_consistency is set to "none," but > the default appears to be "cache." Should these be consistent? I've > attached a patch to change the entry in the sample. Yes, postgresql.conf.sample should reflect the default, and that's PGSTAT_FETCH_CONSISTENCY_CACHE in guc.c. Andres, shouldn't pgstat_fetch_consistency be initialized to the same in pgstat.c? -- Michael
Attachment
At Tue, 24 May 2022 15:01:47 -0700, Nathan Bossart <nathandbossart@gmail.com> wrote in > In postgresql.conf.sample, stats_fetch_consistency is set to "none," but > the default appears to be "cache." Should these be consistent? I've > attached a patch to change the entry in the sample. Good catch:) The base C variable is inirtialized with none. The same GUC is intialized with "cache". The default valur for the GUC is "none" in the sample file. I think we set the same value to C variable. However, I wonder if it would be possible to reduce the burden of unifying the three inital values. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Hi, On 2022-05-25 13:11:40 +0900, Kyotaro Horiguchi wrote: > At Tue, 24 May 2022 15:01:47 -0700, Nathan Bossart <nathandbossart@gmail.com> wrote in > > In postgresql.conf.sample, stats_fetch_consistency is set to "none," but > > the default appears to be "cache." Should these be consistent? I've > > attached a patch to change the entry in the sample. > > Good catch:) > > The base C variable is inirtialized with none. > The same GUC is intialized with "cache". > The default valur for the GUC is "none" in the sample file. > > I think we set the same value to C variable. However, I wonder if it > would be possible to reduce the burden of unifying the three inital > values. Yes, they should be the same. I think we ended up switching the default at some point, and evidently I missed a step when doing so. Will apply. I wonder if we should make src/test/modules/test_misc/t/003_check_guc.pl detect this kind of thing? Greetings, Andres Freund
On 2022-05-24 21:23:32 -0700, Andres Freund wrote: > Will apply. And done. Thanks Nathan!
On Tue, May 24, 2022 at 09:28:49PM -0700, Andres Freund wrote: > And done. Thanks Nathan! Shouldn't you also refresh pgstat_fetch_consistency in pgstat.c for consistency? > I wonder if we should make src/test/modules/test_misc/t/003_check_guc.pl > detect this kind of thing? That sounds like a good idea to me. -- Michael
Attachment
At Wed, 25 May 2022 14:00:23 +0900, Michael Paquier <michael@paquier.xyz> wrote in
> On Tue, May 24, 2022 at 09:28:49PM -0700, Andres Freund wrote:
> > And done. Thanks Nathan!
>
> Shouldn't you also refresh pgstat_fetch_consistency in pgstat.c for
> consistency?
>
> > I wonder if we should make src/test/modules/test_misc/t/003_check_guc.pl
> > detect this kind of thing?
>
> That sounds like a good idea to me.
I think the work as assumed not to use detailed knowledge of each
variable. Due to lack of knowlege about the detail of each variable,
for example, type, unit, internal exansion, we cannot check for the
following values.
- Numbers with unit (MB, kB, s, min ...)
- internally expanded string (FILE, ConfigDir)
So it's hard to automate to check consistency of all variables, but I
found the following inconsistencies between the sample config file and
GUC default value. C default value cannot be revealed so it is
ignored.
The following results are not deeply confirmed yet.
13 apparent inconsistencies are found. These should be fixed.
archive_command = "(disabled)" != ""
bgwriter_flush_after = "64" != "0"
checkpoint_flush_after = "32" != "0"
cluster_name = "main" != ""
default_text_search_config = "pg_catalog.english" != "pg_catalog.simple"
fsync = "off" != "on"
log_replication_commands = "on" != "off"
log_statement = "all" != "none"
max_wal_senders = "0" != "10"
restart_after_crash = "off" != "on"
stats_fetch_consistency = "cache" != "none"
wal_sync_method = "fdatasync" != "fsync"
11 has letter-case inconsistencies. Are these need to be fixed?
event_source = "postgresql" != "PostgreSQL"
lc_messages = "c" != "C"
lc_monetary = "en_us.utf-8" != "C"
lc_numeric = "en_us.utf-8" != "C"
lc_time = "en_us.utf-8" != "C"
log_filename = "postgresql-%y-%m-%d_%h%m%s.log" != "postgresql-%Y-%m-%d_%H%M%S.log"
log_line_prefix = "%m [%p] %q%a " != "%m [%p] "
ssl_ciphers = "high:medium:+3des:!anull" != "HIGH:MEDIUM:+3DES:!aNULL"
ssl_min_protocol_version = "tlsv1.2" != "TLSv1.2"
syslog_facility = "local0" != "LOCAL0"
timezone_abbreviations = "default" != "Default"
The followings are the result of automatic configuration?
client_encoding = "utf8" != "sql_ascii"
data_directory = "/home/horiguti/work/postgresql/src/test/modules/test_misc/tmp_check/t_003_check_guc_main_data/pgdata"
!="ConfigDir"
hba_file =
"/home/horiguti/work/postgresql/src/test/modules/test_misc/tmp_check/t_003_check_guc_main_data/pgdata/pg_hba.conf"!=
"ConfigDir/pg_hba.conf"
ident_file =
"/home/horiguti/work/postgresql/src/test/modules/test_misc/tmp_check/t_003_check_guc_main_data/pgdata/pg_ident.conf"!=
"ConfigDir/pg_ident.conf"
krb_server_keyfile = "file:/home/horiguti/bin/pgsql_work/etc/krb5.keytab" != "FILE:${sysconfdir}/krb5.keytab"
log_timezone = "asia/tokyo" != "GMT"
timezone = "asia/tokyo" != "GMT"
unix_socket_directories = "/tmp/g3fpspvjuy" != "/tmp"
wal_buffers = "512" != "-1"
The followings are the result of TAP harness?
listen_addresses = "" != "localhost"
port = "60866" != "5432"
wal_level = "minimal" != "replica"
The following is inconsistent, but I'm not sure where the "500" came
from. In guc.c it is defined as 5000 and normal (out of TAP test)
server returns 5000.
wal_retrieve_retry_interval = "500" , "5s"
The followings cannot be automaticaly compared due to hidden unit
conversion, but looks consistent.
authentication_timeout = "60" , "1min"
autovacuum_naptime = "60" , "1min"
autovacuum_vacuum_cost_delay = "2" , "2ms"
bgwriter_delay = "200" , "200ms"
checkpoint_timeout = "300" , "5min"
checkpoint_warning = "30" , "30s"
deadlock_timeout = "1000" , "1s"
effective_cache_size = "524288" , "4GB"
gin_pending_list_limit = "4096" , "4MB"
log_autovacuum_min_duration = "600000" , "10min"
log_rotation_age = "1440" , "1d"
log_rotation_size = "10240" , "10MB"
log_startup_progress_interval = "10000" , "10s"
logical_decoding_work_mem = "65536" , "64MB"
maintenance_work_mem = "65536" , "64MB"
max_stack_depth = "2048" , "2MB"
max_standby_archive_delay = "30000" , "30s"
max_standby_streaming_delay = "30000" , "30s"
max_wal_size = "1024" , "1GB"
min_dynamic_shared_memory = "0" , "0MB"
min_parallel_index_scan_size = "64" , "512kB"
min_parallel_table_scan_size = "1024" , "8MB"
min_wal_size = "80" , "80MB"
shared_buffers = "16384" , "128MB"
temp_buffers = "1024" , "8MB"
wal_decode_buffer_size = "524288" , "512kB"
wal_receiver_status_interval = "10" , "10s"
wal_receiver_timeout = "60000" , "60s"
wal_sender_timeout = "60000" , "60s"
wal_skip_threshold = "2048" , "2MB"
wal_writer_delay = "200" , "200ms"
wal_writer_flush_after = "128" , "1MB"
work_mem = "4096" , "4MB"
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
At Wed, 25 May 2022 15:56:23 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in > The following results are not deeply confirmed yet. > > 13 apparent inconsistencies are found. These should be fixed. > > archive_command = "(disabled)" != "" The "(disabled)" is the representation of "". > bgwriter_flush_after = "64" != "0" > checkpoint_flush_after = "32" != "0" They vary according to the existence of sync_file_range(). > cluster_name = "main" != "" This is named by 003_check_guc.pl. > default_text_search_config = "pg_catalog.english" != "pg_catalog.simple" initdb decided this. > fsync = "off" != "on" > log_line_prefix = "%m [%p] %q%a " != "%m [%p] " > log_replication_commands = "on" != "off" > log_statement = "all" != "none" > max_wal_senders = "0" != "10" > restart_after_crash = "off" != "on" These are set by Cluster.pm. > wal_sync_method = "fdatasync" != "fsync" This is platform dependent. > stats_fetch_consistency = "cache" != "none" This has been fixed recently. > 11 has letter-case inconsistencies. Are these need to be fixed? > > event_source = "postgresql" != "PostgreSQL" > lc_messages = "c" != "C" > lc_monetary = "en_us.utf-8" != "C" > lc_numeric = "en_us.utf-8" != "C" > lc_time = "en_us.utf-8" != "C" > log_filename = "postgresql-%y-%m-%d_%h%m%s.log" != "postgresql-%Y-%m-%d_%H%M%S.log" > ssl_ciphers = "high:medium:+3des:!anull" != "HIGH:MEDIUM:+3DES:!aNULL" > ssl_min_protocol_version = "tlsv1.2" != "TLSv1.2" > syslog_facility = "local0" != "LOCAL0" > timezone_abbreviations = "default" != "Default" These are harmless. Since no significant inconsistency is found, there's no need to fix these either. (sigh..) As the result, no need to fix in this area for now, and I don't think there's any generic and reliable way to detect inconsistencies of guc variable definitions. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On Wed, May 25, 2022 at 04:12:07PM +0900, Kyotaro Horiguchi wrote: > (sigh..) As the result, no need to fix in this area for now, and I > don't think there's any generic and reliable way to detect > inconsistencies of guc variable definitions. Hmm. Making the automation test painless in terms of maintenance consists in making it require zero manual filtering in the list of GUCs involved, while still being useful in what it can detect. The units involved in a GUC make the checks between postgresql.conf.sample and pg_settings.boot_value annoying because they would require extra calculations depending on the unit with a logic maintained in the test. I may be missing something obvious, of course, but it seems to me that as long as you fetch the values from postgresql.conf.sample and cross-check them with pg_settings.boot_value for GUCs that do not have units, the maintenance would be painless, while still being useful (it would cover the case of enums, for one). The values need to be lower-cased for consistency, similarly to the GUC names. -- Michael
Attachment
At Thu, 26 May 2022 08:53:55 +0900, Michael Paquier <michael@paquier.xyz> wrote in > On Wed, May 25, 2022 at 04:12:07PM +0900, Kyotaro Horiguchi wrote: > > (sigh..) As the result, no need to fix in this area for now, and I > > don't think there's any generic and reliable way to detect > > inconsistencies of guc variable definitions. > > Hmm. Making the automation test painless in terms of maintenance > consists in making it require zero manual filtering in the list of > GUCs involved, while still being useful in what it can detect. The > units involved in a GUC make the checks between postgresql.conf.sample > and pg_settings.boot_value annoying because they would require extra > calculations depending on the unit with a logic maintained in the > test. > > I may be missing something obvious, of course, but it seems to me that > as long as you fetch the values from postgresql.conf.sample and > cross-check them with pg_settings.boot_value for GUCs that do not have > units, the maintenance would be painless, while still being useful (it > would cover the case of enums, for one). The values need to be > lower-cased for consistency, similarly to the GUC names. Yeah, "boot_val" is appropreate here. And I noticed that pg_settings has the "unit" field. I'll try using them. Thanks for the suggestion! regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On Thu, May 26, 2022 at 11:10:18AM +0900, Kyotaro Horiguchi wrote: > Yeah, "boot_val" is appropreate here. And I noticed that pg_settings > has the "unit" field. I'll try using them. I wrote this in guc.sql, which seems promising, but it needs to be rewritten in check_guc.pl to access postgresql.conf from the source tree. Do you want to handle that ? +\getenv abs_srcdir PG_ABS_SRCDIR +\set filename :abs_srcdir '../../../../src/backend/utils/misc/postgresql.conf.sample' + +begin; +CREATE TEMP TABLE sample_conf AS +-- SELECT m[1] AS name, trim(BOTH '''' FROM m[3]) AS sample_value +SELECT m[1] AS name, COALESCE(m[3], m[5]) AS sample_value +FROM (SELECT regexp_split_to_table(pg_read_file(:'filename'), '\n') AS ln) conf, +-- regexp_match(ln, '^#?([_[:alpha:]]+) (= ([^[:space:]]*)|[^ ]*$).*') AS m +regexp_match(ln, '^#?([_[:alpha:]]+) (= ''([^'']*)''|(= ([^[:space:]]*))|[^ ]*$).*') AS m +WHERE ln ~ '^#?[[:alpha:]]'; + +-- test that GUCs in postgresql.conf have correct default values +SELECT name, tsf.cooked_value, sc.sample_value +FROM tab_settings_flags tsf JOIN sample_conf sc USING(name) +WHERE NOT not_in_sample AND tsf.cooked_value != sc.sample_value AND tsf.cooked_value||'.0' != sc.sample_value +ORDER BY 1; +rollback; It detects the original problem: stats_fetch_consistency | cache | none And I think these should be updated it postgresql.conf to use the same unit as in current_setting(). track_activity_query_size | 1kB | 1024 wal_buffers | 4MB | -1 wal_receiver_timeout | 1min | 60s wal_sender_timeout | 1min | 60s -- Justin
At Wed, 25 May 2022 21:25:53 -0500, Justin Pryzby <pryzby@telsasoft.com> wrote in > On Thu, May 26, 2022 at 11:10:18AM +0900, Kyotaro Horiguchi wrote: > > Yeah, "boot_val" is appropreate here. And I noticed that pg_settings > > has the "unit" field. I'll try using them. > > I wrote this in guc.sql, which seems promising, but it needs to be rewritten in > check_guc.pl to access postgresql.conf from the source tree. Do you want to > handle that ? Yes. > +\getenv abs_srcdir PG_ABS_SRCDIR > +\set filename :abs_srcdir '../../../../src/backend/utils/misc/postgresql.conf.sample' > + > +begin; > +CREATE TEMP TABLE sample_conf AS > +-- SELECT m[1] AS name, trim(BOTH '''' FROM m[3]) AS sample_value > +SELECT m[1] AS name, COALESCE(m[3], m[5]) AS sample_value > +FROM (SELECT regexp_split_to_table(pg_read_file(:'filename'), '\n') AS ln) conf, > +-- regexp_match(ln, '^#?([_[:alpha:]]+) (= ([^[:space:]]*)|[^ ]*$).*') AS m > +regexp_match(ln, '^#?([_[:alpha:]]+) (= ''([^'']*)''|(= ([^[:space:]]*))|[^ ]*$).*') AS m > +WHERE ln ~ '^#?[[:alpha:]]'; > + > +-- test that GUCs in postgresql.conf have correct default values > +SELECT name, tsf.cooked_value, sc.sample_value > +FROM tab_settings_flags tsf JOIN sample_conf sc USING(name) > +WHERE NOT not_in_sample AND tsf.cooked_value != sc.sample_value AND tsf.cooked_value||'.0' != sc.sample_value > +ORDER BY 1; > +rollback; > > It detects the original problem: > > stats_fetch_consistency | cache | none Yeah, it is a straight forward outcome. > And I think these should be updated it postgresql.conf to use the same unit as > in current_setting(). > > track_activity_query_size | 1kB | 1024 > wal_buffers | 4MB | -1 > wal_receiver_timeout | 1min | 60s > wal_sender_timeout | 1min | 60s I'm not sure we should do so. Rather I'd prefer 60s than 1min here. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
At Thu, 26 May 2022 13:00:45 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in > At Wed, 25 May 2022 21:25:53 -0500, Justin Pryzby <pryzby@telsasoft.com> wrote in > > And I think these should be updated it postgresql.conf to use the same unit as > > in current_setting(). > > > > track_activity_query_size | 1kB | 1024 > > wal_buffers | 4MB | -1 > > wal_receiver_timeout | 1min | 60s > > wal_sender_timeout | 1min | 60s > > I'm not sure we should do so. Rather I'd prefer 60s than 1min here. It could be in SQL, but *I* prefer to use perl for this, since it allows me to write a bit complex things (than simple string comparison) simpler. So the attached is a wip version of that Numeric values are compared considering units. But does not require the units of the both values to match. Some variables are ignored by an explicit instruction (ignored_parameters). Some variables are compared case-insensitively by an explicit instruction (case_insensitive_params). bool and enum are compared case-insensitively automatically. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Attachment
Hi, On 2022-05-25 14:00:23 +0900, Michael Paquier wrote: > On Tue, May 24, 2022 at 09:28:49PM -0700, Andres Freund wrote: > > And done. Thanks Nathan! > > Shouldn't you also refresh pgstat_fetch_consistency in pgstat.c for > consistency? Yes. Now that the visible sheen of embarassment in my face has subsided a bit (and pgcon has ended), I pushed this bit too. - Andres
Hi,
On 2022-05-26 16:27:53 +0900, Kyotaro Horiguchi wrote:
> It could be in SQL, but *I* prefer to use perl for this, since it
> allows me to write a bit complex things (than simple string
> comparison) simpler.
I wonder if we shouldn't just expose a C function to do this, rather than
having a separate implementation in a tap test.
> +# parameter names that cannot get consistency check performed
> +my @ignored_parameters =
I think most of these we could ignore by relying on source <> 'override'
instead of listing them?
> +# parameter names that requires case-insensitive check
> +my @case_insensitive_params =
> + ('ssl_ciphers',
> + 'log_filename',
> + 'event_source',
> + 'log_timezone',
> + 'timezone',
> + 'lc_monetary',
> + 'lc_numeric',
> + 'lc_time');
Why do these differ by case?
Greetings,
Andres Freund
On Sat, May 28, 2022 at 01:14:09PM -0700, Andres Freund wrote: > I pushed this bit too. Thanks for taking care of that! -- Michael
Attachment
At Sat, 28 May 2022 13:22:45 -0700, Andres Freund <andres@anarazel.de> wrote in
> Hi,
>
> On 2022-05-26 16:27:53 +0900, Kyotaro Horiguchi wrote:
> > It could be in SQL, but *I* prefer to use perl for this, since it
> > allows me to write a bit complex things (than simple string
> > comparison) simpler.
>
> I wonder if we shouldn't just expose a C function to do this, rather than
> having a separate implementation in a tap test.
It was annoying that I needed to copy the unit-conversion stuff. I
did that in the attached. parse_val() and check_val() and the duped
data is removed.
> > +# parameter names that cannot get consistency check performed
> > +my @ignored_parameters =
>
> I think most of these we could ignore by relying on source <> 'override'
> instead of listing them?
>
>
> > +# parameter names that requires case-insensitive check
> > +my @case_insensitive_params =
> > + ('ssl_ciphers',
> > + 'log_filename',
> > + 'event_source',
> > + 'log_timezone',
> > + 'timezone',
> > + 'lc_monetary',
> > + 'lc_numeric',
> > + 'lc_time');
>
> Why do these differ by case?
Mmm. It just came out of a thinko. I somehow believed that the script
down-cases only the parameter names among the values from
pg_settings. I felt that something's strange while on it,
though.. After fixing it, there are only the following values that
differ only in letter cases. In passing I changed "bool" and "enum"
are case-sensitive, too.
name conf bootval
client_encoding: "sql_ascii" "SQL_ASCII"
datestyle : "iso, mdy" "ISO, MDY"
syslog_facility: "LOCAL0" "local0"
It seems to me that the bootval is right for all variables.
I added a testing-aid function pg_normalize_config_option(name,value)
so the consistency check can be performed like this.
SELECT f.n, f.v, s.boot_val
FROM (VALUES ('work_mem','4MB'),...) f(n,v)
JOIN pg_settings s ON s.name = f.n '.
WHERE pg_normalize_config_value(f.n, f.v) <> '.
pg_normalize_config_value(f.n, s.boot_val)';
There're some concerns on the function.
- _ShowConfig() returns archive_command as "(disabled)" regardless of
its value. The test passes accidentally for the variable...
- _ShowConfig() errors out for "timezone_abbreviations" and "" since
the check function tries to open the timezone file. (It is excluded
from the test.)
I don't want to create a copy of the function only for this purpose.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Attachment
On Mon, May 30, 2022 at 05:27:19PM +0900, Kyotaro Horiguchi wrote: > At Sat, 28 May 2022 13:22:45 -0700, Andres Freund <andres@anarazel.de> wrote in > > Hi, > > > > On 2022-05-26 16:27:53 +0900, Kyotaro Horiguchi wrote: > > > It could be in SQL, but *I* prefer to use perl for this, since it > > > allows me to write a bit complex things (than simple string > > > comparison) simpler. > > > > I wonder if we shouldn't just expose a C function to do this, rather than > > having a separate implementation in a tap test. > > It was annoying that I needed to copy the unit-conversion stuff. I > did that in the attached. parse_val() and check_val() and the duped > data is removed. Note that this gives: guc.c:7573:9: warning: ‘dst’ may be used uninitialized in this function [-Wmaybe-uninitialized] with gcc version 9.2.1 20191008 (Ubuntu 9.2.1-9ubuntu2) I wonder whether you'd consider renaming pg_normalize_config_value() to pg_pretty_config_value() or similar. -- Justin
On Sat, Jun 11, 2022 at 09:41:37AM -0500, Justin Pryzby wrote: > Note that this gives: > > guc.c:7573:9: warning: ‘dst’ may be used uninitialized in this function [-Wmaybe-uninitialized] > > with gcc version 9.2.1 20191008 (Ubuntu 9.2.1-9ubuntu2) > > I wonder whether you'd consider renaming pg_normalize_config_value() to > pg_pretty_config_value() or similar. I have looked at the patch, and I am not convinced that we need a function that does a integer -> integer-with-unit conversion for the purpose of this test. One thing is that it can be unstable with the unit in the conversion where values are close to a given threshold (aka for cases like 2048kB/2MB). On top of that, this overlaps with the existing system function in charge of converting values with bytes as size unit, while this stuff handles more unit types and all GUC types. I think that there could be some room in doing the opposite conversion, feeding the value from postgresql.conf.sample to something and compare it directly with boot_val. That's solvable at SQL level, still a system function may be more user-friendly. Extending the tests to check after the values is something worth doing, but I think that I would limit the checks to the parameters that do not have units for now, until we figure out which interface would be more adapted for doing the normalization of the parameter values. -#syslog_facility = 'LOCAL0' +#syslog_facility = 'local0' Those changes should not be necessary in postgresql.conf.sample. The test should be in charge of applying the lower() conversion, in the same way as guc.c does internally, and that's a mode supported by the parameter parsing. Using an upper-case value in the sample file is actually meaningful sometimes (for example, syslog can use upper-case strings to refer to LOCAL0~7). -- Michael
Attachment
At Thu, 16 Jun 2022 12:07:03 +0900, Michael Paquier <michael@paquier.xyz> wrote in
> On Sat, Jun 11, 2022 at 09:41:37AM -0500, Justin Pryzby wrote:
> > Note that this gives:
> >
> > guc.c:7573:9: warning: ‘dst’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> >
> > with gcc version 9.2.1 20191008 (Ubuntu 9.2.1-9ubuntu2)
> >
> > I wonder whether you'd consider renaming pg_normalize_config_value() to
> > pg_pretty_config_value() or similar.
>
> I have looked at the patch, and I am not convinced that we need a
> function that does a integer -> integer-with-unit conversion for the
> purpose of this test. One thing is that it can be unstable with the
> unit in the conversion where values are close to a given threshold
> (aka for cases like 2048kB/2MB). On top of that, this overlaps with
I agree that needing to-with-unit conversion is a bit crumsy. One of
the reason is I didn't want to add a function that has no use of other
than testing,
> the existing system function in charge of converting values with bytes
> as size unit, while this stuff handles more unit types and all GUC
> types. I think that there could be some room in doing the opposite
> conversion, feeding the value from postgresql.conf.sample to something
> and compare it directly with boot_val. That's solvable at SQL level,
> still a system function may be more user-friendly.
The output value must be the same with what pg_settings shows, so it
needs to take in some code from GetConfigOptionByNum() (and needs to
keep in-sync with it), which is what I didn't wnat to do. Anyway done
in the attached.
This method has a problem for wal_buffers. parse_and_validate_value()
returns 512 for -1 input since check_wal_buffers() converts it to 512.
It is added to the exclusion list. (Conversely the previous method
regarded "-1" and "512" as identical.)
> Extending the tests to check after the values is something worth
> doing, but I think that I would limit the checks to the parameters
> that do not have units for now, until we figure out which interface
> would be more adapted for doing the normalization of the parameter
> values.
The attached second is that. FWIW, I'd like to support integer/real
values since I think they need more support of this kind of check.
> -#syslog_facility = 'LOCAL0'
> +#syslog_facility = 'local0'
> Those changes should not be necessary in postgresql.conf.sample. The
> test should be in charge of applying the lower() conversion, in the
> same way as guc.c does internally, and that's a mode supported by the
> parameter parsing. Using an upper-case value in the sample file is
> actually meaningful sometimes (for example, syslog can use upper-case
> strings to refer to LOCAL0~7).
I didn't notice, but now know parse_and_validate_value() convers
values the same way with bootval so finally case-unification is not
needed.
=# select pg_config_unitless_value('datestyle', 'iso, mdy');
pg_config_unitless_value
--------------------------
ISO, MDY
However, the "datestyle" variable is shown as "DateStyle" in the
pg_settings view. So the name in the view needs to be lower-cased
instead. The same can be said to "TimeZone" and "IntervalStyle". The
old query missed the case where there's no variable with the names
appear in the config file. Fixed it.
At Sat, 11 Jun 2022 09:41:37 -0500, Justin Pryzby <pryzby@telsasoft.com> wrote in
> Note that this gives:
>
> guc.c:7573:9: warning: ‘dst’ may be used uninitialized in this function [-Wmaybe-uninitialized]
Mmm. I don't have an idea where the 'dst' came from...
> I wonder whether you'd consider renaming pg_normalize_config_value() to
> pg_pretty_config_value() or similar.
Yeah, that's sensible, the function is now changed (not renamed) to
pg_config_unitless_value(). This name also doesn't satisfies me at
all..:(
So, the attached are:
v2-0001-Add-fileval-bootval-consistency-check-of-GUC-para.patch:
New version of the previous patch. It is changed after Michael's
suggestions.
0001-Add-fileval-bootval-consistency-check-of-GUC-paramet-simple.patch
Another version that doesn't need new C function. It ignores
variables that have units but I didn't count how many variables are
ignored by this chnage.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Attachment
On Thu, Jun 16, 2022 at 05:19:46PM +0900, Kyotaro Horiguchi wrote: > At Sat, 11 Jun 2022 09:41:37 -0500, Justin Pryzby <pryzby@telsasoft.com> wrote in > > Note that this gives: > > > > guc.c:7573:9: warning: ‘dst’ may be used uninitialized in this function [-Wmaybe-uninitialized] > > Mmm. I don't have an idea where the 'dst' came from... Well, in your latest patch, you've renamed it. guc.c:7586:19: warning: ‘result’ may be used uninitialized in this function [-Wmaybe-uninitialized] 7586 | PG_RETURN_TEXT_P(cstring_to_text(result)); -- Justin
At Thu, 16 Jun 2022 08:23:07 -0500, Justin Pryzby <pryzby@telsasoft.com> wrote in > On Thu, Jun 16, 2022 at 05:19:46PM +0900, Kyotaro Horiguchi wrote: > > At Sat, 11 Jun 2022 09:41:37 -0500, Justin Pryzby <pryzby@telsasoft.com> wrote in > > > Note that this gives: > > > > > > guc.c:7573:9: warning: ‘dst’ may be used uninitialized in this function [-Wmaybe-uninitialized] > > > > Mmm. I don't have an idea where the 'dst' came from... > > Well, in your latest patch, you've renamed it. > > guc.c:7586:19: warning: ‘result’ may be used uninitialized in this function [-Wmaybe-uninitialized] > 7586 | PG_RETURN_TEXT_P(cstring_to_text(result)); Ooo. I find that the patch on my hand was different from that on this list by some reason uncertain to me. I now understand what's happening. At Sat, 11 Jun 2022 09:41:37 -0500, Justin Pryzby <pryzby@telsasoft.com> wrote in > with gcc version 9.2.1 20191008 (Ubuntu 9.2.1-9ubuntu2) My compiler (gcc 8.5.0) (with -Wswitch) is satisfied by finding that the switch() covers all enum values. I don't know why the new compiler complains with this, but compilers in such environment should shut up by the following change. - char *result; + char *result = ""; regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Attachment
Hi,
On 2022-06-17 09:43:58 +0900, Kyotaro Horiguchi wrote:
> +/*
> + * Convert value to unitless value according to the specified GUC variable
> + */
> +Datum
> +pg_config_unitless_value(PG_FUNCTION_ARGS)
> +{
> + char *name = "";
> + char *value = "";
> + struct config_generic *record;
> + char *result = "";
> + void *extra;
> + union config_var_val val;
> + const char *p;
> + char buffer[256];
> +
> + if (!PG_ARGISNULL(0))
> + name = text_to_cstring(PG_GETARG_TEXT_PP(0));
> + if (!PG_ARGISNULL(1))
> + value = text_to_cstring(PG_GETARG_TEXT_PP(1));
> +
> + record = find_option(name, true, false, ERROR);
> +
> + parse_and_validate_value(record, name, value, PGC_S_TEST, WARNING,
> + &val, &extra);
> +
Hm. I think this should error out for options that the user doesn't have the
permissions for - good. I suggest adding a test for that.
> + switch (record->vartype)
> + {
> + case PGC_BOOL:
> + result = (val.boolval ? "on" : "off");
> + break;
> + case PGC_INT:
> + snprintf(buffer, sizeof(buffer), "%d", val.intval);
> + result = pstrdup(buffer);
> + break;
> + case PGC_REAL:
> + snprintf(buffer, sizeof(buffer), "%g", val.realval);
> + result = pstrdup(buffer);
> + break;
> + case PGC_STRING:
> + p = val.stringval;
> + if (p == NULL)
> + p = "";
> + result = pstrdup(p);
> + break;
Is this a good idea? I wonder if we shouldn't instead return NULL, rather than
making NULL and "" undistinguishable.
Not that it matters for efficiency here, but why are you pstrdup'ing the
buffers? cstring_to_text() will already copy the string, no?
> +# parameter names that cannot get consistency check performed
> +my @ignored_parameters = (
Perhaps worth adding comments explaining why these can't get checked?
> +foreach my $line (split("\n", $all_params))
> +{
> + my @f = split('\|', $line);
> + fail("query returned wrong number of columns: $#f : $line") if ($#f != 4);
> + $all_params_hash{$f[0]}->{type} = $f[1];
> + $all_params_hash{$f[0]}->{unit} = $f[2];
> + $all_params_hash{$f[0]}->{bootval} = $f[3];
> +}
>
Might look a bit nicer to generate the hash in a local variable and then
assign to $all_params_hash{$f[0]} once, rather than repeating that part
multiple times.
> - if ($line =~ m/^#?([_[:alpha:]]+) = .*/)
> + if ($line =~ m/^#?([_[:alpha:]]+) = (.*)$/)
> {
> # Lower-case conversion matters for some of the GUCs.
> my $param_name = lc($1);
>
> + # extract value
> + my $file_value = $2;
> + $file_value =~ s/\s*#.*$//; # strip trailing comment
> + $file_value =~ s/^'(.*)'$/$1/; # strip quotes
> +
> # Ignore some exceptions.
> next if $param_name eq "include";
> next if $param_name eq "include_dir";
So there's now two ignore mechanisms? Why not just handle include[_dir] via
@ignored_parameters?
> @@ -66,19 +94,39 @@ while (my $line = <$contents>)
> # Update the list of GUCs found in the sample file, for the
> # follow-up tests.
> push @gucs_in_file, $param_name;
> +
> + # Check for consistency between bootval and file value.
You're not checking the consistency here though?
> + if (!grep { $_ eq $param_name } @ignored_parameters)
> + {
> + push (@check_elems, "('$param_name','$file_value')");
> + }
> }
> }
>
> close $contents;
>
> +# Run consistency check between config-file's default value and boot
> +# values. To show sample setting that is not found in the view, use
> +# LEFT JOIN and make sure pg_settings.name is not NULL.
> +my $check_query =
> + 'SELECT f.n, f.v, s.boot_val FROM (VALUES '.
> + join(',', @check_elems).
> + ') f(n,v) LEFT JOIN pg_settings s ON lower(s.name) = f.n '.
> + "WHERE pg_config_unitless_value(f.n, f.v) <> COALESCE(s.boot_val, '') ".
> + 'OR s.name IS NULL';
> +
> +print $check_query;
> +
> +is ($node->safe_psql('postgres', $check_query), '',
> + 'check if fileval-bootval consistency is fine');
"fileval-bootval" isn't that easy to understand, "is fine" doesn't quite sound
right. Maybe something like "GUC values in .sample and boot value match"?
I wonder if it'd not result in easier to understand output if the query just
called pg_config_unitless_value() for all the .sample values, but then did the
comparison of the results in perl.
Greetings,
Andres Freund
Thanks!
At Wed, 22 Jun 2022 16:07:10 -0700, Andres Freund <andres@anarazel.de> wrote in
> Hi,
>
> On 2022-06-17 09:43:58 +0900, Kyotaro Horiguchi wrote:
> > +/*
> > + * Convert value to unitless value according to the specified GUC variable
> > + */
> > +Datum
> > +pg_config_unitless_value(PG_FUNCTION_ARGS)
> > +{
...
> > + record = find_option(name, true, false, ERROR);
> > +
> > + parse_and_validate_value(record, name, value, PGC_S_TEST, WARNING,
> > + &val, &extra);
> > +
>
> Hm. I think this should error out for options that the user doesn't have the
> permissions for - good. I suggest adding a test for that.
Generally sounds reasonable but it doesn't reveal its setting. It
just translates (or decodes) given string to internal value. And
currently most of all values are string and only two are enum (TLS
version), that are returned almost as-is. That being said, the
suggested behavior seems better. So I did that in the attached.
And I added the test for this to rolenames in modules/unsafe_tests.
> > + switch (record->vartype)
> > + {
> > + case PGC_BOOL:
> > + result = (val.boolval ? "on" : "off");
> > + break;
> > + case PGC_INT:
> > + snprintf(buffer, sizeof(buffer), "%d", val.intval);
> > + result = pstrdup(buffer);
> > + break;
> > + case PGC_REAL:
> > + snprintf(buffer, sizeof(buffer), "%g", val.realval);
> > + result = pstrdup(buffer);
> > + break;
> > + case PGC_STRING:
> > + p = val.stringval;
> > + if (p == NULL)
> > + p = "";
> > + result = pstrdup(p);
> > + break;
>
> Is this a good idea? I wonder if we shouldn't instead return NULL, rather than
> making NULL and "" undistinguishable.
Anyway NULL cannot be seen there and I don't recall the reason I made
the function non-strict. I changed the SQL function back to 'strict',
which makes things cleaner and simpler.
> Not that it matters for efficiency here, but why are you pstrdup'ing the
> buffers? cstring_to_text() will already copy the string, no?
Right. That's a silly thinko, omitting that behavior..
>
> > +# parameter names that cannot get consistency check performed
> > +my @ignored_parameters = (
>
> Perhaps worth adding comments explaining why these can't get checked?
Mmm. I agree. I rewrote it as the follows.
> # The following parameters are defaultly set with
> # environment-dependent values which may not match the default values
> # written in the sample config file.
> > +foreach my $line (split("\n", $all_params))
> > +{
> > + my @f = split('\|', $line);
> > + fail("query returned wrong number of columns: $#f : $line") if ($#f != 4);
> > + $all_params_hash{$f[0]}->{type} = $f[1];
> > + $all_params_hash{$f[0]}->{unit} = $f[2];
> > + $all_params_hash{$f[0]}->{bootval} = $f[3];
> > +}
> >
>
> Might look a bit nicer to generate the hash in a local variable and then
> assign to $all_params_hash{$f[0]} once, rather than repeating that part
> multiple times.
Yeah, but I noticed that that hash is no longer needed..
> > - if ($line =~ m/^#?([_[:alpha:]]+) = .*/)
> > + if ($line =~ m/^#?([_[:alpha:]]+) = (.*)$/)
> > {
> > # Lower-case conversion matters for some of the GUCs.
> > my $param_name = lc($1);
> >
> > + # extract value
> > + my $file_value = $2;
> > + $file_value =~ s/\s*#.*$//; # strip trailing comment
> > + $file_value =~ s/^'(.*)'$/$1/; # strip quotes
> > +
> > # Ignore some exceptions.
> > next if $param_name eq "include";
> > next if $param_name eq "include_dir";
>
> So there's now two ignore mechanisms? Why not just handle include[_dir] via
> @ignored_parameters?
The two ignore mechanisms work for different arrays. So we need to
distinct between the two uses. I tried that but it looks like
reseparating particles that were uselessly mixed. Finally I changed
the variable to hash and apply the same mechanism to "include" and
friends but by using different hash.
> > @@ -66,19 +94,39 @@ while (my $line = <$contents>)
> > # Update the list of GUCs found in the sample file, for the
> > # follow-up tests.
> > push @gucs_in_file, $param_name;
> > +
> > + # Check for consistency between bootval and file value.
>
> You're not checking the consistency here though?
Mmm. Right. I reworded it following the comment just above.
> > + if (!grep { $_ eq $param_name } @ignored_parameters)
> > + {
> > + push (@check_elems, "('$param_name','$file_value')");
> > + }
> > }
> > }
>
> >
> > close $contents;
> >
> > +# Run consistency check between config-file's default value and boot
> > +# values. To show sample setting that is not found in the view, use
> > +# LEFT JOIN and make sure pg_settings.name is not NULL.
> > +my $check_query =
> > + 'SELECT f.n, f.v, s.boot_val FROM (VALUES '.
> > + join(',', @check_elems).
> > + ') f(n,v) LEFT JOIN pg_settings s ON lower(s.name) = f.n '.
> > + "WHERE pg_config_unitless_value(f.n, f.v) <> COALESCE(s.boot_val, '') ".
> > + 'OR s.name IS NULL';
> > +
> > +print $check_query;
> > +
> > +is ($node->safe_psql('postgres', $check_query), '',
> > + 'check if fileval-bootval consistency is fine');
>
> "fileval-bootval" isn't that easy to understand, "is fine" doesn't quite sound
> right. Maybe something like "GUC values in .sample and boot value match"?
No objection. Changed.
> I wonder if it'd not result in easier to understand output if the query just
> called pg_config_unitless_value() for all the .sample values, but then did the
> comparison of the results in perl.
It is a fair alternative. I said exactly the same thing (perl is
easier to understand than the same (procedual) logic in SQL)
upthread:p So done that in the attached.
I tempted to find extra filevals by the code added here but it is
cleaner to leave it to the existing checking code.
- Change the behavior of pg_config_unitless_value according to the comment.
- and added the test for the function's behavior about privileges.
- Skip "include" and friends by using a hash similar to ignore_parameters.
- Removed %all_params_hash. (Currently it is @file_vals)
- A comment reworded (but it donesn't look fine..).
- Moved value-check logic from SQL to perl.
And I'll add this to the coming CF.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Attachment
> +# The following parameters are defaultly set with
> +# environment-dependent values at run-time which may not match the
> +# default values written in the sample config file.
> +my %ignore_parameters =
> + map { $_ => 1 } (
> + 'data_directory',
> + 'hba_file',
> + 'ident_file',
> + 'krb_server_keyfile',
> + 'max_stack_depth',
> + 'bgwriter_flush_after',
> + 'wal_sync_method',
> + 'checkpoint_flush_after',
> + 'timezone_abbreviations',
> + 'lc_messages',
> + 'wal_buffers');
How did you make this list ? Was it by excluding things that failed for you ?
cfbot is currently failing due to io_concurrency on windows.
I think there are more GUC which should be included here.
http://cfbot.cputube.org/kyotaro-horiguchi.html
--
Justin
On Wed, Jul 13, 2022 at 12:30:00PM -0500, Justin Pryzby wrote: > How did you make this list ? Was it by excluding things that failed for you ? > > cfbot is currently failing due to io_concurrency on windows. > I think there are more GUC which should be included here. > > http://cfbot.cputube.org/kyotaro-horiguchi.html FWIW, I am not really a fan of making this test depend on a hardcoded list of GUCs. The design strength of the existing test is that we don't have such a dependency now, making less to think about in terms of maintenance in the long-term, even if this is now run automatically. -- Michael
Attachment
Hi, On 2022-07-14 08:46:02 +0900, Michael Paquier wrote: > On Wed, Jul 13, 2022 at 12:30:00PM -0500, Justin Pryzby wrote: > > How did you make this list ? Was it by excluding things that failed for you ? > > > > cfbot is currently failing due to io_concurrency on windows. > > I think there are more GUC which should be included here. > > > > http://cfbot.cputube.org/kyotaro-horiguchi.html > > FWIW, I am not really a fan of making this test depend on a hardcoded > list of GUCs. I wonder if we should add flags indicating platform dependency etc to guc.c? That should allow to remove most of them? > The design strength of the existing test is that we > don't have such a dependency now, making less to think about in terms > of maintenance in the long-term, even if this is now run > automatically. There's no existing test for things covered by these exceptions, unless I am missing something? Greetings, Andres Freund
On Thu, Jul 14, 2022 at 08:46:02AM +0900, Michael Paquier wrote: > On Wed, Jul 13, 2022 at 12:30:00PM -0500, Justin Pryzby wrote: > > How did you make this list ? Was it by excluding things that failed for you ? > > > > cfbot is currently failing due to io_concurrency on windows. > > I think there are more GUC which should be included here. > > > > http://cfbot.cputube.org/kyotaro-horiguchi.html > > FWIW, I am not really a fan of making this test depend on a hardcoded > list of GUCs. The design strength of the existing test is that we > don't have such a dependency now, making less to think about in terms > of maintenance in the long-term, even if this is now run > automatically. It doesn't really need to be stated that an inclusive list wouldn't be useful. That's a list of GUCs to be excluded. Which is hardly different from the pre-existing list of exceptions. # Ignore some exceptions. next if $param_name eq "include"; next if $param_name eq "include_dir"; next if $param_name eq "include_if_exists"; -- Exceptions are transaction_*. SELECT name FROM tab_settings_flags WHERE NOT no_show_all AND no_reset_all ORDER BY 1; name ------------------------ transaction_deferrable transaction_isolation transaction_read_only (3 rows) How else do you propose to make this work for guc whose defaults vary by platform in guc.c or in initdb ? -- Justin
At Wed, 13 Jul 2022 18:54:45 -0500, Justin Pryzby <pryzby@telsasoft.com> wrote in > On Thu, Jul 14, 2022 at 08:46:02AM +0900, Michael Paquier wrote: > > On Wed, Jul 13, 2022 at 12:30:00PM -0500, Justin Pryzby wrote: > > > How did you make this list ? Was it by excluding things that failed for you ? Yes. I didn't confirm each variable. They are the variables differ on RHEL-family OSes. io_concurrency differs according to USE_PREFETCH. Regarding to effects of macro definitions, I searched guc.c for non-GUC_NOT_IN_SAMPLE variables with macro-affected defaults. WIN32 affects update_process_title USE_PREFETCH affects effective_io_concurrency and maintenance_io_concurrency HAVE_UNIX_SOCKETS affects unix_socket_directories and unix_socket_directories USE_SSL affects ssl_ecdh_curve USE_OPENSSL affects ssl_ciphers HAVE_SYSLOG affects syslog_facility Different from most of the variables already in the exclusion list, these could be changed at build time, but I haven't found a sensible way to do that. Otherwise we need to add them to the exclusion list... > > > cfbot is currently failing due to io_concurrency on windows. > > > I think there are more GUC which should be included here. > > > > > > http://cfbot.cputube.org/kyotaro-horiguchi.html > > > > FWIW, I am not really a fan of making this test depend on a hardcoded > > list of GUCs. The design strength of the existing test is that we > > don't have such a dependency now, making less to think about in terms > > of maintenance in the long-term, even if this is now run > > automatically. > > It doesn't really need to be stated that an inclusive list wouldn't be useful. +1 > That's a list of GUCs to be excluded. > Which is hardly different from the pre-existing list of exceptions. > > # Ignore some exceptions. > next if $param_name eq "include"; > next if $param_name eq "include_dir"; > next if $param_name eq "include_if_exists"; > > -- Exceptions are transaction_*. > SELECT name FROM tab_settings_flags > WHERE NOT no_show_all AND no_reset_all > ORDER BY 1; > name > ------------------------ > transaction_deferrable > transaction_isolation > transaction_read_only > (3 rows) > > How else do you propose to make this work for guc whose defaults vary by > platform in guc.c or in initdb ? regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On Tue, Jul 19, 2022 at 03:04:27PM +0900, Kyotaro Horiguchi wrote: > At Wed, 13 Jul 2022 18:54:45 -0500, Justin Pryzby <pryzby@telsasoft.com> wrote in > > On Thu, Jul 14, 2022 at 08:46:02AM +0900, Michael Paquier wrote: > > > On Wed, Jul 13, 2022 at 12:30:00PM -0500, Justin Pryzby wrote: > > > > How did you make this list ? Was it by excluding things that failed for you ? > > Yes. I didn't confirm each variable. They are the variables differ on > RHEL-family OSes. io_concurrency differs according to > USE_PREFETCH. Regarding to effects of macro definitions, I searched > guc.c for non-GUC_NOT_IN_SAMPLE variables with macro-affected defaults. I think you'd also need to handle the ones which are changed by initdb.c. This patch takes Andres' suggestion. The list of GUCs I flagged is probably incomplete, maybe inaccurate, and at least up for discussion. BTW I still think it might have been better to leave pg_settings_get_flags() deliberately undocumented. -- Justin
Attachment
Note that this can currently exposes internal elog() errors to users:
postgres=# select pg_normalize_config_value('log_min_messages','abc');
WARNING: invalid value for parameter "log_min_messages": "abc"
HINT: Available values: debug5, debug4, debug3, debug2, debug1, info, notice, warning, error, log, fatal, panic.
ERROR: could not find enum option 0 for log_min_messages
postgres=# \errverbose
ERROR: XX000: could not find enum option 0 for log_min_messages
LOCATION: config_enum_lookup_by_value, guc.c:7284
Hi,
Checking if you're planning to work on this patch still ?
On Thu, Jul 28, 2022 at 05:27:34PM -0500, Justin Pryzby wrote:
> Note that this can currently exposes internal elog() errors to users:
>
> postgres=# select pg_normalize_config_value('log_min_messages','abc');
> WARNING: invalid value for parameter "log_min_messages": "abc"
> HINT: Available values: debug5, debug4, debug3, debug2, debug1, info, notice, warning, error, log, fatal, panic.
> ERROR: could not find enum option 0 for log_min_messages
>
> postgres=# \errverbose
> ERROR: XX000: could not find enum option 0 for log_min_messages
> LOCATION: config_enum_lookup_by_value, guc.c:7284
This is an alternative implementation, which still relies on adding the GUC_DYNAMIC, flag but doesn't require adding a new, sql-accessible function to convert the GUC to a pretty/human display value.
Attachment
At Sat, 17 Sep 2022 23:53:07 -0500, Justin Pryzby <pryzby@telsasoft.com> wrote in > This is an alternative implementation, which still relies on adding the > GUC_DYNAMIC, flag but doesn't require adding a new, sql-accessible > function to convert the GUC to a pretty/human display value. Thanks! I'm not sure shared_buffer is GUC_DYNAMIC_DEFAULT, and we need to read postgresql.conf.sample using SQL, but +1 for the direction. + AND NOT (sc.sample_value ~ '^0' AND current_setting(name) ~ '^0') -- zeros may be written differently + AND NOT (sc.sample_value='60s' AND current_setting(name) = '1min') -- two ways to write 1min Mmm. Couldn't we get away from that explicit exceptions? regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Hi, I was hoping to use this patch in my other thread [1], but your latest attachment is reported broken in cfbot [2]. Please rebase it. ------ [1] GUC C var sanity check - https://www.postgresql.org/message-id/CAHut%2BPs91wgaE9P7JORnK_dGq7zPB56WLDJwLNCLgGXxqrh9%3DQ%40mail.gmail.com [2] cfbot fail - http://cfbot.cputube.org/patch_40_3736.log Kind Regards, Peter Smith. Fujitsu Australia
At Fri, 21 Oct 2022 11:58:15 +1100, Peter Smith <smithpb2250@gmail.com> wrote in > Hi, I was hoping to use this patch in my other thread [1], but your > latest attachment is reported broken in cfbot [2]. Please rebase it. Ouch. I haven't reach here. I'll do that next Monday. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On Mon, Sep 26, 2022 at 05:29:58PM +0900, Kyotaro Horiguchi wrote: > At Sat, 17 Sep 2022 23:53:07 -0500, Justin Pryzby <pryzby@telsasoft.com> wrote in > > This is an alternative implementation, which still relies on adding the > > GUC_DYNAMIC, flag but doesn't require adding a new, sql-accessible > > function to convert the GUC to a pretty/human display value. > > Thanks! > > I'm not sure shared_buffer is GUC_DYNAMIC_DEFAULT, and we need to read It's set during initdb. > postgresql.conf.sample using SQL, but +1 for the direction. > > + AND NOT (sc.sample_value ~ '^0' AND current_setting(name) ~ '^0') -- zeros may be written differently > + AND NOT (sc.sample_value='60s' AND current_setting(name) = '1min') -- two ways to write 1min > > Mmm. Couldn't we get away from that explicit exceptions? Suggestions are welcomed. Rebased the patch. I also split the flag into DEFAULTS_COMPILE and DEFAULTS_INITDB, since that makes it easier to understand what the flags mean and the intent of the patch. And maybe allows fewer exclusions in patches like Peter's, which I think would only want to exclude compile-time defaults. -- Justin
Attachment
On Tue, Oct 25, 2022 at 9:05 AM Justin Pryzby <pryzby@telsasoft.com> wrote: > > On Mon, Sep 26, 2022 at 05:29:58PM +0900, Kyotaro Horiguchi wrote: > > At Sat, 17 Sep 2022 23:53:07 -0500, Justin Pryzby <pryzby@telsasoft.com> wrote in ... > Rebased the patch. > > I also split the flag into DEFAULTS_COMPILE and DEFAULTS_INITDB, since > that makes it easier to understand what the flags mean and the intent of > the patch. And maybe allows fewer exclusions in patches like Peter's, > which I think would only want to exclude compile-time defaults. > Thanks! FYI, I'm making use of this patch now as a prerequisite for my GUC C var sanity-checker [1]. ------ [1] https://www.postgresql.org/message-id/CAHut%2BPss16YBiYYKyrZBvSp_4uSQfCy7aYfDXU0N8w5VZ5dd_g%40mail.gmail.com Kind Regards, Peter Smith. Fujitsu Australia
@cfbot: re-rebased again
Attachment
On Wed, Jul 13, 2022 at 04:49:00PM -0700, Andres Freund wrote: > On 2022-07-14 08:46:02 +0900, Michael Paquier wrote: > > On Wed, Jul 13, 2022 at 12:30:00PM -0500, Justin Pryzby wrote: > > > How did you make this list ? Was it by excluding things that failed for you ? > > > > > > cfbot is currently failing due to io_concurrency on windows. > > > I think there are more GUC which should be included here. > > > > > > http://cfbot.cputube.org/kyotaro-horiguchi.html > > > > FWIW, I am not really a fan of making this test depend on a hardcoded > > list of GUCs. > > I wonder if we should add flags indicating platform dependency etc to guc.c? > That should allow to remove most of them? Michael commented on this, but on another thread, so I'm copying and pasting it here. On Thu, Mar 23, 2023 at 08:59:57PM -0500, Justin Pryzby wrote: > On Fri, Mar 24, 2023 at 10:24:43AM +0900, Michael Paquier wrote: > > >> * Check consistency of GUC defaults between .sample.conf and pg_settings.boot_val > > > - It looks like this was pretty active until last October and might > > > have been ready to apply at least partially? But no further work or > > > review has happened since. > > > > FWIW, I don't find much appealing the addition of two GUC flags for > > only the sole purpose of that, > > The flags seem independently interesting - adding them here follows > a suggestion Andres made in response to your complaint. > 20220713234900.z4rniuaerkq34s4v@awork3.anarazel.de > > > particularly as we get a stronger > > dependency between GUCs that can be switched dynamically at > > initialization and at compile-time. > > What do you mean by "stronger dependency between GUCs" ? I'm still not clear what that means ? I updated the patch to handle the GUC added at 1671f990d. -- Justin
Attachment
On Wed, Mar 29, 2023 at 11:03:59PM -0500, Justin Pryzby wrote: > On Wed, Jul 13, 2022 at 04:49:00PM -0700, Andres Freund wrote: > > On 2022-07-14 08:46:02 +0900, Michael Paquier wrote: > > > On Wed, Jul 13, 2022 at 12:30:00PM -0500, Justin Pryzby wrote: > > > > How did you make this list ? Was it by excluding things that failed for you ? > > > > > > > > cfbot is currently failing due to io_concurrency on windows. > > > > I think there are more GUC which should be included here. > > > > > > > > http://cfbot.cputube.org/kyotaro-horiguchi.html > > > > > > FWIW, I am not really a fan of making this test depend on a hardcoded > > > list of GUCs. > > > > I wonder if we should add flags indicating platform dependency etc to guc.c? > > That should allow to remove most of them? > > Michael commented on this, but on another thread, so I'm copying and > pasting it here. > > On Thu, Mar 23, 2023 at 08:59:57PM -0500, Justin Pryzby wrote: > > On Fri, Mar 24, 2023 at 10:24:43AM +0900, Michael Paquier wrote: > > > >> * Check consistency of GUC defaults between .sample.conf and pg_settings.boot_val > > > > - It looks like this was pretty active until last October and might > > > > have been ready to apply at least partially? But no further work or > > > > review has happened since. > > > > > > FWIW, I don't find much appealing the addition of two GUC flags for > > > only the sole purpose of that, > > > > The flags seem independently interesting - adding them here follows > > a suggestion Andres made in response to your complaint. > > 20220713234900.z4rniuaerkq34s4v@awork3.anarazel.de > > > > > particularly as we get a stronger > > > dependency between GUCs that can be switched dynamically at > > > initialization and at compile-time. > > > > What do you mean by "stronger dependency between GUCs" ? > > I'm still not clear what that means ? Michael ? This fixes an issue with the last version that failed with log_autovacuum_min_duration in cirrusci's pg_ci_base.conf. And now includes both a perl and a sql-based versions of the test - both of which rely on the flags.
Attachment
On Wed, 10 May 2023 at 06:07, Justin Pryzby <pryzby@telsasoft.com> wrote: > > On Wed, Mar 29, 2023 at 11:03:59PM -0500, Justin Pryzby wrote: > > On Wed, Jul 13, 2022 at 04:49:00PM -0700, Andres Freund wrote: > > > On 2022-07-14 08:46:02 +0900, Michael Paquier wrote: > > > > On Wed, Jul 13, 2022 at 12:30:00PM -0500, Justin Pryzby wrote: > > > > > How did you make this list ? Was it by excluding things that failed for you ? > > > > > > > > > > cfbot is currently failing due to io_concurrency on windows. > > > > > I think there are more GUC which should be included here. > > > > > > > > > > http://cfbot.cputube.org/kyotaro-horiguchi.html > > > > > > > > FWIW, I am not really a fan of making this test depend on a hardcoded > > > > list of GUCs. > > > > > > I wonder if we should add flags indicating platform dependency etc to guc.c? > > > That should allow to remove most of them? > > > > Michael commented on this, but on another thread, so I'm copying and > > pasting it here. > > > > On Thu, Mar 23, 2023 at 08:59:57PM -0500, Justin Pryzby wrote: > > > On Fri, Mar 24, 2023 at 10:24:43AM +0900, Michael Paquier wrote: > > > > >> * Check consistency of GUC defaults between .sample.conf and pg_settings.boot_val > > > > > - It looks like this was pretty active until last October and might > > > > > have been ready to apply at least partially? But no further work or > > > > > review has happened since. > > > > > > > > FWIW, I don't find much appealing the addition of two GUC flags for > > > > only the sole purpose of that, > > > > > > The flags seem independently interesting - adding them here follows > > > a suggestion Andres made in response to your complaint. > > > 20220713234900.z4rniuaerkq34s4v@awork3.anarazel.de > > > > > > > particularly as we get a stronger > > > > dependency between GUCs that can be switched dynamically at > > > > initialization and at compile-time. > > > > > > What do you mean by "stronger dependency between GUCs" ? > > > > I'm still not clear what that means ? > > Michael ? > > This fixes an issue with the last version that failed with > log_autovacuum_min_duration in cirrusci's pg_ci_base.conf. > > And now includes both a perl and a sql-based versions of the test - both > of which rely on the flags. I'm seeing that there has been no activity in this thread for more than 8 months, I'm planning to close this in the current commitfest unless someone is planning to take it forward. Regards, Vignesh
On Sat, Jan 20, 2024 at 07:59:22AM +0530, vignesh C wrote: > I'm seeing that there has been no activity in this thread for more > than 8 months, I'm planning to close this in the current commitfest > unless someone is planning to take it forward. Thanks, that seems right to me. I have been looking again at the patch after seeing your reply (spent some time looking at it but I could not decide what to do), and I am not really excited with the amount of new facilities this requires in the TAP test (especially the list of hardcoded parameters that may change) and the backend-side changes for the GUC flags as well as the requirements to make the checks flexible enough to work across initdb and platform-dependent default values. In short, I'm happy to let 003_check_guc.pl be what check_guc was able to do (script gone in cf29a11ef646) for the parameter names. -- Michael
Attachment
On Sat, 20 Jan 2024 at 08:39, Michael Paquier <michael@paquier.xyz> wrote: > > On Sat, Jan 20, 2024 at 07:59:22AM +0530, vignesh C wrote: > > I'm seeing that there has been no activity in this thread for more > > than 8 months, I'm planning to close this in the current commitfest > > unless someone is planning to take it forward. > > Thanks, that seems right to me. Thanks, I have updated the commitfest entry to "returned with feedback". Feel free to start a new entry when someone wants to pursue it further more actively. Regards, Vignesh