Re: fix stats_fetch_consistency value in postgresql.conf.sample

Hi,

On 2022-06-17 09:43:58 +0900, Kyotaro Horiguchi wrote:
> +/*
> + * Convert value to unitless value according to the specified GUC variable
> + */
> +Datum
> +pg_config_unitless_value(PG_FUNCTION_ARGS)
> +{
> + 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);
> +

Hm. I think this should error out for options that the user doesn't have the
permissions for - good. I suggest adding a test for that.

> + switch (record->vartype)
> + {
> + case PGC_BOOL:
> + result = (val.boolval ? "on" : "off");
> + break;
> + case PGC_INT:
> + snprintf(buffer, sizeof(buffer), "%d", val.intval);
> + result = pstrdup(buffer);
> + break;
> + case PGC_REAL:
> + snprintf(buffer, sizeof(buffer), "%g", val.realval);
> + result = pstrdup(buffer);
> + break;
> + case PGC_STRING:
> + p = val.stringval;
> + if (p == NULL)
> + p = "";
> + result = pstrdup(p);
> + break;

Is this a good idea? I wonder if we shouldn't instead return NULL, rather than
making NULL and "" undistinguishable.

Not that it matters for efficiency here, but why are you pstrdup'ing the
buffers? cstring_to_text() will already copy the string, no?

> +# parameter names that cannot get consistency check performed
> +my @ignored_parameters = (

Perhaps worth adding comments explaining why these can't get checked?

> +foreach my $line (split("\n", $all_params))
> +{
> + my @f = split('\|', $line);
> + fail("query returned wrong number of columns: $#f : $line") if ($#f != 4);
> + $all_params_hash{$f[0]}->{type} = $f[1];
> + $all_params_hash{$f[0]}->{unit} = $f[2];
> + $all_params_hash{$f[0]}->{bootval} = $f[3];
> +}
>

Might look a bit nicer to generate the hash in a local variable and then
assign to $all_params_hash{$f[0]} once, rather than repeating that part
multiple times.

> - if ($line =~ m/^#?([_[:alpha:]]+) = .*/)
> + if ($line =~ m/^#?([_[:alpha:]]+) = (.*)$/)
> {
> # Lower-case conversion matters for some of the GUCs.
> my $param_name = lc($1);
>
> + # extract value
> + my $file_value = $2;
> + $file_value =~ s/\s*#.*$//; # strip trailing comment
> + $file_value =~ s/^'(.*)'$/$1/; # strip quotes
> +
> # Ignore some exceptions.
> next if $param_name eq "include";
> next if $param_name eq "include_dir";

So there's now two ignore mechanisms? Why not just handle include[_dir] via
@ignored_parameters?

> @@ -66,19 +94,39 @@ while (my $line = <$contents>)
> # Update the list of GUCs found in the sample file, for the
> # follow-up tests.
> push @gucs_in_file, $param_name;
> +
> + # Check for consistency between bootval and file value.

You're not checking the consistency here though?

> + if (!grep { $_ eq $param_name } @ignored_parameters)
> + {
> + push (@check_elems, "('$param_name','$file_value')");
> + }
> }
> }

>
> close $contents;
>
> +# Run consistency check between config-file's default value and boot
> +# values. To show sample setting that is not found in the view, use
> +# LEFT JOIN and make sure pg_settings.name is not NULL.
> +my $check_query =
> + 'SELECT f.n, f.v, s.boot_val FROM (VALUES '.
> + join(',', @check_elems).
> + ') f(n,v) LEFT JOIN pg_settings s ON lower(s.name) = f.n '.
> + "WHERE pg_config_unitless_value(f.n, f.v) <> COALESCE(s.boot_val, '') ".
> + 'OR s.name IS NULL';
> +
> +print $check_query;
> +
> +is ($node->safe_psql('postgres', $check_query), '',
> + 'check if fileval-bootval consistency is fine');

"fileval-bootval" isn't that easy to understand, "is fine" doesn't quite sound
right. Maybe something like "GUC values in .sample and boot value match"?

I wonder if it'd not result in easier to understand output if the query just
called pg_config_unitless_value() for all the .sample values, but then did the
comparison of the results in perl.

Greetings,

Andres Freund