psql: tab completion differs on semicolon placement

Daniel Gustafsson <daniel@yesql.se> writes:

While testing a patch I fat-fingered a CREATE DATABASE statement by tab
completing *after* the semicolon, with no space between the objname and
semicolon. The below options were presented, which at this point aren't really
applicable:

db=# create database foo;
ALLOW_CONNECTIONS ENCODING LC_COLLATE LOCALE TABLESPACE
CONNECTION LIMIT IS_TEMPLATE LC_CTYPE OWNER TEMPLATE

DROP DATABASE has a similar tab completion which makes about as much sense:

db=# drop database foo;WITH (

Checking prev_wd for not ending with ';' as per the attached makes "objname;"
behave like "objname ;". Is there a reason for not doing that which I'm
missing? I didn't check for others, but if this seems reasonable I'll go
through to find any other similar cases.

The same applies to any completion after a MatchAny that ends in a any
of the WORD_BREAKS characters (except whitespace and () which are
handled specially).

#define WORD_BREAKS "\t\n@$><=;|&{() "

IMO a fix should be more principled than just special-casing semicolon
and CREATE TABLE. Maybe get_previous_words() should stop when it sees
an unquoted semicolon?

- ilmari

On 20 Sep 2021, at 21:26, Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> wrote:

IMO a fix should be more principled than just special-casing semicolon
and CREATE TABLE. Maybe get_previous_words() should stop when it sees
an unquoted semicolon?

Agreed, something along those lines makes sense. I will familiarize myself
with this file (which until today has been a blank spot) and will see what I
can come up with.

--
Daniel Gustafsson https://vmware.com/

On Mon, Sep 20, 2021 at 08:26:51PM +0100, Dagfinn Ilmari Manns�ker wrote:

Daniel Gustafsson <daniel@yesql.se> writes:

While testing a patch I fat-fingered a CREATE DATABASE statement by tab
completing *after* the semicolon, with no space between the objname and
semicolon. The below options were presented, which at this point aren't really
applicable:

db=# create database foo;
ALLOW_CONNECTIONS ENCODING LC_COLLATE LOCALE TABLESPACE
CONNECTION LIMIT IS_TEMPLATE LC_CTYPE OWNER TEMPLATE

DROP DATABASE has a similar tab completion which makes about as much sense:

db=# drop database foo;WITH (

Checking prev_wd for not ending with ';' as per the attached makes "objname;"
behave like "objname ;". Is there a reason for not doing that which I'm
missing? I didn't check for others, but if this seems reasonable I'll go
through to find any other similar cases.

The same applies to any completion after a MatchAny that ends in a any
of the WORD_BREAKS characters (except whitespace and () which are
handled specially).

#define WORD_BREAKS "\t\n@$><=;|&{() "

IMO a fix should be more principled than just special-casing semicolon
and CREATE TABLE. Maybe get_previous_words() should stop when it sees
an unquoted semicolon?

Is there some reason get_previous_words() shouldn't stop for
everything that's WORD_BREAKS? If not, making that the test might make the
general rule a little simpler to write, and if WORD_BREAKS ever
changed, for example to include all space, or all breaking space, or
similar, the consequences would at least not propagate through
seemingly unrelated code.

At the moment, get_previous_words() does look for everything in
WORD_BREAKS, and then accounts for double quotes (") and then does
something clever to account for double quotes and the quoting behavior
that doubling them ("") accomplishes. Anyhow, that looks like it
should work in this case, but clearly it's not.

Would it be less error prone to do these checks and maybe push or pop
one or more stacks holding state as each character came in? I suspect
the overhead would be unnoticeable even on the slowest* client.

Best,
David.

* One possible exception would be a gigantic paste, a case where psql
can be prevented from attempting tab completion, although the
prevention measures involve a pretty obscure terminal setting:
https://cirw.in/blog/bracketed-paste

Best,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

David Fetter <david@fetter.org> writes:

On Mon, Sep 20, 2021 at 08:26:51PM +0100, Dagfinn Ilmari Mannsåker wrote:

The same applies to any completion after a MatchAny that ends in a any
of the WORD_BREAKS characters (except whitespace and () which are
handled specially).

#define WORD_BREAKS "\t\n@$><=;|&{() "

IMO a fix should be more principled than just special-casing semicolon
and CREATE TABLE. Maybe get_previous_words() should stop when it sees
an unquoted semicolon?

Is there some reason get_previous_words() shouldn't stop for
everything that's WORD_BREAKS? If not, making that the test might make the
general rule a little simpler to write, and if WORD_BREAKS ever
changed, for example to include all space, or all breaking space, or
similar, the consequences would at least not propagate through
seemingly unrelated code.

By "stopping" I meant ignoring everything before the last semicolon when
splitting the buffer into words, i.e. not putting them into the
previous_words array, so they're not considered by the
(Tail|Head)?Matches(CS)? macros. WORD_BREAKS is the list of characters
used for splitting the input buffer into the previous_words array, so it
would need to keep going past those, or you'd only be able to match the
last word when tab completing, rendering the entire exercise pointless.

At the moment, get_previous_words() does look for everything in
WORD_BREAKS, and then accounts for double quotes (") and then does
something clever to account for double quotes and the quoting behavior
that doubling them ("") accomplishes. Anyhow, that looks like it
should work in this case, but clearly it's not.

WORD_BREAK characters inside double-quoted identifiers are handled
correclty, but only after you've typed the closing quote. If you have
an ambiguous prefix that contains a WORD_BREAK character, you can't
tab-complete the rest:

ilmari@[local]:5432 ~=# drop table "foo<tab><tab>
"foo$bar" "foo$zot" "foo-bar" "foo-zot"
ilmari@[local]:5432 ~=# drop table "foo-<tab><tab>
"foo-bar" "foo-zot"
ilmari@[local]:5432 ~=# rop table "foo$<tab><tab>

ilmari@[local]:5432 ~=# drop table "foo$bar" <tab><tab>
cascade restrict

Tangentially, I would argue that $ shouldn't be a WORD_BREAK character,
since it's valid in unquoted identifiers (except at the start, just like
numbers). But you do need to quote such identifiers when
tab-completing, since quote_ident() quotes anything tht's not all
lowercase letters, underscores and numbers.

- ilmari