Re: Some bugs in psql_complete of psql

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 SECURITY LABEL.

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(dot)fujii(at)gmail(dot)com> wrote in <CAHGQGwHyyD2RTuaTSry6-Xu0Mjr4Vneifknn2jdgp43g+yjV8Q(at)mail(dot)gmail(dot)com>
> On Wed, Nov 4, 2015 at 5:27 PM, Kyotaro HORIGUCHI
> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)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