Обсуждение: 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