Feedback-ID: i1ff147bf:Fastmail
From: =?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org>
To: Shinya Kato <shinya11.kato@gmail.com>
Cc: pgsql-hackers@postgresql.org
Subject: Re: Improve tab completion for various SET/RESET forms
References: <87bjqwwtic.fsf@wibble.ilmari.org>
	<18691637-2af2-4a2f-9f12-8201c2a88206@eisentraut.org>
	<871prqwq19.fsf@wibble.ilmari.org> <87tt4lumqz.fsf@wibble.ilmari.org>
	<87seicgu1q.fsf@wibble.ilmari.org>
	<CAOzEurSWaiE7pHhATgxMn2ok5PyQd_C+m=3TFUUmkdbTXRZLMw@mail.gmail.com>
	<87a549h196.fsf@wibble.ilmari.org> <874iuegk7h.fsf@wibble.ilmari.org>
	<CAOzEurTpB+7jBudvmj9PYfdBpDnLJjGtg4C1_3dmrkjjdq=d3Q@mail.gmail.com>
	<CAOzEurS76=cAtSx+WAdMcYqn+D9M9dNOY3jXjNodB6R+R5a9PQ@mail.gmail.com>
	<CAOzEurTXFei36Jo7UwPLc31JHFgX+Xo-c3svc2VGQsOJj2dOCg@mail.gmail.com>
Date: Tue, 11 Nov 2025 20:27:20 +0000
In-Reply-To: 
 <CAOzEurTXFei36Jo7UwPLc31JHFgX+Xo-c3svc2VGQsOJj2dOCg@mail.gmail.com>
	(Shinya Kato's message of "Wed, 13 Aug 2025 18:53:59 +0900")
Message-ID: <871pm48gd3.fsf@wibble.ilmari.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux)
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="=-=-="
Archived-At: 
 <https://www.postgresql.org/message-id/871pm48gd3.fsf%40wibble.ilmari.org>
Precedence: bulk

--=-=-=
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit

Shinya Kato <shinya11.kato@gmail.com> writes:

> On Wed, Aug 13, 2025 at 5:52 PM Shinya Kato <shinya11.kato@gmail.com> wrote:
>> > > I also noticed that ALTER SYSTEM RESET tab completes with all variables,
>> > > not just ones that have been set with ALTER SYSTEM.  Getting this right
>> > > is a bit more complicated, since the only way to distinguish them is
>> > > pg_settings.sourcefile = '$PGDATA/postgresql.auto.conf', but Windows
>> > > uses \ for the directory separator, so we'd have to use a regex.  But
>> > > there's no function for escaping a string to use as a literal match in a
>> > > regex (like Perl's quotemeta()), so we have to use LIKE instead,
>> > > manually escaping %, _ and \, and accepting any character as the
>> > > directory separator.  If people think this over-complicated, we could
>> > > just check sourcefile ~ '[\\/]postgresql\.auto\.conf$', and accept false
>> > > positives if somone has used an include directive with a file with the
>> > > same name in a different directory.
>> > >
>> > > Another complication is that both current_setting('data_directory') and
>> > > pg_settings.sourcefile are only available to superusers, so I added
>> > > another version for non-superusers that completes variables they've been
>> > > granted ALTER SYSTEM on, by querying pg_parameter_acl.
[...]
> While reviewing these patches, I noticed that tab completion for
> parameter names is missing for SET LOCAL and SET SESSION. Would it be
> possible to fix this at the same time?

SESSION is the default for SET <parameter>, and SET SESSION already
completes with AUTHORIZATION and CHARACTERISTICS AS TRANSACTION.  I
don't think it would be useful to drown those out with configuration
parmeters as well.

Adding tab completion for variables and keywords after SET LOCAL would
be easy, but adding support for value completion (e.g. for enums and
timezones) would be harder, since the gen_tabcomplete.pl script doesn't
support condtions on the form ((Matches(...) || Matches(...)) && ...),
which would be necessary in order to add a TailMatches("SET",
"SESSION|LOCAL", MatchAny, "TO|=") alternative to this:

	else if (TailMatches("SET", MatchAny, "TO|=") &&
		 !TailMatches("UPDATE", MatchAny, "SET", MatchAny, "TO|="))

So I agree with Kirill that that's should be a separate patch.  This
series is already much longer than I intended to make it when I started.

> Regarding the pg_parameter_acl catalog, which was introduced in
> PostgreSQL 15, are there any backward-compatibility concerns? I don't
> think that tab completion for ALTER SYSTEM is a high priority, as the
> implementation would likely be overly complex.

I've made the query version-specific now, so it only gets run on 15 and
newer.  I'm not sure whaty you mean by "would likely be overly complex".
You can see the pg_parameter_acl query for non-superusers in patch 5,
and it isn't particularly complex.

However, the sourcefile condition for superusers in patch 4 is quite
hairy.  That could be made much simpler if we accept false positives for
prameters that are set in files named 'postgresql.auto.conf' included
from directories besides the data directory.  That wuold let us change

" WHERE sourcefile LIKE pg_catalog.regexp_replace( " \
"	        pg_catalog.current_setting('data_directory'), " \
"           '[_%%\\\\]', '\\\\\\&') || '_postgresql.auto.conf' " \

to

" WHERE sourcefile ~ '[\\\\/]postgresql\\.auto\\.conf$' " \

On further thought condition is so much nicer that I think it's worth
the extremely unlikey false positives, so I've changed it to that in v6.

Please find attached an updated patch series.

- ilmari


--=-=-=
Content-Type: text/x-diff
Content-Disposition: inline;
 filename=v6-0001-Add-tab-completion-for-ALTER-TABLE-.-ALTER-COLUMN.patch

From 30a69b810ad9d764ea1a11e3449d06061f361a14 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Mon, 9 Jun 2025 20:39:15 +0100
Subject: [PATCH v6 1/5] Add tab completion for ALTER TABLE ... ALTER COLUMN
 ... RESET

Unlike SET, it only takes parenthesised attribute options.
---
 src/bin/psql/tab-complete.in.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/src/bin/psql/tab-complete.in.c b/src/bin/psql/tab-complete.in.c
index 316a2dafbf1..879b8fdeb96 100644
--- a/src/bin/psql/tab-complete.in.c
+++ b/src/bin/psql/tab-complete.in.c
@@ -2936,9 +2936,13 @@ match_previous_words(int pattern_id,
 					  "STATISTICS", "STORAGE",
 		/* a subset of ALTER SEQUENCE options */
 					  "INCREMENT", "MINVALUE", "MAXVALUE", "START", "NO", "CACHE", "CYCLE");
-	/* ALTER TABLE ALTER [COLUMN] <foo> SET ( */
-	else if (Matches("ALTER", "TABLE", MatchAny, "ALTER", "COLUMN", MatchAny, "SET", "(") ||
-			 Matches("ALTER", "TABLE", MatchAny, "ALTER", MatchAny, "SET", "("))
+	/* ALTER TABLE ALTER [COLUMN] <foo> RESET */
+	else if (Matches("ALTER", "TABLE", MatchAny, "ALTER", "COLUMN", MatchAny, "RESET") ||
+			 Matches("ALTER", "TABLE", MatchAny, "ALTER", MatchAny, "RESET"))
+		COMPLETE_WITH("(");
+	/* ALTER TABLE ALTER [COLUMN] <foo> SET|RESET ( */
+	else if (Matches("ALTER", "TABLE", MatchAny, "ALTER", "COLUMN", MatchAny, "SET|RESET", "(") ||
+			 Matches("ALTER", "TABLE", MatchAny, "ALTER", MatchAny, "SET|RESET", "("))
 		COMPLETE_WITH("n_distinct", "n_distinct_inherited");
 	/* ALTER TABLE ALTER [COLUMN] <foo> SET COMPRESSION */
 	else if (Matches("ALTER", "TABLE", MatchAny, "ALTER", "COLUMN", MatchAny, "SET", "COMPRESSION") ||
-- 
2.51.2


--=-=-=
Content-Type: text/x-diff
Content-Disposition: inline;
 filename=v6-0002-Add-tab-completion-for-ALTER-FOREIGN-TABLE-.-SET.patch

From 58006b769a0a13f736ff4f3fe513323dbbd96e5a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Mon, 9 Jun 2025 20:41:29 +0100
Subject: [PATCH v6 2/5] Add tab completion for ALTER FOREIGN TABLE ... SET

The schema is the only thing that can be set for foreign tables.
---
 src/bin/psql/tab-complete.in.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/bin/psql/tab-complete.in.c b/src/bin/psql/tab-complete.in.c
index 879b8fdeb96..2343c87c49a 100644
--- a/src/bin/psql/tab-complete.in.c
+++ b/src/bin/psql/tab-complete.in.c
@@ -2458,6 +2458,10 @@ match_previous_words(int pattern_id,
 		COMPLETE_WITH("ADD", "ALTER", "DISABLE TRIGGER", "DROP", "ENABLE",
 					  "INHERIT", "NO INHERIT", "OPTIONS", "OWNER TO",
 					  "RENAME", "SET", "VALIDATE CONSTRAINT");
+	else if (Matches("ALTER", "FOREIGN", "TABLE", MatchAny, "SET"))
+		COMPLETE_WITH("SCHEMA");
+	else if (Matches("ALTER", "FOREIGN", "TABLE", MatchAny, "SET", "SCHEMA"))
+		COMPLETE_WITH_QUERY(Query_for_list_of_schemas);
 
 	/* ALTER INDEX */
 	else if (Matches("ALTER", "INDEX"))
-- 
2.51.2


--=-=-=
Content-Type: text/x-diff
Content-Disposition: inline;
 filename=v6-0003-Improve-tab-completion-for-RESET.patch

From a52112f1919fc2ccebb7cc2586cdfd6c7943fa66 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Fri, 8 Aug 2025 23:06:36 +0100
Subject: [PATCH v6 3/5] Improve tab completion for RESET

Only complete variables that have been set in the current session,
plus the keywords ALL, ROLE and SESSION (which will subsequently be
completed with AUTHORIZATION).

Because the TailMatches() is now only for "SET" we can also get rid of
the guard against ALTER DATABASE|USER|ROLE ... RESET and keywords that
only apply to RESET, as well as TABLESPACE, which never should have
been offered.
---
 src/bin/psql/tab-complete.in.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/src/bin/psql/tab-complete.in.c b/src/bin/psql/tab-complete.in.c
index 2343c87c49a..3afc3a24231 100644
--- a/src/bin/psql/tab-complete.in.c
+++ b/src/bin/psql/tab-complete.in.c
@@ -1067,6 +1067,11 @@ static const SchemaQuery Query_for_trigger_of_table = {
 " WHERE context IN ('user', 'superuser') "\
 "   AND pg_catalog.lower(name) LIKE pg_catalog.lower('%s')"
 
+#define Query_for_list_of_session_vars \
+"SELECT pg_catalog.lower(name) FROM pg_catalog.pg_settings "\
+" WHERE source = 'session' "\
+"   AND pg_catalog.lower(name) LIKE pg_catalog.lower('%s')"
+
 #define Query_for_list_of_show_vars \
 "SELECT pg_catalog.lower(name) FROM pg_catalog.pg_settings "\
 " WHERE pg_catalog.lower(name) LIKE pg_catalog.lower('%s')"
@@ -5068,16 +5073,19 @@ match_previous_words(int pattern_id,
 
 /* SET, RESET, SHOW */
 	/* Complete with a variable name */
-	else if (TailMatches("SET|RESET") &&
-			 !TailMatches("UPDATE", MatchAny, "SET") &&
-			 !TailMatches("ALTER", "DATABASE|USER|ROLE", MatchAny, "RESET"))
+	else if (TailMatches("SET") &&
+			 !TailMatches("UPDATE", MatchAny, "SET"))
 		COMPLETE_WITH_QUERY_VERBATIM_PLUS(Query_for_list_of_set_vars,
 										  "CONSTRAINTS",
 										  "TRANSACTION",
 										  "SESSION",
+										  "ROLE");
+	/* Complete with variables set in the current session */
+	else if (Matches("RESET"))
+		COMPLETE_WITH_QUERY_VERBATIM_PLUS(Query_for_list_of_session_vars,
+										  "ALL",
 										  "ROLE",
-										  "TABLESPACE",
-										  "ALL");
+										  "SESSION");
 	else if (Matches("SHOW"))
 		COMPLETE_WITH_QUERY_VERBATIM_PLUS(Query_for_list_of_show_vars,
 										  "SESSION AUTHORIZATION",
-- 
2.51.2


--=-=-=
Content-Type: text/x-diff
Content-Disposition: inline;
 filename=v6-0004-Improve-tab-completion-for-ALTER-SYSTEM-RESET.patch

From 69048f486efb6157457cd926d564be5a6800f9e5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Fri, 8 Aug 2025 23:07:39 +0100
Subject: [PATCH v6 4/5] Improve tab completion for ALTER SYSTEM RESET

Only complete variables where sourcefile is `postgresql.conf.auto` in
the data directory.
---
 src/bin/psql/tab-complete.in.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/bin/psql/tab-complete.in.c b/src/bin/psql/tab-complete.in.c
index 3afc3a24231..156fef50085 100644
--- a/src/bin/psql/tab-complete.in.c
+++ b/src/bin/psql/tab-complete.in.c
@@ -1062,6 +1062,11 @@ static const SchemaQuery Query_for_trigger_of_table = {
 " WHERE context != 'internal' "\
 "   AND pg_catalog.lower(name) LIKE pg_catalog.lower('%s')"
 
+#define Query_for_list_of_alter_system_reset_vars \
+"SELECT pg_catalog.lower(name) FROM pg_catalog.pg_settings " \
+" WHERE sourcefile ~ '[\\\\/]postgresql\\.auto\\.conf$' " \
+"   AND pg_catalog.lower(name) LIKE pg_catalog.lower('%s')"
+
 #define Query_for_list_of_set_vars \
 "SELECT pg_catalog.lower(name) FROM pg_catalog.pg_settings "\
 " WHERE context IN ('user', 'superuser') "\
@@ -2647,8 +2652,10 @@ match_previous_words(int pattern_id,
 	/* ALTER SYSTEM SET, RESET, RESET ALL */
 	else if (Matches("ALTER", "SYSTEM"))
 		COMPLETE_WITH("SET", "RESET");
-	else if (Matches("ALTER", "SYSTEM", "SET|RESET"))
-		COMPLETE_WITH_QUERY_VERBATIM_PLUS(Query_for_list_of_alter_system_set_vars,
+	else if (Matches("ALTER", "SYSTEM", "SET"))
+		COMPLETE_WITH_QUERY_VERBATIM(Query_for_list_of_alter_system_set_vars);
+	else if (Matches("ALTER", "SYSTEM", "RESET"))
+		COMPLETE_WITH_QUERY_VERBATIM_PLUS(Query_for_list_of_alter_system_reset_vars,
 										  "ALL");
 	else if (Matches("ALTER", "SYSTEM", "SET", MatchAny))
 		COMPLETE_WITH("TO");
-- 
2.51.2


--=-=-=
Content-Type: text/x-diff
Content-Disposition: inline;
 filename=v6-0005-Improve-tab-completion-for-ALTER-SYSTEM-SET-RESET.patch

From c6f7f87019bf3b95c0139a71f357a78981e23516 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Sat, 9 Aug 2025 00:29:39 +0100
Subject: [PATCH v6 5/5] Improve tab completion for ALTER SYSTEM (SET|RESET)
 for non-superusers

Non-superusers can only use ALTER SYSTEM on variables they've
explicitly been GRANTed access to, and can't read the data_directory
setting or pg_settings.sourceline column, so the existing tab
completion for ALTER SYSTEM isn't very useful for them.

Instead, on servers > v15, query pg_parameter_acl to show the variables
they do have permission to set via ALTER SYSTEM.
---
 src/bin/psql/tab-complete.in.c | 46 +++++++++++++++++++++++++++++++---
 1 file changed, 43 insertions(+), 3 deletions(-)

diff --git a/src/bin/psql/tab-complete.in.c b/src/bin/psql/tab-complete.in.c
index 156fef50085..08a1df85492 100644
--- a/src/bin/psql/tab-complete.in.c
+++ b/src/bin/psql/tab-complete.in.c
@@ -304,6 +304,23 @@ do { \
 	COMPLETE_WITH_VERSIONED_QUERY_LIST(query, list); \
 } while (0)
 
+#define COMPLETE_WITH_VERSIONED_QUERY_VERBATIM(query) \
+	COMPLETE_WITH_VERSIONED_QUERY_VERBATIM_LIST(query, NULL)
+
+#define COMPLETE_WITH_VERSIONED_QUERY_VERBATIM_LIST(query, list) \
+do { \
+	completion_vquery = query; \
+	completion_charpp = list; \
+	completion_verbatim = true; \
+	matches = rl_completion_matches(text, complete_from_versioned_query); \
+} while (0)
+
+#define COMPLETE_WITH_VERSIONED_QUERY_VERBATIM_PLUS(query, ...) \
+do { \
+	static const char *const list[] = { __VA_ARGS__, NULL }; \
+	COMPLETE_WITH_VERSIONED_QUERY_VERBATIM_LIST(query, list); \
+} while (0)
+
 #define COMPLETE_WITH_SCHEMA_QUERY(query) \
 	COMPLETE_WITH_SCHEMA_QUERY_LIST(query, NULL)
 
@@ -1067,6 +1084,19 @@ static const SchemaQuery Query_for_trigger_of_table = {
 " WHERE sourcefile ~ '[\\\\/]postgresql\\.auto\\.conf$' " \
 "   AND pg_catalog.lower(name) LIKE pg_catalog.lower('%s')"
 
+static const VersionedQuery Query_for_list_of_alter_system_granted_vars[] = {
+	{150000,
+		"SELECT pg_catalog.lower(parname) FROM pg_catalog.pg_parameter_acl "
+		" WHERE EXISTS (SELECT FROM pg_catalog.aclexplode(paracl) "
+		"                WHERE pg_has_role(current_role, grantee, 'usage') "
+		"                  AND privilege_type = 'ALTER SYSTEM') "
+		"   AND pg_catalog.lower(parname) LIKE pg_catalog.lower('%s')",
+	},
+	/* this is only used for non-superusers, who can't ALTER SYSTEM before 15,
+	 * so no point in any fallback*/
+	{0, NULL},
+};
+
 #define Query_for_list_of_set_vars \
 "SELECT pg_catalog.lower(name) FROM pg_catalog.pg_settings "\
 " WHERE context IN ('user', 'superuser') "\
@@ -2653,10 +2683,20 @@ match_previous_words(int pattern_id,
 	else if (Matches("ALTER", "SYSTEM"))
 		COMPLETE_WITH("SET", "RESET");
 	else if (Matches("ALTER", "SYSTEM", "SET"))
-		COMPLETE_WITH_QUERY_VERBATIM(Query_for_list_of_alter_system_set_vars);
+	{
+		if (is_superuser())
+			COMPLETE_WITH_QUERY_VERBATIM(Query_for_list_of_alter_system_set_vars);
+		else
+			COMPLETE_WITH_VERSIONED_QUERY_VERBATIM(Query_for_list_of_alter_system_granted_vars);
+	}
 	else if (Matches("ALTER", "SYSTEM", "RESET"))
-		COMPLETE_WITH_QUERY_VERBATIM_PLUS(Query_for_list_of_alter_system_reset_vars,
-										  "ALL");
+	{
+		if (is_superuser())
+			COMPLETE_WITH_QUERY_VERBATIM_PLUS(Query_for_list_of_alter_system_reset_vars,
+											  "ALL");
+		else
+			COMPLETE_WITH_VERSIONED_QUERY_VERBATIM(Query_for_list_of_alter_system_granted_vars);
+	}
 	else if (Matches("ALTER", "SYSTEM", "SET", MatchAny))
 		COMPLETE_WITH("TO");
 	/* ALTER VIEW <name> */
-- 
2.51.2


--=-=-=--