Обсуждение: Improve tab completion for various SET/RESET forms
Hi hackers,
I noticed that psql tab-completes every possible session-settable
variable after RESET, not just the ones that have actually been set in
the current session. However, as I was fixing that I noticed several
other deficiencies around other forms of SET/RESET. So, here's the
resulting yak stack.
Feel free to squash them as desired when committing, I just find it
easier to explain each thing as a separate commit/patch.
1. Fix tab completion for ALTER ROLE/USER ... RESET
It was trying to get the variables set for the current user, but the
query only worked on the intial tab after RESET, not when you started
typing a prefix to pick the one you wanted. Fix in the same way as
ALTER DATABASE ... RESET does it.
2. Add tab completion for ALTER TABLE ... ALTER COLUMN ... RESET
Complete with "(" and then the same as after ALTER COLUMN ... SET (.
There are only two possible attribute options, so no need to filter
down to the ones that have actually been set.
3. Add tab completion for ALTER FOREIGN TABLE ... SET
Setting the schema is the only supported thing.
4. Remove guard against generic SET/RESET completion after ALTER TABLE
... RESET
This is already handled specifically earlier in the code. Only
UPDATE is later and needs guarding against.
5. The actual patch I set out to write: only complete variables that
have actually been set in the current session after RESET.
- ilmari
-- part-time yak stylist
Вложения
Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> writes: > Hi hackers, > > I noticed that psql tab-completes every possible session-settable > variable after RESET, not just the ones that have actually been set in > the current session. However, as I was fixing that I noticed several > other deficiencies around other forms of SET/RESET. So, here's the > resulting yak stack. Added to the next commitfest: https://commitfest.postgresql.org/patch/5810/ - ilmari
On 09.06.25 22:27, Dagfinn Ilmari Mannsåker wrote: > I noticed that psql tab-completes every possible session-settable > variable after RESET, not just the ones that have actually been set in > the current session. However, as I was fixing that I noticed several > other deficiencies around other forms of SET/RESET. So, here's the > resulting yak stack. These technical changes seem fine, but I haven't looked in detail. But I want to say that I don't like this proposed behavior change at all. I don't think tab completion should filter out by things that are probably not interesting or useful depending on session state. That could be very confusing, especially since there is no surface to explain this anywhere. The obvious extreme case is that RESET in a fresh session wouldn't offer any completions at all.
Peter Eisentraut <peter@eisentraut.org> writes: > On 09.06.25 22:27, Dagfinn Ilmari Mannsåker wrote: >> I noticed that psql tab-completes every possible session-settable >> variable after RESET, not just the ones that have actually been set in >> the current session. However, as I was fixing that I noticed several >> other deficiencies around other forms of SET/RESET. So, here's the >> resulting yak stack. > > These technical changes seem fine, but I haven't looked in detail. But > I want to say that I don't like this proposed behavior change at all. This is not the first case of behaviour like this: there's precedence for it for ALTER DATABASE ... RESET and ALTER ROLE ... RESET (but that was buggy and is fixed by the first patch in the series): they only complete variables that are actually set on the relevant entity. See the thread at https://postgr.es/m/CAEP4nAzqiT6VbVC5r3nq5byLTnPzjniVGzEMpYcnAHQyNzEuaw%40mail.gmail.com Why should the session be any different? > don't think tab completion should filter out by things that are probably > not interesting or useful depending on session state. That could be > very confusing, especially since there is no surface to explain this > anywhere. The obvious extreme case is that RESET in a fresh session > wouldn't offer any completions at all. That's not the case, as mentioned in the commit message for the final patch: it also completes with the keywords ALL, ROLE and SESSION (which will subsequently be completed with AUTHORIZATION). Also, it's a handy way of seeing which variables have been set in the current session, withouth having to query pg_settings manually. - ilmari
Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> writes: > Peter Eisentraut <peter@eisentraut.org> writes: > >> On 09.06.25 22:27, Dagfinn Ilmari Mannsåker wrote: >>> I noticed that psql tab-completes every possible session-settable >>> variable after RESET, not just the ones that have actually been set in >>> the current session. However, as I was fixing that I noticed several >>> other deficiencies around other forms of SET/RESET. So, here's the >>> resulting yak stack. >> >> These technical changes seem fine, but I haven't looked in detail. But >> I want to say that I don't like this proposed behavior change at all. > > This is not the first case of behaviour like this: there's precedence > for it for ALTER DATABASE ... RESET and ALTER ROLE ... RESET (but that > was buggy and is fixed by the first patch in the series): they only > complete variables that are actually set on the relevant entity. See the > thread at > https://postgr.es/m/CAEP4nAzqiT6VbVC5r3nq5byLTnPzjniVGzEMpYcnAHQyNzEuaw%40mail.gmail.com I just noticed that in addition to ALTER ROLE ... RESET being buggy, the ALTER DATABASE ... RESET query doesn't schema-qualify the unnest() call. Here's an updated patch series that fixes that too. The first two are bug fixes to features new in 18 and should IMO be committed before that's released. The rest can wait for 19. - ilmari From 8d63ee9cbd118976ee1e5f22b9524099c21ef976 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org> Date: Wed, 11 Jun 2025 13:10:54 +0100 Subject: [PATCH v2 1/6] Fix unqualified unnest() call in tab completion query Commit 9df8727c5067 added tab completion for ALTER DATABASE ... RESET, but forgot to schema-qualifiy the unnest() call. --- src/bin/psql/tab-complete.in.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bin/psql/tab-complete.in.c b/src/bin/psql/tab-complete.in.c index 2c0b4f28c14..85aacc316bf 100644 --- a/src/bin/psql/tab-complete.in.c +++ b/src/bin/psql/tab-complete.in.c @@ -1002,7 +1002,7 @@ static const SchemaQuery Query_for_trigger_of_table = { #define Query_for_list_of_database_vars \ "SELECT conf FROM ("\ -" SELECT setdatabase, pg_catalog.split_part(unnest(setconfig),'=',1) conf"\ +" SELECT setdatabase, pg_catalog.split_part(pg_catalog.unnest(setconfig),'=',1) conf"\ " FROM pg_db_role_setting "\ " ) s, pg_database d "\ " WHERE s.setdatabase = d.oid "\ -- 2.49.0 From 2b0976af72bc8367599b5898b93f37cb4cc21a6a 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:35:29 +0100 Subject: [PATCH v2 2/6] Fix tab completion for ALTER ROLE|USER ... RESET It was showing all the right variables when hitting tab just after RESET, but as soon as you started typing something to pick what to complete, it would no longer work. Fix it in the same way as ALTER DATABASE ... RESET does it. --- src/bin/psql/tab-complete.in.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/bin/psql/tab-complete.in.c b/src/bin/psql/tab-complete.in.c index 85aacc316bf..bde8c1e1c85 100644 --- a/src/bin/psql/tab-complete.in.c +++ b/src/bin/psql/tab-complete.in.c @@ -1078,9 +1078,12 @@ Keywords_for_list_of_owner_roles, "PUBLIC" " WHERE usename LIKE '%s'" #define Query_for_list_of_user_vars \ -" SELECT pg_catalog.split_part(pg_catalog.unnest(rolconfig),'=',1) "\ -" FROM pg_catalog.pg_roles "\ -" WHERE rolname LIKE '%s'" +"SELECT conf FROM ("\ +" SELECT rolname, pg_catalog.split_part(pg_catalog.unnest(rolconfig),'=',1) conf"\ +" FROM pg_catalog.pg_roles"\ +" ) s"\ +" WHERE s.conf like '%s' "\ +" AND s.rolname LIKE '%s'" #define Query_for_list_of_access_methods \ " SELECT amname "\ @@ -2495,7 +2498,10 @@ match_previous_words(int pattern_id, /* ALTER USER,ROLE <name> RESET */ else if (Matches("ALTER", "USER|ROLE", MatchAny, "RESET")) + { + set_completion_reference(prev2_wd); COMPLETE_WITH_QUERY_PLUS(Query_for_list_of_user_vars, "ALL"); + } /* ALTER USER,ROLE <name> WITH */ else if (Matches("ALTER", "USER|ROLE", MatchAny, "WITH")) -- 2.49.0 From 788c79bf607d7159005fe5c425d67bbbbbb7ea65 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 v2 3/6] 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 bde8c1e1c85..1eaae544e12 100644 --- a/src/bin/psql/tab-complete.in.c +++ b/src/bin/psql/tab-complete.in.c @@ -2884,9 +2884,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.49.0 From b1d3f55613545d2156db6d5b50a060f88e8c8f35 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 v2 4/6] 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 1eaae544e12..6210786017e 100644 --- a/src/bin/psql/tab-complete.in.c +++ b/src/bin/psql/tab-complete.in.c @@ -2413,6 +2413,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.49.0 From 0ae96d4df25c63173d956a366cb2049d3b02f321 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:45:30 +0100 Subject: [PATCH v2 5/6] Remove guard against generic completion after ALTER DATABASE ... RESET Commit 9df8727c5067 added explicit handling for ALTER DATABASE ... RESET earlier in the code, so no need to guard against it here. --- src/bin/psql/tab-complete.in.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/bin/psql/tab-complete.in.c b/src/bin/psql/tab-complete.in.c index 6210786017e..c3184239a17 100644 --- a/src/bin/psql/tab-complete.in.c +++ b/src/bin/psql/tab-complete.in.c @@ -4957,8 +4957,7 @@ 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", MatchAny, "RESET")) + !TailMatches("UPDATE", MatchAny, "SET")) COMPLETE_WITH_QUERY_VERBATIM_PLUS(Query_for_list_of_set_vars, "CONSTRAINTS", "TRANSACTION", -- 2.49.0 From 4b1459293f28b4ccf90d0ae524474613e8873f18 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:48:43 +0100 Subject: [PATCH v2 6/6] 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). --- src/bin/psql/tab-complete.in.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/bin/psql/tab-complete.in.c b/src/bin/psql/tab-complete.in.c index c3184239a17..4a7a86998e0 100644 --- a/src/bin/psql/tab-complete.in.c +++ b/src/bin/psql/tab-complete.in.c @@ -1039,6 +1039,12 @@ 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 context IN ('user', 'superuser') "\ +" AND 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')" @@ -4955,8 +4961,14 @@ match_previous_words(int pattern_id, /* naah . . . */ /* SET, RESET, SHOW */ + /* Complete with variables set in the current session */ + else if (TailMatches("RESET")) + COMPLETE_WITH_QUERY_VERBATIM_PLUS(Query_for_list_of_session_vars, + "ALL", + "ROLE", + "SESSION"); /* Complete with a variable name */ - else if (TailMatches("SET|RESET") && + else if (TailMatches("SET") && !TailMatches("UPDATE", MatchAny, "SET")) COMPLETE_WITH_QUERY_VERBATIM_PLUS(Query_for_list_of_set_vars, "CONSTRAINTS", -- 2.49.0
Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> writes:
> I just noticed that in addition to ALTER ROLE ... RESET being buggy, the
> ALTER DATABASE ... RESET query doesn't schema-qualify the unnest() call.
> Here's an updated patch series that fixes that too. The first two are
> bug fixes to features new in 18 and should IMO be committed before
> that's released. The rest can wait for 19.
Now that Tomas has committed the two bugfixes, here's the rest of the
patches rebased over that.
- ilmari
From e9880ce7a786306ba075c795030bb81a240d0452 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 v3 1/3] 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 1f2ca946fc5..176ae1284be 100644
--- a/src/bin/psql/tab-complete.in.c
+++ b/src/bin/psql/tab-complete.in.c
@@ -2913,9 +2913,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.50.1
From e3e7fe0378af2d04f4c92e3fdc17fccaf2580d56 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 v3 2/3] 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 176ae1284be..d31f5780727 100644
--- a/src/bin/psql/tab-complete.in.c
+++ b/src/bin/psql/tab-complete.in.c
@@ -2435,6 +2435,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.50.1
From 6c165b8b42346620a92973e1003db806385f6d92 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:48:43 +0100
Subject: [PATCH v3 3/3] 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).
---
src/bin/psql/tab-complete.in.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/src/bin/psql/tab-complete.in.c b/src/bin/psql/tab-complete.in.c
index d31f5780727..e3b685ebe10 100644
--- a/src/bin/psql/tab-complete.in.c
+++ b/src/bin/psql/tab-complete.in.c
@@ -1047,6 +1047,12 @@ 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 context IN ('user', 'superuser') "\
+" AND 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')"
@@ -5027,9 +5033,8 @@ 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",
@@ -5037,6 +5042,12 @@ match_previous_words(int pattern_id,
"ROLE",
"TABLESPACE",
"ALL");
+ /* 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",
+ "SESSION");
else if (Matches("SHOW"))
COMPLETE_WITH_QUERY_VERBATIM_PLUS(Query_for_list_of_show_vars,
"SESSION AUTHORIZATION",
--
2.50.1
On Fri, Aug 1, 2025 at 2:16 AM Dagfinn Ilmari Mannsåker
<ilmari@ilmari.org> wrote:
>
> Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> writes:
>
> > I just noticed that in addition to ALTER ROLE ... RESET being buggy, the
> > ALTER DATABASE ... RESET query doesn't schema-qualify the unnest() call.
> > Here's an updated patch series that fixes that too. The first two are
> > bug fixes to features new in 18 and should IMO be committed before
> > that's released. The rest can wait for 19.
>
> Now that Tomas has committed the two bugfixes, here's the rest of the
> patches rebased over that.
Thank you for the patches.
+ else if (Matches("ALTER", "FOREIGN", "TABLE", MatchAny, "SET"))
+ COMPLETE_WITH("SCHEMA");
In addition to SET SCHEMA, I think you should add tab completion for
SET WITHOUT OIDS.
+#define Query_for_list_of_session_vars \
+"SELECT pg_catalog.lower(name) FROM pg_catalog.pg_settings "\
+" WHERE context IN ('user', 'superuser') "\
+" AND source = 'session' "\
+" AND pg_catalog.lower(name) LIKE pg_catalog.lower('%s')"
I think the context IN ('user', 'superuser') condition is redundant
here. If a parameter's source is 'session', its context must be either
'user' or 'superuser'. Therefore, the source = 'session' check alone
should be sufficient.
--
Best regards,
Shinya Kato
NTT OSS Center
Hi! On Fri, 8 Aug 2025 at 08:04, Shinya Kato <shinya11.kato@gmail.com> wrote: > In addition to SET SCHEMA, I think you should add tab completion for > SET WITHOUT OIDS. This is unsupported since v12 [1] [1] https://www.postgresql.org/docs/current/sql-altertable.html#SQL-ALTERTABLE-DESC-SET-WITHOUT-OIDS -- Best regards, Kirill Reshke
Shinya Kato <shinya11.kato@gmail.com> writes:
> On Fri, Aug 1, 2025 at 2:16 AM Dagfinn Ilmari Mannsåker
> <ilmari@ilmari.org> wrote:
>>
>> Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> writes:
>>
>> > I just noticed that in addition to ALTER ROLE ... RESET being buggy, the
>> > ALTER DATABASE ... RESET query doesn't schema-qualify the unnest() call.
>> > Here's an updated patch series that fixes that too. The first two are
>> > bug fixes to features new in 18 and should IMO be committed before
>> > that's released. The rest can wait for 19.
>>
>> Now that Tomas has committed the two bugfixes, here's the rest of the
>> patches rebased over that.
>
> Thank you for the patches.
>
> + else if (Matches("ALTER", "FOREIGN", "TABLE", MatchAny, "SET"))
> + COMPLETE_WITH("SCHEMA");
>
> In addition to SET SCHEMA, I think you should add tab completion for
> SET WITHOUT OIDS.
As Kirill pointed out, support for WITH OIDS was removed in v12. The
SET WITHOUT OIDS syntax only remains as a no-op for backwards
compatibility with existing SQL scripts, so there's no point in offering
tab completion for it.
> +#define Query_for_list_of_session_vars \
> +"SELECT pg_catalog.lower(name) FROM pg_catalog.pg_settings "\
> +" WHERE context IN ('user', 'superuser') "\
> +" AND source = 'session' "\
> +" AND pg_catalog.lower(name) LIKE pg_catalog.lower('%s')"
>
> I think the context IN ('user', 'superuser') condition is redundant
> here. If a parameter's source is 'session', its context must be either
> 'user' or 'superuser'. Therefore, the source = 'session' check alone
> should be sufficient.
Good point, updated in the attached v4 patches.
- ilmari
From 59d8c9c4874dadc8754f0dd7ea6d6dc67f3814ff 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 v4 1/4] 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 1f2ca946fc5..176ae1284be 100644
--- a/src/bin/psql/tab-complete.in.c
+++ b/src/bin/psql/tab-complete.in.c
@@ -2913,9 +2913,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.50.1
From d34caaeb8409d58b5abaac825ed1c093bb5e7cc8 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 v4 2/4] 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 176ae1284be..d31f5780727 100644
--- a/src/bin/psql/tab-complete.in.c
+++ b/src/bin/psql/tab-complete.in.c
@@ -2435,6 +2435,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.50.1
From 9e3b600924c4118002fb36187fa268ba2ac37d9a 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 v4 3/4] 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 we're using Matches() insted of TailMatches(), we can also get
rid of the guards against ALTER DATABASE|USER|ROLE ... RESET.
---
src/bin/psql/tab-complete.in.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/src/bin/psql/tab-complete.in.c b/src/bin/psql/tab-complete.in.c
index d31f5780727..c4f50880965 100644
--- a/src/bin/psql/tab-complete.in.c
+++ b/src/bin/psql/tab-complete.in.c
@@ -1047,6 +1047,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')"
@@ -5027,9 +5032,8 @@ 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",
@@ -5037,6 +5041,12 @@ match_previous_words(int pattern_id,
"ROLE",
"TABLESPACE",
"ALL");
+ /* 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",
+ "SESSION");
else if (Matches("SHOW"))
COMPLETE_WITH_QUERY_VERBATIM_PLUS(Query_for_list_of_show_vars,
"SESSION AUTHORIZATION",
--
2.50.1
Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> writes:
> Shinya Kato <shinya11.kato@gmail.com> writes:
>
>> On Fri, Aug 1, 2025 at 2:16 AM Dagfinn Ilmari Mannsåker
>> <ilmari@ilmari.org> wrote:
>>>
>>> Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> writes:
>>>
>>> > I just noticed that in addition to ALTER ROLE ... RESET being buggy, the
>>> > ALTER DATABASE ... RESET query doesn't schema-qualify the unnest() call.
>>> > Here's an updated patch series that fixes that too. The first two are
>>> > bug fixes to features new in 18 and should IMO be committed before
>>> > that's released. The rest can wait for 19.
>>>
>>> Now that Tomas has committed the two bugfixes, here's the rest of the
>>> patches rebased over that.
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.
- ilmari
Вложения
On Sat, Aug 9, 2025 at 7:55 AM Dagfinn Ilmari Mannsåker
<ilmari@ilmari.org> wrote:
>
> Shinya Kato <shinya11.kato@gmail.com> writes:
>
> > On Fri, Aug 1, 2025 at 2:16 AM Dagfinn Ilmari Mannsåker
> > <ilmari@ilmari.org> wrote:
> >>
> >> Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> writes:
> >>
> >> > I just noticed that in addition to ALTER ROLE ... RESET being buggy, the
> >> > ALTER DATABASE ... RESET query doesn't schema-qualify the unnest() call.
> >> > Here's an updated patch series that fixes that too. The first two are
> >> > bug fixes to features new in 18 and should IMO be committed before
> >> > that's released. The rest can wait for 19.
> >>
> >> Now that Tomas has committed the two bugfixes, here's the rest of the
> >> patches rebased over that.
> >
> > Thank you for the patches.
> >
> > + else if (Matches("ALTER", "FOREIGN", "TABLE", MatchAny, "SET"))
> > + COMPLETE_WITH("SCHEMA");
> >
> > In addition to SET SCHEMA, I think you should add tab completion for
> > SET WITHOUT OIDS.
>
> As Kirill pointed out, support for WITH OIDS was removed in v12. The
> SET WITHOUT OIDS syntax only remains as a no-op for backwards
> compatibility with existing SQL scripts, so there's no point in offering
> tab completion for it.
I got it, thanks!
> > +#define Query_for_list_of_session_vars \
> > +"SELECT pg_catalog.lower(name) FROM pg_catalog.pg_settings "\
> > +" WHERE context IN ('user', 'superuser') "\
> > +" AND source = 'session' "\
> > +" AND pg_catalog.lower(name) LIKE pg_catalog.lower('%s')"
> >
> > I think the context IN ('user', 'superuser') condition is redundant
> > here. If a parameter's source is 'session', its context must be either
> > 'user' or 'superuser'. Therefore, the source = 'session' check alone
> > should be sufficient.
>
> Good point, updated in the attached v4 patches.
Thank you.
On Mon, Aug 11, 2025 at 8:40 PM Dagfinn Ilmari Mannsåker
<ilmari@ilmari.org> wrote:
>
> Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> writes:
>
> > Shinya Kato <shinya11.kato@gmail.com> writes:
> >
> >> On Fri, Aug 1, 2025 at 2:16 AM Dagfinn Ilmari Mannsåker
> >> <ilmari@ilmari.org> wrote:
> >>>
> >>> Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> writes:
> >>>
> >>> > I just noticed that in addition to ALTER ROLE ... RESET being buggy, the
> >>> > ALTER DATABASE ... RESET query doesn't schema-qualify the unnest() call.
> >>> > Here's an updated patch series that fixes that too. The first two are
> >>> > bug fixes to features new in 18 and should IMO be committed before
> >>> > that's released. The rest can wait for 19.
> >>>
> >>> Now that Tomas has committed the two bugfixes, here's the rest of the
> >>> patches rebased over that.
>
> 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.
I failed to apply these patches.
----
$ git apply v5-000* -v
Checking patch src/bin/psql/tab-complete.in.c...
error: while searching for:
"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", "("))?
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") ||?
error: patch failed: src/bin/psql/tab-complete.in.c:2913
error: src/bin/psql/tab-complete.in.c: patch does not apply
----
And you should move the CF entry to the next CF. If so, CFBot reports
the above error.
--
Best regards,
Shinya Kato
NTT OSS Center
On Wed, Aug 13, 2025 at 3:56 PM Shinya Kato <shinya11.kato@gmail.com> wrote:
>
> On Sat, Aug 9, 2025 at 7:55 AM Dagfinn Ilmari Mannsåker
> <ilmari@ilmari.org> wrote:
> >
> > Shinya Kato <shinya11.kato@gmail.com> writes:
> >
> > > On Fri, Aug 1, 2025 at 2:16 AM Dagfinn Ilmari Mannsåker
> > > <ilmari@ilmari.org> wrote:
> > >>
> > >> Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> writes:
> > >>
> > >> > I just noticed that in addition to ALTER ROLE ... RESET being buggy, the
> > >> > ALTER DATABASE ... RESET query doesn't schema-qualify the unnest() call.
> > >> > Here's an updated patch series that fixes that too. The first two are
> > >> > bug fixes to features new in 18 and should IMO be committed before
> > >> > that's released. The rest can wait for 19.
> > >>
> > >> Now that Tomas has committed the two bugfixes, here's the rest of the
> > >> patches rebased over that.
> > >
> > > Thank you for the patches.
> > >
> > > + else if (Matches("ALTER", "FOREIGN", "TABLE", MatchAny, "SET"))
> > > + COMPLETE_WITH("SCHEMA");
> > >
> > > In addition to SET SCHEMA, I think you should add tab completion for
> > > SET WITHOUT OIDS.
> >
> > As Kirill pointed out, support for WITH OIDS was removed in v12. The
> > SET WITHOUT OIDS syntax only remains as a no-op for backwards
> > compatibility with existing SQL scripts, so there's no point in offering
> > tab completion for it.
>
> I got it, thanks!
>
> > > +#define Query_for_list_of_session_vars \
> > > +"SELECT pg_catalog.lower(name) FROM pg_catalog.pg_settings "\
> > > +" WHERE context IN ('user', 'superuser') "\
> > > +" AND source = 'session' "\
> > > +" AND pg_catalog.lower(name) LIKE pg_catalog.lower('%s')"
> > >
> > > I think the context IN ('user', 'superuser') condition is redundant
> > > here. If a parameter's source is 'session', its context must be either
> > > 'user' or 'superuser'. Therefore, the source = 'session' check alone
> > > should be sufficient.
> >
> > Good point, updated in the attached v4 patches.
>
> Thank you.
>
>
> On Mon, Aug 11, 2025 at 8:40 PM Dagfinn Ilmari Mannsåker
> <ilmari@ilmari.org> wrote:
> >
> > Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> writes:
> >
> > > Shinya Kato <shinya11.kato@gmail.com> writes:
> > >
> > >> On Fri, Aug 1, 2025 at 2:16 AM Dagfinn Ilmari Mannsåker
> > >> <ilmari@ilmari.org> wrote:
> > >>>
> > >>> Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> writes:
> > >>>
> > >>> > I just noticed that in addition to ALTER ROLE ... RESET being buggy, the
> > >>> > ALTER DATABASE ... RESET query doesn't schema-qualify the unnest() call.
> > >>> > Here's an updated patch series that fixes that too. The first two are
> > >>> > bug fixes to features new in 18 and should IMO be committed before
> > >>> > that's released. The rest can wait for 19.
> > >>>
> > >>> Now that Tomas has committed the two bugfixes, here's the rest of the
> > >>> patches rebased over that.
> >
> > 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.
>
> I failed to apply these patches.
> ----
> $ git apply v5-000* -v
> Checking patch src/bin/psql/tab-complete.in.c...
> error: while searching for:
> "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", "("))?
> 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") ||?
>
> error: patch failed: src/bin/psql/tab-complete.in.c:2913
> error: src/bin/psql/tab-complete.in.c: patch does not apply
> ----
Oh, sorry. I can apply them with git am.
--
Best regards,
Shinya Kato
NTT OSS Center
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.
> >
> > I failed to apply these patches.
> > ----
> > $ git apply v5-000* -v
> > Checking patch src/bin/psql/tab-complete.in.c...
> > error: while searching for:
> > "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", "("))?
> > 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") ||?
> >
> > error: patch failed: src/bin/psql/tab-complete.in.c:2913
> > error: src/bin/psql/tab-complete.in.c: patch does not apply
> > ----
>
> Oh, sorry. I can apply them with git am.
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?
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.
--
Best regards,
Shinya Kato
NTT OSS Center
On Wed, 13 Aug 2025 at 14:54, Shinya Kato <shinya11.kato@gmail.com> wrote: > > 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? Indeed, SET SESSION <tab> does not suggest any parameter. That should be a separate patch though. Also, SET <tab> suggests SESSION but not LOCAL, which also can be improved -- Best regards, Kirill Reshke
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
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
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
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
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
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