Thread: Some bugs in psql_complete of psql
Hello, I found that a typo(?) in tab-complete.c. > /* ALTER TABLE,INDEX,MATERIALIZED VIEW xxx ALL IN TABLESPACE xxx OWNED BY */ > else if (pg_strcasecmp(prev6_wd, "ALL") == 0 && > pg_strcasecmp(prev5_wd, "IN") == 0 && > pg_strcasecmp(prev4_wd, "TABLESPACE") == 0 && > pg_strcasecmp(prev2_wd, "OWNED") == 0 && > pg_strcasecmp(prev4_wd, "BY") == 0) "BY" is compared against the word in wrong position and it prevents this completion from matching. I also found some other bugs in psql-completion. The attached patch applied on master and fixes them all togher. - Fix completion for ALL IN TABLESPACE OWNED BY. - Fix the assumed syntax of CREATE INDEX where the CONCURRENTLY is misplaced. (It is assuming the order "CREATE INDEX sthCONCURRENTLY") - Provide missing terminating NULL to the preposition list for SECURITY LABEL. - Add the preposition list with some missing items. However, this would not be a kind of bug in constrast to the three itemsabove, though. This applied back to 9.3 and 9.2 doesn't have "EVENT TRIGGER" and "MATERIALIZED VIEW" and 9.1 additionallydoesn't have "DATABASE" and "ROLE". I'll provide individual patches if necessary. regards, -- Kyotaro Horiguchi NTT Open Source Software Center
On Wed, Nov 4, 2015 at 5:27 PM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> Hello, I found that a typo(?) in tab-complete.c.
>
>> /* ALTER TABLE,INDEX,MATERIALIZED VIEW xxx ALL IN TABLESPACE xxx OWNED BY */
>> else if (pg_strcasecmp(prev6_wd, "ALL") == 0 &&
>> pg_strcasecmp(prev5_wd, "IN") == 0 &&
>> pg_strcasecmp(prev4_wd, "TABLESPACE") == 0 &&
>> pg_strcasecmp(prev2_wd, "OWNED") == 0 &&
>> pg_strcasecmp(prev4_wd, "BY") == 0)
>
> "BY" is compared against the word in wrong position and it
> prevents this completion from matching.
>
> I also found some other bugs in psql-completion. The attached
> patch applied on master and fixes them all togher.
+ /* If we have INDEX CONCURRENTLY <sth>, then add exiting indexes */
+ else if (pg_strcasecmp(prev2_wd, "INDEX") == 0 &&
+ pg_strcasecmp(prev_wd, "CONCURRENTLY") == 0)
+ COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_indexes, NULL);
Is this for DROP INDEX CONCURRENTLY case?
If yes, we should check that prev3_wd is "DROP".
+ /* If we have CREATE|UNIQUE INDEX [CONCURRENTLY] <sth>, then add "ON" */
+ else if (((pg_strcasecmp(prev3_wd, "CREATE") == 0 ||
+ pg_strcasecmp(prev3_wd, "UNIQUE") == 0) &&
+ pg_strcasecmp(prev2_wd, "INDEX") == 0 &&
+ pg_strcasecmp(prev_wd, "CONCURRENTLY") != 0) ||
The "!= 0" in the above last condition should be "== 0" ?
+ {"TABLE", "COLUMN", "AGGREGATE", "DATABASE", "DOMAIN",
+ "EVENT TRIGGER", "FOREIGN TABLE", "FUNCTION", "LARGE OBJECT",
+ "MATERIALIZED VIEW", "LANGUAGE", "ROLE", "SCHEMA",
+ "SEQUENCE", "TYPE", "VIEW", NULL};
TABLESPACE also should be added to the list?
Regards,
--
Fujii Masao
Hello, thank you for the comments.
The revised version of this patch is attached.
- Prevent complete with ON to the sequence "[CREATE] [UNIQUE] INDEX ON".- Added TABLESPACE to the preposition list for
SECURITYLABEL.
I think that the current completion mechanism is simple, fast and
maybe enough but lacks of flexibility for optional items and
requires extra attention for false match.
At Fri, 6 Nov 2015 00:26:44 +0900, Fujii Masao <masao.fujii@gmail.com> wrote in
<CAHGQGwHyyD2RTuaTSry6-Xu0Mjr4Vneifknn2jdgp43g+yjV8Q@mail.gmail.com>
> On Wed, Nov 4, 2015 at 5:27 PM, Kyotaro HORIGUCHI
> <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> > Hello, I found that a typo(?) in tab-complete.c.
> >
> >> /* ALTER TABLE,INDEX,MATERIALIZED VIEW xxx ALL IN TABLESPACE xxx OWNED BY */
> >> else if (pg_strcasecmp(prev6_wd, "ALL") == 0 &&
> >> pg_strcasecmp(prev5_wd, "IN") == 0 &&
> >> pg_strcasecmp(prev4_wd, "TABLESPACE") == 0 &&
> >> pg_strcasecmp(prev2_wd, "OWNED") == 0 &&
> >> pg_strcasecmp(prev4_wd, "BY") == 0)
> >
> > "BY" is compared against the word in wrong position and it
> > prevents this completion from matching.
> >
> > I also found some other bugs in psql-completion. The attached
> > patch applied on master and fixes them all togher.
>
> + /* If we have INDEX CONCURRENTLY <sth>, then add exiting indexes */
> + else if (pg_strcasecmp(prev2_wd, "INDEX") == 0 &&
> + pg_strcasecmp(prev_wd, "CONCURRENTLY") == 0)
> + COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_indexes, NULL);
>
> Is this for DROP INDEX CONCURRENTLY case?
> If yes, we should check that prev3_wd is "DROP".
No. Both for CREATE/DROP, their syntax is as follows ingoring
optional "IF [NOT] EXITS" and "UNIQUE". "ALTER INDEX" doesn't
take CONCURRENTLY.
DROP INDEX [CONCURRENTLY] name ...CREATE INDEX [CONCURRENTLY] name ...
> + /* If we have CREATE|UNIQUE INDEX [CONCURRENTLY] <sth>, then add "ON" */
> + else if (((pg_strcasecmp(prev3_wd, "CREATE") == 0 ||
> + pg_strcasecmp(prev3_wd, "UNIQUE") == 0) &&
> + pg_strcasecmp(prev2_wd, "INDEX") == 0 &&
> + pg_strcasecmp(prev_wd, "CONCURRENTLY") != 0) ||
>
> The "!= 0" in the above last condition should be "== 0" ?
No. It intends to match the case where prev_wd is not
CONCURRENTLY but some name for the index to be created. As
writing this answer, I noticed that the index name is
optional. For such case, this completion rule completes with ON
after "INDEX ON". It should be fixed as the following.
> + else if (((pg_strcasecmp(prev3_wd, "CREATE") == 0 ||
> + pg_strcasecmp(prev3_wd, "UNIQUE") == 0) &&
> + pg_strcasecmp(prev2_wd, "INDEX") == 0 &&
> + pg_strcasecmp(prev_wd, "CONCURRENTLY") != 0 &&
> + pg_strcasecmp(prev_wd, "ON") != 0) ||
The "CONCURRENTLY" case is matched by the condition after the
'||' at the tail in the codelet cited just above..
+ ((pg_strcasecmp(prev4_wd, "CREATE") == 0 ||
+ pg_strcasecmp(prev4_wd, "UNIQUE") == 0) &&
+ pg_strcasecmp(prev3_wd, "INDEX") == 0 &&
+ pg_strcasecmp(prev2_wd, "CONCURRENTLY") == 0))
> + {"TABLE", "COLUMN", "AGGREGATE", "DATABASE", "DOMAIN",
> + "EVENT TRIGGER", "FOREIGN TABLE", "FUNCTION", "LARGE OBJECT",
> + "MATERIALIZED VIEW", "LANGUAGE", "ROLE", "SCHEMA",
> + "SEQUENCE", "TYPE", "VIEW", NULL};
>
> TABLESPACE also should be added to the list?
Mmm... I don't understand what the patch attached to my first
mail is.. Yes, the list is missing TABLESPACE which exists in my
branch. It is surely contained in the revised patch.
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
On Fri, Nov 6, 2015 at 11:27 AM, Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote: > Hello, thank you for the comments. > > The revised version of this patch is attached. Thanks for updating the patch! I tested whether the following patterns work as expected or not. CREATE UNIQUE INDEX CONCURRENTLY name ON CREATE UNIQUE INDEX CONCURRENTLY ON CREATE UNIQUE INDEX name ON CREATE UNIQUE INDEX ON CREATE INDEX CONCURRENTLY name ON CREATE INDEX CONCURRENTLY ON CREATE INDEX name ON CREATE INDEX ON Then I found the following problems. "CREATE UNIQUE INDEX CONCURRENTLY <tab>" didn't suggest "ON". "CREATE UNIQUE INDEX ON <tab>" suggested nothing. "CREATE INDEX CONCURRENTLY <tab>" didn't suggest "ON". "CREATE INDEX ON <tab>" suggested nothing. BTW, I found that tab-completion for DROP INDEX has the following problems. "DROP INDEX <tab>" didn't suggest "CONCURRENTLY". "DROP INDEX CONCURRENTLY name <tab>" suggested nothing. Regards, -- Fujii Masao
Hello, thank you for reviewing. # I injured at fingertip, It's quite nuisance and make me more # slower to type in.. I posted another patch to totally refactor psql-complete so I don't put revised patch of this for now but this discussion has an importance for me so please continue to discuss on this a bit more with me. At Fri, 4 Dec 2015 22:39:08 +0900, Fujii Masao <masao.fujii@gmail.com> wrote in <CAHGQGwGB91SNyZWnLGEytRBx1Kbi0W2PO3NnH5QG0C2g6MmbdA@mail.gmail.com> > On Fri, Nov 6, 2015 at 11:27 AM, Kyotaro HORIGUCHI > <horiguchi.kyotaro@lab.ntt.co.jp> wrote: > > Hello, thank you for the comments. > > > > The revised version of this patch is attached. > > Thanks for updating the patch! > > I tested whether the following patterns work as expected or not. > > CREATE UNIQUE INDEX CONCURRENTLY name ON > CREATE UNIQUE INDEX CONCURRENTLY ON > CREATE UNIQUE INDEX name ON > CREATE UNIQUE INDEX ON > CREATE INDEX CONCURRENTLY name ON > CREATE INDEX CONCURRENTLY ON > CREATE INDEX name ON > CREATE INDEX ON > > Then I found the following problems. > > "CREATE UNIQUE INDEX CONCURRENTLY <tab>" didn't suggest "ON". This worked fine for me.. > "CREATE UNIQUE INDEX ON <tab>" suggested nothing. > "CREATE INDEX ON <tab>" suggested nothing. Oops... The comment for the entry says (tab-complete.c:2355 @ master). | * Complete INDEX <name> ON <table> with a list of table columns (which | * should really be in parens) But it was *actually* a completion for "INDEX [CONCURRENTLY] ON name" or "INDEX name ON" in doubtful way. I broke it by fixing it so as to fit the comment. I saw the same kind of doubtfulness at some place and this is one of the nuisance of the current implement. I proposed to use regex-like minilanguage in another thread. This enables write matching description by almost the same format as what currently written as comments. What do you think about this? (I'll post this later.) http://www.postgresql.org/message-id/20151126.144512.10228250.horiguchi.kyotaro@lab.ntt.co.jp > "CREATE INDEX CONCURRENTLY <tab>" didn't suggest "ON". This is not suggested before this. > BTW, I found that tab-completion for DROP INDEX has the following problems. > > "DROP INDEX <tab>" didn't suggest "CONCURRENTLY". > "DROP INDEX CONCURRENTLY name <tab>" suggested nothing. Yeah, these are the same behavior with the current. We shall find many instances of this kind of incompleteness of completion in the current implelent. But the current way prohibits us from enrich the completion in this direction, I believe. regards, -- Kyotaro Horiguchi NTT Open Source Software Center
Hello, I returned to this since Thomas' psql-completion patch has
been committed.
This patch has been recreated on it.
"IN TABLESPACE xxx OWNED BY" has been alredy fixed so I removed it.
The attched files are the following,
1. 0001-Fix-tab-complete-of-CREATE-INDEX.patch
Fixes completion for CREATE INDEX in ordinary way.
2. 0001-Fix-completion-for-CREATE-INDEX-type-2.patch
An alternative for 1. Introduces multilevel matching. This effectively can avoid false matching, simplify each
expressionand reduce the number of matching operations.
3. 0002-Fix-tab-completion-for-DROP-INDEX.patch
Fix of DROP INDEX completion in the type-2 way.
=====
At Tue, 08 Dec 2015 17:56:19 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in
<20151208.175619.245979824.horiguchi.kyotaro@lab.ntt.co.jp>
> > I tested whether the following patterns work as expected or not.
> >
> > CREATE UNIQUE INDEX CONCURRENTLY name ON
> > CREATE UNIQUE INDEX CONCURRENTLY ON
> > CREATE UNIQUE INDEX name ON
> > CREATE UNIQUE INDEX ON
> > CREATE INDEX CONCURRENTLY name ON
> > CREATE INDEX CONCURRENTLY ON
> > CREATE INDEX name ON
> > CREATE INDEX ON
> >
> > Then I found the following problems.
> >
> > "CREATE UNIQUE INDEX CONCURRENTLY <tab>" didn't suggest "ON".
> > "CREATE UNIQUE INDEX ON <tab>" suggested nothing.
> > "CREATE INDEX ON <tab>" suggested nothing.
All of the cases above are completed correctly. As a new feature,
this patch allows "IF NOT EXISTS". As the result, the following
part of the syntax of CREATE INDEX is completed correctly.
CREATE [UNIQUE] INDEX [CONCURRENTLY] [[IF NOT EXISTS] iname] ON tname
> > "CREATE INDEX CONCURRENTLY <tab>" didn't suggest "ON".
Now it suggests "ON", "IF NOT EXISTS" and existing index names.
> > BTW, I found that tab-completion for DROP INDEX has the following problems.
> >
> > "DROP INDEX <tab>" didn't suggest "CONCURRENTLY".
> > "DROP INDEX CONCURRENTLY name <tab>" suggested nothing.
They are completed by the "things" completion. The first patch
gives a wrong suggestion for DROP INDEX CONCURRENTLY with "IF NOT
EXISTS". This can be avoided adding HeadMatches to every matching
for "CREATE INDEX..." syntexes but it is too bothersome.
Instaed, in the second alternative patch, I enclosed the whole
"CREATE INDEX" stuff by an if(HeadMatches2("CREATE",
"UNIQUE|INDEX")) block to avoid the false match and simplify each
matching expression. As a side effect, the total number of the
executions of matching expressions can be reduced. (This wouldn't
be just a bug fix, but rarther a kind of refactoring.)
The third patch fixes the behavior for DROP INDEX in this way.
Thoughs? Suggestions?
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
On 12/22/15 4:44 AM, Kyotaro HORIGUCHI wrote: > 1. 0001-Fix-tab-complete-of-CREATE-INDEX.patch > > Fixes completion for CREATE INDEX in ordinary way. This part has been fixed in another thread. Please check whether that satisfies all your issues. > 3. 0002-Fix-tab-completion-for-DROP-INDEX.patch > > Fix of DROP INDEX completion in the type-2 way. I agree that we could use completion support for DROP INDEX CONCURRENTLY, but I would rather not throw IF NOT EXISTS into the same patch. We don't have support for IF NOT EXISTS anywhere else. If you think about, it's rather unnecessary, because tab completion will determine for you whether an object exists.
On 1/12/16 9:46 PM, Peter Eisentraut wrote: > On 12/22/15 4:44 AM, Kyotaro HORIGUCHI wrote: >> 1. 0001-Fix-tab-complete-of-CREATE-INDEX.patch >> >> Fixes completion for CREATE INDEX in ordinary way. > > This part has been fixed in another thread. Please check whether that > satisfies all your issues. > >> 3. 0002-Fix-tab-completion-for-DROP-INDEX.patch >> >> Fix of DROP INDEX completion in the type-2 way. > > I agree that we could use completion support for DROP INDEX > CONCURRENTLY, but I would rather not throw IF NOT EXISTS into the same > patch. We don't have support for IF NOT EXISTS anywhere else. If you > think about, it's rather unnecessary, because tab completion will > determine for you whether an object exists. I have applied a reduced version of the DROP INDEX patch. I think that covers everything in your submission, but please check.
Hello, thank you for committing this. At Sat, 16 Jan 2016 21:09:26 -0500, Peter Eisentraut <peter_e@gmx.net> wrote in <569AF7D6.9090107@gmx.net> > On 1/12/16 9:46 PM, Peter Eisentraut wrote: > > On 12/22/15 4:44 AM, Kyotaro HORIGUCHI wrote: > >> 1. 0001-Fix-tab-complete-of-CREATE-INDEX.patch > >> > >> Fixes completion for CREATE INDEX in ordinary way. > > > > This part has been fixed in another thread. Please check whether that > > satisfies all your issues. > > > >> 3. 0002-Fix-tab-completion-for-DROP-INDEX.patch > >> > >> Fix of DROP INDEX completion in the type-2 way. > > > > I agree that we could use completion support for DROP INDEX > > CONCURRENTLY, but I would rather not throw IF NOT EXISTS into the same > > patch. We don't have support for IF NOT EXISTS anywhere else. If you > > think about, it's rather unnecessary, because tab completion will > > determine for you whether an object exists. > > I have applied a reduced version of the DROP INDEX patch. I think that > covers everything in your submission, but please check. I examined the commit 4189e3d659abb48d159a6c3faabaa7e99498ca3e and it looks fine. I'll post another patch for IF (NOT) EXISTS for all possible part later. Thank you Peter. regards, -- Kyotaro Horiguchi NTT Open Source Software Center