Re: [HACKERS] EXECUTE tab completion

2011-10-24 Thread Magnus Hagander
On Mon, Oct 24, 2011 at 01:26, Tom Lane t...@sss.pgh.pa.us wrote:
 Andreas Karlsson andr...@proxel.se writes:
 Thanks for cleaning up the code to some sanity, I should have done so
 myself when I noticed the problem.

 A new version is attached.

 Committed with minor adjustments --- I didn't see any need to make this
 wait for the next commitfest.

Thanks - I was planning to pick that one up along with my TABLE patch,
but was too busy with pgconf.eu over the past couple of weeks..

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

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


Re: [HACKERS] EXECUTE tab completion

2011-10-23 Thread Tom Lane
Andreas Karlsson andr...@proxel.se writes:
 Thanks for cleaning up the code to some sanity, I should have done so 
 myself when I noticed the problem.

 A new version is attached.

Committed with minor adjustments --- I didn't see any need to make this
wait for the next commitfest.

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


Re: [HACKERS] EXECUTE tab completion

2011-10-21 Thread Josh Kupershmidt
On Thu, Oct 20, 2011 at 5:16 PM, Andreas Karlsson andr...@proxel.se wrote:
 A new version is attached.

Looks fine. Marking ready for committer (CF 2011-11).

Josh

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


Re: [HACKERS] EXECUTE tab completion

2011-10-20 Thread Alvaro Herrera

Excerpts from Josh Kupershmidt's message of mié oct 19 23:50:58 -0300 2011:
 On Wed, Oct 19, 2011 at 10:40 PM, Tom Lane t...@sss.pgh.pa.us wrote:
  Josh Kupershmidt schmi...@gmail.com writes:
  Incidentally, I was wondering what the heck was up with a clause like this:
      else if (pg_strcasecmp(prev_wd, EXECUTE) == 0 
               pg_strcasecmp(prev2_wd, EXECUTE) == 0)
 
  Hmm, maybe || was meant not  ?  It seems pretty unlikely that the
  above test would ever trigger on valid SQL input.
 
 Well, changing '' to '||' breaks the stated comment of the patch, namely:
   /* must not match CREATE TRIGGER ... EXECUTE PROCEDURE */
 
 I assume this is an accepted quirk of previous_word() since we have
 this existing similar code:
 
 /* DROP, but watch out for DROP embedded in other commands */
 /* complete with something you can drop */
 else if (pg_strcasecmp(prev_wd, DROP) == 0 
  pg_strcasecmp(prev2_wd, DROP) == 0)

Maybe both are wrong, though the DROP case seems to work so maybe it's
just dead code.  This was introduced in commit
90725929465474648de133d216b873bdb69fe357:

***
*** 674,685  psql_completion(char *text, int start, int end)
else if (pg_strcasecmp(prev_wd, CREATE) == 0)
matches = completion_matches(text, create_command_generator);
  
! /* DROP, except ALTER (TABLE|DOMAIN|GROUP) sth DROP */
/* complete with something you can drop */
else if (pg_strcasecmp(prev_wd, DROP) == 0 
!pg_strcasecmp(prev3_wd, TABLE) != 0 
!pg_strcasecmp(prev3_wd, DOMAIN) != 0 
!pg_strcasecmp(prev3_wd, GROUP) != 0)
matches = completion_matches(text, drop_command_generator);
  
  /* ALTER */
--- 674,683 
else if (pg_strcasecmp(prev_wd, CREATE) == 0)
matches = completion_matches(text, create_command_generator);
  
! /* DROP, but watch out for DROP embedded in other commands */
/* complete with something you can drop */
else if (pg_strcasecmp(prev_wd, DROP) == 0 
!pg_strcasecmp(prev2_wd, DROP) == 0)
matches = completion_matches(text, drop_command_generator);
  
  /* ALTER */


-- 
Álvaro Herrera alvhe...@commandprompt.com
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [HACKERS] EXECUTE tab completion

2011-10-20 Thread Tom Lane
Alvaro Herrera alvhe...@commandprompt.com writes:
 Excerpts from Josh Kupershmidt's message of mié oct 19 23:50:58 -0300 2011:
 I assume this is an accepted quirk of previous_word() since we have
 this existing similar code:

 Maybe both are wrong, though the DROP case seems to work so maybe it's
 just dead code.

I looked at the code more closely and I now see what Josh saw and why
this code is like this.  If you take a desultory look at previous_word()
you will come away with the impression that it returns NULL when asked
for a word before the first word on the line.  But in reality, it
returns NULL only if there is nothing before the current point.
Otherwise, you get duplicates of the first word on the line.  For
example in DROP TABLE tab we'll get
prev_wd = TABLE
prev2_wd = DROP
prev3_wd = DROP
prev4_wd = DROP
prev5_wd = DROP
prev6_wd = DROP
I think this is just a plain old coding bug: the stop-test assumes that
end is reset to -1 each time through the outer loop, but it isn't.

However, we can't just fix the bug, because if previous_word() actually
starts to return NULLs like its author seems to have intended, the
strcasecmp calls that use its output will dump core.

What I suggest is that we redefine previous_word() as returning an empty
string, not NULL, anytime there is no such preceding word.  This is
better than the current behavior because (a) it's less surprising and
(b) it's not ambiguous.  Right now the caller can't tell the difference
between DROP and DROP DROP DROP.

With that change, the correct test at line 795 would become

else if (pg_strcasecmp(prev_wd, DROP) == 0 
 prev2_wd[0] == '\0')

(or any other way you care to write a test for empty string).  It looks
like there is only one place in the file that is actually expecting a
null result in prev_wd, so there wouldn't be much collateral damage
to fix.

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


Re: [HACKERS] EXECUTE tab completion

2011-10-20 Thread Tom Lane
I wrote:
 What I suggest is that we redefine previous_word() as returning an empty
 string, not NULL, anytime there is no such preceding word.  This is
 better than the current behavior because (a) it's less surprising and
 (b) it's not ambiguous.  Right now the caller can't tell the difference
 between DROP and DROP DROP DROP.

 With that change, the correct test at line 795 would become

 else if (pg_strcasecmp(prev_wd, DROP) == 0 
  prev2_wd[0] == '\0')

I've committed this --- please adjust the EXECUTE patch to match.

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


Re: [HACKERS] EXECUTE tab completion

2011-10-20 Thread Andreas Karlsson

On 10/20/2011 09:53 PM, Tom Lane wrote:

With that change, the correct test at line 795 would become



 else if (pg_strcasecmp(prev_wd, DROP) == 0
  prev2_wd[0] == '\0')


I've committed this --- please adjust the EXECUTE patch to match.


Thanks for cleaning up the code to some sanity, I should have done so 
myself when I noticed the problem.


A new version is attached.

Best regards,
Andreas

--
Andreas Karlsson
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
new file mode 100644
index abf9bc7..ee63198
*** a/src/bin/psql/tab-complete.c
--- b/src/bin/psql/tab-complete.c
*** static const SchemaQuery Query_for_list_
*** 588,593 
--- 588,598 
 FROM pg_catalog.pg_available_extensions \
WHERE substring(pg_catalog.quote_ident(name),1,%d)='%s' AND installed_version IS NULL
  
+ #define Query_for_list_of_prepared_statements \
+  SELECT pg_catalog.quote_ident(name) \
+FROM pg_catalog.pg_prepared_statements \
+   WHERE substring(pg_catalog.quote_ident(name),1,%d)='%s'
+ 
  /*
   * This is a list of all things in Pgsql, which can show up after CREATE or
   * DROP; and there is also a query to get a list of them.
*** psql_completion(char *text, int start, i
*** 1640,1645 
--- 1645,1656 
  		COMPLETE_WITH_LIST(list_CSV);
  	}
  
+ /* EXECUTE */
+ 	/* must not match CREATE TRIGGER ... EXECUTE PROCEDURE */
+ 	else if (pg_strcasecmp(prev_wd, EXECUTE) == 0 
+ 			 prev2_wd[0] == '\0')
+ 		COMPLETE_WITH_QUERY(Query_for_list_of_prepared_statements);
+ 
  	/* CREATE DATABASE */
  	else if (pg_strcasecmp(prev3_wd, CREATE) == 0 
  			 pg_strcasecmp(prev2_wd, DATABASE) == 0)

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


Re: [HACKERS] EXECUTE tab completion

2011-10-19 Thread Josh Kupershmidt
On Mon, Sep 26, 2011 at 5:03 PM, Andreas Karlsson andr...@proxel.se wrote:
 Magnus's patch for adding tab completion of views to the TABLE statement
 reminded me of a minor annoyance of mine -- that EXECUTE always completes
 with PROCEDURE as if it would have been part of CREATE TRIGGER ... EXECUTE
 even when it is the first word of the line.

+1

 Attached is a simple patch which adds completion of prepared statement names
 to the EXECUTE statement.

 What could perhaps be added is that if you press tab again after completing
 the prepared statement name you might want to see a single ( appear. Did
 not add that though since EXECUTE foo(); is not valid syntax (while
 EXECUTE foo(1); is) and I did not feel the extra lines of code to add a
 query to check if the number of expected parameters is greater than 0 were
 worth it.

Yeah, that doesn't seem worth the trouble. The patch looks fine to me;
it doesn't break the existing EXECUTE completion intended for CREATE
TRIGGER and seems to work OK on a few examples I tried.

I guess the only quibble I can see is that the two comment lines might
be better written together, to mimic the neighboring comment styles,
as in:
/* EXECUTE */
/* must not match CREATE TRIGGER ... EXECUTE PROCEDURE */
else if ...

Incidentally, I was wondering what the heck was up with a clause like this:
else if (pg_strcasecmp(prev_wd, EXECUTE) == 0 
 pg_strcasecmp(prev2_wd, EXECUTE) == 0)

though that looks to be some strange quirk of previous_word()'s behavior.

Josh

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


Re: [HACKERS] EXECUTE tab completion

2011-10-19 Thread Tom Lane
Josh Kupershmidt schmi...@gmail.com writes:
 Incidentally, I was wondering what the heck was up with a clause like this:
 else if (pg_strcasecmp(prev_wd, EXECUTE) == 0 
  pg_strcasecmp(prev2_wd, EXECUTE) == 0)

Hmm, maybe || was meant not  ?  It seems pretty unlikely that the
above test would ever trigger on valid SQL input.

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


Re: [HACKERS] EXECUTE tab completion

2011-10-19 Thread Josh Kupershmidt
On Wed, Oct 19, 2011 at 10:40 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Josh Kupershmidt schmi...@gmail.com writes:
 Incidentally, I was wondering what the heck was up with a clause like this:
     else if (pg_strcasecmp(prev_wd, EXECUTE) == 0 
              pg_strcasecmp(prev2_wd, EXECUTE) == 0)

 Hmm, maybe || was meant not  ?  It seems pretty unlikely that the
 above test would ever trigger on valid SQL input.

Well, changing '' to '||' breaks the stated comment of the patch, namely:
  /* must not match CREATE TRIGGER ... EXECUTE PROCEDURE */

I assume this is an accepted quirk of previous_word() since we have
this existing similar code:

/* DROP, but watch out for DROP embedded in other commands */
/* complete with something you can drop */
else if (pg_strcasecmp(prev_wd, DROP) == 0 
 pg_strcasecmp(prev2_wd, DROP) == 0)

and the patch does seem to auto-complete a beginning EXECUTE
correctly. We could probably use a comment somewhere explaining this
quirk.

Josh

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


[HACKERS] EXECUTE tab completion

2011-09-26 Thread Andreas Karlsson

Hi,

Magnus's patch for adding tab completion of views to the TABLE statement 
reminded me of a minor annoyance of mine -- that EXECUTE always 
completes with PROCEDURE as if it would have been part of CREATE 
TRIGGER ... EXECUTE even when it is the first word of the line.


Attached is a simple patch which adds completion of prepared statement 
names to the EXECUTE statement.


What could perhaps be added is that if you press tab again after 
completing the prepared statement name you might want to see a single 
( appear. Did not add that though since EXECUTE foo(); is not valid 
syntax (while EXECUTE foo(1); is) and I did not feel the extra lines 
of code to add a query to check if the number of expected parameters is 
greater than 0 were worth it.


--
Andreas Karlsson
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
new file mode 100644
index 4f7df36..15bb8c1
*** a/src/bin/psql/tab-complete.c
--- b/src/bin/psql/tab-complete.c
*** static const SchemaQuery Query_for_list_
*** 588,593 
--- 588,598 
 FROM pg_catalog.pg_available_extensions \
WHERE substring(pg_catalog.quote_ident(name),1,%d)='%s' AND installed_version IS NULL
  
+ #define Query_for_list_of_prepared_statements \
+  SELECT pg_catalog.quote_ident(name) \
+FROM pg_catalog.pg_prepared_statements \
+   WHERE substring(pg_catalog.quote_ident(name),1,%d)='%s'
+ 
  /*
   * This is a list of all things in Pgsql, which can show up after CREATE or
   * DROP; and there is also a query to get a list of them.
*** psql_completion(char *text, int start, i
*** 1642,1647 
--- 1647,1658 
  		COMPLETE_WITH_LIST(list_CSV);
  	}
  
+ /* EXECUTE */
+ 	else if (pg_strcasecmp(prev_wd, EXECUTE) == 0 
+ 			 pg_strcasecmp(prev2_wd, EXECUTE) == 0)
+ 		/* must not match CREATE TRIGGER ... EXECUTE PROCEDURE */
+ 	COMPLETE_WITH_QUERY(Query_for_list_of_prepared_statements);
+ 
  	/* CREATE DATABASE */
  	else if (pg_strcasecmp(prev3_wd, CREATE) == 0 
  			 pg_strcasecmp(prev2_wd, DATABASE) == 0)

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