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.fu...@gmail.com> wrote in
<cahgqgwhyyd2rtuatsry6-xu0mjr4vneifknn2jdgp43g+yj...@mail.gmail.com>
> On Wed, Nov 4, 2015 at 5:27 PM, Kyotaro HORIGUCHI
> <horiguchi.kyot...@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
>From 7b42a01556ef2ea2291e1d4748a9bf0f80046069 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyot...@lab.ntt.co.jp>
Date: Mon, 2 Nov 2015 14:10:53 +0900
Subject: [PATCH] Fix some tab-completino bugs.
- 'ALTER [TABLE|xx] xxx ALL IN TABLE SPACE xx OWNED BY' cannot be completed.
- CONCURRENTLY in CREATE INDEX is misplaced.
- Preposition list of SECURITY LABEL is not terminated.
- The target list of SECURITY LABEL ON misses some alternatives.
---
src/bin/psql/tab-complete.c | 55 +++++++++++++++++++++++----------------------
1 file changed, 28 insertions(+), 27 deletions(-)
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 0e8d395..5a3930f 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1040,7 +1040,7 @@ psql_completion(const char *text, int start, int end)
pg_strcasecmp(prev5_wd, "IN") == 0 &&
pg_strcasecmp(prev4_wd, "TABLESPACE") == 0 &&
pg_strcasecmp(prev2_wd, "OWNED") == 0 &&
- pg_strcasecmp(prev4_wd, "BY") == 0)
+ pg_strcasecmp(prev_wd, "BY") == 0)
{
COMPLETE_WITH_QUERY(Query_for_list_of_roles);
}
@@ -2320,35 +2320,35 @@ psql_completion(const char *text, int start, int end)
COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_indexes,
" UNION SELECT 'ON'"
" UNION SELECT 'CONCURRENTLY'");
+ /* 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);
/* Complete ... INDEX [<name>] ON with a list of tables */
else if ((pg_strcasecmp(prev3_wd, "INDEX") == 0 ||
- pg_strcasecmp(prev2_wd, "INDEX") == 0 ||
- pg_strcasecmp(prev2_wd, "CONCURRENTLY") == 0) &&
+ (pg_strcasecmp(prev4_wd, "INDEX") == 0 &&
+ pg_strcasecmp(prev3_wd, "CONCURRENTLY") == 0)) &&
pg_strcasecmp(prev_wd, "ON") == 0)
COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tm, NULL);
- /* If we have CREATE|UNIQUE INDEX <sth> CONCURRENTLY, then add "ON" */
- else if ((pg_strcasecmp(prev3_wd, "INDEX") == 0 ||
- pg_strcasecmp(prev2_wd, "INDEX") == 0) &&
- pg_strcasecmp(prev_wd, "CONCURRENTLY") == 0)
+ /* 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 &&
+ pg_strcasecmp(prev_wd, "ON") != 0) ||
+ ((pg_strcasecmp(prev4_wd, "CREATE") == 0 ||
+ pg_strcasecmp(prev4_wd, "UNIQUE") == 0) &&
+ pg_strcasecmp(prev3_wd, "INDEX") == 0 &&
+ pg_strcasecmp(prev2_wd, "CONCURRENTLY") == 0))
COMPLETE_WITH_CONST("ON");
- /* If we have CREATE|UNIQUE INDEX <sth>, then add "ON" or "CONCURRENTLY" */
- else if ((pg_strcasecmp(prev3_wd, "CREATE") == 0 ||
- pg_strcasecmp(prev3_wd, "UNIQUE") == 0) &&
- pg_strcasecmp(prev2_wd, "INDEX") == 0)
- {
- static const char *const list_CREATE_INDEX[] =
- {"CONCURRENTLY", "ON", NULL};
-
- COMPLETE_WITH_LIST(list_CREATE_INDEX);
- }
/*
- * Complete INDEX <name> ON <table> with a list of table columns (which
- * should really be in parens)
+ * Complete INDEX [CONCURRENTLY] <name> ON <table> with a list of table
+ * columns (which should really be in parens)
*/
else if ((pg_strcasecmp(prev4_wd, "INDEX") == 0 ||
- pg_strcasecmp(prev3_wd, "INDEX") == 0 ||
- pg_strcasecmp(prev3_wd, "CONCURRENTLY") == 0) &&
+ (pg_strcasecmp(prev5_wd, "INDEX") == 0 &&
+ pg_strcasecmp(prev4_wd, "CONCURRENTLY") == 0)) &&
pg_strcasecmp(prev2_wd, "ON") == 0)
{
static const char *const list_CREATE_INDEX2[] =
@@ -2357,8 +2357,8 @@ psql_completion(const char *text, int start, int end)
COMPLETE_WITH_LIST(list_CREATE_INDEX2);
}
else if ((pg_strcasecmp(prev5_wd, "INDEX") == 0 ||
- pg_strcasecmp(prev4_wd, "INDEX") == 0 ||
- pg_strcasecmp(prev4_wd, "CONCURRENTLY") == 0) &&
+ (pg_strcasecmp(prev6_wd, "INDEX") == 0 &&
+ pg_strcasecmp(prev5_wd, "CONCURRENTLY") == 0)) &&
pg_strcasecmp(prev3_wd, "ON") == 0 &&
pg_strcasecmp(prev_wd, "(") == 0)
COMPLETE_WITH_ATTR(prev2_wd, "");
@@ -3555,7 +3555,7 @@ psql_completion(const char *text, int start, int end)
pg_strcasecmp(prev_wd, "LABEL") == 0)
{
static const char *const list_SECURITY_LABEL_preposition[] =
- {"ON", "FOR"};
+ {"ON", "FOR", NULL};
COMPLETE_WITH_LIST(list_SECURITY_LABEL_preposition);
}
@@ -3572,9 +3572,10 @@ psql_completion(const char *text, int start, int end)
pg_strcasecmp(prev_wd, "ON") == 0))
{
static const char *const list_SECURITY_LABEL[] =
- {"LANGUAGE", "SCHEMA", "SEQUENCE", "TABLE", "TYPE", "VIEW",
- "MATERIALIZED VIEW", "COLUMN", "AGGREGATE", "FUNCTION", "DOMAIN",
- "LARGE OBJECT", NULL};
+ {"TABLE", "COLUMN", "AGGREGATE", "DATABASE", "DOMAIN",
+ "EVENT TRIGGER", "FOREIGN TABLE", "FUNCTION", "LARGE OBJECT",
+ "MATERIALIZED VIEW", "LANGUAGE", "ROLE", "SCHEMA",
+ "SEQUENCE", "TABLESPACE", "TYPE", "VIEW", NULL};
COMPLETE_WITH_LIST(list_SECURITY_LABEL);
}
--
1.8.3.1
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
