Tab completion for SET TimeZone

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

=?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

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

=?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

... 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