From: | Andres Freund <andres(at)anarazel(dot)de> |
---|---|
To: | Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com> |
Cc: | pryzby(at)telsasoft(dot)com, michael(at)paquier(dot)xyz, nathandbossart(at)gmail(dot)com, pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: fix stats_fetch_consistency value in postgresql.conf.sample |
Date: | 2022-06-22 23:07:10 |
Message-ID: | 20220622230710.r2m4ex5tlf6mveyh@alap3.anarazel.de |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2022-06-22 23:12:14 | Re: SLRUs in the main buffer pool - Page Header definitions |
Previous Message | Imseih (AWS), Sami | 2022-06-22 23:05:54 | Re: [PROPOSAL] Detecting plan changes with plan_id in pg_stat_activity |