Tab completion for ALTER COLUMN SET STATISTICS

Started by Jeff Janesover 10 years ago7 messages

jeff.janes@gmail.com

over 10 years ago

1 attachment(s)

If I have "alter table foo alter COLUMN bar SET STATISTICS" in the line
buffer,
it tab completes to add " TO", which is not legal.

The attached patch makes it not tab complete anything at all, which is at
least not actively misleading.

I thought of having it complete "-1", "<integer>" so that it gives a clue
about what is needed, but I didn't see any precedence for non-literal
clue-giving and I did not want to try to create new precedence.

Attachments:

alter_column_tabcomplete_v1.patchapplication/octet-stream; name=alter_column_tabcomplete_v1.patchDownload

diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
new file mode 100644
index 0cb3464..98ce351
*** a/src/bin/psql/tab-complete.c
--- b/src/bin/psql/tab-complete.c
*************** psql_completion(const char *text, int st
*** 1758,1763 ****
--- 1758,1776 ----
  
  		COMPLETE_WITH_LIST(list_COLUMNSTORAGE);
  	}
+ 	/* ALTER TABLE ALTER [COLUMN] <foo> SET STATISICS */
+ 	else if (((pg_strcasecmp(prev5_wd, "ALTER") == 0 &&
+ 			   pg_strcasecmp(prev4_wd, "COLUMN") == 0) ||
+ 			  pg_strcasecmp(prev4_wd, "ALTER") == 0) &&
+ 			 pg_strcasecmp(prev2_wd, "SET") == 0 &&
+ 			 pg_strcasecmp(prev_wd, "STATISTICS") == 0)
+ 	{
+ 		/* Prevent suggestion of TO */
+ 		static const char *const list_COLUMNSTORAGE[] =
+ 		{NULL};
+ 
+ 		COMPLETE_WITH_LIST(list_COLUMNSTORAGE);
+ 	}
  	/* ALTER TABLE ALTER [COLUMN] <foo> DROP */
  	else if (((pg_strcasecmp(prev4_wd, "ALTER") == 0 &&
  			   pg_strcasecmp(prev3_wd, "COLUMN") == 0) ||

Michael Paquier

michael.paquier@gmail.com

over 10 years ago

In reply to: Jeff Janes (#1)

Re: Tab completion for ALTER COLUMN SET STATISTICS

On Sat, Sep 26, 2015 at 7:18 AM, Jeff Janes <jeff.janes@gmail.com> wrote:

If I have "alter table foo alter COLUMN bar SET STATISTICS" in the line
buffer,
it tab completes to add " TO", which is not legal.

The attached patch makes it not tab complete anything at all, which is at
least not actively misleading.
I thought of having it complete "-1", "<integer>" so that it gives a clue
about what is needed, but I didn't see any precedence for non-literal
clue-giving and I did not want to try to create new precedence.

+1 for the way you are doing it in your patch.
-- 
Michael

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

Robert Haas

robertmhaas@gmail.com

over 10 years ago

In reply to: Michael Paquier (#2)

Re: Tab completion for ALTER COLUMN SET STATISTICS

On Sat, Sep 26, 2015 at 7:24 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Sat, Sep 26, 2015 at 7:18 AM, Jeff Janes <jeff.janes@gmail.com> wrote:

If I have "alter table foo alter COLUMN bar SET STATISTICS" in the line
buffer,
it tab completes to add " TO", which is not legal.

The attached patch makes it not tab complete anything at all, which is at
least not actively misleading.
I thought of having it complete "-1", "<integer>" so that it gives a clue
about what is needed, but I didn't see any precedence for non-literal
clue-giving and I did not want to try to create new precedence.

+1 for the way you are doing it in your patch.

Before we take that approach, can I back up and ask whether we
shouldn't instead narrow the rule that's inserting TO? I think that
completion is coming from here:

else if (pg_strcasecmp(prev2_wd, "SET") == 0 &&
pg_strcasecmp(prev4_wd, "UPDATE") != 0 &&
pg_strcasecmp(prev_wd, "TABLESPACE") != 0 &&
pg_strcasecmp(prev_wd, "SCHEMA") != 0 &&
prev_wd[strlen(prev_wd) - 1] != ')' &&
prev_wd[strlen(prev_wd) - 1] != '=' &&
pg_strcasecmp(prev4_wd, "DOMAIN") != 0)
COMPLETE_WITH_CONST("TO");

Now, that is basically an incredibly broad production: every time the
second-most recent word is SET, complete with TO. It looks to me like
this has already been patched around multiple times to make it
slightly narrower. Those exceptions were added by three different
commits, in 2004, 2010, and 2012.

Maybe it's time to back up and make the whole thing a lot narrower.
Like, if second-most-recent word of the command is also the FIRST word
of the command, this is probably right. And there may be a few other
situations where it's right. But in general, SET is used in lots of
places and the fact that we've seen one recently isn't enough to
decide in TO.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

Jeff Janes

jeff.janes@gmail.com

about 10 years ago

In reply to: Robert Haas (#3)

Re: Tab completion for ALTER COLUMN SET STATISTICS

On Mon, Sep 28, 2015 at 8:48 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Sat, Sep 26, 2015 at 7:24 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Sat, Sep 26, 2015 at 7:18 AM, Jeff Janes <jeff.janes@gmail.com> wrote:

If I have "alter table foo alter COLUMN bar SET STATISTICS" in the line
buffer,
it tab completes to add " TO", which is not legal.

The attached patch makes it not tab complete anything at all, which is at
least not actively misleading.
I thought of having it complete "-1", "<integer>" so that it gives a clue
about what is needed, but I didn't see any precedence for non-literal
clue-giving and I did not want to try to create new precedence.

+1 for the way you are doing it in your patch.

Before we take that approach, can I back up and ask whether we
shouldn't instead narrow the rule that's inserting TO? I think that
completion is coming from here:

else if (pg_strcasecmp(prev2_wd, "SET") == 0 &&
pg_strcasecmp(prev4_wd, "UPDATE") != 0 &&
pg_strcasecmp(prev_wd, "TABLESPACE") != 0 &&
pg_strcasecmp(prev_wd, "SCHEMA") != 0 &&
prev_wd[strlen(prev_wd) - 1] != ')' &&
prev_wd[strlen(prev_wd) - 1] != '=' &&
pg_strcasecmp(prev4_wd, "DOMAIN") != 0)
COMPLETE_WITH_CONST("TO");

Now, that is basically an incredibly broad production: every time the
second-most recent word is SET, complete with TO. It looks to me like
this has already been patched around multiple times to make it
slightly narrower. Those exceptions were added by three different
commits, in 2004, 2010, and 2012.

Maybe it's time to back up and make the whole thing a lot narrower.
Like, if second-most-recent word of the command is also the FIRST word
of the command, this is probably right. And there may be a few other
situations where it's right. But in general, SET is used in lots of
places and the fact that we've seen one recently isn't enough to
decide in TO.

There are quite a few places where TO is legitimately the 2nd word
after SET. I don't know how to do an exhaustive survey of them, but
here are the ones I know about:

SET <word> TO
ALTER DATABASE <word> SET <word> TO
ALTER ROLE <word> SET <word> TO
ALTER USER <word> SET <word> TO
ALTER FUNCTION <word> ( <any number of words with commas between> )
SET <word other than 'schema'> TO

I don't know if coding the non-TO cases as exceptions is easier or
harder than coding the TO cases more tightly.

In the case of "SET SCHEMA", it seems like we might be able to just
move that case higher in the giant list of 'else if' so that it
triggers before getting to the generic SET <word> code. But I can't
figure out where in the file that code is, to see if it might be
moved. And I'm not sure that intentionally relying on order more than
already is a better strategy, it seems kind of fragile.

In any case, I'd rather not try re-factor this part of the code while
there is another large refactoring pending. But I think an
incremental improvement would be acceptable.

Cheers,

Jeff

--
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: Jeff Janes (#4)

Re: Tab completion for ALTER COLUMN SET STATISTICS

On Wed, Nov 18, 2015 at 2:12 AM, Jeff Janes <jeff.janes@gmail.com> wrote:

On Mon, Sep 28, 2015 at 8:48 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Sat, Sep 26, 2015 at 7:24 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Sat, Sep 26, 2015 at 7:18 AM, Jeff Janes <jeff.janes@gmail.com> wrote:

If I have "alter table foo alter COLUMN bar SET STATISTICS" in the line
buffer,
it tab completes to add " TO", which is not legal.

The attached patch makes it not tab complete anything at all, which is at
least not actively misleading.
I thought of having it complete "-1", "<integer>" so that it gives a clue
about what is needed, but I didn't see any precedence for non-literal
clue-giving and I did not want to try to create new precedence.

+1 for the way you are doing it in your patch.

Before we take that approach, can I back up and ask whether we
shouldn't instead narrow the rule that's inserting TO? I think that
completion is coming from here:

else if (pg_strcasecmp(prev2_wd, "SET") == 0 &&
pg_strcasecmp(prev4_wd, "UPDATE") != 0 &&
pg_strcasecmp(prev_wd, "TABLESPACE") != 0 &&
pg_strcasecmp(prev_wd, "SCHEMA") != 0 &&
prev_wd[strlen(prev_wd) - 1] != ')' &&
prev_wd[strlen(prev_wd) - 1] != '=' &&
pg_strcasecmp(prev4_wd, "DOMAIN") != 0)
COMPLETE_WITH_CONST("TO");

Now, that is basically an incredibly broad production: every time the
second-most recent word is SET, complete with TO. It looks to me like
this has already been patched around multiple times to make it
slightly narrower. Those exceptions were added by three different
commits, in 2004, 2010, and 2012.

Maybe it's time to back up and make the whole thing a lot narrower.
Like, if second-most-recent word of the command is also the FIRST word
of the command, this is probably right. And there may be a few other
situations where it's right. But in general, SET is used in lots of
places and the fact that we've seen one recently isn't enough to
decide in TO.

There are quite a few places where TO is legitimately the 2nd word
after SET. I don't know how to do an exhaustive survey of them, but
here are the ones I know about:

SET <word> TO
ALTER DATABASE <word> SET <word> TO
ALTER ROLE <word> SET <word> TO
ALTER USER <word> SET <word> TO
ALTER FUNCTION <word> ( <any number of words with commas between> )
SET <word other than 'schema'> TO

I don't know if coding the non-TO cases as exceptions is easier or
harder than coding the TO cases more tightly.

In the case of "SET SCHEMA", it seems like we might be able to just
move that case higher in the giant list of 'else if' so that it
triggers before getting to the generic SET <word> code. But I can't
figure out where in the file that code is, to see if it might be
moved. And I'm not sure that intentionally relying on order more than
already is a better strategy, it seems kind of fragile.

In any case, I'd rather not try re-factor this part of the code while
there is another large refactoring pending. But I think an
incremental improvement would be acceptable.

With the refactoring of tab-complete.c that is moving on, perhaps this
entry should be moved to next CF. Thoughts?
--
Michael

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

Jeff Janes

jeff.janes@gmail.com

about 10 years ago

In reply to: Michael Paquier (#5)

Re: Tab completion for ALTER COLUMN SET STATISTICS

On Tue, Dec 1, 2015 at 10:37 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

With the refactoring of tab-complete.c that is moving on, perhaps this
entry should be moved to next CF. Thoughts?

The now-committed tab-complete.c refactoring renders this unnecessary,
so I've marked this patch as rejected.

Thanks,

Jeff

--
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: Jeff Janes (#6)

Re: Tab completion for ALTER COLUMN SET STATISTICS

On Mon, Dec 21, 2015 at 3:34 PM, Jeff Janes <jeff.janes@gmail.com> wrote:

On Tue, Dec 1, 2015 at 10:37 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

With the refactoring of tab-complete.c that is moving on, perhaps this
entry should be moved to next CF. Thoughts?

The now-committed tab-complete.c refactoring renders this unnecessary,
so I've marked this patch as rejected.

Thanks for the follow-up.
--
Michael

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