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
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 978b385568..93da5d24ba 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -1383,11 +1383,14 @@ check_GUC_name_for_parameter_acl(const char *name)
}
/*
- * Routine in charge of checking that the initial value of a GUC is the
- * same when declared and when loaded to prevent anybody looking at the
- * C declarations of these GUCS from being fooled by mismatched values.
+ * Routine in charge of checking various states of a GUC.
*
- * The following validation rules apply:
+ * This performs two sanity checks. First, it checks that the initial
+ * value of a GUC is the same when declared and when loaded to prevent
+ * anybody looking at the C declarations of these GUCS from being fooled by
+ * mismatched values. Second, it checks for incorrect flag combinations.
+ *
+ * The following validation rules apply for the values:
* bool - can be false, otherwise must be same as the boot_val
* int - can be 0, otherwise must be same as the boot_val
* real - can be 0.0, otherwise must be same as the boot_val
@@ -1398,6 +1401,7 @@ check_GUC_name_for_parameter_acl(const char *name)
static bool
check_GUC_init(struct config_generic *gconf)
{
+ /* Checks on values */
switch (gconf->vartype)
{
case PGC_BOOL:
@@ -1462,6 +1466,16 @@ check_GUC_init(struct config_generic *gconf)
}
}
+ /* Flag combinations */
+ /* GUC_NO_SHOW_ALL requires GUC_NOT_IN_SAMPLE */
+ if ((gconf->flags & GUC_NO_SHOW_ALL) &&
+ !(gconf->flags & GUC_NOT_IN_SAMPLE))
+ {
+ elog(LOG, "GUC %s flags: NO_SHOW_ALL and !NOT_IN_SAMPLE",
+ gconf->name);
+ return false;
+ }
+
return true;
}
#endif
diff --git a/src/test/regress/expected/guc.out b/src/test/regress/expected/guc.out
index 2914b950b4..3185ec0063 100644
--- a/src/test/regress/expected/guc.out
+++ b/src/test/regress/expected/guc.out
@@ -880,15 +880,7 @@ SELECT name FROM tab_settings_flags
------
(0 rows)
--- NO_SHOW_ALL implies NO_RESET_ALL, and vice-versa.
-SELECT name FROM tab_settings_flags
- WHERE no_show_all AND NOT no_reset_all
- ORDER BY 1;
- name
-------
-(0 rows)
-
--- Exceptions are transaction_*.
+-- NO_RESET_ALL can be specified without NO_SHOW_ALL, like transaction_*.
SELECT name FROM tab_settings_flags
WHERE NOT no_show_all AND no_reset_all
ORDER BY 1;
@@ -899,14 +891,6 @@ SELECT name FROM tab_settings_flags
transaction_read_only
(3 rows)
--- NO_SHOW_ALL implies NOT_IN_SAMPLE.
-SELECT name FROM tab_settings_flags
- WHERE no_show_all AND NOT not_in_sample
- ORDER BY 1;
- name
-------
-(0 rows)
-
-- NO_RESET implies NO_RESET_ALL.
SELECT name FROM tab_settings_flags
WHERE no_reset AND NOT no_reset_all
diff --git a/src/test/regress/sql/guc.sql b/src/test/regress/sql/guc.sql
index d40d0c178f..f672b88b54 100644
--- a/src/test/regress/sql/guc.sql
+++ b/src/test/regress/sql/guc.sql
@@ -349,18 +349,10 @@ SELECT name FROM tab_settings_flags
SELECT name FROM tab_settings_flags
WHERE category = 'Preset Options' AND NOT not_in_sample
ORDER BY 1;
--- NO_SHOW_ALL implies NO_RESET_ALL, and vice-versa.
-SELECT name FROM tab_settings_flags
- WHERE no_show_all AND NOT no_reset_all
- ORDER BY 1;
--- Exceptions are transaction_*.
+-- NO_RESET_ALL can be specified without NO_SHOW_ALL, like transaction_*.
SELECT name FROM tab_settings_flags
WHERE NOT no_show_all AND no_reset_all
ORDER BY 1;
--- NO_SHOW_ALL implies NOT_IN_SAMPLE.
-SELECT name FROM tab_settings_flags
- WHERE no_show_all AND NOT not_in_sample
- ORDER BY 1;
-- NO_RESET implies NO_RESET_ALL.
SELECT name FROM tab_settings_flags
WHERE no_reset AND NOT no_reset_all
signature.asc
Description: PGP signature
