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
Attachments:
0001-Fix-tab-completion-for-ALTER-ROLE-USER-.-RESET.patchtext/x-diffDownload+9-4
0002-Add-tab-completion-for-ALTER-TABLE-.-ALTER-COLUMN-.-.patchtext/x-diffDownload+7-4
0003-Add-tab-completion-for-ALTER-FOREIGN-TABLE-.-SET.patchtext/x-diffDownload+4-1
0004-Remove-guard-against-generic-completion-after-ALTER-.patchtext/x-diffDownload+1-3
0005-Improve-tab-completion-for-RESET.patchtext/x-diffDownload+13-2
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
/messages/by-id/CAEP4nAzqiT6VbVC5r3nq5byLTnPzjniVGzEMpYcnAHQyNzEuaw@mail.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
/messages/by-id/CAEP4nAzqiT6VbVC5r3nq5byLTnPzjniVGzEMpYcnAHQyNzEuaw@mail.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
Attachments:
v2-0001-Fix-unqualified-unnest-call-in-tab-completion-que.patchtext/x-diffDownload+1-2
v2-0002-Fix-tab-completion-for-ALTER-ROLE-USER-.-RESET.patchtext/x-diffDownload+9-4
v2-0003-Add-tab-completion-for-ALTER-TABLE-.-ALTER-COLUMN.patchtext/x-diffDownload+7-4
v2-0004-Add-tab-completion-for-ALTER-FOREIGN-TABLE-.-SET.patchtext/x-diffDownload+4-1
v2-0005-Remove-guard-against-generic-completion-after-ALT.patchtext/x-diffDownload+1-3
v2-0006-Improve-tab-completion-for-RESET.patchtext/x-diffDownload+13-2
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
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]https://www.postgresql.org/docs/current/sql-altertable.html#SQL-ALTERTABLE-DESC-SET-WITHOUT-OIDS
[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
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
Attachments:
v5-0001-Add-tab-completion-for-ALTER-TABLE-.-ALTER-COLUMN.patchtext/x-diffDownload+7-4
v5-0002-Add-tab-completion-for-ALTER-FOREIGN-TABLE-.-SET.patchtext/x-diffDownload+4-1
v5-0003-Improve-tab-completion-for-RESET.patchtext/x-diffDownload+13-4
v5-0004-Improve-tab-completion-for-ALTER-SYSTEM-RESET.patchtext/x-diffDownload+11-3
v5-0005-Improve-tab-completion-for-ALTER-SYSTEM-RESET-for.patchtext/x-diffDownload+16-4
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