Subject: fix stats_fetch_consistency value in postgresql.conf.sample

From: Nathan Bossart <[email-masked]>
Date: Tue, 24 May 2022 15:01:47 -0700

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
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index a5a6d14cd4..48ad80cf2e 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -619,7 +619,7 @@
 #track_io_timing = off
 #track_wal_io_timing = off
 #track_functions = none			# none, pl, all
-#stats_fetch_consistency = none
+#stats_fetch_consistency = cache

 # - Monitoring -

---

From: Michael Paquier <[email-masked]>
Date: Wed, 25 May 2022 13:08:08 +0900

> 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

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Wed, 25 May 2022 13:11:40 +0900 (JST)

At Tue, 24 May 2022 15:01:47 -0700, Nathan Bossart <[email-masked]> wrote in 
> 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

---

From: Andres Freund <[email-masked]>
Date: Tue, 24 May 2022 21:23:32 -0700

Hi,

> would be possible to reduce the burden of unifying the three inital

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

---

From: Andres Freund <[email-masked]>
Date: Tue, 24 May 2022 21:28:49 -0700

> Will apply.

And done. Thanks Nathan!

---

From: Michael Paquier <[email-masked]>
Date: Wed, 25 May 2022 14:00:23 +0900

> And done. Thanks Nathan!

Shouldn't you also refresh pgstat_fetch_consistency in pgstat.c for
consistency?

> detect this kind of thing?

That sounds like a good idea to me.
--
Michael

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Wed, 25 May 2022 15:56:23 +0900 (JST)

At Wed, 25 May 2022 14:00:23 +0900, Michael Paquier <[email-masked]> wrote in 
> 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.

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Wed, 25 May 2022 16:12:07 +0900 (JST)

At Wed, 25 May 2022 15:56:23 +0900 (JST), Kyotaro Horiguchi <[email-masked]> wrote in 
> archive_command = "(disabled)" != ""

The "(disabled)" is the representation of "".

> 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.

> 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.

> 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.

---

From: Michael Paquier <[email-masked]>
Date: Thu, 26 May 2022 08:53:55 +0900

> 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

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Thu, 26 May 2022 11:10:18 +0900 (JST)

At Thu, 26 May 2022 08:53:55 +0900, Michael Paquier <[email-masked]> wrote in 
> 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.

---

From: Justin Pryzby <[email-masked]>
Date: Wed, 25 May 2022 21:25:53 -0500

> 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

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Thu, 26 May 2022 13:00:45 +0900 (JST)

At Wed, 25 May 2022 21:25:53 -0500, Justin Pryzby <[email-masked]> wrote in 
> check_guc.pl to access postgresql.conf from the source tree.  Do you want to

Yes.

>  stats_fetch_consistency     | cache                                                                    | none

Yeah, it is a straight forward outcome.

>  wal_sender_timeout          | 1min                                                                     | 60s

I'm not sure we should do so.  Rather I'd prefer 60s than 1min here.

regards.

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Thu, 26 May 2022 16:27:53 +0900 (JST)

At Thu, 26 May 2022 13:00:45 +0900 (JST), Kyotaro Horiguchi <[email-masked]> wrote in 
> 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.

---

From: Andres Freund <[email-masked]>
Date: Sat, 28 May 2022 13:14:09 -0700

Hi,

> Shouldn't you also refresh pgstat_fetch_consistency in pgstat.c for

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

---

From: Andres Freund <[email-masked]>
Date: Sat, 28 May 2022 13:22:45 -0700

Hi,

> allows me to write a bit complex things (than simple string

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

---

From: Michael Paquier <[email-masked]>
Date: Mon, 30 May 2022 11:49:07 +0900

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

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Mon, 30 May 2022 17:27:19 +0900 (JST)

At Sat, 28 May 2022 13:22:45 -0700, Andres Freund <[email-masked]> wrote in 
> 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.

> 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.

---

From: Justin Pryzby <[email-masked]>
Date: Sat, 11 Jun 2022 09:41:37 -0500

> did that in the attached.  parse_val() and check_val() and the duped

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

---

From: Michael Paquier <[email-masked]>
Date: Thu, 16 Jun 2022 12:07:03 +0900

> 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

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Thu, 16 Jun 2022 17:19:46 +0900 (JST)

At Thu, 16 Jun 2022 12:07:03 +0900, Michael Paquier <[email-masked]> wrote in 
> (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,

> 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.)

> would be more adapted for doing the normalization of the parameter

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.

> 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 <[email-masked]> wrote in 
> 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...

> 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.

---

From: Justin Pryzby <[email-masked]>
Date: Thu, 16 Jun 2022 08:23:07 -0500

> 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

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Fri, 17 Jun 2022 09:43:58 +0900 (JST)

At Thu, 16 Jun 2022 08:23:07 -0500, Justin Pryzby <[email-masked]> wrote in 
>  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.

> 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.

---

From: Andres Freund <[email-masked]>
Date: Wed, 22 Jun 2022 16:07:10 -0700

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?

> +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

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Thu, 30 Jun 2022 17:38:01 +0900 (JST)

Thanks!

At Wed, 22 Jun 2022 16:07:10 -0700, Andres Freund <[email-masked]> wrote in 
> > +pg_config_unitless_value(PG_FUNCTION_ARGS)
...
> 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.

> 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.

> buffers? cstring_to_text() will already copy the string, no?

Right. That's a silly thinko, omitting that behavior..

> 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.

> assign to $all_params_hash{$f[0]} once, rather than repeating that part

Yeah, but I noticed that that hash is no longer needed..

> So there's now two ignore mechanisms? Why not just handle include[_dir] via

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.

> You're not checking the consistency here though?

Mmm. Right. I reworded it following the comment just above.

> right. Maybe something like "GUC values in .sample and boot value match"?

No objection. Changed.

> 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.

---

From: Justin Pryzby <[email-masked]>
Date: Wed, 13 Jul 2022 12:30:00 -0500

> +# 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

---

From: Michael Paquier <[email-masked]>
Date: Thu, 14 Jul 2022 08:46:02 +0900

> 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

---

From: Andres Freund <[email-masked]>
Date: Wed, 13 Jul 2022 16:49:00 -0700

Hi,

On 2022-07-14 08:46:02 +0900, Michael Paquier wrote:
> 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

There's no existing test for things covered by these exceptions, unless I am
missing something?

Greetings,

Andres Freund

---

From: Justin Pryzby <[email-masked]>
Date: Wed, 13 Jul 2022 18:54:45 -0500

> of maintenance in the long-term, even if this is now run

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

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Tue, 19 Jul 2022 15:04:27 +0900 (JST)

At Wed, 13 Jul 2022 18:54:45 -0500, Justin Pryzby <[email-masked]> wrote in 
> > > 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...

> It doesn't really need to be stated that an inclusive list wouldn't be useful.

+1

regards.

---

From: Justin Pryzby <[email-masked]>
Date: Wed, 20 Jul 2022 00:12:26 -0500

> 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

---

From: Justin Pryzby <[email-masked]>
Date: Thu, 28 Jul 2022 17:27:34 -0500

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

---

From: Justin Pryzby <[email-masked]>
Date: Mon, 12 Sep 2022 19:43:03 -0500

Hi,

Checking if you're planning to work on this patch still ?

---

From: Justin Pryzby <[email-masked]>
Date: Sat, 17 Sep 2022 23:53:07 -0500

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.

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Mon, 26 Sep 2022 17:29:58 +0900 (JST)

At Sat, 17 Sep 2022 23:53:07 -0500, Justin Pryzby <[email-masked]> wrote in 

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.

---

From: Peter Smith <[email-masked]>
Date: Fri, 21 Oct 2022 11:58:15 +1100

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

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Fri, 21 Oct 2022 17:50:34 +0900 (JST)

At Fri, 21 Oct 2022 11:58:15 +1100, Peter Smith <[email-masked]> wrote in 
> latest attachment is reported broken in cfbot [2]. Please rebase it.

Ouch. I haven't reach here. I'll do that next Monday.

regards.

---

From: Justin Pryzby <[email-masked]>
Date: Mon, 24 Oct 2022 17:05:44 -0500

> I'm not sure shared_buffer is GUC_DYNAMIC_DEFAULT, and we need to read

It's set during initdb.

> 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

---

From: Peter Smith <[email-masked]>
Date: Tue, 25 Oct 2022 14:57:36 +1100

...

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

---

From: Justin Pryzby <[email-masked]>
Date: Fri, 18 Nov 2022 15:37:31 -0600

@cfbot: re-rebased again

---

From: vignesh C <[email-masked]>
Date: Sat, 20 Jan 2024 07:59:22 +0530

On Wed, 10 May 2023 at 06:07, Justin Pryzby <[email-masked]> 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:
> > 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.
> > > [email-masked]
> > >
> > > > 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

---

From: Michael Paquier <[email-masked]>
Date: Sat, 20 Jan 2024 12:09:18 +0900

> 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

---

From: vignesh C <[email-masked]>
Date: Fri, 26 Jan 2024 18:48:30 +0530

> 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