Feedback-ID: id4a34324:Fastmail
Date: Wed, 22 Jun 2022 16:07:10 -0700
From: Andres Freund <andres@anarazel.de>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: pryzby@telsasoft.com, michael@paquier.xyz, nathandbossart@gmail.com,
	pgsql-hackers@postgresql.org
Subject: Re: fix stats_fetch_consistency value in postgresql.conf.sample
Message-ID: <20220622230710.r2m4ex5tlf6mveyh@alap3.anarazel.de>
References: <YqqeVyiwttFhJgA2@paquier.xyz>
 <20220616.171946.472801583794269334.horikyota.ntt@gmail.com>
 <20220616132306.GD29853@telsasoft.com>
 <20220617.094358.2201653243066915172.horikyota.ntt@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220617.094358.2201653243066915172.horikyota.ntt@gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/20220622230710.r2m4ex5tlf6mveyh%40alap3.anarazel.de>
Precedence: bulk

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