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
>From 727361c5fe4c9af16ef66c1090a9e376dfc891d2 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Mon, 6 Feb 2023 16:05:11 +0900
Subject: [PATCH v3 1/2] Clean up SQL tests for NO_SHOW_ALL
---
src/test/regress/expected/guc.out | 28 ----------------------------
src/test/regress/sql/guc.sql | 13 -------------
2 files changed, 41 deletions(-)
diff --git a/src/test/regress/expected/guc.out b/src/test/regress/expected/guc.out
index 2914b950b4..127c953297 100644
--- a/src/test/regress/expected/guc.out
+++ b/src/test/regress/expected/guc.out
@@ -841,7 +841,6 @@ CREATE TABLE tab_settings_flags AS SELECT name, category,
'EXPLAIN' = ANY(flags) AS explain,
'NO_RESET' = ANY(flags) AS no_reset,
'NO_RESET_ALL' = ANY(flags) AS no_reset_all,
- 'NO_SHOW_ALL' = ANY(flags) AS no_show_all,
'NOT_IN_SAMPLE' = ANY(flags) AS not_in_sample,
'RUNTIME_COMPUTED' = ANY(flags) AS runtime_computed
FROM pg_show_all_settings() AS psas,
@@ -880,33 +879,6 @@ 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_*.
-SELECT name FROM tab_settings_flags
- WHERE NOT no_show_all AND no_reset_all
- ORDER BY 1;
- name
-------------------------
- transaction_deferrable
- transaction_isolation
- 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..dc79761955 100644
--- a/src/test/regress/sql/guc.sql
+++ b/src/test/regress/sql/guc.sql
@@ -326,7 +326,6 @@ CREATE TABLE tab_settings_flags AS SELECT name, category,
'EXPLAIN' = ANY(flags) AS explain,
'NO_RESET' = ANY(flags) AS no_reset,
'NO_RESET_ALL' = ANY(flags) AS no_reset_all,
- 'NO_SHOW_ALL' = ANY(flags) AS no_show_all,
'NOT_IN_SAMPLE' = ANY(flags) AS not_in_sample,
'RUNTIME_COMPUTED' = ANY(flags) AS runtime_computed
FROM pg_show_all_settings() AS psas,
@@ -349,18 +348,6 @@ 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_*.
-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
--
2.39.1
>From ab73606a127fca6199baef6e92a6c621a49bf070 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Mon, 6 Feb 2023 16:17:08 +0900
Subject: [PATCH v3 2/2] Add new GUC test checking that DISALLOW_IN_FILE =>
NOT_IN_SAMPLE
---
src/backend/utils/misc/guc_tables.c | 2 +-
src/test/regress/expected/guc.out | 9 +++++++++
src/test/regress/sql/guc.sql | 5 +++++
3 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index b46e3b8c55..fcc0aacfac 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -4291,7 +4291,7 @@ struct config_string ConfigureNamesString[] =
{"config_file", PGC_POSTMASTER, FILE_LOCATIONS,
gettext_noop("Sets the server's main configuration file."),
NULL,
- GUC_DISALLOW_IN_FILE | GUC_SUPERUSER_ONLY
+ GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE | GUC_SUPERUSER_ONLY
},
&ConfigFileName,
NULL,
diff --git a/src/test/regress/expected/guc.out b/src/test/regress/expected/guc.out
index 127c953297..c7170d0446 100644
--- a/src/test/regress/expected/guc.out
+++ b/src/test/regress/expected/guc.out
@@ -838,6 +838,7 @@ SELECT pg_settings_get_flags('does_not_exist');
(1 row)
CREATE TABLE tab_settings_flags AS SELECT name, category,
+ 'DISALLOW_IN_FILE' = ANY(flags) AS disallow_in_file,
'EXPLAIN' = ANY(flags) AS explain,
'NO_RESET' = ANY(flags) AS no_reset,
'NO_RESET_ALL' = ANY(flags) AS no_reset_all,
@@ -887,4 +888,12 @@ SELECT name FROM tab_settings_flags
------
(0 rows)
+-- DISALLOW_IN_FILE implies NOT_IN_SAMPLE.
+SELECT name FROM tab_settings_flags
+ WHERE disallow_in_file AND NOT not_in_sample
+ ORDER BY 1;
+ name
+------
+(0 rows)
+
DROP TABLE tab_settings_flags;
diff --git a/src/test/regress/sql/guc.sql b/src/test/regress/sql/guc.sql
index dc79761955..37982008b0 100644
--- a/src/test/regress/sql/guc.sql
+++ b/src/test/regress/sql/guc.sql
@@ -323,6 +323,7 @@ set default_with_oids to t;
SELECT pg_settings_get_flags(NULL);
SELECT pg_settings_get_flags('does_not_exist');
CREATE TABLE tab_settings_flags AS SELECT name, category,
+ 'DISALLOW_IN_FILE' = ANY(flags) AS disallow_in_file,
'EXPLAIN' = ANY(flags) AS explain,
'NO_RESET' = ANY(flags) AS no_reset,
'NO_RESET_ALL' = ANY(flags) AS no_reset_all,
@@ -352,4 +353,8 @@ SELECT name FROM tab_settings_flags
SELECT name FROM tab_settings_flags
WHERE no_reset AND NOT no_reset_all
ORDER BY 1;
+-- DISALLOW_IN_FILE implies NOT_IN_SAMPLE.
+SELECT name FROM tab_settings_flags
+ WHERE disallow_in_file AND NOT not_in_sample
+ ORDER BY 1;
DROP TABLE tab_settings_flags;
--
2.39.1
