Re: fix stats_fetch_consistency value in postgresql.conf.sample - Mailing list pgsql-hackers

At Thu, 16 Jun 2022 12:07:03 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> On Sat, Jun 11, 2022 at 09:41:37AM -0500, Justin Pryzby wrote:
> > Note that this gives:
> > 
> > guc.c:7573:9: warning: ‘dst’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> > 
> > with gcc version 9.2.1 20191008 (Ubuntu 9.2.1-9ubuntu2)
> > 
> > I wonder whether you'd consider renaming pg_normalize_config_value() to
> > pg_pretty_config_value() or similar.
> 
> I have looked at the patch, and I am not convinced that we need a
> function that does a integer -> integer-with-unit conversion for the
> purpose of this test.  One thing is that it can be unstable with the
> unit in the conversion where values are close to a given threshold
> (aka for cases like 2048kB/2MB).  On top of that, this overlaps with

I agree that needing to-with-unit conversion is a bit crumsy.  One of
the reason is I didn't want to add a function that has no use of other
than testing,

> the existing system function in charge of converting values with bytes
> as size unit, while this stuff handles more unit types and all GUC
> types.  I think that there could be some room in doing the opposite
> conversion, feeding the value from postgresql.conf.sample to something
> and compare it directly with boot_val.  That's solvable at SQL level,
> still a system function may be more user-friendly.

The output value must be the same with what pg_settings shows, so it
needs to take in some code from GetConfigOptionByNum() (and needs to
keep in-sync with it), which is what I didn't wnat to do. Anyway done
in the attached.

This method has a problem for wal_buffers. parse_and_validate_value()
returns 512 for -1 input since check_wal_buffers() converts it to 512.
It is added to the exclusion list.  (Conversely the previous method
regarded "-1" and "512" as identical.)

> Extending the tests to check after the values is something worth
> doing, but I think that I would limit the checks to the parameters  
> that do not have units for now, until we figure out which interface
> would be more adapted for doing the normalization of the parameter
> values.

The attached second is that.  FWIW, I'd like to support integer/real
values since I think they need more support of this kind of check.

> -#syslog_facility = 'LOCAL0'
> +#syslog_facility = 'local0'
> Those changes should not be necessary in postgresql.conf.sample.  The
> test should be in charge of applying the lower() conversion, in the
> same way as guc.c does internally, and that's a mode supported by the
> parameter parsing.  Using an upper-case value in the sample file is
> actually meaningful sometimes (for example, syslog can use upper-case
> strings to refer to LOCAL0~7).

I didn't notice, but now know parse_and_validate_value() convers
values the same way with bootval so finally case-unification is not
needed.

=# select pg_config_unitless_value('datestyle', 'iso, mdy');
 pg_config_unitless_value 
--------------------------
 ISO, MDY

However, the "datestyle" variable is shown as "DateStyle" in the
pg_settings view. So the name in the view needs to be lower-cased
instead. The same can be said to "TimeZone" and "IntervalStyle".  The
old query missed the case where there's no variable with the names
appear in the config file. Fixed it.

At Sat, 11 Jun 2022 09:41:37 -0500, Justin Pryzby <pryzby@telsasoft.com> wrote in 
> Note that this gives:
> 
> guc.c:7573:9: warning: ‘dst’ may be used uninitialized in this function [-Wmaybe-uninitialized]

Mmm. I don't have an idea where the 'dst' came from...


> I wonder whether you'd consider renaming pg_normalize_config_value() to
> pg_pretty_config_value() or similar.

Yeah, that's sensible, the function is now changed (not renamed) to
pg_config_unitless_value().  This name also doesn't satisfies me at
all..:(


So, the attached are:

v2-0001-Add-fileval-bootval-consistency-check-of-GUC-para.patch:

  New version of the previous patch.  It is changed after Michael's
  suggestions.
  

0001-Add-fileval-bootval-consistency-check-of-GUC-paramet-simple.patch

  Another version that doesn't need new C function.  It ignores
  variables that have units but I didn't count how many variables are
  ignored by this chnage.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center