Thread

Re: Fix GUC_NO_SHOW_ALL test scenario in 003_check_guc.pl

Nitin Jadhav <nitinjadhavpostgres@gmail.com> — 2023-01-29T11:52:13Z

> 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