fix stats_fetch_consistency value in postgresql.conf.sample

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

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

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

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

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

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

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

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