Re: Fix GUC_NO_SHOW_ALL test scenario in 003_check_guc.pl

> > 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(at)paquier(dot)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