public inbox for [email protected]
help / color / mirror / Atom feed[PATCH] Add fileval-bootval consistency check of GUC parameters
19+ messages / 5 participants
[nested] [flat]
* [PATCH] Add fileval-bootval consistency check of GUC parameters
@ 2022-05-26 06:55 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 19+ messages in thread
From: Kyotaro Horiguchi @ 2022-05-26 06:55 UTC (permalink / raw)
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/test/modules/test_misc/t/003_check_guc.pl | 151 +++++++++++++++++-
1 file changed, 146 insertions(+), 5 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..efaa7ec182 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,65 @@ my $node = PostgreSQL::Test::Cluster->new('main');
$node->init;
$node->start;
+# unit conversion factors
+my %unit_conv_factor =
+ ('b', 1,
+ 'kb', 1024,
+ 'mb', 1024 * 1024,
+ 'gb', 1024 * 1024 * 1024,
+ 'tb', 1024 * 1024 * 1024 * 1024,
+ '8kb', 8 * 1024,
+ 'us', 1.0 / (1000 / 1000),
+ 'ms', 1.0 / 1000,
+ 's', 1,
+ 'min', 60,
+ 'h', 3600,
+ 'd', 24 * 3600);
+
+# 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');
+
+# 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');
+
# 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 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", lc($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(
@@ -44,6 +91,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 $failed = 0;
open(my $contents, '<', $sample_file)
|| die "Could not open $sample_file: $!";
while (my $line = <$contents>)
@@ -53,11 +101,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 +119,28 @@ 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.
+ $failed++
+ if (!check_val($param_name,
+ $all_params_hash{$param_name}->{type},
+ $all_params_hash{$param_name}->{bootval},
+ $all_params_hash{$param_name}->{unit},
+ $file_value));
}
}
close $contents;
+pass("fileval-bootval consistency is fine") if ($failed == 0);
+
# 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");
@@ -108,3 +170,82 @@ foreach my $param (@sample_intersect)
}
done_testing();
+
+
+# parse $val according to $type.
+# $val may be suffixed by a unit if $optunit is not given.
+sub parse_val
+{
+ my ($type, $val, $optunit) = @_;
+
+ if ($val =~ /^(\d+)([a-zA-Z]*)$/)
+ {
+ $val = $1;
+ my $unit = $2;
+
+ # integer allows octal representaion
+ $val = oct($val) if ($type eq "integer" && $val =~ /^0./);
+
+ # accept $optunit only if $val was not suffixed by a unit
+ if (length $optunit)
+ {
+ fail("check if unit \"$unit\" is known to this script")
+ if (length $unit);
+
+ $unit = $optunit;
+ }
+
+
+ if (length($unit))
+ {
+ $unit = lc($unit);
+
+ fail("check if unit \"$unit\" is known to this script")
+ if (!defined $unit_conv_factor{$unit});
+
+ $val *= $unit_conv_factor{$unit};
+ }
+ }
+
+ return $val;
+}
+
+# check if $bootval and $fileval match according to $type and $unit
+sub check_val
+{
+ my ($param_name, $type, $bootval, $unit, $fileval) = @_;
+
+ my $match = 0;
+
+ if (grep { $_ eq $param_name } @ignored_parameters)
+ {
+ # this parameter cannot be checked, skip.
+ $match = 1;
+ }
+ elsif ($type eq "integer" || $type eq "real")
+ {
+ # this parameter is numeric, parse with unit conversion
+ my $bootval = parse_val($type, $bootval, $unit);
+ my $fileval = parse_val($type, $fileval);
+
+ $match = 1 if ($bootval == $fileval);
+ }
+ elsif (($type eq "bool" || $type eq "enum") ||
+ grep { $_ eq $param_name} @case_insensitive_params)
+ {
+ # this parameter is compared case-insensitively
+ $match = 1 if (lc($bootval) eq lc($fileval));
+ }
+ else
+ {
+ # exact match
+ $match = 1 if ($bootval eq $fileval);
+ }
+
+ if (!$match)
+ {
+ fail("$param_name inconsistent (\"$bootval\" vs \"$fileval\")");
+ }
+
+ return $match;
+}
--
2.31.1
----Next_Part(Thu_May_26_16_27_53_2022_916)----
^ permalink raw reply [nested|flat] 19+ messages in thread
* [PATCH] Add fileval-bootval consistency check of GUC parameters
@ 2022-05-26 06:55 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 19+ messages in thread
From: Kyotaro Horiguchi @ 2022-05-26 06:55 UTC (permalink / raw)
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/test/modules/test_misc/t/003_check_guc.pl | 151 +++++++++++++++++-
1 file changed, 146 insertions(+), 5 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..efaa7ec182 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,65 @@ my $node = PostgreSQL::Test::Cluster->new('main');
$node->init;
$node->start;
+# unit conversion factors
+my %unit_conv_factor =
+ ('b', 1,
+ 'kb', 1024,
+ 'mb', 1024 * 1024,
+ 'gb', 1024 * 1024 * 1024,
+ 'tb', 1024 * 1024 * 1024 * 1024,
+ '8kb', 8 * 1024,
+ 'us', 1.0 / (1000 / 1000),
+ 'ms', 1.0 / 1000,
+ 's', 1,
+ 'min', 60,
+ 'h', 3600,
+ 'd', 24 * 3600);
+
+# 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');
+
+# 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');
+
# 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 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", lc($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(
@@ -44,6 +91,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 $failed = 0;
open(my $contents, '<', $sample_file)
|| die "Could not open $sample_file: $!";
while (my $line = <$contents>)
@@ -53,11 +101,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 +119,28 @@ 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.
+ $failed++
+ if (!check_val($param_name,
+ $all_params_hash{$param_name}->{type},
+ $all_params_hash{$param_name}->{bootval},
+ $all_params_hash{$param_name}->{unit},
+ $file_value));
}
}
close $contents;
+pass("fileval-bootval consistency is fine") if ($failed == 0);
+
# 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");
@@ -108,3 +170,82 @@ foreach my $param (@sample_intersect)
}
done_testing();
+
+
+# parse $val according to $type.
+# $val may be suffixed by a unit if $optunit is not given.
+sub parse_val
+{
+ my ($type, $val, $optunit) = @_;
+
+ if ($val =~ /^(\d+)([a-zA-Z]*)$/)
+ {
+ $val = $1;
+ my $unit = $2;
+
+ # integer allows octal representaion
+ $val = oct($val) if ($type eq "integer" && $val =~ /^0./);
+
+ # accept $optunit only if $val was not suffixed by a unit
+ if (length $optunit)
+ {
+ fail("check if unit \"$unit\" is known to this script")
+ if (length $unit);
+
+ $unit = $optunit;
+ }
+
+
+ if (length($unit))
+ {
+ $unit = lc($unit);
+
+ fail("check if unit \"$unit\" is known to this script")
+ if (!defined $unit_conv_factor{$unit});
+
+ $val *= $unit_conv_factor{$unit};
+ }
+ }
+
+ return $val;
+}
+
+# check if $bootval and $fileval match according to $type and $unit
+sub check_val
+{
+ my ($param_name, $type, $bootval, $unit, $fileval) = @_;
+
+ my $match = 0;
+
+ if (grep { $_ eq $param_name } @ignored_parameters)
+ {
+ # this parameter cannot be checked, skip.
+ $match = 1;
+ }
+ elsif ($type eq "integer" || $type eq "real")
+ {
+ # this parameter is numeric, parse with unit conversion
+ my $bootval = parse_val($type, $bootval, $unit);
+ my $fileval = parse_val($type, $fileval);
+
+ $match = 1 if ($bootval == $fileval);
+ }
+ elsif (($type eq "bool" || $type eq "enum") ||
+ grep { $_ eq $param_name} @case_insensitive_params)
+ {
+ # this parameter is compared case-insensitively
+ $match = 1 if (lc($bootval) eq lc($fileval));
+ }
+ else
+ {
+ # exact match
+ $match = 1 if ($bootval eq $fileval);
+ }
+
+ if (!$match)
+ {
+ fail("$param_name inconsistent (\"$bootval\" vs \"$fileval\")");
+ }
+
+ return $match;
+}
--
2.31.1
----Next_Part(Thu_May_26_16_27_53_2022_916)----
^ permalink raw reply [nested|flat] 19+ messages in thread
* [PATCH] Add fileval-bootval consistency check of GUC parameters
@ 2022-05-26 06:55 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 19+ messages in thread
From: Kyotaro Horiguchi @ 2022-05-26 06:55 UTC (permalink / raw)
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/test/modules/test_misc/t/003_check_guc.pl | 151 +++++++++++++++++-
1 file changed, 146 insertions(+), 5 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..efaa7ec182 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,65 @@ my $node = PostgreSQL::Test::Cluster->new('main');
$node->init;
$node->start;
+# unit conversion factors
+my %unit_conv_factor =
+ ('b', 1,
+ 'kb', 1024,
+ 'mb', 1024 * 1024,
+ 'gb', 1024 * 1024 * 1024,
+ 'tb', 1024 * 1024 * 1024 * 1024,
+ '8kb', 8 * 1024,
+ 'us', 1.0 / (1000 / 1000),
+ 'ms', 1.0 / 1000,
+ 's', 1,
+ 'min', 60,
+ 'h', 3600,
+ 'd', 24 * 3600);
+
+# 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');
+
+# 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');
+
# 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 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", lc($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(
@@ -44,6 +91,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 $failed = 0;
open(my $contents, '<', $sample_file)
|| die "Could not open $sample_file: $!";
while (my $line = <$contents>)
@@ -53,11 +101,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 +119,28 @@ 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.
+ $failed++
+ if (!check_val($param_name,
+ $all_params_hash{$param_name}->{type},
+ $all_params_hash{$param_name}->{bootval},
+ $all_params_hash{$param_name}->{unit},
+ $file_value));
}
}
close $contents;
+pass("fileval-bootval consistency is fine") if ($failed == 0);
+
# 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");
@@ -108,3 +170,82 @@ foreach my $param (@sample_intersect)
}
done_testing();
+
+
+# parse $val according to $type.
+# $val may be suffixed by a unit if $optunit is not given.
+sub parse_val
+{
+ my ($type, $val, $optunit) = @_;
+
+ if ($val =~ /^(\d+)([a-zA-Z]*)$/)
+ {
+ $val = $1;
+ my $unit = $2;
+
+ # integer allows octal representaion
+ $val = oct($val) if ($type eq "integer" && $val =~ /^0./);
+
+ # accept $optunit only if $val was not suffixed by a unit
+ if (length $optunit)
+ {
+ fail("check if unit \"$unit\" is known to this script")
+ if (length $unit);
+
+ $unit = $optunit;
+ }
+
+
+ if (length($unit))
+ {
+ $unit = lc($unit);
+
+ fail("check if unit \"$unit\" is known to this script")
+ if (!defined $unit_conv_factor{$unit});
+
+ $val *= $unit_conv_factor{$unit};
+ }
+ }
+
+ return $val;
+}
+
+# check if $bootval and $fileval match according to $type and $unit
+sub check_val
+{
+ my ($param_name, $type, $bootval, $unit, $fileval) = @_;
+
+ my $match = 0;
+
+ if (grep { $_ eq $param_name } @ignored_parameters)
+ {
+ # this parameter cannot be checked, skip.
+ $match = 1;
+ }
+ elsif ($type eq "integer" || $type eq "real")
+ {
+ # this parameter is numeric, parse with unit conversion
+ my $bootval = parse_val($type, $bootval, $unit);
+ my $fileval = parse_val($type, $fileval);
+
+ $match = 1 if ($bootval == $fileval);
+ }
+ elsif (($type eq "bool" || $type eq "enum") ||
+ grep { $_ eq $param_name} @case_insensitive_params)
+ {
+ # this parameter is compared case-insensitively
+ $match = 1 if (lc($bootval) eq lc($fileval));
+ }
+ else
+ {
+ # exact match
+ $match = 1 if ($bootval eq $fileval);
+ }
+
+ if (!$match)
+ {
+ fail("$param_name inconsistent (\"$bootval\" vs \"$fileval\")");
+ }
+
+ return $match;
+}
--
2.31.1
----Next_Part(Thu_May_26_16_27_53_2022_916)----
^ permalink raw reply [nested|flat] 19+ messages in thread
* [PATCH v2] Add fileval-bootval consistency check of GUC parameters
@ 2022-05-30 07:11 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 19+ messages in thread
From: Kyotaro Horiguchi @ 2022-05-30 07:11 UTC (permalink / raw)
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
----Next_Part(Mon_May_30_17_27_19_2022_009)----
^ permalink raw reply [nested|flat] 19+ messages in thread
* [PATCH 2/2] Add fileval-bootval consistency check of GUC parameters
@ 2022-05-30 07:11 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 19+ messages in thread
From: Kyotaro Horiguchi @ 2022-05-30 07:11 UTC (permalink / raw)
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 | 57 +++++++++++++--
4 files changed, 129 insertions(+), 9 deletions(-)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index dbbadc5a475..a700cbbf4bc 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -7526,6 +7526,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 b4bc06e5f5a..b6403a14496 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 8873078c160..a92630d768b 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 60459ef759e..dfb2d203247 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,37 @@ 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 '.
+ 'JOIN pg_settings_get_flags(s.name) flags ON true '.
+ "WHERE 'DYNAMIC_DEFAULT' <> ALL(flags) AND " .
+ '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.17.1
--xaMk4Io5JJdpkLEb--
^ permalink raw reply [nested|flat] 19+ messages in thread
* [PATCH v2] Add fileval-bootval consistency check of GUC parameters
@ 2022-05-30 07:11 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 19+ messages in thread
From: Kyotaro Horiguchi @ 2022-05-30 07:11 UTC (permalink / raw)
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
----Next_Part(Mon_May_30_17_27_19_2022_009)----
^ permalink raw reply [nested|flat] 19+ messages in thread
* [PATCH] Add fileval-bootval consistency check of GUC parameters
@ 2022-06-16 08:00 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 19+ messages in thread
From: Kyotaro Horiguchi @ 2022-06-16 08:00 UTC (permalink / raw)
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
----Next_Part(Thu_Jun_16_17_19_46_2022_958)----
^ permalink raw reply [nested|flat] 19+ messages in thread
* [PATCH v2] Add fileval-bootval consistency check of GUC parameters
@ 2022-06-16 08:08 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 19+ messages in thread
From: Kyotaro Horiguchi @ 2022-06-16 08:08 UTC (permalink / raw)
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
----Next_Part(Thu_Jun_16_17_19_46_2022_958)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename="0001-Add-fileval-bootval-consistency-check-of-GUC-paramet-simple.patch"
^ permalink raw reply [nested|flat] 19+ messages in thread
* [PATCH v4] Add fileval-bootval consistency check of GUC parameters
@ 2022-06-16 08:08 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 19+ messages in thread
From: Kyotaro Horiguchi @ 2022-06-16 08:08 UTC (permalink / raw)
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)----
^ permalink raw reply [nested|flat] 19+ messages in thread
* [PATCH v3] Add fileval-bootval consistency check of GUC parameters
@ 2022-06-16 08:08 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 19+ messages in thread
From: Kyotaro Horiguchi @ 2022-06-16 08:08 UTC (permalink / raw)
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..136a21ba88 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
----Next_Part(Fri_Jun_17_09_43_58_2022_034)----
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: RFI: Extending the TOAST Pointer
@ 2023-05-18 13:11 Aleksander Alekseev <[email protected]>
2023-05-18 14:33 ` Re: RFI: Extending the TOAST Pointer Matthias van de Meent <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Aleksander Alekseev @ 2023-05-18 13:11 UTC (permalink / raw)
To: PostgreSQL Hackers <[email protected]>; +Cc: Matthias van de Meent <[email protected]>; Nikita Malakhov <[email protected]>
Hi,
> I agree that va_tag can have another use. But since we are going to
> make varatt_external variable in size (otherwise I don't see how it
> could be really **extendable**) I don't think this is the right
> approach.
On second thought, perhaps we are talking more or less about the same thing?
It doesn't matter what will be used as a sign of presence of a varint
bitmask in the pointer. My initial proposal to use ToastCompressionId
for this is probably redundant since va_tag > 18 will already tell
that.
--
Best regards,
Aleksander Alekseev
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: RFI: Extending the TOAST Pointer
2023-05-18 13:11 Re: RFI: Extending the TOAST Pointer Aleksander Alekseev <[email protected]>
@ 2023-05-18 14:33 ` Matthias van de Meent <[email protected]>
2023-05-18 15:50 ` Re: RFI: Extending the TOAST Pointer Nikita Malakhov <[email protected]>
2023-05-21 13:39 ` Re: RFI: Extending the TOAST Pointer Aleksander Alekseev <[email protected]>
0 siblings, 2 replies; 19+ messages in thread
From: Matthias van de Meent @ 2023-05-18 14:33 UTC (permalink / raw)
To: Aleksander Alekseev <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Nikita Malakhov <[email protected]>
On Thu, 18 May 2023 at 15:12, Aleksander Alekseev
<[email protected]> wrote:
>
> Hi,
>
> > I agree that va_tag can have another use. But since we are going to
> > make varatt_external variable in size (otherwise I don't see how it
> > could be really **extendable**) I don't think this is the right
> > approach.
Why would we modify va_tag=18; data=varatt_external? A different
va_tag option would allow us to keep the current layout around without
much maintenance and a consistent low overhead.
> On second thought, perhaps we are talking more or less about the same thing?
>
> It doesn't matter what will be used as a sign of presence of a varint
> bitmask in the pointer. My initial proposal to use ToastCompressionId
> for this is probably redundant since va_tag > 18 will already tell
> that.
I'm not sure "extendable" would be the right word, but as I see it:
1. We need more space to store more metadata;
2. Essentially all bits in varatt_external are already accounted for; and
3. There are still many options left in va_tag
It seems to me that adding a new variant to va_att for marking new
features in the toast infrastructure makes the most sense, as we'd
also be able to do the new things like varints etc without needing to
modify existing toast paths significantly.
We'd need to stop using the va_tag as length indicator, but I don't
think it's currently assumed to be a length indicator anyway (see
VARSIZE_EXTERNAL(ptr)). By not using the varatt_external struct
currently in use, we could be able to get down to <18B toast pointers
as well, though I'd consider that unlikely.
Kind regards,
Matthias van de Meent
Neon, Inc.
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: RFI: Extending the TOAST Pointer
2023-05-18 13:11 Re: RFI: Extending the TOAST Pointer Aleksander Alekseev <[email protected]>
2023-05-18 14:33 ` Re: RFI: Extending the TOAST Pointer Matthias van de Meent <[email protected]>
@ 2023-05-18 15:50 ` Nikita Malakhov <[email protected]>
1 sibling, 0 replies; 19+ messages in thread
From: Nikita Malakhov @ 2023-05-18 15:50 UTC (permalink / raw)
To: Matthias van de Meent <[email protected]>; +Cc: Aleksander Alekseev <[email protected]>; PostgreSQL Hackers <[email protected]>
Hi!
Matthias, in the Pluggable TOAST thread we proposed additional pointer
definition, without modification
of the original varatt_external - we have to keep it untouched for
compatibility issues. The following extension
for the TOAST pointer was proposed:
typedef struct varatt_custom
{
uint16 va_toasterdatalen;/* total size of toast pointer, < BLCKSZ */
uint32 va_rawsize; /* Original data size (includes header) */
uint32 va_toasterid; /* Toaster ID, actually Oid */
char va_toasterdata[FLEXIBLE_ARRAY_MEMBER]; /* Custom toaster data */
} varatt_custom;
with the new tag VARTAG_CUSTOM = 127.
Rawsize we have to keep because it is used by Executor. And Toaster ID is
the OID by which
we identify the extension that uses this pointer invariant.
On Thu, May 18, 2023 at 5:34 PM Matthias van de Meent <
[email protected]> wrote:
> On Thu, 18 May 2023 at 15:12, Aleksander Alekseev
> <[email protected]> wrote:
> >
> > Hi,
> >
> > > I agree that va_tag can have another use. But since we are going to
> > > make varatt_external variable in size (otherwise I don't see how it
> > > could be really **extendable**) I don't think this is the right
> > > approach.
>
> Why would we modify va_tag=18; data=varatt_external? A different
> va_tag option would allow us to keep the current layout around without
> much maintenance and a consistent low overhead.
>
> > On second thought, perhaps we are talking more or less about the same
> thing?
> >
> > It doesn't matter what will be used as a sign of presence of a varint
> > bitmask in the pointer. My initial proposal to use ToastCompressionId
> > for this is probably redundant since va_tag > 18 will already tell
> > that.
>
> I'm not sure "extendable" would be the right word, but as I see it:
>
> 1. We need more space to store more metadata;
> 2. Essentially all bits in varatt_external are already accounted for; and
> 3. There are still many options left in va_tag
>
> It seems to me that adding a new variant to va_att for marking new
> features in the toast infrastructure makes the most sense, as we'd
> also be able to do the new things like varints etc without needing to
> modify existing toast paths significantly.
>
> We'd need to stop using the va_tag as length indicator, but I don't
> think it's currently assumed to be a length indicator anyway (see
> VARSIZE_EXTERNAL(ptr)). By not using the varatt_external struct
> currently in use, we could be able to get down to <18B toast pointers
> as well, though I'd consider that unlikely.
>
> Kind regards,
>
> Matthias van de Meent
> Neon, Inc.
>
--
Regards,
Nikita Malakhov
Postgres Professional
The Russian Postgres Company
https://postgrespro.ru/
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: RFI: Extending the TOAST Pointer
2023-05-18 13:11 Re: RFI: Extending the TOAST Pointer Aleksander Alekseev <[email protected]>
2023-05-18 14:33 ` Re: RFI: Extending the TOAST Pointer Matthias van de Meent <[email protected]>
@ 2023-05-21 13:39 ` Aleksander Alekseev <[email protected]>
2023-05-22 13:07 ` Re: RFI: Extending the TOAST Pointer Matthias van de Meent <[email protected]>
1 sibling, 1 reply; 19+ messages in thread
From: Aleksander Alekseev @ 2023-05-21 13:39 UTC (permalink / raw)
To: Matthias van de Meent <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Nikita Malakhov <[email protected]>
Hi,
> We'd need to stop using the va_tag as length indicator, but I don't
> think it's currently assumed to be a length indicator anyway (see
> VARSIZE_EXTERNAL(ptr)). By not using the varatt_external struct
> currently in use, we could be able to get down to <18B toast pointers
> as well, though I'd consider that unlikely.
Agree.
Another thing we have to decide is what to do exactly in the scope of
this thread.
I imagine it as a refactoring that will find all the places that deal
with current TOAST pointer and changes them to something like:
```
switch(va_tag) {
case DEFAULT_VA_TAG( equals 18 ):
default_toast_process_case_abc(...);
default:
elog(ERROR, "Unknown TOAST tag")
}
```
So that next time somebody is going to need another type of TOAST
pointer this person will have only to add a corresponding tag and
handlers. (Something like "virtual methods" will produce a cleaner
code but will also break branch prediction, so I don't think we should
use those.)
I don't think we need an example of adding a new TOAST tag in scope of
this work since the default one is going to end up being such an
example.
Does it make sense?
--
Best regards,
Aleksander Alekseev
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: RFI: Extending the TOAST Pointer
2023-05-18 13:11 Re: RFI: Extending the TOAST Pointer Aleksander Alekseev <[email protected]>
2023-05-18 14:33 ` Re: RFI: Extending the TOAST Pointer Matthias van de Meent <[email protected]>
2023-05-21 13:39 ` Re: RFI: Extending the TOAST Pointer Aleksander Alekseev <[email protected]>
@ 2023-05-22 13:07 ` Matthias van de Meent <[email protected]>
2023-05-22 13:47 ` Re: RFI: Extending the TOAST Pointer Aleksander Alekseev <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Matthias van de Meent @ 2023-05-22 13:07 UTC (permalink / raw)
To: Aleksander Alekseev <[email protected]>; +Cc: Matthias van de Meent <[email protected]>; PostgreSQL Hackers <[email protected]>; Nikita Malakhov <[email protected]>
On Sun, 21 May 2023, 15:39 Aleksander Alekseev,
<[email protected]> wrote:
>
> Hi,
>
> > We'd need to stop using the va_tag as length indicator, but I don't
> > think it's currently assumed to be a length indicator anyway (see
> > VARSIZE_EXTERNAL(ptr)). By not using the varatt_external struct
> > currently in use, we could be able to get down to <18B toast pointers
> > as well, though I'd consider that unlikely.
>
> Agree.
>
> Another thing we have to decide is what to do exactly in the scope of
> this thread.
>
> I imagine it as a refactoring that will find all the places that deal
> with current TOAST pointer and changes them to something like:
>
> ```
> switch(va_tag) {
> case DEFAULT_VA_TAG( equals 18 ):
> default_toast_process_case_abc(...);
> default:
> elog(ERROR, "Unknown TOAST tag")
> }
> ```
I'm not sure that we need all that.
Many places do some special handling for VARATT_IS_EXTERNAL because
decompressing or detoasting is expensive and doing that as late as
possible can be beneficial (e.g. EXPLAIN ANALYZE can run much faster
because we never detoast returned columns). But only very few of these
cases actually work on explicitly on-disk data: my IDE can't find any
uses of VARATT_IS_EXTERNAL_ONDISK (i.e. the actual TOASTed value)
outside the expected locations of the toast subsystems, amcheck, and
logical decoding (incl. the pgoutput plugin). I'm fairly sure we only
need to update existing paths in those subsystems to support another
format of external (but not the current VARTAG_ONDISK) data.
> So that next time somebody is going to need another type of TOAST
> pointer this person will have only to add a corresponding tag and
> handlers. (Something like "virtual methods" will produce a cleaner
> code but will also break branch prediction, so I don't think we should
> use those.)
Yeah, I'm also not super stoked about using virtual methods for a new
external toast implementation.
> I don't think we need an example of adding a new TOAST tag in scope of
> this work since the default one is going to end up being such an
> example.
>
> Does it make sense?
I see your point, but I do think we should also think about why we do
the change.
E.g.: Our current toast infra is built around 4 uint32 fields in the
toast pointer; but with this change in place we can devise a new toast
pointer that uses varint encoding on the length-indicating fields to
reduce the footprint of 18B to an expected 14 bytes.
Kind regards,
Matthias van de Meent
Neon, Inc.
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: RFI: Extending the TOAST Pointer
2023-05-18 13:11 Re: RFI: Extending the TOAST Pointer Aleksander Alekseev <[email protected]>
2023-05-18 14:33 ` Re: RFI: Extending the TOAST Pointer Matthias van de Meent <[email protected]>
2023-05-21 13:39 ` Re: RFI: Extending the TOAST Pointer Aleksander Alekseev <[email protected]>
2023-05-22 13:07 ` Re: RFI: Extending the TOAST Pointer Matthias van de Meent <[email protected]>
@ 2023-05-22 13:47 ` Aleksander Alekseev <[email protected]>
2023-05-22 16:13 ` Re: RFI: Extending the TOAST Pointer Nikita Malakhov <[email protected]>
2023-05-22 20:47 ` Re: RFI: Extending the TOAST Pointer Jacob Champion <[email protected]>
0 siblings, 2 replies; 19+ messages in thread
From: Aleksander Alekseev @ 2023-05-22 13:47 UTC (permalink / raw)
To: PostgreSQL Hackers <[email protected]>; +Cc: Matthias van de Meent <[email protected]>; Nikita Malakhov <[email protected]>
Hi,
> I see your point, but I do think we should also think about why we do
> the change.
Personally at the moment I care only about implementing compression
dictionaries on top of this, as is discussed in the corresponding
thread [1]. I'm going to need new fields in the TOAST pointer's
including (but not necessarily limited to) dictionary id.
As I understand, Nikita is interested in implementing 64-bit TOAST
pointers [2]. I must admit I didn't follow that thread too closely but
I can imagine the needs are similar.
Last but not least I remember somebody on the mailing list suggested
adding ZSTD compression support for TOAST, besides LZ4. Assuming I
didn't dream it, the proposal was rejected due to the limited amount
of free bits in ToastCompressionId. It was argued that two possible
combinations that are left should be treated with care and ZSTD will
not bring enough value to the users compared to LZ4.
These are 3 recent cases I could recall. This being said I think our
solution should be generic enough to cover possible future cases
and/or cases unknown to us yet.
[1]: https://postgr.es/m/CAJ7c6TM7%2BsTvwREeL74Y5U91%2B5ymNobRbOmnDRfdTonq9trZyQ%40mail.gmail.com
[2]: https://commitfest.postgresql.org/43/4296/
--
Best regards,
Aleksander Alekseev
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: RFI: Extending the TOAST Pointer
2023-05-18 13:11 Re: RFI: Extending the TOAST Pointer Aleksander Alekseev <[email protected]>
2023-05-18 14:33 ` Re: RFI: Extending the TOAST Pointer Matthias van de Meent <[email protected]>
2023-05-21 13:39 ` Re: RFI: Extending the TOAST Pointer Aleksander Alekseev <[email protected]>
2023-05-22 13:07 ` Re: RFI: Extending the TOAST Pointer Matthias van de Meent <[email protected]>
2023-05-22 13:47 ` Re: RFI: Extending the TOAST Pointer Aleksander Alekseev <[email protected]>
@ 2023-05-22 16:13 ` Nikita Malakhov <[email protected]>
2023-05-22 17:14 ` Re: RFI: Extending the TOAST Pointer Matthias van de Meent <[email protected]>
1 sibling, 1 reply; 19+ messages in thread
From: Nikita Malakhov @ 2023-05-22 16:13 UTC (permalink / raw)
To: Aleksander Alekseev <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Matthias van de Meent <[email protected]>
Hi,
Aleksander, I'm interested in extending TOAST pointer in various ways.
64-bit TOAST value ID allows to resolve very complex issue for production
systems with large tables and heavy update rate.
I agree with Matthias that there should not be processing of TOAST pointer
internals outside TOAST macros. Currently, TOASTed value is distinguished
as VARATT_IS_EXTERNAL_ONDISK, and it should stay this way. Adding
compression requires another implementation (extension) of
VARATT_EXTERNAL because current supports only 2 compression methods -
it has only 1 bit responsible for compression method, and there is a safe
way to do so, without affecting default TOAST mechanics - we must keep
it this way for compatibility issues and not to break DB upgrade.
Also, I must remind that we should not forget about field alignment inside
the TOAST pointer.
As it was already mentioned, it seems not very reasonable trying to save
a byte or two while we are storing out-of-line values of at least 2 kb in
size.
On Mon, May 22, 2023 at 4:47 PM Aleksander Alekseev <
[email protected]> wrote:
> Hi,
>
> > I see your point, but I do think we should also think about why we do
> > the change.
>
> Personally at the moment I care only about implementing compression
> dictionaries on top of this, as is discussed in the corresponding
> thread [1]. I'm going to need new fields in the TOAST pointer's
> including (but not necessarily limited to) dictionary id.
>
> As I understand, Nikita is interested in implementing 64-bit TOAST
> pointers [2]. I must admit I didn't follow that thread too closely but
> I can imagine the needs are similar.
>
> Last but not least I remember somebody on the mailing list suggested
> adding ZSTD compression support for TOAST, besides LZ4. Assuming I
> didn't dream it, the proposal was rejected due to the limited amount
> of free bits in ToastCompressionId. It was argued that two possible
> combinations that are left should be treated with care and ZSTD will
> not bring enough value to the users compared to LZ4.
>
> These are 3 recent cases I could recall. This being said I think our
> solution should be generic enough to cover possible future cases
> and/or cases unknown to us yet.
>
> [1]:
> https://postgr.es/m/CAJ7c6TM7%2BsTvwREeL74Y5U91%2B5ymNobRbOmnDRfdTonq9trZyQ%40mail.gmail.com
> [2]: https://commitfest.postgresql.org/43/4296/
>
> --
> Best regards,
> Aleksander Alekseev
>
--
Regards,
Nikita Malakhov
Postgres Professional
The Russian Postgres Company
https://postgrespro.ru/
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: RFI: Extending the TOAST Pointer
2023-05-18 13:11 Re: RFI: Extending the TOAST Pointer Aleksander Alekseev <[email protected]>
2023-05-18 14:33 ` Re: RFI: Extending the TOAST Pointer Matthias van de Meent <[email protected]>
2023-05-21 13:39 ` Re: RFI: Extending the TOAST Pointer Aleksander Alekseev <[email protected]>
2023-05-22 13:07 ` Re: RFI: Extending the TOAST Pointer Matthias van de Meent <[email protected]>
2023-05-22 13:47 ` Re: RFI: Extending the TOAST Pointer Aleksander Alekseev <[email protected]>
2023-05-22 16:13 ` Re: RFI: Extending the TOAST Pointer Nikita Malakhov <[email protected]>
@ 2023-05-22 17:14 ` Matthias van de Meent <[email protected]>
0 siblings, 0 replies; 19+ messages in thread
From: Matthias van de Meent @ 2023-05-22 17:14 UTC (permalink / raw)
To: Nikita Malakhov <[email protected]>; +Cc: Aleksander Alekseev <[email protected]>; PostgreSQL Hackers <[email protected]>
On Mon, 22 May 2023 at 18:13, Nikita Malakhov <[email protected]> wrote:
>
> Hi,
Could you please not top-post.
> Aleksander, I'm interested in extending TOAST pointer in various ways.
> 64-bit TOAST value ID allows to resolve very complex issue for production
> systems with large tables and heavy update rate.
Cool. I agree that this would be nice, though it's quite unlikely
we'll ever be able to use much more than 32 bits concurrently with the
current 32-bit block IDs. But indeed, it is a good way of reducing
time spent searching for unused toast IDs.
> I agree with Matthias that there should not be processing of TOAST pointer
> internals outside TOAST macros. Currently, TOASTed value is distinguished
> as VARATT_IS_EXTERNAL_ONDISK, and it should stay this way. Adding
> compression requires another implementation (extension) of
> VARATT_EXTERNAL because current supports only 2 compression methods -
> it has only 1 bit responsible for compression method, and there is a safe
> way to do so, without affecting default TOAST mechanics - we must keep
> it this way for compatibility issues and not to break DB upgrade.
>
> Also, I must remind that we should not forget about field alignment inside
> the TOAST pointer.
What field alignment inside the TOAST pointers?
Current TOAST pointers are not aligned: the varatt_external struct is
copied into and from the va_data section of varattrib_1b_e when we
need to access the data; so as far as I know this struct has no
alignment to speak of.
> As it was already mentioned, it seems not very reasonable trying to save
> a byte or two while we are storing out-of-line values of at least 2 kb in size.
If we were talking about the data we're storing externally, sure. But
this is data we store in the original tuple, and moving that around is
relatively expensive. Reducing the aligned size of the toast pointer
can help reduce the size of the heap tuple, thus increasing the
efficiency of the primary data table.
Kind regards,
Matthias van de Meent
Neon, Inc.
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: RFI: Extending the TOAST Pointer
2023-05-18 13:11 Re: RFI: Extending the TOAST Pointer Aleksander Alekseev <[email protected]>
2023-05-18 14:33 ` Re: RFI: Extending the TOAST Pointer Matthias van de Meent <[email protected]>
2023-05-21 13:39 ` Re: RFI: Extending the TOAST Pointer Aleksander Alekseev <[email protected]>
2023-05-22 13:07 ` Re: RFI: Extending the TOAST Pointer Matthias van de Meent <[email protected]>
2023-05-22 13:47 ` Re: RFI: Extending the TOAST Pointer Aleksander Alekseev <[email protected]>
@ 2023-05-22 20:47 ` Jacob Champion <[email protected]>
1 sibling, 0 replies; 19+ messages in thread
From: Jacob Champion @ 2023-05-22 20:47 UTC (permalink / raw)
To: Aleksander Alekseev <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Matthias van de Meent <[email protected]>; Nikita Malakhov <[email protected]>
On Mon, May 22, 2023 at 6:47 AM Aleksander Alekseev
<[email protected]> wrote:
> Last but not least I remember somebody on the mailing list suggested
> adding ZSTD compression support for TOAST, besides LZ4. Assuming I
> didn't dream it, the proposal was rejected due to the limited amount
> of free bits in ToastCompressionId. It was argued that two possible
> combinations that are left should be treated with care and ZSTD will
> not bring enough value to the users compared to LZ4.
This thread, I think:
https://www.postgresql.org/message-id/flat/YoMiNmkztrslDbNS%40paquier.xyz
--Jacob
^ permalink raw reply [nested|flat] 19+ messages in thread
end of thread, other threads:[~2023-05-22 20:47 UTC | newest]
Thread overview: 19+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-05-26 06:55 [PATCH] Add fileval-bootval consistency check of GUC parameters Kyotaro Horiguchi <[email protected]>
2022-05-26 06:55 [PATCH] Add fileval-bootval consistency check of GUC parameters Kyotaro Horiguchi <[email protected]>
2022-05-26 06:55 [PATCH] Add fileval-bootval consistency check of GUC parameters Kyotaro Horiguchi <[email protected]>
2022-05-30 07:11 [PATCH v2] Add fileval-bootval consistency check of GUC parameters Kyotaro Horiguchi <[email protected]>
2022-05-30 07:11 [PATCH 2/2] Add fileval-bootval consistency check of GUC parameters Kyotaro Horiguchi <[email protected]>
2022-05-30 07:11 [PATCH v2] Add fileval-bootval consistency check of GUC parameters Kyotaro Horiguchi <[email protected]>
2022-06-16 08:00 [PATCH] Add fileval-bootval consistency check of GUC parameters Kyotaro Horiguchi <[email protected]>
2022-06-16 08:08 [PATCH v2] Add fileval-bootval consistency check of GUC parameters Kyotaro Horiguchi <[email protected]>
2022-06-16 08:08 [PATCH v4] Add fileval-bootval consistency check of GUC parameters Kyotaro Horiguchi <[email protected]>
2022-06-16 08:08 [PATCH v3] Add fileval-bootval consistency check of GUC parameters Kyotaro Horiguchi <[email protected]>
2023-05-18 13:11 Re: RFI: Extending the TOAST Pointer Aleksander Alekseev <[email protected]>
2023-05-18 14:33 ` Re: RFI: Extending the TOAST Pointer Matthias van de Meent <[email protected]>
2023-05-18 15:50 ` Re: RFI: Extending the TOAST Pointer Nikita Malakhov <[email protected]>
2023-05-21 13:39 ` Re: RFI: Extending the TOAST Pointer Aleksander Alekseev <[email protected]>
2023-05-22 13:07 ` Re: RFI: Extending the TOAST Pointer Matthias van de Meent <[email protected]>
2023-05-22 13:47 ` Re: RFI: Extending the TOAST Pointer Aleksander Alekseev <[email protected]>
2023-05-22 16:13 ` Re: RFI: Extending the TOAST Pointer Nikita Malakhov <[email protected]>
2023-05-22 17:14 ` Re: RFI: Extending the TOAST Pointer Matthias van de Meent <[email protected]>
2023-05-22 20:47 ` Re: RFI: Extending the TOAST Pointer Jacob Champion <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox