On Sun, Feb 05, 2023 at 12:56:58AM +0530, Nitin Jadhav wrote:
> Ok. Understood the other problems. I have attached the v2 patch which
> uses the idea present in Michael's patch. In addition, I have removed
> fetching NO_SHOW_ALL parameters while creating tab_settings_flags
> table in guc.sql and adjusted the test which checks for (NO_RESET_ALL
> AND NOT NO_SHOW_ALL) as this was misleading the developer who thinks
> that tab_settings_flags table has NO_SHOW_ALL parameters which is
> incorrect.
Okay, the part to add an initialization check for GUC_NO_SHOW_ALL and
GUC_NOT_IN_SAMPLE looked fine by me, so applied after more comment
polishing.
+-- NO_RESET_ALL can be specified without NO_SHOW_ALL, like transaction_*.
+-- tab_settings_flags does not contain NO_SHOW_ALL flags. Just checking for
+-- NO_RESET_ALL implies NO_RESET_ALL AND NOT NO_SHOW_ALL.
SELECT name FROM tab_settings_flags
- WHERE NOT no_show_all AND no_reset_all
+ WHERE no_reset_all
ORDER BY 1;
Removing entirely no_show_all is fine by me, but keeping this SQL has
little sense, then, because it would include any GUCs loaded by an
external source when they define NO_RESET_ALL. I think that 0001
should be like the attached, instead, backpatched down to 15 (release
week, so it cannot be touched until the next version is stamped),
where we just remove all the checks based on no_show_all.
On top of that, I have noticed an extra combination that would not
make sense and that could be checked with the SQL queries:
GUC_DISALLOW_IN_FILE implies GUC_NOT_IN_SAMPLE. The opposite may not
be true, though, as some developer GUCs are marked as
GUC_NOT_IN_SAMPLE but they are allowed in a file. The only exception
to that currently is config_file. It is just a special case whose
value is enforced at startup and it can be passed down as an option
switch via the postgres binary, still it seems like it would be better
to also mark it as GUC_NOT_IN_SAMPLE? This is done in 0002, only for
HEAD, as that would be a new check.
Thoughts?
--
Michael