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]/messages/by-id/3547066.1642272686@sss.pgh.pa.us.) 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