Обсуждение: Fix GUC_NO_SHOW_ALL test scenario in 003_check_guc.pl

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.


[1]:
SELECT name FROM pg_settings WHERE NOT 'NOT_IN_SAMPLE' = ANY
(pg_settings_get_flags(name)) AND name <> 'config_file' ORDER BY 1;

Thanks & Regards,
Nitin Jadhav

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 Fri, Jan 13, 2023 at 7:32 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> 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

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

    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;
    }
    -
    -
    -
}

On Mon, Jan 16, 2023 at 7:58 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> 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 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.

[1]:
https://www.postgresql.org/message-id/flat/CALj2ACXZMOGEtjk%2Beh0Zeiz%3D46ETVkr0koYAjWt8SoJUJJUe9g%40mail.gmail.com#04705e421e0dc63b1f0c862ae4929e6f

Thanks & Regards,
Nitin Jadhav

On Wed, Jan 18, 2023 at 12:31 PM Nitin Jadhav
<nitinjadhavpostgres@gmail.com> wrote:
>
> > 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]. 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];
>
>     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;
>     }
>     -
>     -
>     -
> }
>
> On Mon, Jan 16, 2023 at 7:58 AM Michael Paquier <michael@paquier.xyz> wrote:
> >
> > 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

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:
https://www.postgresql.org/message-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:
> https://www.postgresql.org/message-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 10:36 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> 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

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

On Wed, Feb 1, 2023 at 10:59 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> 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

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 Sat, Feb 4, 2023 at 1:07 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> 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

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 8, 2023 at 1:29 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> 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

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:

https://www.postgresql.org/message-id/flat/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

Justin Pryzby <pryzby@telsasoft.com> writes:
> On Thu, Feb 09, 2023 at 10:28:14AM +0900, Michael Paquier wrote:
>> 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.

Hm.  On the one hand, if it is in fact not in postgresql.conf.sample,
then that flag should be set for sure.  OTOH I see that that flag
isn't purely documentation: help_config.c thinks it should hide
GUCs that are marked that way.  Do we really want that behavior?
Not sure.  I can see an argument that you might want --describe-config
to tell you that, but there are a lot of other GUC_NOT_IN_SAMPLE
GUCs that maybe do indeed deserve to be left out.

            regards, tom lane

On Wed, Feb 08, 2023 at 09:42:13PM -0500, Tom Lane wrote:
> Hm.  On the one hand, if it is in fact not in postgresql.conf.sample,
> then that flag should be set for sure.  OTOH I see that that flag
> isn't purely documentation: help_config.c thinks it should hide
> GUCs that are marked that way.  Do we really want that behavior?
> Not sure.  I can see an argument that you might want --describe-config
> to tell you that, but there are a lot of other GUC_NOT_IN_SAMPLE
> GUCs that maybe do indeed deserve to be left out.

I am not sure to follow.  help_config() won't show something that's
either marked NO_SHOW_ALL, NOT_IN_SAMPLE or DISALLOW_IN_FILE, hence
config_file does not show up already in what postgres
--describe-config prints, because it has DISALLOW_IN_FILE, so adding
NOT_IN_SAMPLE changes nothing.
--
Michael

At Fri, 10 Feb 2023 16:43:15 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> On Wed, Feb 08, 2023 at 09:42:13PM -0500, Tom Lane wrote:
> > Hm.  On the one hand, if it is in fact not in postgresql.conf.sample,
> > then that flag should be set for sure.  OTOH I see that that flag
> > isn't purely documentation: help_config.c thinks it should hide
> > GUCs that are marked that way.  Do we really want that behavior?
> > Not sure.  I can see an argument that you might want --describe-config
> > to tell you that, but there are a lot of other GUC_NOT_IN_SAMPLE
> > GUCs that maybe do indeed deserve to be left out.
> 
> I am not sure to follow.  help_config() won't show something that's
> either marked NO_SHOW_ALL, NOT_IN_SAMPLE or DISALLOW_IN_FILE, hence
> config_file does not show up already in what postgres
> --describe-config prints, because it has DISALLOW_IN_FILE, so adding
> NOT_IN_SAMPLE changes nothing.

I think currently the output by --describe-config can be used only for
consulting while editing a (possiblly broken) config file.  Thus I
think it's no use showing GIC_DISALLOW_IN_FILE items there unless we
use help_config() for an on-session use.

On the other hand, don't we need to remove the condition
GUC_NOT_IN_SAMPLE from displayStruct? I think that help_config()
should show a value if it is marked as !GUC_DISALLOW_IN_FILE even if
it is GUC_NOT_IN_SAMPLE.  I'm not sure whether there's any variable
that are marked that way, though.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Mon, Feb 13, 2023 at 11:27:58AM +0900, Kyotaro Horiguchi wrote:
> I think currently the output by --describe-config can be used only for
> consulting while editing a (possiblly broken) config file.  Thus I
> think it's no use showing GIC_DISALLOW_IN_FILE items there unless we
> use help_config() for an on-session use.
>
> On the other hand, don't we need to remove the condition
> GUC_NOT_IN_SAMPLE from displayStruct? I think that help_config()
> should show a value if it is marked as !GUC_DISALLOW_IN_FILE even if
> it is GUC_NOT_IN_SAMPLE.  I'm not sure whether there's any variable
> that are marked that way, though.

As in marked with GUC_NOT_IN_SAMPLE but not GUC_DISALLOW_IN_FILE?
There are quite a lot, developer GUCs being one (say
ignore_invalid_pages).  We don't want to list them in the sample file
so as common users don't play with them, still they make sense if
listed in a file.

If you add a check meaning that GUC_DISALLOW_IN_FILE implies
GUC_NOT_IN_SAMPLE, where one change would need to be applied to
config_file as all the other GUC_DISALLOW_IN_FILE GUCs already do
that, you could remove GUC_DISALLOW_IN_FILE.  However,
GUC_NOT_IN_SAMPLE should be around to not expose options, we don't
want common users to know too much about.

The question about how much people rely on --describe-config these
days is a good one, so perhaps there could be an argument in removing
GUC_NOT_IN_SAMPLE from the set.  TBH, I would be really surprised that
anybody able to use a developer option writes an configuration file in
an incorrect format and needs to use this option, though :)
--
Michael

At Mon, 13 Feb 2023 12:18:07 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> On Mon, Feb 13, 2023 at 11:27:58AM +0900, Kyotaro Horiguchi wrote:
> > I think currently the output by --describe-config can be used only for
> > consulting while editing a (possiblly broken) config file.  Thus I
> > think it's no use showing GIC_DISALLOW_IN_FILE items there unless we
> > use help_config() for an on-session use.
> > 
> > On the other hand, don't we need to remove the condition
> > GUC_NOT_IN_SAMPLE from displayStruct? I think that help_config()
> > should show a value if it is marked as !GUC_DISALLOW_IN_FILE even if
> > it is GUC_NOT_IN_SAMPLE.  I'm not sure whether there's any variable
> > that are marked that way, though.
> 
> As in marked with GUC_NOT_IN_SAMPLE but not GUC_DISALLOW_IN_FILE?
> There are quite a lot, developer GUCs being one (say
> ignore_invalid_pages).  We don't want to list them in the sample file
> so as common users don't play with them, still they make sense if
> listed in a file.

Ah, right.  I think I faintly had them in my mind.

> If you add a check meaning that GUC_DISALLOW_IN_FILE implies
> GUC_NOT_IN_SAMPLE, where one change would need to be applied to
> config_file as all the other GUC_DISALLOW_IN_FILE GUCs already do
> that, you could remove GUC_DISALLOW_IN_FILE.  However,
> GUC_NOT_IN_SAMPLE should be around to not expose options, we don't
> want common users to know too much about.

Okay, I thought that "postgres --help-config" was a sort of developer
option, but your explanation above makes sense.

> The question about how much people rely on --describe-config these
> days is a good one, so perhaps there could be an argument in removing

Yeah, that the reason for my thought it was a developer option...

> GUC_NOT_IN_SAMPLE from the set.  TBH, I would be really surprised that
> anybody able to use a developer option writes an configuration file in
> an incorrect format and needs to use this option, though :)

Hmm.  I didn't directly link GUC_NOT_IN_SAMPLE to being a developer
option. But on second thought, it seems that it is. So, the current
code looks good for me now. Thanks for the explanation.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On 14/02/2023 03:42, Kyotaro Horiguchi wrote:
> At Mon, 13 Feb 2023 12:18:07 +0900, Michael Paquier <michael@paquier.xyz> wrote in
>> On Mon, Feb 13, 2023 at 11:27:58AM +0900, Kyotaro Horiguchi wrote:
>>> I think currently the output by --describe-config can be used only for
>>> consulting while editing a (possiblly broken) config file.  Thus I
>>> think it's no use showing GIC_DISALLOW_IN_FILE items there unless we
>>> use help_config() for an on-session use.
>>>
>>> On the other hand, don't we need to remove the condition
>>> GUC_NOT_IN_SAMPLE from displayStruct? I think that help_config()
>>> should show a value if it is marked as !GUC_DISALLOW_IN_FILE even if
>>> it is GUC_NOT_IN_SAMPLE.  I'm not sure whether there's any variable
>>> that are marked that way, though.
>>
>> As in marked with GUC_NOT_IN_SAMPLE but not GUC_DISALLOW_IN_FILE?
>> There are quite a lot, developer GUCs being one (say
>> ignore_invalid_pages).  We don't want to list them in the sample file
>> so as common users don't play with them, still they make sense if
>> listed in a file.
> 
> Ah, right.  I think I faintly had them in my mind.
> 
>> If you add a check meaning that GUC_DISALLOW_IN_FILE implies
>> GUC_NOT_IN_SAMPLE, where one change would need to be applied to
>> config_file as all the other GUC_DISALLOW_IN_FILE GUCs already do
>> that, you could remove GUC_DISALLOW_IN_FILE.  However,
>> GUC_NOT_IN_SAMPLE should be around to not expose options, we don't
>> want common users to know too much about.
> 
> Okay, I thought that "postgres --help-config" was a sort of developer
> option, but your explanation above makes sense.
> 
>> The question about how much people rely on --describe-config these
>> days is a good one, so perhaps there could be an argument in removing
> 
> Yeah, that the reason for my thought it was a developer option...
> 
>> GUC_NOT_IN_SAMPLE from the set.  TBH, I would be really surprised that
>> anybody able to use a developer option writes an configuration file in
>> an incorrect format and needs to use this option, though :)
> 
> Hmm.  I didn't directly link GUC_NOT_IN_SAMPLE to being a developer
> option. But on second thought, it seems that it is. So, the current
> code looks good for me now. Thanks for the explanation.

The first patch was committed, and there's not much enthusiasm for 
disallowing (GUC_DISALLOW_IN_FILE && !GUC_NOT_IN_SAMPLE), so I am 
marking this as Committed in the commitfest app. Thanks!

- Heikki