public inbox for [email protected]
help / color / mirror / Atom feedFrom: Kyotaro Horiguchi <[email protected]>
Subject: [PATCH v4] Add fileval-bootval consistency check of GUC parameters
Date: Thu, 16 Jun 2022 17:08:02 +0900
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 | 57 ++++++++++++++++
src/include/catalog/pg_proc.dat | 5 ++
src/test/modules/test_misc/t/003_check_guc.pl | 67 +++++++++++++++++--
.../unsafe_tests/expected/rolenames.out | 13 ++++
.../modules/unsafe_tests/sql/rolenames.sql | 7 ++
5 files changed, 143 insertions(+), 6 deletions(-)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index a7cc49898b..1e7a0a2edc 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -7520,6 +7520,63 @@ 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;
+ const char *result = "";
+ void *extra;
+ union config_var_val val;
+ char buffer[256];
+
+ name = text_to_cstring(PG_GETARG_TEXT_PP(0));
+ value = text_to_cstring(PG_GETARG_TEXT_PP(1));
+
+ record = find_option(name, true, false, ERROR);
+
+ /*
+ * This function doesn't reveal values of the variables, but be consistent
+ * with similar functions.
+ */
+ if ((record->flags & GUC_SUPERUSER_ONLY) &&
+ !has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS))
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("must be superuser or have privileges of pg_read_all_settings to convert value for \"%s\"",
+ name)));
+
+ 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 = buffer;
+ break;
+ case PGC_REAL:
+ snprintf(buffer, sizeof(buffer), "%g", val.realval);
+ result = buffer;
+ break;
+ case PGC_STRING:
+ result = val.stringval;
+ break;
+ case PGC_ENUM:
+ result = config_enum_lookup_by_value((struct config_enum *)record,
+ val.boolval);
+ 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..794ba12dae 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', 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..d213b3b2bc 100644
--- a/src/test/modules/test_misc/t/003_check_guc.pl
+++ b/src/test/modules/test_misc/t/003_check_guc.pl
@@ -11,6 +11,28 @@ my $node = PostgreSQL::Test::Cluster->new('main');
$node->init;
$node->start;
+# These are non-variables but that are mistakenly parsed as variable
+# settings in the loop below.
+my %skip_names =
+ map { $_ => 1 } ('include', 'include_dir', 'include_if_exists');
+
+# 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');
+
# 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.
@@ -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 @file_vals = ();
open(my $contents, '<', $sample_file)
|| die "Could not open $sample_file: $!";
while (my $line = <$contents>)
@@ -53,19 +75,29 @@ 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);
- # Ignore some exceptions.
- next if $param_name eq "include";
- next if $param_name eq "include_dir";
- next if $param_name eq "include_if_exists";
+ # extract value
+ my $file_value = $2;
+ $file_value =~ s/\s*#.*$//; # strip trailing comment
+ $file_value =~ s/^'(.*)'$/$1/; # strip quotes
+
+ next if (defined $skip_names{$param_name});
# Update the list of GUCs found in the sample file, for the
# follow-up tests.
push @gucs_in_file, $param_name;
+
+ # Update the list of GUCs that value check between the sample
+ # file and pg_setting.boot_val will be performed.
+ if (!defined $ignore_parameters{$param_name})
+ {
+ push(@file_vals, [$param_name, $file_value]);
+ }
+
}
}
@@ -107,4 +139,27 @@ foreach my $param (@sample_intersect)
);
}
+# Check if GUC values in config-file and boot value match
+my $values = $node->safe_psql(
+ 'postgres',
+ 'SELECT f.n, pg_config_unitless_value(f.n, f.v), s.boot_val, \'!\' '.
+ 'FROM (VALUES '.
+ join(',', map { "('${$_}[0]','${$_}[1]')" } @file_vals).
+ ') f(n,v) '.
+ 'JOIN pg_settings s ON (s.name = f.n)');
+
+my $fails = "";
+foreach my $l (split("\n", $values))
+{
+ # $l: <varname>|<fileval>|<boot_val>|!
+ my @t = split("\\|", $l);
+ if ($t[1] ne $t[2])
+ {
+ $fails .= "\n" if ($fails ne "");
+ $fails .= "$t[0]: file \"$t[1]\" != boot_val \"$t[2]\"";
+ }
+}
+
+is($fails, "", "check if GUC values in .sample and boot value match");
+
done_testing();
diff --git a/src/test/modules/unsafe_tests/expected/rolenames.out b/src/test/modules/unsafe_tests/expected/rolenames.out
index 88b1ff843b..17983005d8 100644
--- a/src/test/modules/unsafe_tests/expected/rolenames.out
+++ b/src/test/modules/unsafe_tests/expected/rolenames.out
@@ -1081,6 +1081,19 @@ ERROR: must be superuser or have privileges of pg_read_all_settings to examine
RESET SESSION AUTHORIZATION;
ERROR: current transaction is aborted, commands ignored until end of transaction block
ROLLBACK;
+BEGIN;
+SET SESSION AUTHORIZATION regress_role_haspriv;
+-- passes with role member of pg_read_all_settings
+SELECT pg_config_unitless_value('session_preload_libraries', 'val');
+ pg_config_unitless_value
+--------------------------
+ val
+(1 row)
+
+SET SESSION AUTHORIZATION regress_role_nopriv;
+SELECT pg_config_unitless_value('session_preload_libraries', 'val');
+ERROR: must be superuser or have privileges of pg_read_all_settings to convert value for "session_preload_libraries"
+ROLLBACK;
REVOKE pg_read_all_settings FROM regress_role_haspriv;
-- clean up
\c
diff --git a/src/test/modules/unsafe_tests/sql/rolenames.sql b/src/test/modules/unsafe_tests/sql/rolenames.sql
index adac36536d..355aa32c2a 100644
--- a/src/test/modules/unsafe_tests/sql/rolenames.sql
+++ b/src/test/modules/unsafe_tests/sql/rolenames.sql
@@ -492,6 +492,13 @@ SET SESSION AUTHORIZATION regress_role_nopriv;
SHOW session_preload_libraries;
RESET SESSION AUTHORIZATION;
ROLLBACK;
+BEGIN;
+SET SESSION AUTHORIZATION regress_role_haspriv;
+-- passes with role member of pg_read_all_settings
+SELECT pg_config_unitless_value('session_preload_libraries', 'val');
+SET SESSION AUTHORIZATION regress_role_nopriv;
+SELECT pg_config_unitless_value('session_preload_libraries', 'val');
+ROLLBACK;
REVOKE pg_read_all_settings FROM regress_role_haspriv;
-- clean up
--
2.31.1
----Next_Part(Thu_Jun_30_17_38_01_2022_445)----
view thread (39+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected]
Subject: Re: [PATCH v4] Add fileval-bootval consistency check of GUC parameters
In-Reply-To: <no-message-id-1859773@localhost>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox