> 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 <mich...@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