On Thu, Jan 27, 2022 at 10:36:21PM -0600, Justin Pryzby wrote:
> Maybe you misunderstood - I'm not reading the file specified by
> current_setting('config_file'). Rather, I'm reading
> tmp_check/data/postgresql.conf, which is copied from the sample conf.
> Do you see an issue with that ?Yes, as of: mv $PGDATA/postgresql.conf $PGDATA/popo.conf pg_ctl start -D $PGDATA -o '-c config_file=popo.conf' make installcheck I have not checked, but I am pretty sure that a couple of distributions out there would pass down a custom path for the configuration file, while removing postgresql.conf from the data directory to avoid any confusion because if one finds out that some parameters are defined but not loaded. Your patch fails on that. > The regression tests are only intended run from a postgres source dir, and if > someone runs the from somewhere else, and they "fail", I think that's because > they violated their assumption, not because of a problem with the test. The tests are able to work out on HEAD, I'd rather not break something that has worked this way for years. Two other aspects that we may want to worry about are include_dir and include if we were to add tests for that, perhaps. This last part is not really a strong requirement IMO, though. > I wondered if it should chomp off anything added by pg_regress --temp-regress. > However that's either going to be a valid guc (or else it would fail some > other > test). Or an extention's guc (which this isn't testing), which has a dot, and > which this regex doesn't match, so doesn't cause false positives. I am not sure about those parts, being reserved about the parts that involve the format of postgresql.conf or any other configuration parts, but we could tackle that after, if necessary. For now, I have down a review of the patch, tweaking the docs, the code and the test to take care of all the inconsistencies I could find. This looks like a good first cut to be able to remove check_guc (the attached removes it, but I think that we'd better treat that independently of the actual feature proposed, for clarity). -- Michael
From a87afcfb5757367aeef795505e9361d5a03facc7 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Sat, 29 Jan 2022 15:32:10 +0900
Subject: [PATCH v2] Expose GUC flags in SQL function, replacing check_guc
Note-to-self: CATVERSION bump!
---
src/include/catalog/pg_proc.dat | 6 +++
src/backend/utils/misc/check_guc | 78 -------------------------------
src/backend/utils/misc/guc.c | 39 ++++++++++++++++
src/test/regress/expected/guc.out | 71 ++++++++++++++++++++++++++++
src/test/regress/sql/guc.sql | 41 ++++++++++++++++
doc/src/sgml/func.sgml | 39 ++++++++++++++++
6 files changed, 196 insertions(+), 78 deletions(-)
delete mode 100755 src/backend/utils/misc/check_guc
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 0859dc81ca..3f927acb01 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -6096,6 +6096,12 @@
proargmodes => '{o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}',
proargnames => '{name,setting,unit,category,short_desc,extra_desc,context,vartype,source,min_val,max_val,enumvals,boot_val,reset_val,sourcefile,sourceline,pending_restart}',
prosrc => 'show_all_settings' },
+
+{ oid => '8921', descr => 'return flags for specified GUC',
+ proname => 'pg_settings_get_flags', provolatile => 's',
+ prorettype => '_text', proargtypes => 'text',
+ prosrc => 'pg_settings_get_flags' },
+
{ oid => '3329', descr => 'show config file settings',
proname => 'pg_show_all_file_settings', prorows => '1000', proretset => 't',
provolatile => 'v', prorettype => 'record', proargtypes => '',
diff --git a/src/backend/utils/misc/check_guc b/src/backend/utils/misc/check_guc
deleted file mode 100755
index b171ef0e4f..0000000000
--- a/src/backend/utils/misc/check_guc
+++ /dev/null
@@ -1,78 +0,0 @@
-#!/bin/sh
-
-## currently, this script makes a lot of assumptions:
-## in postgresql.conf.sample:
-## 1) the valid config settings may be preceded by a '#', but NOT '# '
-## (we use this to skip comments)
-## 2) the valid config settings will be followed immediately by ' ='
-## (at least one space preceding the '=')
-## in guc.c:
-## 3) the options have PGC_ on the same line as the option
-## 4) the options have '{' on the same line as the option
-
-## Problems
-## 1) Don't know what to do with TRANSACTION ISOLATION LEVEL
-
-## if an option is valid but shows up in only one file (guc.c but not
-## postgresql.conf.sample), it should be listed here so that it
-## can be ignored
-INTENTIONALLY_NOT_INCLUDED="debug_deadlocks in_hot_standby \
-is_superuser lc_collate lc_ctype lc_messages lc_monetary lc_numeric lc_time \
-pre_auth_delay role seed server_encoding server_version server_version_num \
-session_authorization trace_lock_oidmin trace_lock_table trace_locks trace_lwlocks \
-trace_notify trace_userlocks transaction_isolation transaction_read_only \
-zero_damaged_pages"
-
-### What options are listed in postgresql.conf.sample, but don't appear
-### in guc.c?
-
-# grab everything that looks like a setting and convert it to lower case
-SETTINGS=`grep ' =' postgresql.conf.sample |
-grep -v '^# ' | # strip comments
-sed -e 's/^#//' |
-awk '{print $1}'`
-
-SETTINGS=`echo "$SETTINGS" | tr 'A-Z' 'a-z'`
-
-for i in $SETTINGS ; do
- hidden=0
- ## it sure would be nice to replace this with an sql "not in" statement
- ## it doesn't seem to make sense to have things in .sample and not in guc.c
-# for hidethis in $INTENTIONALLY_NOT_INCLUDED ; do
-# if [ "$hidethis" = "$i" ] ; then
-# hidden=1
-# fi
-# done
- if [ "$hidden" -eq 0 ] ; then
- grep -i '"'$i'"' guc.c > /dev/null
- if [ $? -ne 0 ] ; then
- echo "$i seems to be missing from guc.c";
- fi;
- fi
-done
-
-### What options are listed in guc.c, but don't appear
-### in postgresql.conf.sample?
-
-# grab everything that looks like a setting and convert it to lower case
-
-SETTINGS=`grep '{.* PGC_' guc.c | awk '{print $1}' | \
- sed -e 's/{//g' -e 's/"//g' -e 's/,//'`
-
-SETTINGS=`echo "$SETTINGS" | tr 'A-Z' 'a-z'`
-
-for i in $SETTINGS ; do
- hidden=0
- ## it sure would be nice to replace this with an sql "not in" statement
- for hidethis in $INTENTIONALLY_NOT_INCLUDED ; do
- if [ "$hidethis" = "$i" ] ; then
- hidden=1
- fi
- done
- if [ "$hidden" -eq 0 ] ; then
- grep -i '#'$i' ' postgresql.conf.sample > /dev/null
- if [ $? -ne 0 ] ; then
- echo "$i seems to be missing from postgresql.conf.sample";
- fi
- fi
-done
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 4c94f09c64..b3fd42e0f1 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -9634,6 +9634,45 @@ GetConfigOptionByName(const char *name, const char **varname, bool missing_ok)
return _ShowOption(record, true);
}
+/*
+ * Return some of the flags associated to the specified GUC in the shape of
+ * a text array, and NULL if it does not exist. An empty array is returned
+ * if the GUC exists without any meaningful flags to show.
+ */
+Datum
+pg_settings_get_flags(PG_FUNCTION_ARGS)
+{
+#define MAX_GUC_FLAGS 5
+ char *varname = TextDatumGetCString(PG_GETARG_DATUM(0));
+ struct config_generic *record;
+ int cnt = 0;
+ Datum flags[MAX_GUC_FLAGS];
+ ArrayType *a;
+
+ record = find_option(varname, false, true, ERROR);
+
+ /* return NULL if no such variable */
+ if (record == NULL)
+ PG_RETURN_NULL();
+
+ if (record->flags & GUC_EXPLAIN)
+ flags[cnt++] = CStringGetTextDatum("EXPLAIN");
+ if (record->flags & GUC_NO_RESET_ALL)
+ flags[cnt++] = CStringGetTextDatum("NO_RESET_ALL");
+ if (record->flags & GUC_NO_SHOW_ALL)
+ flags[cnt++] = CStringGetTextDatum("NO_SHOW_ALL");
+ if (record->flags & GUC_NOT_IN_SAMPLE)
+ flags[cnt++] = CStringGetTextDatum("NOT_IN_SAMPLE");
+ if (record->flags & GUC_RUNTIME_COMPUTED)
+ flags[cnt++] = CStringGetTextDatum("RUNTIME_COMPUTED");
+
+ Assert(cnt <= MAX_GUC_FLAGS);
+
+ /* Returns the record as Datum */
+ a = construct_array(flags, cnt, TEXTOID, -1, false, TYPALIGN_INT);
+ PG_RETURN_ARRAYTYPE_P(a);
+}
+
/*
* Return GUC variable value by variable number; optionally return canonical
* form of name. Return value is palloc'd.
diff --git a/src/test/regress/expected/guc.out b/src/test/regress/expected/guc.out
index 59da91ff04..5fe53c82e3 100644
--- a/src/test/regress/expected/guc.out
+++ b/src/test/regress/expected/guc.out
@@ -813,3 +813,74 @@ set default_with_oids to f;
-- Should not allow to set it to true.
set default_with_oids to t;
ERROR: tables declared WITH OIDS are not supported
+-- Test GUC categories and flag patterns
+CREATE TABLE pg_settings_flags AS SELECT name, category,
+ 'EXPLAIN' = ANY(flags) AS explain,
+ '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,
+ pg_settings_get_flags(psas.name) AS flags;
+-- Developer GUCs should be flagged with GUC_NOT_IN_SAMPLE:
+SELECT name FROM pg_settings_flags
+ WHERE category = 'Developer Options' AND NOT not_in_sample
+ ORDER BY 1;
+ name
+------
+(0 rows)
+
+-- Most query-tuning GUCs are flagged as valid for EXPLAIN.
+-- default_statistics_target is an exception.
+SELECT name FROM pg_settings_flags
+ WHERE category ~ '^Query Tuning' AND NOT explain
+ ORDER BY 1;
+ name
+---------------------------
+ default_statistics_target
+(1 row)
+
+-- Runtime-computed GUCs should be part of the preset category.
+SELECT name FROM pg_settings_flags
+ WHERE NOT category = 'Preset Options' AND runtime_computed
+ ORDER BY 1;
+ name
+------
+(0 rows)
+
+-- Preset GUCs are flagged as NOT_IN_SAMPLE.
+SELECT name FROM pg_settings_flags
+ WHERE category = 'Preset Options' AND NOT not_in_sample
+ ORDER BY 1;
+ name
+------
+(0 rows)
+
+-- NO_SHOW_ALL implies NO_RESET_ALL, and vice-versa.
+SELECT name FROM pg_settings_flags
+ WHERE no_show_all AND NOT no_reset_all
+ ORDER BY 1;
+ name
+------
+(0 rows)
+
+-- Three exceptions as of transaction_*
+SELECT name FROM pg_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 pg_settings_flags
+ WHERE no_show_all AND NOT not_in_sample
+ ORDER BY 1;
+ name
+------
+(0 rows)
+
+DROP TABLE pg_settings_flags;
diff --git a/src/test/regress/sql/guc.sql b/src/test/regress/sql/guc.sql
index c39c11388d..7a78b8f3be 100644
--- a/src/test/regress/sql/guc.sql
+++ b/src/test/regress/sql/guc.sql
@@ -311,3 +311,44 @@ reset check_function_bodies;
set default_with_oids to f;
-- Should not allow to set it to true.
set default_with_oids to t;
+
+-- Test GUC categories and flag patterns
+CREATE TABLE pg_settings_flags AS SELECT name, category,
+ 'EXPLAIN' = ANY(flags) AS explain,
+ '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,
+ pg_settings_get_flags(psas.name) AS flags;
+
+-- Developer GUCs should be flagged with GUC_NOT_IN_SAMPLE:
+SELECT name FROM pg_settings_flags
+ WHERE category = 'Developer Options' AND NOT not_in_sample
+ ORDER BY 1;
+-- Most query-tuning GUCs are flagged as valid for EXPLAIN.
+-- default_statistics_target is an exception.
+SELECT name FROM pg_settings_flags
+ WHERE category ~ '^Query Tuning' AND NOT explain
+ ORDER BY 1;
+-- Runtime-computed GUCs should be part of the preset category.
+SELECT name FROM pg_settings_flags
+ WHERE NOT category = 'Preset Options' AND runtime_computed
+ ORDER BY 1;
+-- Preset GUCs are flagged as NOT_IN_SAMPLE.
+SELECT name FROM pg_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 pg_settings_flags
+ WHERE no_show_all AND NOT no_reset_all
+ ORDER BY 1;
+-- Three exceptions as of transaction_*
+SELECT name FROM pg_settings_flags
+ WHERE NOT no_show_all AND no_reset_all
+ ORDER BY 1;
+-- NO_SHOW_ALL implies NOT_IN_SAMPLE:
+SELECT name FROM pg_settings_flags
+ WHERE no_show_all AND NOT not_in_sample
+ ORDER BY 1;
+DROP TABLE pg_settings_flags;
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 0ee6974f1c..0cf1ffbcb0 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -23596,6 +23596,45 @@ SELECT pg_type_is_visible('myschema.widget'::regtype);
</para></entry>
</row>
+ <row>
+ <entry role="func_table_entry"><para role="func_signature">
+ <indexterm>
+ <primary>pg_get_get_flags</primary>
+ </indexterm>
+ <function>pg_get_get_flags</function> ( <parameter>guc</parameter> <type>text</type> )
+ <returnvalue>text[]</returnvalue>
+ </para>
+ <para>
+ Returns an array of the flags associated with the given GUC, or
+ <literal>NULL</literal> if the does not exist. The result is
+ an empty array if the GUC exists but there are no flags to show,
+ as supported by the list below.
+ The following flags are exposed (the most meaningful ones are
+ included):
+ <simplelist>
+ <member>
+ <literal>EXPLAIN</literal>, parameters included in
+ <command>EXPLAIN</command> commands.
+ </member>
+ <member>
+ <literal>NO_SHOW_ALL</literal>, parameters excluded from
+ <command>SHOW ALL</command> commands.
+ </member>
+ <member>
+ <literal>NO_RESET_ALL</literal>, parameters excluded from
+ <command>RESET ALL</command> commands.
+ </member>
+ <member>
+ <literal>NOT_IN_SAMPLE</literal>, parameters not included in
+ <filename>postgresql.conf</filename> by default.
+ </member>
+ <member>
+ <literal>RUNTIME_COMPUTED</literal>, runtime-computed parameters.
+ </member>
+ </simplelist>
+ </para></entry>
+ </row>
+
<row>
<entry role="func_table_entry"><para role="func_signature">
<indexterm>
--
2.34.1
signature.asc
Description: PGP signature
