Re: [HACKERS] EXECUTE tab completion
On Mon, Oct 24, 2011 at 01:26, Tom Lane wrote: > Andreas Karlsson 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
Andreas Karlsson 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
On Thu, Oct 20, 2011 at 5:16 PM, Andreas Karlsson 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
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
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
Alvaro Herrera 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 " 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
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 wrote: > > Josh Kupershmidt 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 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
On Wed, Oct 19, 2011 at 10:40 PM, Tom Lane wrote: > Josh Kupershmidt 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
Re: [HACKERS] EXECUTE tab completion
Josh Kupershmidt 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
On Mon, Sep 26, 2011 at 5:03 PM, Andreas Karlsson 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
[HACKERS] EXECUTE tab completion
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
