Fix GUC_NO_SHOW_ALL test scenario in 003_check_guc.pl

On Fri, Jan 13, 2023 at 06:15:38PM +0530, Nitin Jadhav wrote:

Hi,

The commit 7265dbffad7feac6ea9d373828583b5d3d152e07 has added a script
in src/backend/utils/misc/check_guc that cross-checks the consistency
of the GUCs with postgresql.conf.sample, making sure that its format
is in line with what guc.c has. As per the commit message, the
parameters which are not listed as NOT_IN_SAMPLE in guc.c should be
present in postgresql.conf.sample. But I have observed a test case
failure when the parameters which are listed as GUC_NO_SHOW_ALL in
guc.c and if it is present in postgresql.conf.sample. I feel this
behaviour is not expected and this should be fixed. I spent some time
on the analysis and found that query [1] is used to fetch all the
parameters which are not listed as NOT_IN_SAMPLE. But the pg_settings
view does not return the parameters listed as GUC_NO_SHOW_ALL. Hence
these records will be missed. Please share your thoughts. I would like
to work on the patch if a fix is required.

Looks like you're right ; show_all_settings() elides settings marked
"noshow".

Do you know how you'd implement a fix ?

--
Justin

Looks like you're right ; show_all_settings() elides settings marked
"noshow".

Do you know how you'd implement a fix ?

I could think of the following options.

Option-1 is, expose a function like pg_settings_get_no_show_all()
which just returns the parameters which are just listed as
GUC_NO_SHOW_ALL (Not in combination with NOT_IN_SAMPLE). We can then
use this function in the test file and verify whether there are config
entries for these.

Option-2 is, if exposing new function and that too to expose
parameters which are listed as GUC_NO_SHOW_ALL is not recommended,
then how about exposing a function like pg_settings_get_count() which
returns the count of all parameters including GUC_NO_SHOW_ALL. We can
then use this number to verify whether these many are present in the
sample config file. But we cannot show the name of the parameters if
it is not matching. We can just display an error saying "Parameter
with GUC_NO_SHOW_ALL is missing from postgresql.conf.sample".

Option-3 is, if exposing both of the above functions is not
recommended, then we can use the existing function
pg_settings_get_flags() for each of the parameters while reading the
sample config file in 003_check_guc.pl. This validates the
GUC_NO_SHOW_ALL parameter if that is present in the sample config
file. It does not validate if it is present in guc.c and missing in
the sample config file.

Option-4 is, how about manually adding the parameter name to
'all_params_array' in 003_check_guc.pl whenever we add such GUCs.

I am not able to choose any of the above options as each has some
disadvantages but if no other options exist, then I would like to go
with option-3 as it validates more than the one currently doing.
Please share if any other better ideas.

Thanks & Regards,
Nitin Jadhav

On Sat, Jan 14, 2023 at 07:10:55PM +0530, Nitin Jadhav wrote:

Option-1 is, expose a function like pg_settings_get_no_show_all()
which just returns the parameters which are just listed as
GUC_NO_SHOW_ALL (Not in combination with NOT_IN_SAMPLE). We can then
use this function in the test file and verify whether there are config
entries for these.

Option-2 is, if exposing new function and that too to expose
parameters which are listed as GUC_NO_SHOW_ALL is not recommended,
then how about exposing a function like pg_settings_get_count() which
returns the count of all parameters including GUC_NO_SHOW_ALL. We can
then use this number to verify whether these many are present in the
sample config file. But we cannot show the name of the parameters if
it is not matching. We can just display an error saying "Parameter
with GUC_NO_SHOW_ALL is missing from postgresql.conf.sample".

We would miss the names of the parameters that are marked as NO_SHOW,
missing from pg_settings, making debugging harder.

Option-3 is, if exposing both of the above functions is not
recommended, then we can use the existing function
pg_settings_get_flags() for each of the parameters while reading the
sample config file in 003_check_guc.pl. This validates the
GUC_NO_SHOW_ALL parameter if that is present in the sample config
file. It does not validate if it is present in guc.c and missing in
the sample config file.

This would make the test more costly by forcing one SQL for each
GUC..

Option-4 is, how about manually adding the parameter name to
'all_params_array' in 003_check_guc.pl whenever we add such GUCs.

I am not able to choose any of the above options as each has some
disadvantages but if no other options exist, then I would like to go
with option-3 as it validates more than the one currently doing.
Please share if any other better ideas.

We could extend pg_show_all_settings() with a boolean parameter to
enforce listing all the parameters, even the ones that are marked as
NOSHOW, but this does not count on GetConfigOptionValues() that could
force a parameter to become noshow on a superuser-only GUC depending
on the role that's running the function. At the end, we'd better rely
on a separate superuser-only function to do this job, aka option 1.

How much do we need to care with the duplication this would involve
with show_all_settings(), actually? If you don't use the SRF macros,
the code would just be a couple of lines with InitMaterializedSRF()
doing a loop on GetConfigOptionValues(). Even if that means listing
twice the parameters in pg_proc.dat, the chances of adding new
parameters in pg_settings is rather low so that would be a one-time
change?
--
Michael

We would miss the names of the parameters that are marked as NO_SHOW,
missing from pg_settings, making debugging harder.

This would make the test more costly by forcing one SQL for each
GUC..

I agree.

We could extend pg_show_all_settings() with a boolean parameter to
enforce listing all the parameters, even the ones that are marked as
NOSHOW, but this does not count on GetConfigOptionValues() that could
force a parameter to become noshow on a superuser-only GUC depending
on the role that's running the function. At the end, we'd better rely
on a separate superuser-only function to do this job, aka option 1.

I did not get it completely. To understand it better, I just gave a
thought of adding a boolean parameter to pg_show_all_settings(). Then
we should modify GetConfigOptionValues() like below [1]static void GetConfigOptionValues(struct config_generic *conf, const char **values, bool *noshow, bool is_show_all) { char buffer[256];. When we call
pg_show_all_settings(false), it behaves like existing behaviour (with
super user and without super user). When we call
pg_show_all_settings(true) with super user privileges, it returns all
parameters including GUC_NO_SHOW_ALL as well as GUC_SUPER_USER_ONLY.
If we call pg_show_all_settings(true) without super user privileges,
then it returns all parameters except GUC_NO_SHOW_ALL and
GUC_SUPER_USER_ONLY. Can't we do it this way? Please share your
thoughts.

How much do we need to care with the duplication this would involve
with show_all_settings(), actually? If you don't use the SRF macros,
the code would just be a couple of lines with InitMaterializedSRF()
doing a loop on GetConfigOptionValues(). Even if that means listing
twice the parameters in pg_proc.dat, the chances of adding new
parameters in pg_settings is rather low so that would be a one-time
change?

How about just fetching the parameter name instead of fetching all its
details. This will meet our objective as well as it controls the code
duplication.

[1]: static void GetConfigOptionValues(struct config_generic *conf, const char **values, bool *noshow, bool is_show_all) { char buffer[256];
static void
GetConfigOptionValues(struct config_generic *conf, const char **values,
bool *noshow, bool is_show_all)
{
char buffer[256];

if (noshow)
{
if (((conf->flags & GUC_NO_SHOW_ALL) && !is_show_all) ||
((conf->flags & GUC_NO_SHOW_ALL) &&
!has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS)) ||
((conf->flags & GUC_SUPERUSER_ONLY) &&
!has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS)))
*noshow = true;
else
*noshow = false;
}
-
-
-
}

We could extend pg_show_all_settings() with a boolean parameter to
enforce listing all the parameters, even the ones that are marked as
NOSHOW, but this does not count on GetConfigOptionValues() that could
force a parameter to become noshow on a superuser-only GUC depending
on the role that's running the function. At the end, we'd better rely
on a separate superuser-only function to do this job, aka option 1.

I had started a separate thread [1]/messages/by-id/CALj2ACXZMOGEtjk+eh0Zeiz=46ETVkr0koYAjWt8SoJUJJUe9g@mail.gmail.com to refactor the code around
GetConfigOptionValues() and the patch is already committed. Now it
makes our job simpler to extend pg_show_all_settings() with a boolean
parameter to enforce listing all the parameters, even the ones that
are marked as NOSHOW. I have attached the patch for the same. Kindly
look into it and share your thoughts.

[1]: /messages/by-id/CALj2ACXZMOGEtjk+eh0Zeiz=46ETVkr0koYAjWt8SoJUJJUe9g@mail.gmail.com

Thanks & Regards,
Nitin Jadhav

On Wed, Jan 18, 2023 at 12:31 PM Nitin Jadhav
<nitinjadhavpostgres@gmail.com> wrote:

On Sun, Jan 29, 2023 at 05:22:13PM +0530, Nitin Jadhav wrote:

We could extend pg_show_all_settings() with a boolean parameter to
enforce listing all the parameters, even the ones that are marked as
NOSHOW, but this does not count on GetConfigOptionValues() that could
force a parameter to become noshow on a superuser-only GUC depending
on the role that's running the function. At the end, we'd better rely
on a separate superuser-only function to do this job, aka option 1.

I had started a separate thread [1] to refactor the code around
GetConfigOptionValues() and the patch is already committed. Now it
makes our job simpler to extend pg_show_all_settings() with a boolean
parameter to enforce listing all the parameters, even the ones that
are marked as NOSHOW. I have attached the patch for the same. Kindly
look into it and share your thoughts.

SELECT pg_show_all_settings() ought to keep working when called with no
parameter. Tom gave me a hint how to do that for system catalogs here:
/messages/by-id/17988.1584472261@sss.pgh.pa.us

In this case, it might be cleaner to add a second entry to pg_proc.dat
than to add "CREATE OR REPLACE FUNCTION" to system_functions.sql (I
tried but couldn't get that to work just now).

--
Justin

Justin Pryzby <pryzby@telsasoft.com> writes:

SELECT pg_show_all_settings() ought to keep working when called with no
parameter. Tom gave me a hint how to do that for system catalogs here:
/messages/by-id/17988.1584472261@sss.pgh.pa.us
In this case, it might be cleaner to add a second entry to pg_proc.dat
than to add "CREATE OR REPLACE FUNCTION" to system_functions.sql (I
tried but couldn't get that to work just now).

I kind of think this is a lot of unnecessary work. The case that is
problematic is a GUC that's marked GUC_NO_SHOW_ALL but not marked
GUC_NOT_IN_SAMPLE. There aren't any of those, and I don't think there
are likely to be any in future, because it doesn't make a lot of sense.
Why don't we just make a policy against doing that, and enforce it
with an assertion somewhere in GUC initialization?

regards, tom lane

On Sun, Jan 29, 2023 at 01:05:07PM -0500, Tom Lane wrote:

I kind of think this is a lot of unnecessary work. The case that is
problematic is a GUC that's marked GUC_NO_SHOW_ALL but not marked
GUC_NOT_IN_SAMPLE. There aren't any of those, and I don't think there
are likely to be any in future, because it doesn't make a lot of sense.
Why don't we just make a policy against doing that, and enforce it
with an assertion somewhere in GUC initialization?

[..thinks..]

Looking at guc.sql, I think that these is a second mistake: the test
checks for (no_show_all AND !no_reset_all) but this won't work
because NO_SHOW_ALL GUCs cannot be scanned via SQL. There are two
parameters that include this combination of flags: default_with_oids
and ssl_renegotiation_limit, so the check would not do what it
should. I think that this part should be removed.

For the second part to prevent GUCs to be marked as NO_SHOW_ALL &&
!NOT_IN_SAMPLE, check_GUC_init() looks like the right place to me,
because this routine has been designed exactly for this purpose.

So, what do you think about the attached?
--
Michael

I kind of think this is a lot of unnecessary work. The case that is
problematic is a GUC that's marked GUC_NO_SHOW_ALL but not marked
GUC_NOT_IN_SAMPLE. There aren't any of those, and I don't think there
are likely to be any in future, because it doesn't make a lot of sense.
Why don't we just make a policy against doing that, and enforce it
with an assertion somewhere in GUC initialization?

Looking at guc.sql, I think that these is a second mistake: the test
checks for (no_show_all AND !no_reset_all) but this won't work
because NO_SHOW_ALL GUCs cannot be scanned via SQL. There are two
parameters that include this combination of flags: default_with_oids
and ssl_renegotiation_limit, so the check would not do what it
should. I think that this part should be removed.

Thanks Michael for identifying a new mistake. I am a little confused
here. I don't understand why GUC_NO_SHOW_ALL depends on other GUCs
like GUC_NOT_IN_SAMPLE or GUC_NO_RESET_ALL. Looks like the dependency
between GUC_NO_RESET_ALL and GUC_NO_SHOW_ALL is removed in the above
patch since we have these combinations now. Similarly why can't we
have a GUC marked as GUC_NO_SHOW_ALL but not GUC_NOT_IN_CONFIG. For me
it makes sense if a GUC is marked as NO_SHOW_ALL and it can be present
in a sample file. It's up to the author/developer to choose which all
flags are applicable to the newly introduced GUCs. Please share your
thoughts.

Thanks & Regards,
Nitin Jadhav

On Mon, Jan 30, 2023 at 05:12:27PM +0530, Nitin Jadhav wrote:

Thanks Michael for identifying a new mistake. I am a little confused
here. I don't understand why GUC_NO_SHOW_ALL depends on other GUCs
like GUC_NOT_IN_SAMPLE or GUC_NO_RESET_ALL. Looks like the dependency
between GUC_NO_RESET_ALL and GUC_NO_SHOW_ALL is removed in the above
patch since we have these combinations now.

pg_settings would be unable to show something marked as NO_SHOW_ALL,
so the SQL check that looked after (NO_SHOW_ALL && !NO_RESET_ALL) is
a no-op. Postgres will likely gain more parameters that are kept
around for compability reasons, and forcing a NO_RESET_ALL in such
cases could impact applications using RESET on such GUCs, meaning
potential compatibility breakages.

Similarly why can't we
have a GUC marked as GUC_NO_SHOW_ALL but not GUC_NOT_IN_CONFIG. For me
it makes sense if a GUC is marked as NO_SHOW_ALL and it can be present
in a sample file. It's up to the author/developer to choose which all
flags are applicable to the newly introduced GUCs. Please share your
thoughts.

As also mentioned upthread by Tom, I am not sure that this combination
makes much sense, actually, because I don't see why one would never
want to know what is the effective value loaded for a parameter stored
in a file when he/she has the permission to do so. This could be
changed as of ALTER SYSTEM, postgresql.conf or even an included file,
and the value can only be read if permission to see it is given to the
role querying SHOW or pg_settings. This combination of flags is not a
practice to encourage.
--
Michael

On Wed, Feb 01, 2023 at 02:29:23PM +0900, Michael Paquier wrote:

As also mentioned upthread by Tom, I am not sure that this combination
makes much sense, actually, because I don't see why one would never
want to know what is the effective value loaded for a parameter stored
in a file when he/she has the permission to do so. This could be
changed as of ALTER SYSTEM, postgresql.conf or even an included file,
and the value can only be read if permission to see it is given to the
role querying SHOW or pg_settings. This combination of flags is not a
practice to encourage.

So, coming back quickly to this one, it seems to me that the tests in
guc.sql had better be adjusted down v15 where they have been
introduced, and that the extra check is worth doing on HEAD. Any
thoughts?
--
Michael

Thanks Michael for identifying a new mistake. I am a little confused
here. I don't understand why GUC_NO_SHOW_ALL depends on other GUCs
like GUC_NOT_IN_SAMPLE or GUC_NO_RESET_ALL. Looks like the dependency
between GUC_NO_RESET_ALL and GUC_NO_SHOW_ALL is removed in the above
patch since we have these combinations now.

pg_settings would be unable to show something marked as NO_SHOW_ALL,
so the SQL check that looked after (NO_SHOW_ALL && !NO_RESET_ALL) is
a no-op. Postgres will likely gain more parameters that are kept
around for compability reasons, and forcing a NO_RESET_ALL in such
cases could impact applications using RESET on such GUCs, meaning
potential compatibility breakages.

Similarly why can't we
have a GUC marked as GUC_NO_SHOW_ALL but not GUC_NOT_IN_CONFIG. For me
it makes sense if a GUC is marked as NO_SHOW_ALL and it can be present
in a sample file. It's up to the author/developer to choose which all
flags are applicable to the newly introduced GUCs. Please share your
thoughts.

As also mentioned upthread by Tom, I am not sure that this combination
makes much sense, actually, because I don't see why one would never
want to know what is the effective value loaded for a parameter stored
in a file when he/she has the permission to do so. This could be
changed as of ALTER SYSTEM, postgresql.conf or even an included file,
and the value can only be read if permission to see it is given to the
role querying SHOW or pg_settings. This combination of flags is not a
practice to encourage.

Got it. Makes sense.

For the second part to prevent GUCs to be marked as NO_SHOW_ALL &&
!NOT_IN_SAMPLE, check_GUC_init() looks like the right place to me,
because this routine has been designed exactly for this purpose.

So, what do you think about the attached?

My concern is if we do this, then we will end up having some policies
(which can be read from pg_show_all_settings()) in guc.sql and some in
guc.c. I feel all these should be at one place either at guc.c or
guc.sql. It is better to move all other policies from guc.sql to
guc.c. Otherwise, how about modifying the function
pg_show_all_settings as done in v1 patch and using this (as true)
while creating the table tab_settings_flags in guc.sq and just remove
(NO_SHOW_ALL && !NO_RESET_ALL) check from guc.sql. I don't think doing
this is a problem as we can retain the support of existing signatures
of the pg_show_all_settings function as suggested by Justin upthread
so that it will not cause any compatibility issues. Please share your
thoughts.

Thanks & Regards,
Nitin Jadhav

Nitin Jadhav <nitinjadhavpostgres@gmail.com> writes:

My concern is if we do this, then we will end up having some policies
(which can be read from pg_show_all_settings()) in guc.sql and some in
guc.c. I feel all these should be at one place either at guc.c or
guc.sql.

I don't particularly see why that needs to be the case. Notably,
if we're interested in enforcing a policy even for extension GUCs,
guc.sql can't really do that since who knows whether the extension's
author will bother to run that test with the extension loaded.
On the other hand, moving *all* those checks into guc.c is probably
impractical and certainly will add undesirable startup overhead.

regards, tom lane

I don't particularly see why that needs to be the case. Notably,
if we're interested in enforcing a policy even for extension GUCs,
guc.sql can't really do that since who knows whether the extension's
author will bother to run that test with the extension loaded.
On the other hand, moving *all* those checks into guc.c is probably
impractical and certainly will add undesirable startup overhead.

Ok. Understood the other problems. I have attached the v2 patch which
uses the idea present in Michael's patch. In addition, I have removed
fetching NO_SHOW_ALL parameters while creating tab_settings_flags
table in guc.sql and adjusted the test which checks for (NO_RESET_ALL
AND NOT NO_SHOW_ALL) as this was misleading the developer who thinks
that tab_settings_flags table has NO_SHOW_ALL parameters which is
incorrect.

Please review and share your thoughts.

Thanks & Regards,
Nitin Jadhav

On Sun, Feb 05, 2023 at 12:56:58AM +0530, Nitin Jadhav wrote:

Ok. Understood the other problems. I have attached the v2 patch which
uses the idea present in Michael's patch. In addition, I have removed
fetching NO_SHOW_ALL parameters while creating tab_settings_flags
table in guc.sql and adjusted the test which checks for (NO_RESET_ALL
AND NOT NO_SHOW_ALL) as this was misleading the developer who thinks
that tab_settings_flags table has NO_SHOW_ALL parameters which is
incorrect.

Okay, the part to add an initialization check for GUC_NO_SHOW_ALL and
GUC_NOT_IN_SAMPLE looked fine by me, so applied after more comment
polishing.

+-- NO_RESET_ALL can be specified without NO_SHOW_ALL, like transaction_*.
+-- tab_settings_flags does not contain NO_SHOW_ALL flags. Just checking for
+-- NO_RESET_ALL implies NO_RESET_ALL AND NOT NO_SHOW_ALL.
 SELECT name FROM tab_settings_flags
-  WHERE NOT no_show_all AND no_reset_all
+  WHERE no_reset_all
   ORDER BY 1;

Removing entirely no_show_all is fine by me, but keeping this SQL has
little sense, then, because it would include any GUCs loaded by an
external source when they define NO_RESET_ALL. I think that 0001
should be like the attached, instead, backpatched down to 15 (release
week, so it cannot be touched until the next version is stamped),
where we just remove all the checks based on no_show_all.

On top of that, I have noticed an extra combination that would not
make sense and that could be checked with the SQL queries:
GUC_DISALLOW_IN_FILE implies GUC_NOT_IN_SAMPLE. The opposite may not
be true, though, as some developer GUCs are marked as
GUC_NOT_IN_SAMPLE but they are allowed in a file. The only exception
to that currently is config_file. It is just a special case whose
value is enforced at startup and it can be passed down as an option
switch via the postgres binary, still it seems like it would be better
to also mark it as GUC_NOT_IN_SAMPLE? This is done in 0002, only for
HEAD, as that would be a new check.

Thoughts?
--
Michael

On Mon, Feb 06, 2023 at 04:23:02PM +0900, Michael Paquier wrote:

On top of that, I have noticed an extra combination that would not
make sense and that could be checked with the SQL queries:
GUC_DISALLOW_IN_FILE implies GUC_NOT_IN_SAMPLE. The opposite may not
be true, though, as some developer GUCs are marked as
GUC_NOT_IN_SAMPLE but they are allowed in a file. The only exception
to that currently is config_file. It is just a special case whose
value is enforced at startup and it can be passed down as an option
switch via the postgres binary, still it seems like it would be better
to also mark it as GUC_NOT_IN_SAMPLE? This is done in 0002, only for
HEAD, as that would be a new check.

0001 has been applied to clean up the existing situation. Remains
0002, that I am letting sleep to see if there's interest for it, or
perhaps more ideas around it.
--
Michael

Okay, the part to add an initialization check for GUC_NO_SHOW_ALL and
GUC_NOT_IN_SAMPLE looked fine by me, so applied after more comment
polishing.

0001 has been applied to clean up the existing situation.

Thanks for committing these 2 changes.

On top of that, I have noticed an extra combination that would not
make sense and that could be checked with the SQL queries:
GUC_DISALLOW_IN_FILE implies GUC_NOT_IN_SAMPLE. The opposite may not
be true, though, as some developer GUCs are marked as
GUC_NOT_IN_SAMPLE but they are allowed in a file. The only exception
to that currently is config_file. It is just a special case whose
value is enforced at startup and it can be passed down as an option
switch via the postgres binary, still it seems like it would be better
to also mark it as GUC_NOT_IN_SAMPLE? This is done in 0002, only for
HEAD, as that would be a new check.

Remains
0002, that I am letting sleep to see if there's interest for it, or
perhaps more ideas around it.

Makes sense and the patch looks good to me.

Thanks & Regards,
Nitin Jadhav

On Wed, Feb 08, 2023 at 04:21:57PM +0530, Nitin Jadhav wrote:

Makes sense and the patch looks good to me.

Ah, OK. Thanks for the feedback!

I am wondering.. Did people notice that this adds GUC_NOT_IN_SAMPLE
to config_file in guc_tables.c? This makes sense in the long run
based on what this parameter is by design, still there may be an
objection to doing that?
--
Michael

On Thu, Feb 09, 2023 at 10:28:14AM +0900, Michael Paquier wrote:

On Wed, Feb 08, 2023 at 04:21:57PM +0530, Nitin Jadhav wrote:

Makes sense and the patch looks good to me.

Ah, OK. Thanks for the feedback!

I am wondering.. Did people notice that this adds GUC_NOT_IN_SAMPLE
to config_file in guc_tables.c? This makes sense in the long run
based on what this parameter is by design, still there may be an
objection to doing that?

I think it's fine to add the flag.

See also:

/messages/by-id/20211129030833.GJ17618@telsasoft.com
|Since GUC_DISALLOW_IN_FILE effectively implies GUC_NOT_IN_SAMPLE in
|src/backend/utils/misc/help_config.c:displayStruct(), many of the
|redundant GUC_NOT_IN_SAMPLE could be removed.

--
Justin