strange CREATE INDEX tab completion cases

Started by Peter Eisentrautabout 10 years ago6 messages

peter_e@gmx.net

about 10 years ago

These two tab completion pieces look strange to me:

/* 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)
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);
}

They appear to support a syntax along the lines of

CREATE INDEX name CONCURRENTLY

which is not the actual syntax.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Michael Paquier

michael.paquier@gmail.com

about 10 years ago

In reply to: Peter Eisentraut (#1)

1 attachment(s)

Re: strange CREATE INDEX tab completion cases

On Sat, Dec 12, 2015 at 11:17 AM, Peter Eisentraut <peter_e@gmx.net> wrote:

These two tab completion pieces look strange to me:

/* 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)
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);
}

They appear to support a syntax along the lines of

CREATE INDEX name CONCURRENTLY

which is not the actual syntax.

Yep. That's visibly a bug introduced by this commit:
commit: 37ec19a15ce452ee94f32ebc3d6a9a45868e82fd
author: Itagaki Takahiro <itagaki.takahiro@gmail.com>
date: Wed, 17 Feb 2010 04:09:40 +0000
Support new syntax and improve handling of parentheses in psql tab-completion.

The current implementation is missing a correct completion in a couple
of cases, among them:
-- Should be only ON
=# create index asd
CONCURRENTLY ON
-- should give list of table
=# create index CONCURRENTLY aaa on
-- Should give ON and list of existing indexes
=# create index concurrently

Please see the attached to address those things (and others) with
extra fixes for a couple of comments.
Regards,
--
Michael

Attachments:

20151213_psql_completion_index.patchbinary/octet-stream; name=20151213_psql_completion_index.patchDownload

diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 8c48881..a1bfeab 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -2329,34 +2329,44 @@ psql_completion(const char *text, int start, int end)
 	else if (pg_strcasecmp(prev2_wd, "CREATE") == 0 &&
 			 pg_strcasecmp(prev_wd, "UNIQUE") == 0)
 		COMPLETE_WITH_CONST("INDEX");
-	/* If we have CREATE|UNIQUE INDEX, then add "ON" and existing indexes */
+	/*
+	 * If we have CREATE|UNIQUE INDEX, then add "ON", "CONCURRENTLY"
+	 * and existing indexes.
+	 */
 	else if (pg_strcasecmp(prev_wd, "INDEX") == 0 &&
 			 (pg_strcasecmp(prev2_wd, "CREATE") == 0 ||
 			  pg_strcasecmp(prev2_wd, "UNIQUE") == 0))
 		COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_indexes,
 								   " UNION SELECT 'ON'"
 								   " UNION SELECT 'CONCURRENTLY'");
-	/* Complete ... INDEX [<name>] ON with a list of tables  */
+	/* Complete ... INDEX|CONCURRENTLY [<name>] ON with a list of tables  */
 	else if ((pg_strcasecmp(prev3_wd, "INDEX") == 0 ||
 			  pg_strcasecmp(prev2_wd, "INDEX") == 0 ||
+			  pg_strcasecmp(prev3_wd, "CONCURRENTLY") == 0 ||
 			  pg_strcasecmp(prev2_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" */
+	/*
+	 * If we have CREATE|UNIQUE INDEX CONCURRENTLY, then add "ON" and
+	 * existing indexes.
+	 */
 	else if ((pg_strcasecmp(prev3_wd, "INDEX") == 0 ||
 			  pg_strcasecmp(prev2_wd, "INDEX") == 0) &&
 			 pg_strcasecmp(prev_wd, "CONCURRENTLY") == 0)
-		COMPLETE_WITH_CONST("ON");
-	/* If we have CREATE|UNIQUE INDEX <sth>, then add "ON" or "CONCURRENTLY" */
+		COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_indexes,
+								   " UNION SELECT 'ON'");
+	/* If we have CREATE|UNIQUE INDEX <smth>|!CONCURRENTLY, then add "ON" */
 	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);
-	}
+			 pg_strcasecmp(prev2_wd, "INDEX") == 0 &&
+			 pg_strcasecmp(prev_wd, "CONCURRENTLY") != 0)
+		COMPLETE_WITH_CONST("ON");
+	/* If we have CREATE|UNIQUE INDEX CONCURRENTLY <smth>, then add "ON" */
+	else if ((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");
 
 	/*
 	 * Complete INDEX <name> ON <table> with a list of table columns (which

Peter Eisentraut

peter_e@gmx.net

about 10 years ago

In reply to: Michael Paquier (#2)

3 attachment(s)

Re: strange CREATE INDEX tab completion cases

On 12/13/15 9:16 AM, Michael Paquier wrote:

Please see the attached to address those things (and others) with
extra fixes for a couple of comments.

I have ported these changes to the new world order and divided them up
into more logical changes that are more clearly documented. Please
check that this matches what you had in mind.

Attachments:

0001-psql-Update-tab-completion-comment.patchapplication/x-patch; name=0001-psql-Update-tab-completion-comment.patchDownload

From bce311ab08053aa6d21b31625a1be85a33138300 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
Date: Sun, 10 Jan 2016 11:24:51 -0500
Subject: [PATCH 1/3] psql: Update tab completion comment

This just updates a comment to match the code.

from Michael Paquier
---
 src/bin/psql/tab-complete.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 4d2bee1..dcbe515 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1999,7 +1999,8 @@ psql_completion(const char *text, int start, int end)
 	/* First off we complete CREATE UNIQUE with "INDEX" */
 	else if (TailMatches2("CREATE", "UNIQUE"))
 		COMPLETE_WITH_CONST("INDEX");
-	/* If we have CREATE|UNIQUE INDEX, then add "ON" and existing indexes */
+	/* If we have CREATE|UNIQUE INDEX, then add "ON", "CONCURRENTLY",
+	   and existing indexes */
 	else if (TailMatches2("CREATE|UNIQUE", "INDEX"))
 		COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_indexes,
 								   " UNION SELECT 'ON'"
-- 
2.7.0

0002-psql-Fix-CREATE-INDEX-tab-completion.patchapplication/x-patch; name=0002-psql-Fix-CREATE-INDEX-tab-completion.patchDownload

From d61d232b429e911d73598d9615fc73397fb44bda Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
Date: Sun, 10 Jan 2016 11:43:27 -0500
Subject: [PATCH 2/3] psql: Fix CREATE INDEX tab completion

The previous code supported a syntax like CREATE INDEX name
CONCURRENTLY, which never existed.  Mistake introduced in commit
37ec19a15ce452ee94f32ebc3d6a9a45868e82fd.  Remove the addition of
CONCURRENTLY at that point.
---
 src/bin/psql/tab-complete.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index dcbe515..52336aa 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -2009,13 +2009,12 @@ psql_completion(const char *text, int start, int end)
 	else if (TailMatches3("INDEX", MatchAny, "ON") ||
 			 TailMatches2("INDEX|CONCURRENTLY", "ON"))
 		COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tm, NULL);
-	/* If we have CREATE|UNIQUE INDEX <sth> CONCURRENTLY, then add "ON" */
-	else if (TailMatches3("INDEX", MatchAny, "CONCURRENTLY") ||
-			 TailMatches2("INDEX", "CONCURRENTLY"))
+	/* If we have CREATE|UNIQUE INDEX CONCURRENTLY, then add "ON" */
+	else if (TailMatches2("INDEX", "CONCURRENTLY"))
 		COMPLETE_WITH_CONST("ON");
-	/* If we have CREATE|UNIQUE INDEX <sth>, then add "ON" or "CONCURRENTLY" */
+	/* If we have CREATE|UNIQUE INDEX <sth>, then add "ON" */
 	else if (TailMatches3("CREATE|UNIQUE", "INDEX", MatchAny))
-		COMPLETE_WITH_LIST2("CONCURRENTLY", "ON");
+		COMPLETE_WITH_CONST("ON");
 
 	/*
 	 * Complete INDEX <name> ON <table> with a list of table columns (which
-- 
2.7.0

0003-psql-Improve-CREATE-INDEX-CONCURRENTLY-tab-completio.patchapplication/x-patch; name=0003-psql-Improve-CREATE-INDEX-CONCURRENTLY-tab-completio.patchDownload

From 84725481c29ae2d3e7af0ca7b4e3b58de48c56e4 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
Date: Sun, 10 Jan 2016 12:04:28 -0500
Subject: [PATCH 3/3] psql: Improve CREATE INDEX CONCURRENTLY tab completion

The completion of CREATE INDEX CONCURRENTLY was lacking in several ways
compared to a plain CREATE INDEX command:

- CREATE INDEX <name> ON completes table names, but didn't with
  CONCURRENTLY.

- CREATE INDEX completes ON and existing index names, but with
  CONCURRENTLY it only completed ON.

- CREATE INDEX <name> completes ON, but didn't with CONCURRENTLY.

These are now all fixed.
---
 src/bin/psql/tab-complete.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 52336aa..97580b9 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -2005,15 +2005,18 @@ psql_completion(const char *text, int start, int end)
 		COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_indexes,
 								   " UNION SELECT 'ON'"
 								   " UNION SELECT 'CONCURRENTLY'");
-	/* Complete ... INDEX [<name>] ON with a list of tables  */
-	else if (TailMatches3("INDEX", MatchAny, "ON") ||
+	/* Complete ... INDEX|CONCURRENTLY [<name>] ON with a list of tables  */
+	else if (TailMatches3("INDEX|CONCURRENTLY", MatchAny, "ON") ||
 			 TailMatches2("INDEX|CONCURRENTLY", "ON"))
 		COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tm, NULL);
-	/* If we have CREATE|UNIQUE INDEX CONCURRENTLY, then add "ON" */
+	/* If we have CREATE|UNIQUE INDEX CONCURRENTLY, then add "ON" and existing
+	   indexes */
 	else if (TailMatches2("INDEX", "CONCURRENTLY"))
-		COMPLETE_WITH_CONST("ON");
-	/* If we have CREATE|UNIQUE INDEX <sth>, then add "ON" */
-	else if (TailMatches3("CREATE|UNIQUE", "INDEX", MatchAny))
+		COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_indexes,
+								   " UNION SELECT 'ON'");
+	/* If we have CREATE|UNIQUE INDEX [CONCURRENTLY] <sth>, then add "ON" */
+	else if (TailMatches3("CREATE|UNIQUE", "INDEX", MatchAny) ||
+			 TailMatches4("CREATE|UNIQUE", "INDEX", "CONCURRENTLY", MatchAny))
 		COMPLETE_WITH_CONST("ON");
 
 	/*
-- 
2.7.0

Michael Paquier

michael.paquier@gmail.com

about 10 years ago

In reply to: Peter Eisentraut (#3)

Re: strange CREATE INDEX tab completion cases

On Mon, Jan 11, 2016 at 2:16 AM, Peter Eisentraut <peter_e@gmx.net> wrote:

On 12/13/15 9:16 AM, Michael Paquier wrote:

Please see the attached to address those things (and others) with
extra fixes for a couple of comments.

I have ported these changes to the new world order and divided them up
into more logical changes that are more clearly documented. Please
check that this matches what you had in mind.

Thanks for the new patches, this fell of my radar. This new version
looks fine to me.

+       /* If we have CREATE|UNIQUE INDEX CONCURRENTLY, then add "ON"
and existing
+          indexes */
This comment format is not correct.
-- 
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Alvaro Herrera

alvherre@2ndquadrant.com

about 10 years ago

In reply to: Peter Eisentraut (#3)

Re: strange CREATE INDEX tab completion cases

Peter Eisentraut wrote:

On 12/13/15 9:16 AM, Michael Paquier wrote:

Please see the attached to address those things (and others) with
extra fixes for a couple of comments.

I have ported these changes to the new world order and divided them up
into more logical changes that are more clearly documented. Please
check that this matches what you had in mind.

One thing I just noticed is that CREATE INDEX CONCURRENTLY cannot be
used within CREATE SCHEMA, so perhaps the lines that match the
CONCURRENTLY keyword should use Matches() rather than TailMatches().
Similarly (but perhaps this is not workable) the lines that TailMatch()
but do not Match() should not offer CONCURRENTLY after INDEX.

--
ï¿½lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Tom Lane

tgl@sss.pgh.pa.us

about 10 years ago

In reply to: Alvaro Herrera (#5)

Re: strange CREATE INDEX tab completion cases

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

One thing I just noticed is that CREATE INDEX CONCURRENTLY cannot be
used within CREATE SCHEMA, so perhaps the lines that match the
CONCURRENTLY keyword should use Matches() rather than TailMatches().
Similarly (but perhaps this is not workable) the lines that TailMatch()
but do not Match() should not offer CONCURRENTLY after INDEX.

This seems overcomplicated. I don't think there's any expectation that
tab completion is 100% right all the time. Let's just treat CREATE INDEX
CONCURRENTLY the same as CREATE INDEX.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers