public inbox for [email protected]help / color / mirror / Atom feed
[PATCH] Add fileval-bootval consistency check of GUC parameters 23+ messages / 7 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; 23+ 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] 23+ 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; 23+ 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] 23+ 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; 23+ 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] 23+ 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; 23+ 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] 23+ 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; 23+ 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] 23+ 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; 23+ 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] 23+ 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; 23+ 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] 23+ 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; 23+ 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] 23+ 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; 23+ 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] 23+ 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; 23+ 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] 23+ messages in thread
* Re: Correct the documentation for work_mem @ 2023-04-21 17:15 Tom Lane <[email protected]> 0 siblings, 2 replies; 23+ messages in thread From: Tom Lane @ 2023-04-21 17:15 UTC (permalink / raw) To: Peter Eisentraut <[email protected]>; +Cc: Imseih (AWS), Sami <[email protected]>; pgsql-hackers Peter Eisentraut <[email protected]> writes: > On 21.04.23 16:28, Imseih (AWS), Sami wrote: >> I suggest a small doc fix: >> “Note that for a complex query, several sort or hash operations might be >> running simultaneously;” > Here is a discussion of these terms: > https://takuti.me/note/parallel-vs-concurrent/ > I think "concurrently" is the correct word here. Probably, but it'd do little to remove the confusion Sami is on about, especially since the next sentence uses "concurrently" to describe the other case. I think we need a more thorough rewording, perhaps like - Note that for a complex query, several sort or hash operations might be - running in parallel; each operation will generally be allowed + Note that a complex query may include several sort or hash + operations; each such operation will generally be allowed to use as much memory as this value specifies before it starts to write data into temporary files. Also, several running sessions could be doing such operations concurrently. I also find this wording a bit further down to be poor: Hash-based operations are generally more sensitive to memory availability than equivalent sort-based operations. The memory available for hash tables is computed by multiplying <varname>work_mem</varname> by <varname>hash_mem_multiplier</varname>. This makes it I think "available" is not le mot juste, and it's also unclear from this whether we're speaking of the per-hash-table limit or some (nonexistent) overall limit. How about - memory available for hash tables is computed by multiplying + memory limit for a hash table is computed by multiplying regards, tom lane ^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Correct the documentation for work_mem @ 2023-04-21 17:39 Gurjeet Singh <[email protected]> parent: Tom Lane <[email protected]> 1 sibling, 1 reply; 23+ messages in thread From: Gurjeet Singh @ 2023-04-21 17:39 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Imseih (AWS), Sami <[email protected]>; pgsql-hackers On Fri, Apr 21, 2023 at 10:15 AM Tom Lane <[email protected]> wrote: > > Peter Eisentraut <[email protected]> writes: > > On 21.04.23 16:28, Imseih (AWS), Sami wrote: > >> I suggest a small doc fix: > >> “Note that for a complex query, several sort or hash operations might be > >> running simultaneously;” > > > Here is a discussion of these terms: > > https://takuti.me/note/parallel-vs-concurrent/ > > > I think "concurrently" is the correct word here. > > Probably, but it'd do little to remove the confusion Sami is on about, +1. When discussing this internally, Sami's proposal was in fact to use the word 'concurrently'. But given that when it comes to computers and programming, it's common for someone to not understand the intricate difference between the two terms, we thought it's best to not use any of those, and instead use a word not usually associated with programming and algorithms. Aside: Another pair of words I see regularly used interchangeably, when in fact they mean different things: precise vs. accurate. > especially since the next sentence uses "concurrently" to describe the > other case. I think we need a more thorough rewording, perhaps like > > - Note that for a complex query, several sort or hash operations might be > - running in parallel; each operation will generally be allowed > + Note that a complex query may include several sort or hash > + operations; each such operation will generally be allowed This wording doesn't seem to bring out the fact that there could be more than one work_mem consumer running (in-progress) at the same time. The reader to could mistake it to mean hashes and sorts in a complex query may happen one after the other. + Note that a complex query may include several sort and hash operations, and + more than one of these operations may be in progress simultaneously at any + given time; each such operation will generally be allowed I believe the phrase "several sort _and_ hash" better describes the possible composition of a complex query, than does "several sort _or_ hash". > I also find this wording a bit further down to be poor: > > Hash-based operations are generally more sensitive to memory > availability than equivalent sort-based operations. The > memory available for hash tables is computed by multiplying > <varname>work_mem</varname> by > <varname>hash_mem_multiplier</varname>. This makes it > > I think "available" is not le mot juste, and it's also unclear from > this whether we're speaking of the per-hash-table limit or some > (nonexistent) overall limit. How about > > - memory available for hash tables is computed by multiplying > + memory limit for a hash table is computed by multiplying +1 Best regards, Gurjeet https://Gurje.et Postgres Contributors Team, http://aws.amazon.com ^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Correct the documentation for work_mem @ 2023-04-22 03:36 Imseih (AWS), Sami <[email protected]> parent: Gurjeet Singh <[email protected]> 0 siblings, 1 reply; 23+ messages in thread From: Imseih (AWS), Sami @ 2023-04-22 03:36 UTC (permalink / raw) To: Gurjeet Singh <[email protected]>; Tom Lane <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; pgsql-hackers > > especially since the next sentence uses "concurrently" to describe the > > other case. I think we need a more thorough rewording, perhaps like > > > > - Note that for a complex query, several sort or hash operations might be > > - running in parallel; each operation will generally be allowed > > + Note that a complex query may include several sort or hash > > + operations; each such operation will generally be allowed > This wording doesn't seem to bring out the fact that there could be > more than one work_mem consumer running (in-progress) at the same > time. Do you mean, more than one work_mem consumer running at the same time for a given query? If so, that is precisely the point we need to convey in the docs. i.e. if I have 2 sorts in a query that can use up to 4MB each, at some point in the query execution, I can have 8MB of memory allocated. Regards, Sami Imseih Amazon Web Services (AWS) ^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Correct the documentation for work_mem @ 2023-04-24 16:20 Imseih (AWS), Sami <[email protected]> parent: Imseih (AWS), Sami <[email protected]> 0 siblings, 1 reply; 23+ messages in thread From: Imseih (AWS), Sami @ 2023-04-24 16:20 UTC (permalink / raw) To: Gurjeet Singh <[email protected]>; Tom Lane <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; pgsql-hackers Based on the feedback, here is a v1 of the suggested doc changes. I modified Gurjeets suggestion slightly to make it clear that a specific query execution could have operations simultaneously using up to work_mem. I also added the small hash table memory limit clarification. Regards, Sami Imseih Amazon Web Services (AWS) Attachments: [application/octet-stream] v1-0001-Fix-documentation-for-work_mem.patch (2.2K, ../../[email protected]/2-v1-0001-Fix-documentation-for-work_mem.patch) download | inline diff: From 2fbbe428c25d7d12ad7d818ef5d00fe7c8085433 Mon Sep 17 00:00:00 2001 From: EC2 Default User <[email protected]> Date: Mon, 24 Apr 2023 16:04:45 +0000 Subject: [PATCH 1/1] Fix documentation for work_mem A couple of small documentation fixes to clear up terminology used in the work_mem documentation. The removal of the usage of "parallel" as it does not refer to parallel query in the context of work_mem. Also, a clarification on the memory used by hash tables. Discussion: https://www.postgresql.org/message-id/flat/66590882-F48C-4A25-83E3-73792CF8C51F%40amazon.com --- doc/src/sgml/config.sgml | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 091a79d4f3..bafda1c53a 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -1897,8 +1897,9 @@ include_dir 'conf.d' (such as a sort or hash table) before writing to temporary disk files. If this value is specified without units, it is taken as kilobytes. The default value is four megabytes (<literal>4MB</literal>). - Note that for a complex query, several sort or hash operations might be - running in parallel; each operation will generally be allowed + Note that a complex query may include several sort and hash operations, + and more than one of these operations may be in progress simultaneously + for a given query execution; each such operation will generally be allowed to use as much memory as this value specifies before it starts to write data into temporary files. Also, several running sessions could be doing such operations concurrently. @@ -1914,7 +1915,7 @@ include_dir 'conf.d' <para> Hash-based operations are generally more sensitive to memory availability than equivalent sort-based operations. The - memory available for hash tables is computed by multiplying + memory limit for hash tables is computed by multiplying <varname>work_mem</varname> by <varname>hash_mem_multiplier</varname>. This makes it possible for hash-based operations to use an amount of memory -- 2.39.2 ^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Correct the documentation for work_mem @ 2023-07-12 23:11 David Rowley <[email protected]> parent: Imseih (AWS), Sami <[email protected]> 0 siblings, 1 reply; 23+ messages in thread From: David Rowley @ 2023-07-12 23:11 UTC (permalink / raw) To: Imseih (AWS), Sami <[email protected]>; +Cc: Gurjeet Singh <[email protected]>; Tom Lane <[email protected]>; Peter Eisentraut <[email protected]>; pgsql-hackers On Tue, 25 Apr 2023 at 04:20, Imseih (AWS), Sami <[email protected]> wrote: > > Based on the feedback, here is a v1 of the suggested doc changes. > > I modified Gurjeets suggestion slightly to make it clear that a specific > query execution could have operations simultaneously using up to > work_mem. > - Note that for a complex query, several sort or hash operations might be > - running in parallel; each operation will generally be allowed > + Note that a complex query may include several sort and hash operations, > + and more than one of these operations may be in progress simultaneously > + for a given query execution; each such operation will generally be allowed > to use as much memory as this value specifies before it starts > to write data into temporary files. Also, several running > sessions could be doing such operations concurrently. I'm wondering about adding "and more than one of these operations may be in progress simultaneously". Are you talking about concurrent sessions running other queries which are using work_mem too? If so, isn't that already covered by the final sentence in the quoted text above? if not, what is running simultaneously? I think Tom's suggestion looks fine. I'd maybe change "sort or hash" to "sort and hash" per the suggestion from Gurjeet above. David ^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Correct the documentation for work_mem @ 2023-07-31 20:44 Tristen Raab <[email protected]> parent: David Rowley <[email protected]> 0 siblings, 0 replies; 23+ messages in thread From: Tristen Raab @ 2023-07-31 20:44 UTC (permalink / raw) To: [email protected]; +Cc: Sami Imseih <[email protected]> The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: not tested Documentation: tested, passed Hello, I've reviewed and built the documentation for the updated patch. As it stands right now I think the documentation for this section is quite clear. > I'm wondering about adding "and more than one of these operations may > be in progress simultaneously". Are you talking about concurrent > sessions running other queries which are using work_mem too? This appears to be referring to the "sort and hash" operations mentioned prior. > If so, > isn't that already covered by the final sentence in the quoted text > above? if not, what is running simultaneously? I believe the last sentence is referring to another session that is running its own sort and hash operations. So the first section you mention is describing how sort and hash operations can be in execution at the same time for a query, while the second refers to how sessions may overlap in their execution of sort and hash operations if I am understanding this correctly. I also agree that changing "sort or hash" to "sort and hash" is a better description. Tristen ^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Correct the documentation for work_mem @ 2023-09-08 00:16 Bruce Momjian <[email protected]> parent: Tom Lane <[email protected]> 1 sibling, 1 reply; 23+ messages in thread From: Bruce Momjian @ 2023-09-08 00:16 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Imseih (AWS), Sami <[email protected]>; pgsql-hackers On Fri, Apr 21, 2023 at 01:15:01PM -0400, Tom Lane wrote: > Peter Eisentraut <[email protected]> writes: > > On 21.04.23 16:28, Imseih (AWS), Sami wrote: > >> I suggest a small doc fix: > >> “Note that for a complex query, several sort or hash operations might be > >> running simultaneously;” > > > Here is a discussion of these terms: > > https://takuti.me/note/parallel-vs-concurrent/ > > > I think "concurrently" is the correct word here. > > Probably, but it'd do little to remove the confusion Sami is on about, > especially since the next sentence uses "concurrently" to describe the > other case. I think we need a more thorough rewording, perhaps like > > - Note that for a complex query, several sort or hash operations might be > - running in parallel; each operation will generally be allowed > + Note that a complex query may include several sort or hash > + operations; each such operation will generally be allowed > to use as much memory as this value specifies before it starts > to write data into temporary files. Also, several running > sessions could be doing such operations concurrently. > > I also find this wording a bit further down to be poor: > > Hash-based operations are generally more sensitive to memory > availability than equivalent sort-based operations. The > memory available for hash tables is computed by multiplying > <varname>work_mem</varname> by > <varname>hash_mem_multiplier</varname>. This makes it > > I think "available" is not le mot juste, and it's also unclear from > this whether we're speaking of the per-hash-table limit or some > (nonexistent) overall limit. How about > > - memory available for hash tables is computed by multiplying > + memory limit for a hash table is computed by multiplying Adjusted patch attached. -- Bruce Momjian <[email protected]> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you. Attachments: [text/x-diff] workmem.diff (1.6K, ../../[email protected]/2-workmem.diff) download | inline diff: diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 6bc1b215db..45d1bb4b7b 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -1829,9 +1829,10 @@ include_dir 'conf.d' (such as a sort or hash table) before writing to temporary disk files. If this value is specified without units, it is taken as kilobytes. The default value is four megabytes (<literal>4MB</literal>). - Note that for a complex query, several sort or hash operations might be - running in parallel; each operation will generally be allowed - to use as much memory as this value specifies before it starts + Note that a complex query might perform several sort or hash + operations at the same time, with each operation generally being + allowed to use as much memory as this value specifies before + it starts to write data into temporary files. Also, several running sessions could be doing such operations concurrently. Therefore, the total memory used could be many times the value @@ -1845,7 +1846,7 @@ include_dir 'conf.d' <para> Hash-based operations are generally more sensitive to memory availability than equivalent sort-based operations. The - memory available for hash tables is computed by multiplying + memory limit for a hash table is computed by multiplying <varname>work_mem</varname> by <varname>hash_mem_multiplier</varname>. This makes it possible for hash-based operations to use an amount of memory ^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Correct the documentation for work_mem @ 2023-09-08 05:23 David Rowley <[email protected]> parent: Bruce Momjian <[email protected]> 0 siblings, 1 reply; 23+ messages in thread From: David Rowley @ 2023-09-08 05:23 UTC (permalink / raw) To: Bruce Momjian <[email protected]>; +Cc: Tom Lane <[email protected]>; Peter Eisentraut <[email protected]>; Imseih (AWS), Sami <[email protected]>; pgsql-hackers On Fri, 8 Sept 2023 at 15:24, Bruce Momjian <[email protected]> wrote: > Adjusted patch attached. This looks mostly fine to me modulo "sort or hash". I do see many instances of "and/or" in the docs. Maybe that would work better. David ^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Correct the documentation for work_mem @ 2023-09-09 02:25 Imseih (AWS), Sami <[email protected]> parent: David Rowley <[email protected]> 0 siblings, 1 reply; 23+ messages in thread From: Imseih (AWS), Sami @ 2023-09-09 02:25 UTC (permalink / raw) To: David Rowley <[email protected]>; Bruce Momjian <[email protected]>; +Cc: Tom Lane <[email protected]>; Peter Eisentraut <[email protected]>; pgsql-hackers > This looks mostly fine to me modulo "sort or hash". I do see many > instances of "and/or" in the docs. Maybe that would work better. "sort or hash operations at the same time" is clear explanation IMO. This latest version of the patch looks good to me. Regards, Sami ^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Correct the documentation for work_mem @ 2023-09-11 10:02 David Rowley <[email protected]> parent: Imseih (AWS), Sami <[email protected]> 0 siblings, 1 reply; 23+ messages in thread From: David Rowley @ 2023-09-11 10:02 UTC (permalink / raw) To: Imseih (AWS), Sami <[email protected]>; +Cc: Bruce Momjian <[email protected]>; Tom Lane <[email protected]>; Peter Eisentraut <[email protected]>; pgsql-hackers On Sat, 9 Sept 2023 at 14:25, Imseih (AWS), Sami <[email protected]> wrote: > > > This looks mostly fine to me modulo "sort or hash". I do see many > > instances of "and/or" in the docs. Maybe that would work better. > > "sort or hash operations at the same time" is clear explanation IMO. Just for anyone else following along that haven't seen the patch. The full text in question is: + Note that a complex query might perform several sort or hash + operations at the same time, with each operation generally being It's certainly not a show-stopper. I do believe the patch makes some improvements. The reason I'd prefer to see either "and" or "and/or" in place of "or" is because the text is trying to imply that many of these operations can run at the same time. I'm struggling to understand why, given that there could be many sorts and many hashes going on at once that we'd claim it could only be one *or* the other. If we have 12 sorts and 4 hashes then that's not "several sort or hash operations", it's "several sort and hash operations". Of course, it could just be sorts or just hashes, so "and/or" works fine for that. David ^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Correct the documentation for work_mem @ 2023-09-11 15:03 Bruce Momjian <[email protected]> parent: David Rowley <[email protected]> 0 siblings, 1 reply; 23+ messages in thread From: Bruce Momjian @ 2023-09-11 15:03 UTC (permalink / raw) To: David Rowley <[email protected]>; +Cc: Imseih (AWS), Sami <[email protected]>; Tom Lane <[email protected]>; Peter Eisentraut <[email protected]>; pgsql-hackers On Mon, Sep 11, 2023 at 10:02:55PM +1200, David Rowley wrote: > On Sat, 9 Sept 2023 at 14:25, Imseih (AWS), Sami <[email protected]> wrote: > > > > > This looks mostly fine to me modulo "sort or hash". I do see many > > > instances of "and/or" in the docs. Maybe that would work better. > > > > "sort or hash operations at the same time" is clear explanation IMO. > > Just for anyone else following along that haven't seen the patch. The > full text in question is: > > + Note that a complex query might perform several sort or hash > + operations at the same time, with each operation generally being > > It's certainly not a show-stopper. I do believe the patch makes some > improvements. The reason I'd prefer to see either "and" or "and/or" > in place of "or" is because the text is trying to imply that many of > these operations can run at the same time. I'm struggling to > understand why, given that there could be many sorts and many hashes > going on at once that we'd claim it could only be one *or* the other. > If we have 12 sorts and 4 hashes then that's not "several sort or hash > operations", it's "several sort and hash operations". Of course, it > could just be sorts or just hashes, so "and/or" works fine for that. Yes, I see your point and went with "and", updated patch attached. -- Bruce Momjian <[email protected]> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you. Attachments: [text/x-diff] workmem.diff (1.6K, ../../[email protected]/2-workmem.diff) download | inline diff: diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 6bc1b215db..8ed7ae57c2 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -1829,9 +1829,10 @@ include_dir 'conf.d' (such as a sort or hash table) before writing to temporary disk files. If this value is specified without units, it is taken as kilobytes. The default value is four megabytes (<literal>4MB</literal>). - Note that for a complex query, several sort or hash operations might be - running in parallel; each operation will generally be allowed - to use as much memory as this value specifies before it starts + Note that a complex query might perform several sort and hash + operations at the same time, with each operation generally being + allowed to use as much memory as this value specifies before + it starts to write data into temporary files. Also, several running sessions could be doing such operations concurrently. Therefore, the total memory used could be many times the value @@ -1845,7 +1846,7 @@ include_dir 'conf.d' <para> Hash-based operations are generally more sensitive to memory availability than equivalent sort-based operations. The - memory available for hash tables is computed by multiplying + memory limit for a hash table is computed by multiplying <varname>work_mem</varname> by <varname>hash_mem_multiplier</varname>. This makes it possible for hash-based operations to use an amount of memory ^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Correct the documentation for work_mem @ 2023-09-26 13:05 David Rowley <[email protected]> parent: Bruce Momjian <[email protected]> 0 siblings, 1 reply; 23+ messages in thread From: David Rowley @ 2023-09-26 13:05 UTC (permalink / raw) To: Bruce Momjian <[email protected]>; +Cc: Imseih (AWS), Sami <[email protected]>; Tom Lane <[email protected]>; Peter Eisentraut <[email protected]>; pgsql-hackers On Tue, 12 Sept 2023 at 03:03, Bruce Momjian <[email protected]> wrote: > > On Mon, Sep 11, 2023 at 10:02:55PM +1200, David Rowley wrote: > > It's certainly not a show-stopper. I do believe the patch makes some > > improvements. The reason I'd prefer to see either "and" or "and/or" > > in place of "or" is because the text is trying to imply that many of > > these operations can run at the same time. I'm struggling to > > understand why, given that there could be many sorts and many hashes > > going on at once that we'd claim it could only be one *or* the other. > > If we have 12 sorts and 4 hashes then that's not "several sort or hash > > operations", it's "several sort and hash operations". Of course, it > > could just be sorts or just hashes, so "and/or" works fine for that. > > Yes, I see your point and went with "and", updated patch attached. Looks good to me. David ^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Correct the documentation for work_mem @ 2023-09-26 23:44 Bruce Momjian <[email protected]> parent: David Rowley <[email protected]> 0 siblings, 0 replies; 23+ messages in thread From: Bruce Momjian @ 2023-09-26 23:44 UTC (permalink / raw) To: David Rowley <[email protected]>; +Cc: Imseih (AWS), Sami <[email protected]>; Tom Lane <[email protected]>; Peter Eisentraut <[email protected]>; pgsql-hackers On Wed, Sep 27, 2023 at 02:05:44AM +1300, David Rowley wrote: > On Tue, 12 Sept 2023 at 03:03, Bruce Momjian <[email protected]> wrote: > > > > On Mon, Sep 11, 2023 at 10:02:55PM +1200, David Rowley wrote: > > > It's certainly not a show-stopper. I do believe the patch makes some > > > improvements. The reason I'd prefer to see either "and" or "and/or" > > > in place of "or" is because the text is trying to imply that many of > > > these operations can run at the same time. I'm struggling to > > > understand why, given that there could be many sorts and many hashes > > > going on at once that we'd claim it could only be one *or* the other. > > > If we have 12 sorts and 4 hashes then that's not "several sort or hash > > > operations", it's "several sort and hash operations". Of course, it > > > could just be sorts or just hashes, so "and/or" works fine for that. > > > > Yes, I see your point and went with "and", updated patch attached. > > Looks good to me. Patch applied back to Postgres 11. -- Bruce Momjian <[email protected]> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you. ^ permalink raw reply [nested|flat] 23+ messages in thread
end of thread, other threads:[~2023-09-26 23:44 UTC | newest] Thread overview: 23+ 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 v3] 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]> 2023-04-21 17:15 Re: Correct the documentation for work_mem Tom Lane <[email protected]> 2023-04-21 17:39 ` Re: Correct the documentation for work_mem Gurjeet Singh <[email protected]> 2023-04-22 03:36 ` Re: Correct the documentation for work_mem Imseih (AWS), Sami <[email protected]> 2023-04-24 16:20 ` Re: Correct the documentation for work_mem Imseih (AWS), Sami <[email protected]> 2023-07-12 23:11 ` Re: Correct the documentation for work_mem David Rowley <[email protected]> 2023-07-31 20:44 ` Re: Correct the documentation for work_mem Tristen Raab <[email protected]> 2023-09-08 00:16 ` Re: Correct the documentation for work_mem Bruce Momjian <[email protected]> 2023-09-08 05:23 ` Re: Correct the documentation for work_mem David Rowley <[email protected]> 2023-09-09 02:25 ` Re: Correct the documentation for work_mem Imseih (AWS), Sami <[email protected]> 2023-09-11 10:02 ` Re: Correct the documentation for work_mem David Rowley <[email protected]> 2023-09-11 15:03 ` Re: Correct the documentation for work_mem Bruce Momjian <[email protected]> 2023-09-26 13:05 ` Re: Correct the documentation for work_mem David Rowley <[email protected]> 2023-09-26 23:44 ` Re: Correct the documentation for work_mem Bruce Momjian <[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