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