Thanks!
At Wed, 22 Jun 2022 16:07:10 -0700, Andres Freund <and...@anarazel.de> wrote in
> 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)
> > +{
...
> > + 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.
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.
> > + 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.
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.
> Not that it matters for efficiency here, but why are you pstrdup'ing the
> buffers? cstring_to_text() will already copy the string, no?
Right. That's a silly thinko, omitting that behavior..
>
> > +# parameter names that cannot get consistency check performed
> > +my @ignored_parameters = (
>
> 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.
> > +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.
Yeah, but I noticed that that hash is no longer needed..
> > - 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?
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.
> > @@ -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?
Mmm. Right. I reworded it following the comment just above.
> > + 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"?
No objection. Changed.
> 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.
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.
--
Kyotaro Horiguchi
NTT Open Source Software Center
>From 8ab61286529f819a08e0e70d0b5522cabf4592b1 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota....@gmail.com>
Date: Thu, 16 Jun 2022 17:08:02 +0900
Subject: [PATCH v4] 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 | 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
