Обсуждение: tab-complete COMPLETE_WITH_ATTR can become confused by table-lists.

Hi.

I stumbled onto a small quirk/bug in the tab-complete code.

There are some places that suggest tab completions using the current
table columns. These are coded like:
COMPLETE_WITH_ATTR(prev2_wd, "");

The assumption is that the prev2_wd represents the table to select from.

Normally, this works fine. However, there are also cases where a
table-list can be specified (not just a single table) and in this
scenario, the 'prev2_wd' can sometimes become confused about what is
table name to use.

e.g.

If there are spaces in the table-list like "t1, t2" then the word is
recognized as "t2" and it works as expected.

But, if there are no spaces in the table-list like "t1,t2" then the
word is recognized as "t1,t2", and since that is no such table name
the COMPLETE_WITH_ATTR does nothing.

~~

Examples (press <tab> after the "(")

// setup

test=# create table t1(a int, b int, c int);
test=# create table t2(d int, e int, f int);

// test single table --> OK

test=# analyze t1 (
a b c
test=# analyze t2 (
d e f

// test table-list with spaces --> OK

test=# analyze t1, t2 (
d e f
test=# analyze t2, t1 (
a b c

// test table-list without spaces --> does not work

test=# analyze t2,t1 (

~~

I found that this is easily fixed just by adding a comma to the
WORD_BREAKS. Then words all get tokenized properly and so 'prev2_wd'
is what you'd like it to be.

 /* word break characters */
-#define WORD_BREAKS "\t\n@$><=;|&{() "
+#define WORD_BREAKS "\t\n,@$><=;|&{() "

OTOH, this seemed a pretty fundamental change to the 12-year-old (!!)
code so I don't know if it may be too risky and/or could adversely
affect something else?

The tests are all still passing, but there aren't so many tab-complete
tests anyway so that might not mean much.

Thoughts?

------
Kind Regards,
Peter Smith.
Fujitsu Australia

Peter Smith <smithpb2250@gmail.com> writes:
> If there are spaces in the table-list like "t1, t2" then the word is
> recognized as "t2" and it works as expected.
> But, if there are no spaces in the table-list like "t1,t2" then the
> word is recognized as "t1,t2", and since that is no such table name
> the COMPLETE_WITH_ATTR does nothing.

Hmm, another variant is

=# create table foobar(z int);
CREATE TABLE
=# analyze foo<TAB>          --> completes "foobar"
=# analyze foobar,foo<TAB>   --> nothing

> I found that this is easily fixed just by adding a comma to the
> WORD_BREAKS. Then words all get tokenized properly and so 'prev2_wd'
> is what you'd like it to be.
>  /* word break characters */
> -#define WORD_BREAKS "\t\n@$><=;|&{() "
> +#define WORD_BREAKS "\t\n,@$><=;|&{() "

Nice catch.  Now that I look at it, that WORD_BREAKS list seems quite
random -- for example, why "{" but not "}", and why neither of "["
or "]"?  If we have "><=", why not "+-*/", to say nothing of other
operator characters?

I can see reasons for not listing ' " or :, because those are handled
elsewhere.  But I'm not sure about the other omissions.

Experimenting a bit, I see that

=# create table "amp&sand" (f int);
CREATE TABLE
=# insert into "amp<TAB>     --> completes "amp&sand"
=# insert into "amp&<TAB>    --> nothing

So populating WORD_BREAKS more fully would tend to break completion
of names using the added characters.  But probably the answer for
that is to have less ad-hoc handling of quoted names.  (See also
my screed at [1].)  Anyway, optimizing for weird quoted names is
probably not what we want to do here.

I feel like we should populate WORD_BREAKS more fully and document
the reasons for omissions, rather than leave future hackers to
guess about it.

> OTOH, this seemed a pretty fundamental change to the 12-year-old (!!)
> code so I don't know if it may be too risky and/or could adversely
> affect something else?

It's a bit scary, and I wouldn't consider back-patching it, but
TBH tab-complete.c is all chewing gum and baling wire anyway.
What I'd *really* like to do is nuke the whole thing and build
something that's mechanically derived from the actual backend
grammar.  But I don't know how to make that happen.  In the
meantime, incrementally moving it towards the real SQL parsing
rules seems like it ought to be an improvement.

> The tests are all still passing, but there aren't so many tab-complete
> tests anyway so that might not mean much.

Yeah, we certainly haven't got coverage for these sorts of details.

            regards, tom lane

[1] https://www.postgresql.org/message-id/3547066.1642272686%40sss.pgh.pa.us