Thread: Tab completion for SET TimeZone

Hi hackers,

I noticed there was no tab completion for time zones in psql, so here's
a patch that implements that.  I chose lower-casing the names since they
are case insensitive, and verbatim since ones without slashes can be
entered without quotes, and (at least my version of) readline completes
them correctly if the entered value starts with a single quote.

- ilmari

From 96ce5843641d990b278c0038f5b97941df6e73a9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Wed, 16 Mar 2022 12:52:21 +0000
Subject: [PATCH] =?UTF-8?q?Add=20tab=20completion=20for=20SET=20TimeZone?=
 =?UTF-8?q?=20TO=20=E2=80=A6?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Using verbatim and lower-case for maximum convenience.
---
 src/bin/psql/tab-complete.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 17172827a9..58e62a5e6e 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1105,6 +1105,13 @@ static const SchemaQuery Query_for_trigger_of_table = {
 "   FROM pg_catalog.pg_cursors "\
 "  WHERE name LIKE '%s'"
 
+#define Query_for_list_of_timezones \
+" SELECT name FROM ("\
+"   SELECT pg_catalog.lower(name) AS name "\
+"     FROM pg_catalog.pg_timezone_names() "\
+"  ) ss "\
+"  WHERE name LIKE '%s'"
+
 /*
  * These object types were introduced later than our support cutoff of
  * server version 9.2.  We use the VersionedQuery infrastructure so that
@@ -4171,6 +4178,8 @@ psql_completion(const char *text, int start, int end)
                                      " AND nspname NOT LIKE E'pg\\\\_temp%%'",
                                      "DEFAULT");
         }
+        else if (TailMatches("TimeZone", "TO|"))
+            COMPLETE_WITH_QUERY_VERBATIM_PLUS(Query_for_list_of_timezones, "DEFAULT");
         else
         {
             /* generic, type based, GUC support */
-- 
2.30.2

Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> writes:

> Hi hackers,
>
> I noticed there was no tab completion for time zones in psql, so here's
> a patch that implements that.

I just noticed I left out the = in the match check, here's an updated
patch that fixes that.

- ilmari

From bcfa40ff3a6702e1bd7112eeaecfde87efaa43c2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Wed, 16 Mar 2022 12:52:21 +0000
Subject: [PATCH v2] =?UTF-8?q?Add=20tab=20completion=20for=20SET=20TimeZon?=
 =?UTF-8?q?e=20TO=20=E2=80=A6?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Using verbatim and lower-case for maximum convenience.
---
 src/bin/psql/tab-complete.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 17172827a9..d68d001085 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1105,6 +1105,13 @@ static const SchemaQuery Query_for_trigger_of_table = {
 "   FROM pg_catalog.pg_cursors "\
 "  WHERE name LIKE '%s'"
 
+#define Query_for_list_of_timezones \
+" SELECT name FROM ("\
+"   SELECT pg_catalog.lower(name) AS name "\
+"     FROM pg_catalog.pg_timezone_names() "\
+"  ) ss "\
+"  WHERE name LIKE '%s'"
+
 /*
  * These object types were introduced later than our support cutoff of
  * server version 9.2.  We use the VersionedQuery infrastructure so that
@@ -4171,6 +4178,8 @@ psql_completion(const char *text, int start, int end)
                                      " AND nspname NOT LIKE E'pg\\\\_temp%%'",
                                      "DEFAULT");
         }
+        else if (TailMatches("TimeZone", "TO|="))
+            COMPLETE_WITH_QUERY_VERBATIM_PLUS(Query_for_list_of_timezones, "DEFAULT");
         else
         {
             /* generic, type based, GUC support */
-- 
2.30.2

=?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org> writes:
> I just noticed I left out the = in the match check, here's an updated
> patch that fixes that.

Hmm .... is that actually going to be useful in that form?
Most time zone names contain slashes and will therefore require
single-quoting.  I think you might need pushups comparable to
COMPLETE_WITH_ENUM_VALUE.

Also, personally, I'd rather not smash the names to lower case.
I think that's a significant decrement of readability.

            regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:

> =?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org> writes:
>> I just noticed I left out the = in the match check, here's an updated
>> patch that fixes that.
>
> Hmm .... is that actually going to be useful in that form?
> Most time zone names contain slashes and will therefore require
> single-quoting.  I think you might need pushups comparable to
> COMPLETE_WITH_ENUM_VALUE.

With readline (which is what I tested on) the completion works with or
without a single quote, but the user has to supply the quote themselves
for non-identifier-syntax timezone names.

I wasn't aware of the difference in behaviour with libedit, but now that
I've tested I agree that quoting things even when not strictly needed is
better.

This does however have the unfortunate side effect that on readline it
will suggest DEFAULT even after a single quote, which is not valid.

> Also, personally, I'd rather not smash the names to lower case.
> I think that's a significant decrement of readability.

That was mainly for convenience of not having to capitalise the place
names when typing (since they are accepted case insensitively).  A
compromise would be to lower-case it in the WHERE, but not in the
SELECT, as in the attached v3 patch.

I've tested this version on Debian stable with both readline 8.1-1 and
libedit 3.1-20191231-2.

- ilmari

From 07ed215e983fe4fda10cf92ddd12991f76e1410d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Wed, 16 Mar 2022 12:52:21 +0000
Subject: [PATCH v3] =?UTF-8?q?Add=20tab=20completion=20for=20SET=20TimeZon?=
 =?UTF-8?q?e=20TO=20=E2=80=A6?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Because the value (usually) needs single-quoting, and libedit includes
the leading quote in the text passed to the tab completion, but
readline doesn't, handle it simlarly to what we do for enum values.

This does however mean that under readline it'll include DEFAULT even
after an opening single quote, which is not valid.

Match then names case-insensitively for convenience, but return them
in the original case for legibility.
---
 src/bin/psql/tab-complete.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 17172827a9..bb2e6f47a7 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -402,6 +402,19 @@ do { \
     matches = rl_completion_matches(text, complete_from_schema_query); \
 } while (0)
 
+#define COMPLETE_WITH_TIMEZONE_NAME()            \
+do { \
+    static const char *const list[] = { "DEFAULT", NULL }; \
+    if (text[0] == '\'' || \
+        start == 0 || rl_line_buffer[start - 1] != '\'') \
+        completion_charp = Query_for_list_of_timezone_names_quoted; \
+    else \
+        completion_charp = Query_for_list_of_timezone_names_unquoted; \
+    completion_charpp = list;                              \
+    completion_verbatim = true; \
+    matches = rl_completion_matches(text, complete_from_query); \
+} while (0)
+
 #define COMPLETE_WITH_FUNCTION_ARG(function) \
 do { \
     set_completion_reference(function); \
@@ -1105,6 +1118,18 @@ static const SchemaQuery Query_for_trigger_of_table = {
 "   FROM pg_catalog.pg_cursors "\
 "  WHERE name LIKE '%s'"
 
+#define Query_for_list_of_timezone_names_unquoted \
+" SELECT name "\
+"   FROM pg_catalog.pg_timezone_names() "\
+"  WHERE pg_catalog.lower(name) LIKE pg_catalog.lower('%s')"
+
+#define Query_for_list_of_timezone_names_quoted \
+" SELECT name FROM ("\
+"   SELECT pg_catalog.quote_literal(name) AS name "\
+"     FROM pg_catalog.pg_timezone_names() "\
+"  ) ss "\
+"  WHERE pg_catalog.lower(name) LIKE pg_catalog.lower('%s')"
+
 /*
  * These object types were introduced later than our support cutoff of
  * server version 9.2.  We use the VersionedQuery infrastructure so that
@@ -4171,6 +4196,8 @@ psql_completion(const char *text, int start, int end)
                                      " AND nspname NOT LIKE E'pg\\\\_temp%%'",
                                      "DEFAULT");
         }
+        else if (TailMatches("TimeZone", "TO|="))
+            COMPLETE_WITH_TIMEZONE_NAME();
         else
         {
             /* generic, type based, GUC support */
-- 
2.30.2

Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> writes:

> Tom Lane <tgl@sss.pgh.pa.us> writes:
>
>> Also, personally, I'd rather not smash the names to lower case.
>> I think that's a significant decrement of readability.
>
> That was mainly for convenience of not having to capitalise the place
> names when typing (since they are accepted case insensitively).  A
> compromise would be to lower-case it in the WHERE, but not in the
> SELECT, as in the attached v3 patch.

I just realised there's no point in the subselect when I'm not applying
the same function in the WHERE and the SELECT, so here's an updated
version that simplifies that.  It also fixes a typo in the commit
message.

- ilmari

From ad4f37359852fa08ec377d86f0bad5f23dff3a0b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Wed, 16 Mar 2022 12:52:21 +0000
Subject: [PATCH v4] =?UTF-8?q?Add=20tab=20completion=20for=20SET=20TimeZon?=
 =?UTF-8?q?e=20TO=20=E2=80=A6?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Because the value (usually) needs single-quoting, and libedit includes
the leading quote in the text passed to the tab completion, but
readline doesn't, handle it simlarly to what we do for enum values.

This does however mean that under readline it'll include DEFAULT even
after an opening single quote, which is not valid.

Match the names case-insensitively for convenience, but return them in
the original case for legibility.
---
 src/bin/psql/tab-complete.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 17172827a9..c985781b27 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -402,6 +402,19 @@ do { \
     matches = rl_completion_matches(text, complete_from_schema_query); \
 } while (0)
 
+#define COMPLETE_WITH_TIMEZONE_NAME()            \
+do { \
+    static const char *const list[] = { "DEFAULT", NULL }; \
+    if (text[0] == '\'' || \
+        start == 0 || rl_line_buffer[start - 1] != '\'') \
+        completion_charp = Query_for_list_of_timezone_names_quoted; \
+    else \
+        completion_charp = Query_for_list_of_timezone_names_unquoted; \
+    completion_charpp = list;                              \
+    completion_verbatim = true; \
+    matches = rl_completion_matches(text, complete_from_query); \
+} while (0)
+
 #define COMPLETE_WITH_FUNCTION_ARG(function) \
 do { \
     set_completion_reference(function); \
@@ -1105,6 +1118,16 @@ static const SchemaQuery Query_for_trigger_of_table = {
 "   FROM pg_catalog.pg_cursors "\
 "  WHERE name LIKE '%s'"
 
+#define Query_for_list_of_timezone_names_unquoted \
+" SELECT name "\
+"   FROM pg_catalog.pg_timezone_names() "\
+"  WHERE pg_catalog.lower(name) LIKE pg_catalog.lower('%s')"
+
+#define Query_for_list_of_timezone_names_quoted \
+"SELECT pg_catalog.quote_literal(name) AS name "\
+"  FROM pg_catalog.pg_timezone_names() "\
+" WHERE pg_catalog.lower(name) LIKE pg_catalog.lower('%s')"
+
 /*
  * These object types were introduced later than our support cutoff of
  * server version 9.2.  We use the VersionedQuery infrastructure so that
@@ -4171,6 +4194,8 @@ psql_completion(const char *text, int start, int end)
                                      " AND nspname NOT LIKE E'pg\\\\_temp%%'",
                                      "DEFAULT");
         }
+        else if (TailMatches("TimeZone", "TO|="))
+            COMPLETE_WITH_TIMEZONE_NAME();
         else
         {
             /* generic, type based, GUC support */
-- 
2.30.2

=?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org> writes:
> I just realised there's no point in the subselect when I'm not applying
> the same function in the WHERE and the SELECT, so here's an updated
> version that simplifies that.  It also fixes a typo in the commit
> message.

This doesn't work right for me under libedit -- it will correctly
complete "am<TAB>" to "'America/", but then it fails to complete
anything past that.  The reason seems to be that once we have a
leading single quote, libedit will include that in the text passed
to future completion attempts, while readline won't.  I ended up
needing three query variants, as attached (bikeshedding welcome).

I think the reason the COMPLETE_WITH_ENUM_VALUE macro doesn't look
similar is that it hasn't made an attempt to work with input that
the user didn't quote --- that is, if you type
    alter type planets rename value ur<TAB>
it just fails to match anything, instead of providing "'uranus'".
Should we upgrade that likewise?  Not sure it's worth the trouble
though; I think COMPLETE_WITH_ENUM_VALUE is there more as a finger
exercise than because people use it regularly.

I added a regression test case too.

            regards, tom lane

diff --git a/src/bin/psql/t/010_tab_completion.pl b/src/bin/psql/t/010_tab_completion.pl
index a54910680e..c6db1b1591 100644
--- a/src/bin/psql/t/010_tab_completion.pl
+++ b/src/bin/psql/t/010_tab_completion.pl
@@ -338,6 +338,19 @@ check_completion(

 clear_line();

+# check timezone name completion
+check_completion(
+    "SET timezone TO am\t",
+    qr|'America/|,
+    "offer partial timezone name");
+
+check_completion(
+    "new_\t",
+    qr|New_York|,
+    "complete partial timezone name");
+
+clear_line();
+
 # check completion of a keyword offered in addition to object names;
 # such a keyword should obey COMP_KEYWORD_CASE
 foreach (
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 380cbc0b1f..510ea1d926 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -402,6 +402,21 @@ do { \
     matches = rl_completion_matches(text, complete_from_schema_query); \
 } while (0)

+/* timezone completion is mostly like enum label completion */
+#define COMPLETE_WITH_TIMEZONE_NAME() \
+do { \
+    static const char *const list[] = { "DEFAULT", NULL }; \
+    if (text[0] == '\'') \
+        completion_charp = Query_for_list_of_timezone_names_quoted_in; \
+    else if (start == 0 || rl_line_buffer[start - 1] != '\'') \
+        completion_charp = Query_for_list_of_timezone_names_quoted_out; \
+    else \
+        completion_charp = Query_for_list_of_timezone_names_unquoted; \
+    completion_charpp = list;                              \
+    completion_verbatim = true; \
+    matches = rl_completion_matches(text, complete_from_query); \
+} while (0)
+
 #define COMPLETE_WITH_FUNCTION_ARG(function) \
 do { \
     set_completion_reference(function); \
@@ -1105,6 +1120,21 @@ static const SchemaQuery Query_for_trigger_of_table = {
 "   FROM pg_catalog.pg_cursors "\
 "  WHERE name LIKE '%s'"

+#define Query_for_list_of_timezone_names_unquoted \
+" SELECT name "\
+"   FROM pg_catalog.pg_timezone_names() "\
+"  WHERE pg_catalog.lower(name) LIKE pg_catalog.lower('%s')"
+
+#define Query_for_list_of_timezone_names_quoted_out \
+"SELECT pg_catalog.quote_literal(name) AS name "\
+"  FROM pg_catalog.pg_timezone_names() "\
+" WHERE pg_catalog.lower(name) LIKE pg_catalog.lower('%s')"
+
+#define Query_for_list_of_timezone_names_quoted_in \
+"SELECT pg_catalog.quote_literal(name) AS name "\
+"  FROM pg_catalog.pg_timezone_names() "\
+" WHERE pg_catalog.quote_literal(pg_catalog.lower(name)) LIKE pg_catalog.lower('%s')"
+
 /*
  * These object types were introduced later than our support cutoff of
  * server version 9.2.  We use the VersionedQuery infrastructure so that
@@ -4172,6 +4202,8 @@ psql_completion(const char *text, int start, int end)
                                      " AND nspname NOT LIKE E'pg\\\\_temp%%'",
                                      "DEFAULT");
         }
+        else if (TailMatches("TimeZone", "TO|="))
+            COMPLETE_WITH_TIMEZONE_NAME();
         else
         {
             /* generic, type based, GUC support */

... btw, I forgot to mention that I don't see any problem with
the patch's behavior for DEFAULT.  What I see with both readline
and libedit is that if you type something reasonable, like
    set timezone to d<TAB>
then it will correctly complete "efault ", without any extra
quotes.  Now, if you're silly and do
    set timezone to 'd<TAB>
then readline gives you "efault' " and libedit gives you nothing.
But I think that's your own, um, fault.  We don't have any other
places where tab completion is so aggressive as to remove quotes
from a keyword.

            regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:

> =?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org> writes:
>> I just realised there's no point in the subselect when I'm not applying
>> the same function in the WHERE and the SELECT, so here's an updated
>> version that simplifies that.  It also fixes a typo in the commit
>> message.
>
> This doesn't work right for me under libedit -- it will correctly
> complete "am<TAB>" to "'America/", but then it fails to complete
> anything past that.  The reason seems to be that once we have a
> leading single quote, libedit will include that in the text passed
> to future completion attempts, while readline won't.  I ended up
> needing three query variants, as attached (bikeshedding welcome).

Well spotted, I must have just tested that it completed something on the
first tab. The fix looks reasonable to me, and I have no better ideas
for the names of the query string #defines.

> I think the reason the COMPLETE_WITH_ENUM_VALUE macro doesn't look
> similar is that it hasn't made an attempt to work with input that
> the user didn't quote --- that is, if you type
>     alter type planets rename value ur<TAB>
> it just fails to match anything, instead of providing "'uranus'".
> Should we upgrade that likewise?`

The comment says it will add the quote before text if it's not there, so
maybe we should adjust that to say that it will only add the quote if
the user hasn't typed anything?

> Not sure it's worth the trouble though; I think
> COMPLETE_WITH_ENUM_VALUE is there more as a finger exercise than
> because people use it regularly.

I agree, I mostly implemented that for completeness after adding ALTER
TYPE … RENAME VALUE. Also, enum values always need quoting, while SET
TIMEZONE doesn't if the zone name follows identifier syntax and people
might start typing it without quotes if they intend set one of those, so
the extra effort to make that work even though we can't suggest a mix of
quoted and non-quoted names is worth it.

> I added a regression test case too.

Good idea idea, I keep forgetting that we actually have tests for tab
completion, since most cases are so simple and obviously correct that we
don't bother with tests for them.

> ... btw, I forgot to mention that I don't see any problem with
> the patch's behavior for DEFAULT.  What I see with both readline
> and libedit is that if you type something reasonable, like
>     set timezone to d<TAB>
> then it will correctly complete "efault ", without any extra
> quotes.  Now, if you're silly and do
>     set timezone to 'd<TAB>
> then readline gives you "efault' " and libedit gives you nothing.
> But I think that's your own, um, fault.

I agree, it was just a quirk noticed while testing. Also, it helps that
there aren't any actual time zone names starting with 'd'.

> We don't have any other places where tab completion is so aggressive
> as to remove quotes from a keyword.

I noticed afterwards that other config varables behave the same, so even
if we wanted to improve that, it's outwith the scope of this patch.

>             regards, tom lane

- ilmari

=?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org> writes:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>> I think the reason the COMPLETE_WITH_ENUM_VALUE macro doesn't look
>> similar is that it hasn't made an attempt to work with input that
>> the user didn't quote --- that is, if you type
>> alter type planets rename value ur<TAB>
>> it just fails to match anything, instead of providing "'uranus'".
>> Should we upgrade that likewise?`

> The comment says it will add the quote before text if it's not there, so
> maybe we should adjust that to say that it will only add the quote if
> the user hasn't typed anything?

After thinking a bit harder, I realized that the SchemaQuery
infrastructure has no way to deal with the case of the input text not
being a prefix of what we want the output to be, so it can't do something
comparable to Query_for_list_of_timezone_names_quoted_out.  Maybe someday
we'll feel like adding that, but COMPLETE_WITH_ENUM_VALUE isn't a
compelling enough reason in current usage.  So I just tweaked the comment
a bit.

Pushed that way.

            regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:

> After thinking a bit harder, I realized that the SchemaQuery
> infrastructure has no way to deal with the case of the input text not
> being a prefix of what we want the output to be, so it can't do something
> comparable to Query_for_list_of_timezone_names_quoted_out.  Maybe someday
> we'll feel like adding that, but COMPLETE_WITH_ENUM_VALUE isn't a
> compelling enough reason in current usage.  So I just tweaked the comment
> a bit.

Fair enough.

> Pushed that way.

Thanks!

>             regards, tom lane

- ilmari