Re: Fix GUC_NO_SHOW_ALL test scenario in 003_check_guc.pl - Mailing list pgsql-hackers

> 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