Thread

Re: Fix GUC_NO_SHOW_ALL test scenario in 003_check_guc.pl

Nitin Jadhav <nitinjadhavpostgres@gmail.com> — 2023-01-30T11:42:27Z

> 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