Re: fix stats_fetch_consistency value in postgresql.conf.sample

Thanks!

At Wed, 22 Jun 2022 16:07:10 -0700, Andres Freund <andres(at)anarazel(dot)de> wrote in
> 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)
> > +{
...
> > + 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.

Generally sounds reasonable but it doesn't reveal its setting. It
just translates (or decodes) given string to internal value. And
currently most of all values are string and only two are enum (TLS
version), that are returned almost as-is. That being said, the
suggested behavior seems better. So I did that in the attached.
And I added the test for this to rolenames in modules/unsafe_tests.

> > + 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.

Anyway NULL cannot be seen there and I don't recall the reason I made
the function non-strict. I changed the SQL function back to 'strict',
which makes things cleaner and simpler.

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

Right. That's a silly thinko, omitting that behavior..

>
> > +# parameter names that cannot get consistency check performed
> > +my @ignored_parameters = (
>
> Perhaps worth adding comments explaining why these can't get checked?

Mmm. I agree. I rewrote it as the follows.

> # The following parameters are defaultly set with
> # environment-dependent values which may not match the default values
> # written in the sample config file.

> > +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.

Yeah, but I noticed that that hash is no longer needed..

> > - 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?

The two ignore mechanisms work for different arrays. So we need to
distinct between the two uses. I tried that but it looks like
reseparating particles that were uselessly mixed. Finally I changed
the variable to hash and apply the same mechanism to "include" and
friends but by using different hash.

> > @@ -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?

Mmm. Right. I reworded it following the comment just above.

> > + 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"?

No objection. Changed.

> 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.

It is a fair alternative. I said exactly the same thing (perl is
easier to understand than the same (procedual) logic in SQL)
upthread:p So done that in the attached.

I tempted to find extra filevals by the code added here but it is
cleaner to leave it to the existing checking code.

- Change the behavior of pg_config_unitless_value according to the comment.
- and added the test for the function's behavior about privileges.
- Skip "include" and friends by using a hash similar to ignore_parameters.
- Removed %all_params_hash. (Currently it is @file_vals)
- A comment reworded (but it donesn't look fine..).
- Moved value-check logic from SQL to perl.

And I'll add this to the coming CF.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center