public inbox for [email protected]help / color / mirror / Atom feed
[PATCH] Add fileval-bootval consistency check of GUC parameters 11+ messages / 2 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ messages in thread
* Re: Periodic authorization expiration checks using GoAway message @ 2026-01-27 08:19 Ajit Awekar <[email protected]> 0 siblings, 0 replies; 11+ messages in thread From: Ajit Awekar @ 2026-01-27 08:19 UTC (permalink / raw) To: Zsolt Parragi <[email protected]>; +Cc: Jacob Champion <[email protected]>; Jelte Fennema-Nio <[email protected]>; Hannu Krosing <[email protected]>; pgsql-hackers; Dave Cramer <[email protected]>; Heikki Linnakangas <[email protected]> Hi Zsolt, Thanks a lot for your review comments. >After one more look at this, shouldn't we refresh the cache if AuthCheckNeeded, regardless of the valid until timestamp value? (for example by moving the >0 condition to the inner if) Done. I agree and I handled the scenario in the updated patch. >I missed this in my previos review, the error message should start with a lowercase character Done. >if the user was dropped, but the connection is still active, the patch silently ignores it. That matches the current behavior of postgres, but is that the expected behavior in the context of this patch? Thanks a lot for this use case. I agree that this patch should handle that scenario more explicitly. I have updated the logic in the attached version to address this use case. I have attached an updated patch. Request a review. Thanks & Best Regards, Ajit On Thu, 22 Jan 2026 at 13:10, Zsolt Parragi <[email protected]> wrote: > Hello! > > > Done. I have modified the condition check so as it will not impact users > > having rolvaliduntil to NULL. > > Thanks! After one more look at this, shouldn't we refresh the cache if > AuthCheckNeeded, regardless of the valid until timestamp value? (for > example by moving the >0 condition to the inner if) > > Consider the following scenario: > > 1. user logs in, without a valid until date set > 2. valid until is set to something > 3. existing session started in 1 will never be terminated > > > postgres.c:5368 - I missed this in my previos review, the error > message should start with a lowercase character. > > > > This patch introduces a mechanism to address the security issue of stale, > > authorized connections persisting beyond their validity period. . > > postgres.c:5326 - if the user was dropped, but the connection is still > active, the patch silently ignores it. That matches the current > behavior of postgres, but is that the expected behavior in the context > of this patch? > Attachments: [application/octet-stream] password_expiration_enforcement_V2.diff (10.6K, ../../CAER375NXhqSeH3ShxG67x82ZN0dtZiUmN_iMu0SxZbscDTP31g@mail.gmail.com/3-password_expiration_enforcement_V2.diff) download | inline diff: diff --git a/src/backend/libpq/crypt.c b/src/backend/libpq/crypt.c index 4c1052b3d42..eafc4309748 100644 --- a/src/backend/libpq/crypt.c +++ b/src/backend/libpq/crypt.c @@ -20,6 +20,7 @@ #include "common/scram-common.h" #include "libpq/crypt.h" #include "libpq/scram.h" +#include "miscadmin.h" #include "utils/builtins.h" #include "utils/syscache.h" #include "utils/timestamp.h" @@ -66,7 +67,19 @@ get_role_password(const char *role, const char **logdetail) datum = SysCacheGetAttr(AUTHNAME, roleTup, Anum_pg_authid_rolvaliduntil, &isnull); if (!isnull) + { vuntil = DatumGetTimestampTz(datum); + /* + * Cache the password expiration timestamp from pg_authid.rolvaliduntil + * during initial authentication so it can be checked throughout the + * lifetime of the connection. By changing this value from -1 to >= 0 + * we signal that password authentication was used. + */ + password_valid_until_timestamp = vuntil; + } + /* No expiration limit set */ + else + password_valid_until_timestamp = 0; ReleaseSysCache(roleTup); diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index e54bf1e760f..97d0ddb91af 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -34,6 +34,7 @@ #include "access/parallel.h" #include "access/printtup.h" #include "access/xact.h" +#include "catalog/pg_authid.h" #include "catalog/pg_type.h" #include "commands/async.h" #include "commands/event_trigger.h" @@ -74,10 +75,12 @@ #include "tcop/utility.h" #include "utils/guc_hooks.h" #include "utils/injection_point.h" +#include "utils/inval.h" #include "utils/lsyscache.h" #include "utils/memutils.h" #include "utils/ps_status.h" #include "utils/snapmgr.h" +#include "utils/syscache.h" #include "utils/timeout.h" #include "utils/timestamp.h" #include "utils/varlena.h" @@ -105,6 +108,13 @@ int client_connection_check_interval = 0; /* flags for non-system relation kinds to restrict use */ int restrict_nonsystem_relation_kind; +/* + * Flag set by syscache listener to indicate if the user's password validity + * (rolvaliduntil) needs to be checked for expiration before the next + * command execution. + */ +static bool AuthCheckNeeded = false; + /* ---------------- * private typedefs etc * ---------------- @@ -163,6 +173,13 @@ static volatile sig_atomic_t RecoveryConflictPendingReasons[NUM_PROCSIGNALS]; static MemoryContext row_description_context = NULL; static StringInfoData row_description_buf; +/* + * Tracks whether the SysCache callback for AUTHOID has been registered. + * This ensures CacheRegisterSyscacheCallback is called exactly once during + * backend initialization, preventing redundant registrations in the main loop. + */ +static bool password_auth_cache_callback_registered = false; + /* ---------------------------------------------------------------- * decls for routines only used in this file * ---------------------------------------------------------------- @@ -186,6 +203,8 @@ static void drop_unnamed_stmt(void); static void log_disconnections(int code, Datum arg); static void enable_statement_timeout(void); static void disable_statement_timeout(void); +static void AuthCacheInvalidated(Datum arg, int cacheid, uint32 hashvalue); +static void enforce_password_expiration(void); /* ---------------------------------------------------------------- @@ -1050,6 +1069,11 @@ exec_simple_query(const char *query_string) */ start_xact_command(); + /* + * Verify that the user's password has not expired. + */ + enforce_password_expiration(); + /* * Zap any pre-existing unnamed statement. (While not strictly necessary, * it seems best to define simple-Query mode as if it used the unnamed @@ -1432,6 +1456,11 @@ exec_parse_message(const char *query_string, /* string to execute */ */ start_xact_command(); + /* + * Verify that the user's password has not expired. + */ + enforce_password_expiration(); + /* * Switch to appropriate context for constructing parsetrees. * @@ -1708,6 +1737,11 @@ exec_bind_message(StringInfo input_message) */ start_xact_command(); + /* + * Verify that the user's password has not expired. + */ + enforce_password_expiration(); + /* Switch back to message context */ MemoryContextSwitchTo(MessageContext); @@ -2221,6 +2255,11 @@ exec_execute_message(const char *portal_name, long max_rows) */ start_xact_command(); + /* + * Verify that the user's password has not expired. + */ + enforce_password_expiration(); + /* * If we re-issue an Execute protocol request against an existing portal, * then we are only fetching more rows rather than completely re-executing @@ -2654,6 +2693,11 @@ exec_describe_statement_message(const char *stmt_name) */ start_xact_command(); + /* + * Verify that the user's password has not expired. + */ + enforce_password_expiration(); + /* Switch back to message context */ MemoryContextSwitchTo(MessageContext); @@ -2747,6 +2791,11 @@ exec_describe_portal_message(const char *portal_name) */ start_xact_command(); + /* + * Verify that the user's password has not expired. + */ + enforce_password_expiration(); + /* Switch back to message context */ MemoryContextSwitchTo(MessageContext); @@ -4518,6 +4567,20 @@ PostgresMain(const char *dbname, const char *username) if (!ignore_till_sync) send_ready_for_query = true; /* initially, or after error */ + + /* + * Register a SysCache listener for pg_authid changes (specifically for + * rolvaliduntil). This provides an event-driven mechanism to enforce + * password/authorization expiration immediately upon change, rather than + * relying on polling. The callback sets a flag (AuthCheckNeeded) which + * is checked before executing each simple query. + */ + if (!password_auth_cache_callback_registered) + { + CacheRegisterSyscacheCallback(AUTHOID, AuthCacheInvalidated, (Datum) 0); + password_auth_cache_callback_registered = true; + } + /* * Non-error queries loop here. */ @@ -5237,3 +5300,107 @@ disable_statement_timeout(void) if (get_timeout_active(STATEMENT_TIMEOUT)) disable_timeout(STATEMENT_TIMEOUT, false); } + +/* + * CheckPasswordExpiration + * Refreshes the cached password expiration timestamp from the system cache. + * This function looks up the current user's entry in pg_authid and updates + * 'password_valid_until_timestamp' with the current value of 'rolvaliduntil'. + * It is called by enforce_password_expiration() when the 'AuthCheckNeeded' + * flag is set, typically due to a syscache invalidation (AuthCacheInvalidated). + * If the user no longer exists (e.g., the role was dropped concurrently), this + * function will trigger a FATAL error to prevent further operations. + */ +static void +CheckPasswordExpiration(void) +{ + HeapTuple tuple; + + /* + * Look up the current user's entry in pg_authid. We must do this, even + * if only AuthCheckNeeded is set, because GetUserId() might return a + * different user ID than the one that triggered the invalidation (though + * that's unlikely for AUTHOID). + */ + + tuple = SearchSysCache1(AUTHOID, ObjectIdGetDatum(GetUserId())); + + if (HeapTupleIsValid(tuple)) + { + Datum rolvaliduntil_datum; + bool validUntil_null; + + /* Get the expiration time column */ + rolvaliduntil_datum = SysCacheGetAttr(AUTHOID, tuple, + Anum_pg_authid_rolvaliduntil, + &validUntil_null); + + if (!validUntil_null) + password_valid_until_timestamp = DatumGetTimestampTz(rolvaliduntil_datum); + else + password_valid_until_timestamp = 0; + + ReleaseSysCache(tuple); + + /* Reset the flag after performing the check */ + AuthCheckNeeded = false; + } + + /* + * Handle the case where the user was deleted.This can happen if the role was + * dropped by another session after this process started. + */ + else + ereport(FATAL, + (errcode(ERRCODE_UNDEFINED_OBJECT), + errmsg("user with OID %u does not exist", GetUserId()), + errdetail("A lookup in the pg_authid system catalog failed for the current " + "user OID."), + errhint("The user might have been dropped concurrently by another session."))); +} + +/* + * enforce_password_expiration + * + * Check if the user's password has expired and terminate the connection + * if necessary. This encapsulates the state checks and the FATAL report. + * CheckPasswordExpiration must only be called when the system is out of + * recovery and inside a valid transaction. + */ +static void +enforce_password_expiration(void) +{ + + if (!RecoveryInProgress() && IsTransactionState()) + { + if (AuthCheckNeeded) + CheckPasswordExpiration(); + + if (password_valid_until_timestamp > 0 && + password_valid_until_timestamp < GetCurrentTransactionStartTimestamp()) + ereport(FATAL, + (errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION), + errmsg("connection expired due to internal password policy enforcement"), + errdetail("User's password expired at %s.", + timestamptz_to_str(password_valid_until_timestamp)), + errhint("Reconnect with a renewed password."))); + } +} + +/* + * AuthCacheInvalidated + * Syscache callback function registered for the AUTHOID cache (pg_authid). + * + * This function is executed whenever a tuple in pg_authid is updated, inserted, + * or deleted. Its primary purpose is to catch changes to the currently + * connected user's 'rolvaliduntil' field. + * + * It sets the static flag AuthCheckNeeded to true, signaling that the user's + * password expiration status must be checked. + */ +static void +AuthCacheInvalidated(Datum arg, int cacheid, uint32 hashvalue) +{ + /* This callback is executed when an entry in pg_authid changes */ + AuthCheckNeeded = true; +} diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c index 36ad708b360..069138ec1d5 100644 --- a/src/backend/utils/init/globals.c +++ b/src/backend/utils/init/globals.c @@ -165,3 +165,10 @@ int notify_buffers = 16; int serializable_buffers = 32; int subtransaction_buffers = 0; int transaction_buffers = 0; + +/* + * Cached value of the current user's password expiration time (pg_authid.rolvaliduntil). + * This value is updated via CheckPasswordExpiration() when the AuthCheckNeeded + * flag is set by a syscache invalidation callback. + */ +TimestampTz password_valid_until_timestamp = -1; diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h index db559b39c4d..146c3769b2f 100644 --- a/src/include/miscadmin.h +++ b/src/include/miscadmin.h @@ -287,7 +287,7 @@ extern PGDLLIMPORT double VacuumCostDelay; extern PGDLLIMPORT int VacuumCostBalance; extern PGDLLIMPORT bool VacuumCostActive; - +extern PGDLLIMPORT TimestampTz password_valid_until_timestamp; /* in utils/misc/stack_depth.c */ ^ permalink raw reply [nested|flat] 11+ messages in thread
end of thread, other threads:[~2026-01-27 08:19 UTC | newest] Thread overview: 11+ 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 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-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 v3] 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]> 2026-01-27 08:19 Re: Periodic authorization expiration checks using GoAway message Ajit Awekar <[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