Re: Some bugs in psql_complete of psql - Mailing list pgsql-hackers

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