At Thu, 16 Jun 2022 12:07:03 +0900, Michael Paquier <mich...@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 <pry...@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
>From 7ffa4f5f59564880107ffc3fa0bbd631e020054e Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota....@gmail.com>
Date: Thu, 16 Jun 2022 17:08:02 +0900
Subject: [PATCH v2] Add fileval-bootval consistency check of GUC parameters
We should keep GUC variables consistent between the default values
written in postgresql.conf.sample and defined in guc.c. Add an
automated way to check for the consistency to the TAP test suite. Some
variables are still excluded since they cannot be checked simple way.
---
src/backend/utils/misc/guc.c | 53 ++++++++++++++++
src/include/catalog/pg_proc.dat | 5 ++
src/test/modules/test_misc/t/003_check_guc.pl | 60 +++++++++++++++++--
3 files changed, 112 insertions(+), 6 deletions(-)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index a7cc49898b..7b8b3a4417 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -7520,6 +7520,59 @@ parse_and_validate_value(struct config_generic *record,
return true;
}
+/*
+ * 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);
+
+ 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;
+ case PGC_ENUM:
+ p = config_enum_lookup_by_value((struct config_enum *)record,
+ val.boolval);
+ result = pstrdup(p);
+ break;
+ }
+
+ PG_RETURN_TEXT_P(cstring_to_text(result));
+}
/*
* Sets option `name' to given value.
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 87aa571a33..9eb2a584e1 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -6118,6 +6118,11 @@
proname => 'pg_settings_get_flags', provolatile => 's', prorettype => '_text',
proargtypes => 'text', prosrc => 'pg_settings_get_flags' },
+{ oid => '9956', descr => 'normalize value to the unit of specified GUC',
+ proname => 'pg_config_unitless_value', proisstrict => 'f',
+ provolatile => 's', prorettype => 'text', proargtypes => 'text text',
+ proargnames => '{varname,value}', prosrc => 'pg_config_unitless_value' },
+
{ oid => '3329', descr => 'show config file settings',
proname => 'pg_show_all_file_settings', prorows => '1000', proretset => 't',
provolatile => 'v', prorettype => 'record', proargtypes => '',
diff --git a/src/test/modules/test_misc/t/003_check_guc.pl b/src/test/modules/test_misc/t/003_check_guc.pl
index 60459ef759..9de6a386de 100644
--- a/src/test/modules/test_misc/t/003_check_guc.pl
+++ b/src/test/modules/test_misc/t/003_check_guc.pl
@@ -11,18 +11,41 @@ my $node = PostgreSQL::Test::Cluster->new('main');
$node->init;
$node->start;
+# parameter names that cannot get consistency check performed
+my @ignored_parameters = (
+ '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'
+ );
+
# Grab the names of all the parameters that can be listed in the
# configuration sample file. config_file is an exception, it is not
# in postgresql.conf.sample but is part of the lists from guc.c.
my $all_params = $node->safe_psql(
'postgres',
- "SELECT name
+ "SELECT lower(name), vartype, unit, boot_val, '!'
FROM pg_settings
WHERE NOT 'NOT_IN_SAMPLE' = ANY (pg_settings_get_flags(name)) AND
name <> 'config_file'
ORDER BY 1");
# Note the lower-case conversion, for consistency.
-my @all_params_array = split("\n", lc($all_params));
+my %all_params_hash;
+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];
+}
# Grab the names of all parameters marked as NOT_IN_SAMPLE.
my $not_in_sample = $node->safe_psql(
@@ -43,7 +66,7 @@ my @gucs_in_file;
# Read the sample file line-by-line, checking its contents to build a list
# of everything known as a GUC.
-my $num_tests = 0;
+my @check_elems = ();
open(my $contents, '<', $sample_file)
|| die "Could not open $sample_file: $!";
while (my $line = <$contents>)
@@ -53,11 +76,16 @@ while (my $line = <$contents>)
# file.
# - Valid configuration options are followed immediately by " = ",
# with one space before and after the equal sign.
- 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";
@@ -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.
+ 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');
+
# Cross-check that all the GUCs found in the sample file match the ones
# fetched above. This maps the arrays to a hash, making the creation of
# each exclude and intersection list easier.
my %gucs_in_file_hash = map { $_ => 1 } @gucs_in_file;
-my %all_params_hash = map { $_ => 1 } @all_params_array;
my %not_in_sample_hash = map { $_ => 1 } @not_in_sample_array;
-my @missing_from_file = grep(!$gucs_in_file_hash{$_}, @all_params_array);
+my @missing_from_file = grep(!$gucs_in_file_hash{$_}, keys %all_params_hash);
is(scalar(@missing_from_file),
0, "no parameters missing from postgresql.conf.sample");
--
2.31.1
>From 4385545514099eb94c1a624d2a082e544ca9c6bc Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota....@gmail.com>
Date: Thu, 16 Jun 2022 17:00:14 +0900
Subject: [PATCH] Add fileval-bootval consistency check of GUC parameters
We should keep GUC variables consistent between the default values
written in postgresql.conf.sample and defined in guc.c. Add an
automated way to check for the consistency to the TAP test suite.
For simplicity we don't check values that require unit conversion.
Some variables are still excluded since they cannot be checked simple
way.
---
src/test/modules/test_misc/t/003_check_guc.pl | 76 +++++++++++++++++--
1 file changed, 70 insertions(+), 6 deletions(-)
diff --git a/src/test/modules/test_misc/t/003_check_guc.pl b/src/test/modules/test_misc/t/003_check_guc.pl
index 60459ef759..a3334fad76 100644
--- a/src/test/modules/test_misc/t/003_check_guc.pl
+++ b/src/test/modules/test_misc/t/003_check_guc.pl
@@ -11,18 +11,39 @@ my $node = PostgreSQL::Test::Cluster->new('main');
$node->init;
$node->start;
+# parameter names that cannot get consistency check performed
+my @ignored_parameters = (
+ 'data_directory',
+ 'hba_file',
+ 'ident_file',
+ 'krb_server_keyfile',
+ 'timezone_abbreviations',
+ 'lc_messages',
+ 'log_file_mode',
+ 'unix_socket_permissions',
+ 'wal_sync_method'
+ );
+
# Grab the names of all the parameters that can be listed in the
# configuration sample file. config_file is an exception, it is not
# in postgresql.conf.sample but is part of the lists from guc.c.
my $all_params = $node->safe_psql(
'postgres',
- "SELECT name
+ "SELECT lower(name), vartype, unit, boot_val, '!'
FROM pg_settings
WHERE NOT 'NOT_IN_SAMPLE' = ANY (pg_settings_get_flags(name)) AND
name <> 'config_file'
ORDER BY 1");
# Note the lower-case conversion, for consistency.
-my @all_params_array = split("\n", lc($all_params));
+my %all_params_hash;
+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];
+}
# Grab the names of all parameters marked as NOT_IN_SAMPLE.
my $not_in_sample = $node->safe_psql(
@@ -43,7 +64,7 @@ my @gucs_in_file;
# Read the sample file line-by-line, checking its contents to build a list
# of everything known as a GUC.
-my $num_tests = 0;
+my @check_elems = ();
open(my $contents, '<', $sample_file)
|| die "Could not open $sample_file: $!";
while (my $line = <$contents>)
@@ -53,11 +74,16 @@ while (my $line = <$contents>)
# file.
# - Valid configuration options are followed immediately by " = ",
# with one space before and after the equal sign.
- 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";
@@ -66,19 +92,57 @@ 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.
+ 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. For now we check only variables in string and integers
+# without unit.
+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 (lower(f.v) <> COALESCE(lower(s.boot_val), '') ".
+ " AND (s.vartype = 'string' OR s.vartype = 'enum'))".
+ 'OR s.name IS NULL';
+
+is ($node->safe_psql('postgres', $check_query), '',
+ 'check if fileval-bootval consistency is fine for string variables');
+
+$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 ((s.vartype = 'integer' AND s.unit IS NULL) AND f.v <> s.boot_val) ".
+ 'OR s.name IS NULL';
+is ($node->safe_psql('postgres', $check_query), '',
+ 'check if fileval-bootval consistency is fine for integer variables');
+
+$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 ((s.vartype = 'real' AND s.unit IS NULL) ".
+ " AND (f.v::real <> s.boot_val::real)) ".
+ 'OR s.name IS NULL';
+is ($node->safe_psql('postgres', $check_query), '',
+ 'check if fileval-bootval consistency is fine for real variables');
+
# Cross-check that all the GUCs found in the sample file match the ones
# fetched above. This maps the arrays to a hash, making the creation of
# each exclude and intersection list easier.
my %gucs_in_file_hash = map { $_ => 1 } @gucs_in_file;
-my %all_params_hash = map { $_ => 1 } @all_params_array;
my %not_in_sample_hash = map { $_ => 1 } @not_in_sample_array;
-my @missing_from_file = grep(!$gucs_in_file_hash{$_}, @all_params_array);
+my @missing_from_file = grep(!$gucs_in_file_hash{$_}, keys %all_params_hash);
is(scalar(@missing_from_file),
0, "no parameters missing from postgresql.conf.sample");
--
2.31.1
