At Sat, 28 May 2022 13:22:45 -0700, Andres Freund <and...@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
>From 1e52b23111b5e8860092dcfea0ac9459f1725f0d Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota....@gmail.com>
Date: Mon, 30 May 2022 16:11:34 +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 | 70 +++++++++++++++++++
src/backend/utils/misc/postgresql.conf.sample | 6 +-
src/include/catalog/pg_proc.dat | 5 ++
src/test/modules/test_misc/t/003_check_guc.pl | 55 +++++++++++++--
4 files changed, 127 insertions(+), 9 deletions(-)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 55d41ae7d6..cd47dede1a 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -7514,6 +7514,76 @@ parse_and_validate_value(struct config_generic *record,
return true;
}
+/*
+ * Helper function for pg_normalize_config_value().
+ * Makes a palloced copy of src then link val to it.
+ * DO NOT destroy val while dst is in use.
+ */
+static struct config_generic *
+copy_config_and_set_value(struct config_generic *src, union config_var_val *val)
+{
+ struct config_generic *dst;
+
+#define CREATE_CONFIG_COPY(dst, src, t) \
+ dst = palloc(sizeof(struct t)); \
+ *(struct t *) dst = *(struct t *) src; \
+
+ switch (src->vartype)
+ {
+ case PGC_BOOL:
+ CREATE_CONFIG_COPY(dst, src, config_bool);
+ ((struct config_bool *)dst)->variable = &val->boolval;
+ break;
+ case PGC_INT:
+ CREATE_CONFIG_COPY(dst, src, config_int);
+ ((struct config_int *)dst)->variable = &val->intval;
+ break;
+ case PGC_REAL:
+ CREATE_CONFIG_COPY(dst, src, config_real);
+ ((struct config_real *)dst)->variable = &val->realval;
+ break;
+ case PGC_STRING:
+ CREATE_CONFIG_COPY(dst, src, config_string);
+ ((struct config_string *)dst)->variable = &val->stringval;
+ break;
+ case PGC_ENUM:
+ CREATE_CONFIG_COPY(dst, src, config_enum);
+ ((struct config_enum *)dst)->variable = &val->enumval;
+ break;
+ }
+
+ return dst;
+}
+
+
+/*
+ * Normalize given value according to the specified GUC variable
+ */
+Datum
+pg_normalize_config_value(PG_FUNCTION_ARGS)
+{
+ char *name = "";
+ char *value = "";
+ struct config_generic *record;
+ char *result;
+ void *extra;
+ union config_var_val altval;
+
+ 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,
+ &altval, &extra);
+ record = copy_config_and_set_value(record, &altval);
+
+ result = _ShowOption(record, true);
+
+ PG_RETURN_TEXT_P(cstring_to_text(result));
+}
/*
* Sets option `name' to given value.
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index 48ad80cf2e..a6d5e401a9 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -476,7 +476,7 @@
# in all cases.
# These are relevant when logging to syslog:
-#syslog_facility = 'LOCAL0'
+#syslog_facility = 'local0'
#syslog_ident = 'postgres'
#syslog_sequence_numbers = on
#syslog_split_messages = on
@@ -709,7 +709,7 @@
# - Locale and Formatting -
-#datestyle = 'iso, mdy'
+#datestyle = 'ISO, MDY'
#intervalstyle = 'postgres'
#timezone = 'GMT'
#timezone_abbreviations = 'Default' # Select the set of available time zone
@@ -721,7 +721,7 @@
# share/timezonesets/.
#extra_float_digits = 1 # min -15, max 3; any value >0 actually
# selects precise output mode
-#client_encoding = sql_ascii # actually, defaults to database
+#client_encoding = SQL_ASCII # actually, defaults to database
# encoding
# These settings are initialized by initdb, but they can be changed.
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 87aa571a33..c089b351e9 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_normalize_config_value', proisstrict => 'f',
+ provolatile => 's', prorettype => 'text', proargtypes => 'text text',
+ proargnames => '{varname,value}', prosrc => 'pg_normalize_config_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..a92206942f 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,40 @@ 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'
+ );
+
# 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 +65,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 +75,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 +93,35 @@ 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.
+my $check_query =
+ 'SELECT f.n, f.v, s.boot_val FROM (VALUES '.
+ join(',', @check_elems).
+ ') 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)';
+
+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
