Re: [HACKERS] Making tab-complete.c easier to maintain
On 01/06/2016 01:13 AM, Michael Paquier wrote: On Wed, Jan 6, 2016 at 2:03 AM, Tom Lane wrote: I've pushed the second patch now. I made a few adjustments --- notably, I didn't like the way you'd implemented '*' wildcards, because they wouldn't have behaved very sanely in combination with '|'. The case doesn't come up in the current set of patterns, but we'll likely want it sometime. Thanks! Let's consider this project as done then. That was a long way to it... Thanks to everyone involved in cleaning this up, it is much easier to add tab completions now. Andreas -- 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] Making tab-complete.c easier to maintain
On Wed, Jan 6, 2016 at 2:03 AM, Tom Lane wrote: > I wrote: >> Michael Paquier writes: >>> So, here are the commands that still remain with TailMatches to cover >>> this case, per gram.y: >>> - CREATE TABLE >>> - CREATE INDEX >>> - CREATE VIEW >>> - GRANT >>> - CREATE TRIGGER >>> - CREATE SEQUENCE >>> New patches are attached. > >> I've reviewed and committed the first of these patches. > > I've pushed the second patch now. I made a few adjustments --- notably, > I didn't like the way you'd implemented '*' wildcards, because they > wouldn't have behaved very sanely in combination with '|'. The case > doesn't come up in the current set of patterns, but we'll likely want it > sometime. Thanks! Let's consider this project as done then. That was a long way to it... -- Michael -- 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] Making tab-complete.c easier to maintain
I wrote: > Michael Paquier writes: >> So, here are the commands that still remain with TailMatches to cover >> this case, per gram.y: >> - CREATE TABLE >> - CREATE INDEX >> - CREATE VIEW >> - GRANT >> - CREATE TRIGGER >> - CREATE SEQUENCE >> New patches are attached. > I've reviewed and committed the first of these patches. I've pushed the second patch now. I made a few adjustments --- notably, I didn't like the way you'd implemented '*' wildcards, because they wouldn't have behaved very sanely in combination with '|'. The case doesn't come up in the current set of patterns, but we'll likely want it sometime. 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] Making tab-complete.c easier to maintain
On Tue, Jan 5, 2016 at 10:13 AM, Tom Lane wrote: > Michael Paquier writes: >> So, here are the commands that still remain with TailMatches to cover >> this case, per gram.y: >> - CREATE TABLE >> - CREATE INDEX >> - CREATE VIEW >> - GRANT >> - CREATE TRIGGER >> - CREATE SEQUENCE >> New patches are attached. > > I've reviewed and committed the first of these patches. I found a few > mistakes, mostly where you'd converted TailMatches to Matches without > adding the leading words that the original author had left out of the > pattern. But man, this is mind-numbingly tedious work :-(. I probably > made a few more mistakes myself. Still, the code is enormously more > readable now than when we started, and almost certainly more reliable. Thanks. My best advice is to avoid doing such after 10PM, that's a good way to finish with a headache. I did it. > I'm too burned out to look at the second patch tonight, but hopefully > will get to it tomorrow. I see that you are on fire these days, still bodies need some rest. This second one still applies cleanly, but it is far less urgent, the other psql-tab patches do not depend on it. -- Michael -- 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] Making tab-complete.c easier to maintain
Michael Paquier writes: > So, here are the commands that still remain with TailMatches to cover > this case, per gram.y: > - CREATE TABLE > - CREATE INDEX > - CREATE VIEW > - GRANT > - CREATE TRIGGER > - CREATE SEQUENCE > New patches are attached. I've reviewed and committed the first of these patches. I found a few mistakes, mostly where you'd converted TailMatches to Matches without adding the leading words that the original author had left out of the pattern. But man, this is mind-numbingly tedious work :-(. I probably made a few more mistakes myself. Still, the code is enormously more readable now than when we started, and almost certainly more reliable. I'm too burned out to look at the second patch tonight, but hopefully will get to it tomorrow. 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] Making tab-complete.c easier to maintain
On Thu, Dec 31, 2015 at 9:13 AM, Michael Paquier wrote: > On Wed, Dec 30, 2015 at 11:21 PM, Alvaro Herrera > wrote: >> Michael Paquier wrote: >> >>> OK, here are new patches. >>> - 0001 switches a bunch of TailMatches to Matches. Do we want to care >>> about the case where a schema is created following by a bunch of >>> objects? I mean stuff like "CREATE SCHEMA hoge CREATE TABLE ..." where >>> the current completion would work fine. The performance gains seem >>> worth it compared to the number of people actually using it, the point >>> has just not been raised yet. >> >> I'd rather have the completion work for that case than get a few >> microseconds speedup. As far as I recall, it's only four commands that >> must retain the old coding. > > Fine for me this way. So, here are the commands that still remain with TailMatches to cover this case, per gram.y: - CREATE TABLE - CREATE INDEX - CREATE VIEW - GRANT - CREATE TRIGGER - CREATE SEQUENCE New patches are attached. Regards, -- Michael From 99d664506b712defaaae378bdc0763201d3d4721 Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Fri, 1 Jan 2016 21:07:01 +0900 Subject: [PATCH 1/2] Improve performance of psql tab completion TailMatches are based on a lower-bound check and Matches uses a direct match for the number of words. It happens that the former is used in many places where the latter could be used. Doing the switch improve the performance of tab completion by having to match only a number of words for many commands. --- src/bin/psql/tab-complete.c | 499 +++- 1 file changed, 257 insertions(+), 242 deletions(-) diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c index 4c93ae9..2472a99 100644 --- a/src/bin/psql/tab-complete.c +++ b/src/bin/psql/tab-complete.c @@ -1137,6 +1137,21 @@ psql_completion(const char *text, int start, int end) #define Matches4(p1, p2, p3, p4) \ (previous_words_count == 4 && \ TailMatches4(p1, p2, p3, p4)) +#define Matches5(p1, p2, p3, p4, p5) \ + (previous_words_count == 5 && \ + TailMatches5(p1, p2, p3, p4, p5)) +#define Matches6(p1, p2, p3, p4, p5, p6) \ + (previous_words_count == 6 && \ + TailMatches6(p1, p2, p3, p4, p5, p6)) +#define Matches7(p1, p2, p3, p4, p5, p6, p7) \ + (previous_words_count == 7 && \ + TailMatches7(p1, p2, p3, p4, p5, p6, p7)) +#define Matches8(p1, p2, p3, p4, p5, p6, p7, p8) \ + (previous_words_count == 8 && \ + TailMatches8(p1, p2, p3, p4, p5, p6, p7, p8)) +#define Matches9(p1, p2, p3, p4, p5, p6, p7, p8, p9) \ + (previous_words_count == 9 && \ + TailMatches9(p1, p2, p3, p4, p5, p6, p7, p8, p9)) /* * Macros for matching N words at the start of the line, regardless of @@ -1266,10 +1281,10 @@ psql_completion(const char *text, int start, int end) else if (TailMatches7("ALL", "IN", "TABLESPACE", MatchAny, "OWNED", "BY", MatchAny)) COMPLETE_WITH_CONST("SET TABLESPACE"); /* ALTER AGGREGATE,FUNCTION */ - else if (TailMatches3("ALTER", "AGGREGATE|FUNCTION", MatchAny)) + else if (Matches3("ALTER", "AGGREGATE|FUNCTION", MatchAny)) COMPLETE_WITH_CONST("("); /* ALTER AGGREGATE,FUNCTION (...) */ - else if (TailMatches4("ALTER", "AGGREGATE|FUNCTION", MatchAny, MatchAny)) + else if (Matches4("ALTER", "AGGREGATE|FUNCTION", MatchAny, MatchAny)) { if (ends_with(prev_wd, ')')) COMPLETE_WITH_LIST3("OWNER TO", "RENAME TO", "SET SCHEMA"); @@ -1278,49 +1293,49 @@ psql_completion(const char *text, int start, int end) } /* ALTER SCHEMA */ - else if (TailMatches3("ALTER", "SCHEMA", MatchAny)) + else if (Matches3("ALTER", "SCHEMA", MatchAny)) COMPLETE_WITH_LIST2("OWNER TO", "RENAME TO"); /* ALTER COLLATION */ - else if (TailMatches3("ALTER", "COLLATION", MatchAny)) + else if (Matches3("ALTER", "COLLATION", MatchAny)) COMPLETE_WITH_LIST3("OWNER TO", "RENAME TO", "SET SCHEMA"); /* ALTER CONVERSION */ - else if (TailMatches3("ALTER", "CONVERSION", MatchAny)) + else if (Matches3("ALTER", "CONVERSION", MatchAny)) COMPLETE_WITH_LIST3("OWNER TO", "RENAME TO", "SET SCHEMA"); /* ALTER DATABASE */ - else if (TailMatches3("ALTER", "DATABASE", MatchAny)) + else if (Matches3("ALTER", "DATABASE", MatchAny)) COMPLETE_WITH_LIST7("RESET", "SET", "OWNER TO", "RENAME TO", "IS_TEMPLATE", "ALLOW_CONNECTIONS", "CONNECTION LIMIT"); /* ALTER EVENT TRIGGER */ - else if (TailMatches3("ALTER", "EVENT", "TRIGGER")) + else if (Matches3("ALTER", "EVENT", "TRIGGER")) COMPLETE_WITH_QUERY(Query_for_list_of_event_triggers); /* ALTER EVENT TRIGGER */ - else if (TailMatches4("ALTER", "EVENT", "TRIGGER", MatchAny)) + else if (Matches4("ALTER", "EVENT", "TRIGGER", MatchAny)) COMPLETE_WITH_LIST4("DISABLE", "ENABLE", "OWNER TO", "RENAME TO"); /* ALTER EVENT TRIGGER ENABLE */ - else if (TailMatches5("ALTER", "EVENT", "TRIGGER", MatchAny, "ENABLE")) + else if (Matches5("ALTER", "EVENT", "TRIGGER", MatchAny, "ENABLE")) COMPLETE_WITH_LIST2("REPLICA", "ALWAYS"); /* ALTER EXTENSION */ -
Re: [HACKERS] Making tab-complete.c easier to maintain
On Wed, Dec 30, 2015 at 11:21 PM, Alvaro Herrera wrote: > Michael Paquier wrote: > >> OK, here are new patches. >> - 0001 switches a bunch of TailMatches to Matches. Do we want to care >> about the case where a schema is created following by a bunch of >> objects? I mean stuff like "CREATE SCHEMA hoge CREATE TABLE ..." where >> the current completion would work fine. The performance gains seem >> worth it compared to the number of people actually using it, the point >> has just not been raised yet. > > I'd rather have the completion work for that case than get a few > microseconds speedup. As far as I recall, it's only four commands that > must retain the old coding. Fine for me this way. -- Michael -- 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] Making tab-complete.c easier to maintain
Michael Paquier wrote: > OK, here are new patches. > - 0001 switches a bunch of TailMatches to Matches. Do we want to care > about the case where a schema is created following by a bunch of > objects? I mean stuff like "CREATE SCHEMA hoge CREATE TABLE ..." where > the current completion would work fine. The performance gains seem > worth it compared to the number of people actually using it, the point > has just not been raised yet. I'd rather have the completion work for that case than get a few microseconds speedup. As far as I recall, it's only four commands that must retain the old coding. -- Álvaro Herrerahttp://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
Re: [HACKERS] Making tab-complete.c easier to maintain
On Wed, Dec 30, 2015 at 9:14 AM, Michael Paquier wrote: > On Wed, Dec 30, 2015 at 6:26 AM, Thomas Munro wrote: >> I see that you changed INSERT and DELETE (but not UPDATE) to use >> MatchesN rather than TailMatchesN. Shouldn't these stay with >> TailMatchesN for the reason Tom gave above? > > Er, yeah. They had better be TailMatches, or even COPY DML stuff will be > broken. OK, here are new patches. - 0001 switches a bunch of TailMatches to Matches. Do we want to care about the case where a schema is created following by a bunch of objects? I mean stuff like "CREATE SCHEMA hoge CREATE TABLE ..." where the current completion would work fine. The performance gains seem worth it compared to the number of people actually using it, the point has just not been raised yet. - 0002 that implements the new tab completion for backslash commands, with the wildcard "*" as suggested by Tom. I fixed in 0001 the stuff with DML queries, and also found one bug for another query while re-reading the code. Regards, -- Michael From 3a2e63b984548d2f4826dabd61e0efcae3aabe22 Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Wed, 30 Dec 2015 20:32:18 +0900 Subject: [PATCH 1/2] Improve performance of psql tab completion TailMatches are based on a lower-bound check and Matches uses a direct match for the number of words. It happens that the former is used in many places where the latter could be used. Doing the switch improve the performance of tab completion by having to match only a number of words for many commands. --- src/bin/psql/tab-complete.c | 547 +++- 1 file changed, 281 insertions(+), 266 deletions(-) diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c index 4c93ae9..b36bd73 100644 --- a/src/bin/psql/tab-complete.c +++ b/src/bin/psql/tab-complete.c @@ -1137,6 +1137,21 @@ psql_completion(const char *text, int start, int end) #define Matches4(p1, p2, p3, p4) \ (previous_words_count == 4 && \ TailMatches4(p1, p2, p3, p4)) +#define Matches5(p1, p2, p3, p4, p5) \ + (previous_words_count == 5 && \ + TailMatches5(p1, p2, p3, p4, p5)) +#define Matches6(p1, p2, p3, p4, p5, p6) \ + (previous_words_count == 6 && \ + TailMatches6(p1, p2, p3, p4, p5, p6)) +#define Matches7(p1, p2, p3, p4, p5, p6, p7) \ + (previous_words_count == 7 && \ + TailMatches7(p1, p2, p3, p4, p5, p6, p7)) +#define Matches8(p1, p2, p3, p4, p5, p6, p7, p8) \ + (previous_words_count == 8 && \ + TailMatches8(p1, p2, p3, p4, p5, p6, p7, p8)) +#define Matches9(p1, p2, p3, p4, p5, p6, p7, p8, p9) \ + (previous_words_count == 9 && \ + TailMatches9(p1, p2, p3, p4, p5, p6, p7, p8, p9)) /* * Macros for matching N words at the start of the line, regardless of @@ -1266,10 +1281,10 @@ psql_completion(const char *text, int start, int end) else if (TailMatches7("ALL", "IN", "TABLESPACE", MatchAny, "OWNED", "BY", MatchAny)) COMPLETE_WITH_CONST("SET TABLESPACE"); /* ALTER AGGREGATE,FUNCTION */ - else if (TailMatches3("ALTER", "AGGREGATE|FUNCTION", MatchAny)) + else if (Matches3("ALTER", "AGGREGATE|FUNCTION", MatchAny)) COMPLETE_WITH_CONST("("); /* ALTER AGGREGATE,FUNCTION (...) */ - else if (TailMatches4("ALTER", "AGGREGATE|FUNCTION", MatchAny, MatchAny)) + else if (Matches4("ALTER", "AGGREGATE|FUNCTION", MatchAny, MatchAny)) { if (ends_with(prev_wd, ')')) COMPLETE_WITH_LIST3("OWNER TO", "RENAME TO", "SET SCHEMA"); @@ -1278,49 +1293,49 @@ psql_completion(const char *text, int start, int end) } /* ALTER SCHEMA */ - else if (TailMatches3("ALTER", "SCHEMA", MatchAny)) + else if (Matches3("ALTER", "SCHEMA", MatchAny)) COMPLETE_WITH_LIST2("OWNER TO", "RENAME TO"); /* ALTER COLLATION */ - else if (TailMatches3("ALTER", "COLLATION", MatchAny)) + else if (Matches3("ALTER", "COLLATION", MatchAny)) COMPLETE_WITH_LIST3("OWNER TO", "RENAME TO", "SET SCHEMA"); /* ALTER CONVERSION */ - else if (TailMatches3("ALTER", "CONVERSION", MatchAny)) + else if (Matches3("ALTER", "CONVERSION", MatchAny)) COMPLETE_WITH_LIST3("OWNER TO", "RENAME TO", "SET SCHEMA"); /* ALTER DATABASE */ - else if (TailMatches3("ALTER", "DATABASE", MatchAny)) + else if (Matches3("ALTER", "DATABASE", MatchAny)) COMPLETE_WITH_LIST7("RESET", "SET", "OWNER TO", "RENAME TO", "IS_TEMPLATE", "ALLOW_CONNECTIONS", "CONNECTION LIMIT"); /* ALTER EVENT TRIGGER */ - else if (TailMatches3("ALTER", "EVENT", "TRIGGER")) + else if (Matches3("ALTER", "EVENT", "TRIGGER")) COMPLETE_WITH_QUERY(Query_for_list_of_event_triggers); /* ALTER EVENT TRIGGER */ - else if (TailMatches4("ALTER", "EVENT", "TRIGGER", MatchAny)) + else if (Matches4("ALTER", "EVENT", "TRIGGER", MatchAny)) COMPLETE_WITH_LIST4("DISABLE", "ENABLE", "OWNER TO", "RENAME TO"); /* ALTER EVENT TRIGGER ENABLE */ - else if (TailMatches5("ALTER", "EVENT", "TRIGGER", MatchAny, "ENABLE")) + else if (Matches5("ALTER", "EVENT", "TRIGGER", MatchAny, "ENABLE")) COMPLETE_WITH_LIST2("REPLICA", "ALWAYS"); /*
Re: [HACKERS] Making tab-complete.c easier to maintain
On Wed, Dec 30, 2015 at 6:26 AM, Thomas Munro wrote: > On Wed, Dec 30, 2015 at 3:14 AM, Michael Paquier > wrote: >> On Sun, Dec 20, 2015 at 8:08 AM, Michael Paquier >> wrote: >>> On Sun, Dec 20, 2015 at 6:24 AM, Tom Lane wrote: 2. I believe that a very large fraction of the TailMatches() rules really ought to be Matches(), ie, they should not consider matches that don't start at the start of the line. And there's another bunch that could be Matches() if the author hadn't been unaccountably lazy about checking all words of the expected command. If we converted as much as we could that way, it would make psql_completion faster because many inapplicable rules could be discarded after a single integer comparison on previous_words_count, and it would greatly reduce the risk of inapplicable matches. We can't do that for rules meant to apply to DML statements, since they can be buried in WITH, EXPLAIN, etc ... but an awful lot of the DDL rules could be changed. >> >> Yep, clearly. We may gain a bit of performance by matching directly >> with an equal number of words using Matches instead of a lower bound >> with TailMatches. I have looked at this thing and hacked a patch as >> attached. > > I see that you changed INSERT and DELETE (but not UPDATE) to use > MatchesN rather than TailMatchesN. Shouldn't these stay with > TailMatchesN for the reason Tom gave above? Er, yeah. They had better be TailMatches, or even COPY DML stuff will be broken. -- Michael -- 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] Making tab-complete.c easier to maintain
On Wed, Dec 30, 2015 at 1:21 AM, Tom Lane wrote: > This is because of the use of strncmp instead of plain strcmp > in most of the backslash matching rules, eg the above case is > covered by > > else if (strncmp(prev_wd, "\\df", strlen("\\df")) == 0) Ah, OK. The length of the name and not the pattern is used in word_matches, but we had better use something based on the pattern shape. And the current logic for backslash commands uses the length of the pattern, and not the word for its checks. > I was envisioning that we'd want to convert this to something like > > else if (TailMatchesCS1("\\df*")) That's a better macro name... -- Michael -- 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] Making tab-complete.c easier to maintain
On Wed, Dec 30, 2015 at 3:14 AM, Michael Paquier wrote: > On Sun, Dec 20, 2015 at 8:08 AM, Michael Paquier > wrote: >> On Sun, Dec 20, 2015 at 6:24 AM, Tom Lane wrote: >>> 2. I believe that a very large fraction of the TailMatches() rules really >>> ought to be Matches(), ie, they should not consider matches that don't >>> start at the start of the line. And there's another bunch that could >>> be Matches() if the author hadn't been unaccountably lazy about checking >>> all words of the expected command. If we converted as much as we could >>> that way, it would make psql_completion faster because many inapplicable >>> rules could be discarded after a single integer comparison on >>> previous_words_count, and it would greatly reduce the risk of inapplicable >>> matches. We can't do that for rules meant to apply to DML statements, >>> since they can be buried in WITH, EXPLAIN, etc ... but an awful lot of >>> the DDL rules could be changed. > > Yep, clearly. We may gain a bit of performance by matching directly > with an equal number of words using Matches instead of a lower bound > with TailMatches. I have looked at this thing and hacked a patch as > attached. I see that you changed INSERT and DELETE (but not UPDATE) to use MatchesN rather than TailMatchesN. Shouldn't these stay with TailMatchesN for the reason Tom gave above? -- Thomas Munro http://www.enterprisedb.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] Making tab-complete.c easier to maintain
Michael Paquier writes: >> On Sun, Dec 20, 2015 at 6:24 AM, Tom Lane wrote: >>> 1. I think it would be a good idea to convert the matching rules for >>> backslash commands too. To do that, we'd need to provide a case-sensitive >>> equivalent to word_match and the matching macros. I think we'd also have >>> to extend word_match to allow a trailing wildcard character, maybe "*". > I am not really sure I follow much the use of the wildcard, do you > mean to be able to work with the [S] extensions of the backslash > commands which are not completed now? But they are completed: regression=# \dfS str string_agg string_agg_transfn strip string_agg_finalfn string_to_array strpos This is because of the use of strncmp instead of plain strcmp in most of the backslash matching rules, eg the above case is covered by else if (strncmp(prev_wd, "\\df", strlen("\\df")) == 0) I was envisioning that we'd want to convert this to something like else if (TailMatchesCS1("\\df*")) 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] Making tab-complete.c easier to maintain
On Sun, Dec 20, 2015 at 8:08 AM, Michael Paquier wrote: > On Sun, Dec 20, 2015 at 6:24 AM, Tom Lane wrote: >> 1. I think it would be a good idea to convert the matching rules for >> backslash commands too. To do that, we'd need to provide a case-sensitive >> equivalent to word_match and the matching macros. I think we'd also have >> to extend word_match to allow a trailing wildcard character, maybe "*". I am not really sure I follow much the use of the wildcard, do you mean to be able to work with the [S] extensions of the backslash commands which are not completed now? I am attaching a patch that adds support for a case-sensitive comparison facility without this wildcard system, simplifying the backslash commands. >> 2. I believe that a very large fraction of the TailMatches() rules really >> ought to be Matches(), ie, they should not consider matches that don't >> start at the start of the line. And there's another bunch that could >> be Matches() if the author hadn't been unaccountably lazy about checking >> all words of the expected command. If we converted as much as we could >> that way, it would make psql_completion faster because many inapplicable >> rules could be discarded after a single integer comparison on >> previous_words_count, and it would greatly reduce the risk of inapplicable >> matches. We can't do that for rules meant to apply to DML statements, >> since they can be buried in WITH, EXPLAIN, etc ... but an awful lot of >> the DDL rules could be changed. Yep, clearly. We may gain a bit of performance by matching directly with an equal number of words using Matches instead of a lower bound with TailMatches. I have looked at this thing and hacked a patch as attached. >> 3. The HeadMatches macros are pretty iffy because they can only look back >> nine words. I'm tempted to redesign get_previous_words so it just >> tokenizes the whole line rather than having an arbitrary limitation. >> (For that matter, it's long overdue for it to be able to deal with >> multiline input...) >> >> I might go look at #3, but I can't currently summon the energy to tackle >> #1 or #2 --- any volunteers? #3 has been already done in the mean time... > I could have a look at both of them and submit patch for next CF, both > things do not seem that much complicated. Those things are as well added to the next CF. -- Michael From 351894c975e72d62b6c49e8ea203fc194ccc59ee Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Tue, 29 Dec 2015 22:13:27 +0900 Subject: [PATCH 1/2] Improve performance of psql tab completion TailMatches are based on a lower-bound check and Matches uses a direct match for the number of words. It happens that the former is used in many places where the latter could be used. Doing the switch improve the performance of tab completion by having to match only a number of words for many commands. --- src/bin/psql/tab-complete.c | 567 +++- 1 file changed, 291 insertions(+), 276 deletions(-) diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c index 4c93ae9..c920353 100644 --- a/src/bin/psql/tab-complete.c +++ b/src/bin/psql/tab-complete.c @@ -1137,6 +1137,21 @@ psql_completion(const char *text, int start, int end) #define Matches4(p1, p2, p3, p4) \ (previous_words_count == 4 && \ TailMatches4(p1, p2, p3, p4)) +#define Matches5(p1, p2, p3, p4, p5) \ + (previous_words_count == 5 && \ + TailMatches5(p1, p2, p3, p4, p5)) +#define Matches6(p1, p2, p3, p4, p5, p6) \ + (previous_words_count == 6 && \ + TailMatches6(p1, p2, p3, p4, p5, p6)) +#define Matches7(p1, p2, p3, p4, p5, p6, p7) \ + (previous_words_count == 7 && \ + TailMatches7(p1, p2, p3, p4, p5, p6, p7)) +#define Matches8(p1, p2, p3, p4, p5, p6, p7, p8) \ + (previous_words_count == 8 && \ + TailMatches8(p1, p2, p3, p4, p5, p6, p7, p8)) +#define Matches9(p1, p2, p3, p4, p5, p6, p7, p8, p9) \ + (previous_words_count == 9 && \ + TailMatches9(p1, p2, p3, p4, p5, p6, p7, p8, p9)) /* * Macros for matching N words at the start of the line, regardless of @@ -1266,10 +1281,10 @@ psql_completion(const char *text, int start, int end) else if (TailMatches7("ALL", "IN", "TABLESPACE", MatchAny, "OWNED", "BY", MatchAny)) COMPLETE_WITH_CONST("SET TABLESPACE"); /* ALTER AGGREGATE,FUNCTION */ - else if (TailMatches3("ALTER", "AGGREGATE|FUNCTION", MatchAny)) + else if (Matches3("ALTER", "AGGREGATE|FUNCTION", MatchAny)) COMPLETE_WITH_CONST("("); /* ALTER AGGREGATE,FUNCTION (...) */ - else if (TailMatches4("ALTER", "AGGREGATE|FUNCTION", MatchAny, MatchAny)) + else if (Matches4("ALTER", "AGGREGATE|FUNCTION", MatchAny, MatchAny)) { if (ends_with(prev_wd, ')')) COMPLETE_WITH_LIST3("OWNER TO", "RENAME TO", "SET SCHEMA"); @@ -1278,49 +1293,49 @@ psql_completion(const char *text, int start, int end) } /* ALTER SCHEMA */ - else if (TailMatches3("ALTER", "SCHEMA", MatchAny)) + else if (Matches3("ALTER", "SCHEMA", MatchAny)) COMPLETE_WITH_LIST2("OWNE
Re: [HACKERS] Making tab-complete.c easier to maintain
On Sun, Dec 20, 2015 at 10:24 AM, Tom Lane wrote: > I've committed this now with a number of changes, many of them just > stylistic. Thanks! And thanks also to Michael, Kyotaro, Alvaro and Jeff. +1 for the suggested further improvements, which I will help out with where I can. -- Thomas Munro http://www.enterprisedb.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] Making tab-complete.c easier to maintain
I wrote: > 3. The HeadMatches macros are pretty iffy because they can only look back > nine words. I'm tempted to redesign get_previous_words so it just > tokenizes the whole line rather than having an arbitrary limitation. > (For that matter, it's long overdue for it to be able to deal with > multiline input...) I poked into this and found that it's really not hard, if you don't mind still another global variable associated with tab-completion. See attached patch. The main objection to this change might be speed, but I experimented with the longest query in information_schema.sql (CREATE VIEW usage_privileges, 162 lines) and found that pressing tab at the end of that seemed to take well under a millisecond on my workstation. So I think it's probably insignificant, especially compared to any of the code paths that send a query to the server for tab completion. regards, tom lane diff --git a/src/bin/psql/input.c b/src/bin/psql/input.c index 2bc065a..ccd9a3e 100644 *** a/src/bin/psql/input.c --- b/src/bin/psql/input.c *** static void finishInput(void); *** 53,64 * gets_interactive() * * Gets a line of interactive input, using readline if desired. * The result is a malloc'd string. * * Caller *must* have set up sigint_interrupt_jmp before calling. */ char * ! gets_interactive(const char *prompt) { #ifdef USE_READLINE if (useReadline) --- 53,69 * gets_interactive() * * Gets a line of interactive input, using readline if desired. + * + * prompt: the prompt string to be used + * query_buf: buffer containing lines already read in the current command + * (query_buf is not modified here, but may be consulted for tab completion) + * * The result is a malloc'd string. * * Caller *must* have set up sigint_interrupt_jmp before calling. */ char * ! gets_interactive(const char *prompt, PQExpBuffer query_buf) { #ifdef USE_READLINE if (useReadline) *** gets_interactive(const char *prompt) *** 76,81 --- 81,89 rl_reset_screen_size(); #endif + /* Make current query_buf available to tab completion callback */ + tab_completion_query_buf = query_buf; + /* Enable SIGINT to longjmp to sigint_interrupt_jmp */ sigint_interrupt_enabled = true; *** gets_interactive(const char *prompt) *** 85,90 --- 93,101 /* Disable SIGINT again */ sigint_interrupt_enabled = false; + /* Pure neatnik-ism */ + tab_completion_query_buf = NULL; + return result; } #endif diff --git a/src/bin/psql/input.h b/src/bin/psql/input.h index abd7012..c9d30b9 100644 *** a/src/bin/psql/input.h --- b/src/bin/psql/input.h *** *** 38,44 #include "pqexpbuffer.h" ! char *gets_interactive(const char *prompt); char *gets_fromFile(FILE *source); void initializeInput(int flags); --- 38,44 #include "pqexpbuffer.h" ! char *gets_interactive(const char *prompt, PQExpBuffer query_buf); char *gets_fromFile(FILE *source); void initializeInput(int flags); diff --git a/src/bin/psql/mainloop.c b/src/bin/psql/mainloop.c index b6cef94..cb86457 100644 *** a/src/bin/psql/mainloop.c --- b/src/bin/psql/mainloop.c *** MainLoop(FILE *source) *** 133,139 /* May need to reset prompt, eg after \r command */ if (query_buf->len == 0) prompt_status = PROMPT_READY; ! line = gets_interactive(get_prompt(prompt_status)); } else { --- 133,139 /* May need to reset prompt, eg after \r command */ if (query_buf->len == 0) prompt_status = PROMPT_READY; ! line = gets_interactive(get_prompt(prompt_status), query_buf); } else { diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c index 731df2a..fec7c38 100644 *** a/src/bin/psql/tab-complete.c --- b/src/bin/psql/tab-complete.c *** extern char *filename_completion_functio *** 70,75 --- 70,82 #define WORD_BREAKS "\t\n@$><=;|&{() " /* + * Since readline doesn't let us pass any state through to the tab completion + * callback, we have to use this global variable to let get_previous_words() + * get at the previous lines of the current command. Ick. + */ + PQExpBuffer tab_completion_query_buf = NULL; + + /* * This struct is used to define "schema queries", which are custom-built * to obtain possibly-schema-qualified names of database objects. There is * enough similarity in the structure that we don't want to repeat it each *** static char *pg_strdup_keyword_case(cons *** 923,929 static char *escape_string(const char *text); static PGresult *exec_query(const char *query); ! static int get_previous_words(int point, char **previous_words, int nwords); static char *get_guctype(const char *varname); --- 930,936 static char *escape_string(const char *text); static PGresult *exec_query(const char *query); ! static
Re: [HACKERS] Making tab-complete.c easier to maintain
On Sun, Dec 20, 2015 at 6:24 AM, Tom Lane wrote: > 1. I think it would be a good idea to convert the matching rules for > backslash commands too. To do that, we'd need to provide a case-sensitive > equivalent to word_match and the matching macros. I think we'd also have > to extend word_match to allow a trailing wildcard character, maybe "*". > > 2. I believe that a very large fraction of the TailMatches() rules really > ought to be Matches(), ie, they should not consider matches that don't > start at the start of the line. And there's another bunch that could > be Matches() if the author hadn't been unaccountably lazy about checking > all words of the expected command. If we converted as much as we could > that way, it would make psql_completion faster because many inapplicable > rules could be discarded after a single integer comparison on > previous_words_count, and it would greatly reduce the risk of inapplicable > matches. We can't do that for rules meant to apply to DML statements, > since they can be buried in WITH, EXPLAIN, etc ... but an awful lot of > the DDL rules could be changed. > > 3. The HeadMatches macros are pretty iffy because they can only look back > nine words. I'm tempted to redesign get_previous_words so it just > tokenizes the whole line rather than having an arbitrary limitation. > (For that matter, it's long overdue for it to be able to deal with > multiline input...) > > I might go look at #3, but I can't currently summon the energy to tackle > #1 or #2 --- any volunteers? I could have a look at both of them and submit patch for next CF, both things do not seem that much complicated. -- Michael -- 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] Making tab-complete.c easier to maintain
Michael Paquier writes: > On Sat, Dec 19, 2015 at 5:42 AM, Tom Lane wrote: >> 1. It seems inconsistent that all the new macros are named in CamelCase >> style, whereas there is still plenty of usage of the existing macros like >> COMPLETE_WITH_LIST. It looks pretty jarring IMO. I think we should >> either rename the new macros back to all-upper-case style, or rename the >> existing macros in CamelCase style. >> >> I slightly favor the latter option; we're already pretty much breaking any >> hope of tab-complete fixes applying backwards over this patch, so changing >> the code even more doesn't seem like a problem. Either way, it's a quick >> search-and-replace. Thoughts? > Both would be fine, honestly. Now if we look at the current code there > are already a lot of macros IN_UPPER_CASE, so it would make more sense > on the contrary to have MATCHES_N and MATCHES_EXCEPT? After some contemplation I decided that what was bugging me was mainly the inconsistency of having some completion actions in camelcase and immediately adjacent actions in all-upper-case. Making the test macros camelcase isn't such a problem as long as they all look similar, and I think it's more readable anyway. So I changed CompleteWithListN to COMPLETE_WITH_LISTN and otherwise left the naming alone. I've committed this now with a number of changes, many of them just stylistic. Notable is that I rewrote word_matches to rely on pg_strncasecmp rather than using toupper/tolower directly, so as to avoid any possible change in semantics. (The code as submitted was flat wrong anyway, since it wasn't being careful about passing only unsigned chars to those functions.) I also got rid of its inconsistent treatment of empty strings, in favor of not ever calling word_matches() on nonexistent words in the first place. That just requires testing previous_words_count in the TailMatches macros. I think it'd now be possible for get_previous_words to skip generating empty strings for word positions before the start of line, but have not experimented. I found a couple more small errors in the converted match rules too, but I have to admit my eyes started to glaze over while looking through them. It's possible there are some remaining errors there. On the other hand, it's at least as likely that we've gotten rid of some bugs. There's a good deal more that could be done here: 1. I think it would be a good idea to convert the matching rules for backslash commands too. To do that, we'd need to provide a case-sensitive equivalent to word_match and the matching macros. I think we'd also have to extend word_match to allow a trailing wildcard character, maybe "*". 2. I believe that a very large fraction of the TailMatches() rules really ought to be Matches(), ie, they should not consider matches that don't start at the start of the line. And there's another bunch that could be Matches() if the author hadn't been unaccountably lazy about checking all words of the expected command. If we converted as much as we could that way, it would make psql_completion faster because many inapplicable rules could be discarded after a single integer comparison on previous_words_count, and it would greatly reduce the risk of inapplicable matches. We can't do that for rules meant to apply to DML statements, since they can be buried in WITH, EXPLAIN, etc ... but an awful lot of the DDL rules could be changed. 3. The HeadMatches macros are pretty iffy because they can only look back nine words. I'm tempted to redesign get_previous_words so it just tokenizes the whole line rather than having an arbitrary limitation. (For that matter, it's long overdue for it to be able to deal with multiline input...) I might go look at #3, but I can't currently summon the energy to tackle #1 or #2 --- any volunteers? 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] Making tab-complete.c easier to maintain
Michael Paquier writes: > On Sat, Dec 19, 2015 at 5:42 AM, Tom Lane wrote: >> 2. Why does MatchAnyExcept use "'" as the inversion flag, rather than >> say "!" or "~" ? Seems pretty random. > Actually, "'" is not that much a good idea, no? There could be single > quotes in queries so there is a risk of interfering with the > completion... What do you think would be good candidates? "?", "!", > "#" or "&"? We don't care whether the character appears in queries; it only matters that it not be something we'd need to use in patterns. My inclination is to use "!". 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] Making tab-complete.c easier to maintain
On Sat, Dec 19, 2015 at 5:42 AM, Tom Lane wrote: > Thomas Munro writes: >> [ tab-complete-macrology-v11.patch.gz ] > > A couple of stylistic reactions after looking through the patch for the > first time in a long time: > > 1. It seems inconsistent that all the new macros are named in CamelCase > style, whereas there is still plenty of usage of the existing macros like > COMPLETE_WITH_LIST. It looks pretty jarring IMO. I think we should > either rename the new macros back to all-upper-case style, or rename the > existing macros in CamelCase style. > > I slightly favor the latter option; we're already pretty much breaking any > hope of tab-complete fixes applying backwards over this patch, so changing > the code even more doesn't seem like a problem. Either way, it's a quick > search-and-replace. Thoughts? Both would be fine, honestly. Now if we look at the current code there are already a lot of macros IN_UPPER_CASE, so it would make more sense on the contrary to have MATCHES_N and MATCHES_EXCEPT? > 2. Why does MatchAnyExcept use "'" as the inversion flag, rather than > say "!" or "~" ? Seems pretty random. Actually, "'" is not that much a good idea, no? There could be single quotes in queries so there is a risk of interfering with the completion... What do you think would be good candidates? "?", "!", "#" or "&"? -- Michael -- 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] Making tab-complete.c easier to maintain
Thomas Munro writes: > [ tab-complete-macrology-v11.patch.gz ] A couple of stylistic reactions after looking through the patch for the first time in a long time: 1. It seems inconsistent that all the new macros are named in CamelCase style, whereas there is still plenty of usage of the existing macros like COMPLETE_WITH_LIST. It looks pretty jarring IMO. I think we should either rename the new macros back to all-upper-case style, or rename the existing macros in CamelCase style. I slightly favor the latter option; we're already pretty much breaking any hope of tab-complete fixes applying backwards over this patch, so changing the code even more doesn't seem like a problem. Either way, it's a quick search-and-replace. Thoughts? 2. Why does MatchAnyExcept use "'" as the inversion flag, rather than say "!" or "~" ? Seems pretty random. 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] Making tab-complete.c easier to maintain
Michael Paquier writes: > OK, I am marking that as ready for committer. Let's see what happens next. I'll pick this up, as penance for not having done much in this commitfest. I think it's important to get it pushed quickly so that Thomas doesn't have to keep tracking unrelated changes in tab-complete.c. 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] Making tab-complete.c easier to maintain
On Thu, Dec 17, 2015 at 6:04 PM, Thomas Munro wrote: > On Thu, Dec 17, 2015 at 7:24 PM, Michael Paquier > wrote: > Kyotaro's suggestion of using a macro NEG x to avoid complicating the > string constants seems good to me. But perhaps like this? > > TailMatches4("COMMENT", "ON", MatchAny, MatchAnyExcept("IS")) > > See attached patch which does it that way. Fine for me. >>> Addition to that, I feel that successive "MatchAny"s are a bit >>> bothersome. >>> TailMatches6("COMMENT", "ON", MatchAny, MatchAny, MatchAny, MatchAny)) && !TailMatches1("IS") >>> >>> Is MachAny acceptable? On concern is the two n's >>> (TailMatches and MatchAny) looks a bit confising. >>> TailMatches4("COMMENT", "ON", MatchAny3, "!IS") >> >> Ah, OK, so you would want to be able to have an inner list, MatchAnyN >> meaning actually a list of MatchAny items repeated N times. I am not >> sure if that's worth it.. I would just keep it simple, and we are just >> discussing about a couple of places only that would benefit from that. > > +1 for simple. +#define CompleteWithList10(s1, s2, s3, s4, s5, s6, s7, s8, s9, s10)\ +do { \ + static const char *const list[] = { s1, s2, s3, s4, s5, s6, s7, s8, s9, s10, NULL }; \ + completion_charpp = list; \ + completion_case_sensitive = false; \ + matches = completion_matches(text, complete_from_list); \ +} while (0) Actually we are not using this one. I am fine if this is kept, just worth noting. OK, I am marking that as ready for committer. Let's see what happens next. Regards, -- Michael -- 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] Making tab-complete.c easier to maintain
On Thu, Dec 17, 2015 at 7:24 PM, Michael Paquier wrote: > On Thu, Dec 17, 2015 at 3:06 PM, Kyotaro HORIGUCHI > wrote: >> At Mon, 14 Dec 2015 14:13:02 +0900, Michael Paquier >> wrote in >> >>> On Mon, Dec 14, 2015 at 8:10 AM, Thomas Munro >>> wrote: >>> Yeah, I guess that's an improvement for those cases, and there is no >>> immediate need for a per-keyword NOT operator in our cases to allow >>> things of the type (foo1 OR NOT foo2). Still, in the case of this >>> patch !foo1|foo2 actually means (NOT foo1 AND NOT foo2). It does not >>> seem that much intuitive. Reading straight this expression it seems >>> that !foo1|foo2 means actually (NOT foo1 OR foo2) because the lack of >>> parenthesis. Thoughts? >> >> I used just the same expression as Thomas in my patch since it >> was enough intuitive in this context in my view. The expression >> "(!FOO1)|FOO2" is a nonsence in the context of tab-completion and >> won't be used in future. But it is true that "!(FOO|BAR|BAZ)" is >> clearer than without the parenthesis. > > Yeah that's my whole point. If we decide to support that I think that > we had better go all the way through it, with stuff like: > - & for AND operator > - Support of parenthesis to prioritize expressions > - ! for NOT operator > - | for OR OPERATOR > But honestly at this stage this would just benefit 5 places (citing > Thomas' quote), hence let's just go to the most simple approach which > is the one without all this fancy operator stuff... That will be less > bug prone, and still benefits more than 95% of the tab completion code > path. At least I think that's the most realistic thing to do if we > want to get something for this commit fest. If someone wants those > operators, well I guess that he could add them afterwards.. Thomas, > what do you think? I agree with both of you that using "!" without parentheses is not ideal. I also don't think it's worth trying to make a clever language here: in future it will be a worthy and difficult project to do something far cleverer based on the productions of the real grammar as several people have said. (Presumably with some extra annotations to enable/disable productions and mark out points where database object names are looked up?). In the meantime, I think we should just do the simplest thing that will replace the repetitive strcasecmp-based code with something readable/writable, and avoid the fully-general-pattern-language rabbit hole. Kyotaro's suggestion of using a macro NEG x to avoid complicating the string constants seems good to me. But perhaps like this? TailMatches4("COMMENT", "ON", MatchAny, MatchAnyExcept("IS")) See attached patch which does it that way. >> Addition to that, I feel that successive "MatchAny"s are a bit >> bothersome. >> >>> TailMatches6("COMMENT", "ON", MatchAny, MatchAny, MatchAny, MatchAny)) && >>> !TailMatches1("IS") >> >> Is MachAny acceptable? On concern is the two n's >> (TailMatches and MatchAny) looks a bit confising. >> >>> TailMatches4("COMMENT", "ON", MatchAny3, "!IS") > > Ah, OK, so you would want to be able to have an inner list, MatchAnyN > meaning actually a list of MatchAny items repeated N times. I am not > sure if that's worth it.. I would just keep it simple, and we are just > discussing about a couple of places only that would benefit from that. +1 for simple. -- Thomas Munro http://www.enterprisedb.com tab-complete-macrology-v11.patch.gz Description: GNU Zip compressed data -- 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] Making tab-complete.c easier to maintain
On Thu, Dec 17, 2015 at 3:06 PM, Kyotaro HORIGUCHI wrote: > At Mon, 14 Dec 2015 14:13:02 +0900, Michael Paquier > wrote in > >> On Mon, Dec 14, 2015 at 8:10 AM, Thomas Munro >> wrote: >> Yeah, I guess that's an improvement for those cases, and there is no >> immediate need for a per-keyword NOT operator in our cases to allow >> things of the type (foo1 OR NOT foo2). Still, in the case of this >> patch !foo1|foo2 actually means (NOT foo1 AND NOT foo2). It does not >> seem that much intuitive. Reading straight this expression it seems >> that !foo1|foo2 means actually (NOT foo1 OR foo2) because the lack of >> parenthesis. Thoughts? > > I used just the same expression as Thomas in my patch since it > was enough intuitive in this context in my view. The expression > "(!FOO1)|FOO2" is a nonsence in the context of tab-completion and > won't be used in future. But it is true that "!(FOO|BAR|BAZ)" is > clearer than without the parenthesis. Yeah that's my whole point. If we decide to support that I think that we had better go all the way through it, with stuff like: - & for AND operator - Support of parenthesis to prioritize expressions - ! for NOT operator - | for OR OPERATOR But honestly at this stage this would just benefit 5 places (citing Thomas' quote), hence let's just go to the most simple approach which is the one without all this fancy operator stuff... That will be less bug prone, and still benefits more than 95% of the tab completion code path. At least I think that's the most realistic thing to do if we want to get something for this commit fest. If someone wants those operators, well I guess that he could add them afterwards.. Thomas, what do you think? > Addition to that, I feel that successive "MatchAny"s are a bit > bothersome. > >> TailMatches6("COMMENT", "ON", MatchAny, MatchAny, MatchAny, MatchAny)) && >> !TailMatches1("IS") > > Is MachAny acceptable? On concern is the two n's > (TailMatches and MatchAny) looks a bit confising. > >> TailMatches4("COMMENT", "ON", MatchAny3, "!IS") Ah, OK, so you would want to be able to have an inner list, MatchAnyN meaning actually a list of MatchAny items repeated N times. I am not sure if that's worth it.. I would just keep it simple, and we are just discussing about a couple of places only that would benefit from that. -- Michael -- 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] Making tab-complete.c easier to maintain
Hello. Ok, I withdraw the minilang solution and I'll go on Thomas's way, which is still good to have. At Mon, 14 Dec 2015 14:13:02 +0900, Michael Paquier wrote in > On Mon, Dec 14, 2015 at 8:10 AM, Thomas Munro > wrote: > > I've also add (very) primitive negative pattern support and used it in > > 5 places. Improvement? Examples: > > > > /* ALTER TABLE xxx RENAME yyy */ > > - else if (TailMatches5("ALTER", "TABLE", MatchAny, "RENAME", > > MatchAny) && > > -!TailMatches1("CONSTRAINT|TO")) > > + else if (TailMatches5("ALTER", "TABLE", MatchAny, "RENAME", > > "!CONSTRAINT|TO")) > > COMPLETE_WITH_CONST("TO"); > > > > /* If we have CLUSTER , then add "USING" */ > > - else if (TailMatches2("CLUSTER", MatchAny) && > > !TailMatches1("VERBOSE|ON")) > > + else if (TailMatches2("CLUSTER", "!VERBOSE|ON")) > > COMPLETE_WITH_CONST("USING"); > > + /* Handle negated patterns. */ > + if (*pattern == '!') > + return !word_matches(pattern + 1, word); > > Yeah, I guess that's an improvement for those cases, and there is no > immediate need for a per-keyword NOT operator in our cases to allow > things of the type (foo1 OR NOT foo2). Still, in the case of this > patch !foo1|foo2 actually means (NOT foo1 AND NOT foo2). It does not > seem that much intuitive. Reading straight this expression it seems > that !foo1|foo2 means actually (NOT foo1 OR foo2) because the lack of > parenthesis. Thoughts? I used just the same expression as Thomas in my patch since it was enough intuitive in this context in my view. The expression "(!FOO1)|FOO2" is a nonsence in the context of tab-completion and won't be used in future. But it is true that "!(FOO|BAR|BAZ)" is clearer than without the parenthesis. We could use other characters as the operator, but it also might make it a bit difficalt to read the meaning. "~FOO|BAR|BAZ", "-FOO|BAR|BAZ" "TailMatches2("CLUSTER", NEG "VERBOSE|ON")" is mere a syntax sugar but reduces the uneasiness. But rather longer than adding parenthesis. As the result, I vote for "!(FOO|BAR|BAZ)", then "-FOO|BAR|BAZ" for now. Addition to that, I feel that successive "MatchAny"s are a bit bothersome. > TailMatches6("COMMENT", "ON", MatchAny, MatchAny, MatchAny, MatchAny)) && > !TailMatches1("IS") Is MachAny acceptable? On concern is the two n's (TailMatches and MatchAny) looks a bit confising. > TailMatches4("COMMENT", "ON", MatchAny3, "!IS") regards, -- Kyotaro Horiguchi NTT Open Source Software Center -- 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] Making tab-complete.c easier to maintain
On Mon, Dec 14, 2015 at 8:10 AM, Thomas Munro wrote: > I've also add (very) primitive negative pattern support and used it in > 5 places. Improvement? Examples: > > /* ALTER TABLE xxx RENAME yyy */ > - else if (TailMatches5("ALTER", "TABLE", MatchAny, "RENAME", MatchAny) > && > -!TailMatches1("CONSTRAINT|TO")) > + else if (TailMatches5("ALTER", "TABLE", MatchAny, "RENAME", > "!CONSTRAINT|TO")) > COMPLETE_WITH_CONST("TO"); > > /* If we have CLUSTER , then add "USING" */ > - else if (TailMatches2("CLUSTER", MatchAny) && > !TailMatches1("VERBOSE|ON")) > + else if (TailMatches2("CLUSTER", "!VERBOSE|ON")) > COMPLETE_WITH_CONST("USING"); + /* Handle negated patterns. */ + if (*pattern == '!') + return !word_matches(pattern + 1, word); Yeah, I guess that's an improvement for those cases, and there is no immediate need for a per-keyword NOT operator in our cases to allow things of the type (foo1 OR NOT foo2). Still, in the case of this patch !foo1|foo2 actually means (NOT foo1 AND NOT foo2). It does not seem that much intuitive. Reading straight this expression it seems that !foo1|foo2 means actually (NOT foo1 OR foo2) because the lack of parenthesis. 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
Re: [HACKERS] Making tab-complete.c easier to maintain
On Sun, Dec 13, 2015 at 1:08 AM, Michael Paquier wrote: > On Wed, Dec 9, 2015 at 8:17 PM, Thomas Munro wrote: >> Thanks for looking at this Michael. It's probably not much fun to >> review! Here is a new version with that bit removed. More responses >> inline below. > > Could this patch be rebased? There are now conflicts with 8b469bd7 and > it does not apply cleanly. New version attached. I've also add (very) primitive negative pattern support and used it in 5 places. Improvement? Examples: /* ALTER TABLE xxx RENAME yyy */ - else if (TailMatches5("ALTER", "TABLE", MatchAny, "RENAME", MatchAny) && -!TailMatches1("CONSTRAINT|TO")) + else if (TailMatches5("ALTER", "TABLE", MatchAny, "RENAME", "!CONSTRAINT|TO")) COMPLETE_WITH_CONST("TO"); /* If we have CLUSTER , then add "USING" */ - else if (TailMatches2("CLUSTER", MatchAny) && !TailMatches1("VERBOSE|ON")) + else if (TailMatches2("CLUSTER", "!VERBOSE|ON")) COMPLETE_WITH_CONST("USING"); -- Thomas Munro http://www.enterprisedb.com tab-complete-macrology-v10.patch.gz Description: GNU Zip compressed data -- 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] Making tab-complete.c easier to maintain
On Wed, Dec 9, 2015 at 8:17 PM, Thomas Munro wrote: > Thanks for looking at this Michael. It's probably not much fun to > review! Here is a new version with that bit removed. More responses > inline below. Could this patch be rebased? There are now conflicts with 8b469bd7 and it does not apply cleanly. -- Michael -- 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] Making tab-complete.c easier to maintain
On Sat, Dec 12, 2015 at 12:00 AM, Tom Lane wrote: > Greg Stark writes: >> So I don't think it makes sense to hold up improvements today hoping >> for something like this. > > Yeah, it's certainly a wishlist item rather than something that should > block shorter-term improvements. OK, hence it seems that the next move is to move on with Thomas' stuff. -- Michael -- 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] Making tab-complete.c easier to maintain
Greg Stark writes: > Fwiw I poked at the bison output to see if it would be possible to do. > I think it's theoretically possible but a huge project and would > create dependencies on bison internals that we would be unlikelly to > accept. That's the impression I got when I looked at it briefly, too. Without some new APIs from bison it seems like it'd be way too messy/fragile. > You would need > a) A new protocol message to send the partial query to the server and > get back a list of completions As far as that goes, I'd imagined the functionality continuing to be on the psql side. If we make it wait for a protocol upgrade, that makes it even more improbable that it will ever happen. psql already has its own copy of the lexer, so making it have something derived from the grammar doesn't seem like a maintainability problem. > b) Machinery in bison to return both all terminals that could come > next as well as all possible terminals it could reduce to Yeah, this is the hard part. > d) Some way to deal with the partially parsed query to find out what > schemas, tables, column aliases, etc should be considered for possible > completion I was imagining that some of that knowledge could be pushed back into the grammar. That is, instead of just using generic nonterminals like ColId, we'd need to have TableId, SchemaId, etc and be careful to use the appropriate one(s) in each production of the grammar. Then, psql would know which completion query to issue by noting which of these particular nonterminals is a candidate for the next token right now. However, that moves the goalposts in terms of what we'd have to be able to get back from the alternate bison machinery. Also, it's not just a SMOP to modify the grammar like that: it's not at all unlikely that attempting to introduce such a finer categorization would lead to a broken grammar, ie shift/reduce or reduce/reduce conflicts. We couldn't be sure it would work till we've tried it. > So I don't think it makes sense to hold up improvements today hoping > for something like this. Yeah, it's certainly a wishlist item rather than something that should block shorter-term improvements. 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] Making tab-complete.c easier to maintain
On Fri, Dec 11, 2015 at 8:12 AM, Michael Paquier wrote: > Also, if > we prioritize a dynamically generated tab completion using gram.y, so > be it and let's reject both patches then... Fwiw I poked at the bison output to see if it would be possible to do. I think it's theoretically possible but a huge project and would create dependencies on bison internals that we would be unlikelly to accept. (Unless we can get new API methods added to bison which is not entirely unreasonable). The problem is that bison is only a small part of the problem. You would need a) A new protocol message to send the partial query to the server and get back a list of completions b) Machinery in bison to return both all terminals that could come next as well as all possible terminals it could reduce to c) Some kind of reverse lexer to determine for each terminal what the current partial input would have to match to be accepted d) Some way to deal with the partially parsed query to find out what schemas, tables, column aliases, etc should be considered for possible completion The machinery to do (b) is actually there in bison for the error reporting. It's currently hard coded to limit the output to 5 and there's no API for it, just a function that returns an error string. But it might be possible to get bison to add an API method for it. But that's as far as I got. I have no idea what (c) and (d) would look like. So I don't think it makes sense to hold up improvements today hoping for something like this. What might be more realistic is making sure to design the minilanguage to be easily generated by perl scripts or the like and then write something picking up easy patterns in gram.y or possibly poking through the bison table to generate a table of minilanguage matchers. My instinct is that would be easier to do with a real minilanguage instead of a regular expression system. -- greg -- 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] Making tab-complete.c easier to maintain
On Thu, Dec 10, 2015 at 5:38 PM, Kyotaro HORIGUCHI wrote: > I'm unhappy with context matching using previous_words in two > points. Current code needs human-readable comments describing > almost all matchings. It is hard to maintain and some of them > actually are wrong. The hardness is largely alleviated by > Thomas's approach exept for complex ones. Another is that > previous_words method is not-enough adaptable for optional words > in syntax. For example, CREATE INDEX has a complex syntax and > current rather complex code does not cover it fully (or enough). Yep. > On the other hand, regexp is quite heavy-weight. Current code > does one completion in 1 milliseconds but regexps simplly > replaced with current matching code takes nearly 100ms on my > environment. But appropriate refactoring reduces it to under 10 > ms. That's quite a difference in performance. A good responsiveness is always nice for such things to make the user confortable. > If we need more powerful completion (which means it covers more > wide area of syntax including more optional words), Thomas's > approach would face difficulties of another level of > complexity. I'd like to overcome it. That's a valid concern for sure because the patch of Thomas is not much smart in emulating negative checks, still the main idea to not rely anymore on some checks based on pg_strcmp or similar but have something that is list-based, with a primitive sub-language in it is very appealing. As a next step, more committer and hacker input (people who have worked on tab completion of psql) would be a nice next step. IMO, as someone who has hacked tab-complete.c a couple of times I think that Thomas' patch has merit, now it would make backpatch harder. Also, if we prioritize a dynamically generated tab completion using gram.y, so be it and let's reject both patches then... -- Michael -- 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] Making tab-complete.c easier to maintain
Hello, At Wed, 9 Dec 2015 10:31:06 -0800, David Fetter wrote in <20151209183106.gc10...@fetter.org> > On Wed, Dec 09, 2015 at 03:49:20PM +, Greg Stark wrote: > > On Wed, Dec 9, 2015 at 2:27 PM, David Fetter wrote: > > > Agreed that the "whole new language" aspect seems like way too big a > > > hammer, given what it actually does. > > > > Which would be easier to update when things change? > > This question seems closer to being on point with the patch sets > proposed here. I agree to some extent. I'm unhappy with context matching using previous_words in two points. Current code needs human-readable comments describing almost all matchings. It is hard to maintain and some of them actually are wrong. The hardness is largely alleviated by Thomas's approach exept for complex ones. Another is that previous_words method is not-enough adaptable for optional words in syntax. For example, CREATE INDEX has a complex syntax and current rather complex code does not cover it fully (or enough). On the other hand, regexp is quite heavy-weight. Current code does one completion in 1 milliseconds but regexps simplly replaced with current matching code takes nearly 100ms on my environment. But appropriate refactoring reduces it to under 10 ms. If we need more powerful completion (which means it covers more wide area of syntax including more optional words), Thomas's approach would face difficulties of another level of complexity. I'd like to overcome it. > > Which would be possible to automatically generate from gram.y? > > This seems like it goes to a wholesale context-aware reworking of tab > completion rather than the myopic ("What has happened within the past N > tokens?", for slowly increasing N) versions of tab completions in both > the current code and in the two proposals here. > > A context-aware tab completion wouldn't care how many columns you were > into a target list, or a FROM list, or whatever, as it would complete > based on the (possibly nested) context ("in a target list", e.g.) > rather than on inferences made from some slightly variable number of > previous tokens. It's unknown to me how much the full-context-aware completion makes me happy. But it would be another high-wall to overcome.. regards, -- Kyotaro Horiguchi NTT Open Source Software Center -- 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] Making tab-complete.c easier to maintain
On Thu, Dec 10, 2015 at 12:49 AM, Greg Stark wrote: > On Wed, Dec 9, 2015 at 2:27 PM, David Fetter wrote: >> Agreed that the "whole new language" aspect seems like way too big a >> hammer, given what it actually does. > > Which would be easier to update when things change? Regarding that both patches are equal compared to the current methods with strcmp. > Which would be possible to automatically generate from gram.y? None of those patches take this approach. -- Michael -- 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] Making tab-complete.c easier to maintain
On Wed, Dec 09, 2015 at 03:49:20PM +, Greg Stark wrote: > On Wed, Dec 9, 2015 at 2:27 PM, David Fetter wrote: > > Agreed that the "whole new language" aspect seems like way too big a > > hammer, given what it actually does. > > Which would be easier to update when things change? This question seems closer to being on point with the patch sets proposed here. > Which would be possible to automatically generate from gram.y? This seems like it goes to a wholesale context-aware reworking of tab completion rather than the myopic ("What has happened within the past N tokens?", for slowly increasing N) versions of tab completions in both the current code and in the two proposals here. A context-aware tab completion wouldn't care how many columns you were into a target list, or a FROM list, or whatever, as it would complete based on the (possibly nested) context ("in a target list", e.g.) rather than on inferences made from some slightly variable number of previous tokens. Cheers, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- 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] Making tab-complete.c easier to maintain
On Wed, Dec 9, 2015 at 2:27 PM, David Fetter wrote: > Agreed that the "whole new language" aspect seems like way too big a > hammer, given what it actually does. Which would be easier to update when things change? Which would be possible to automatically generate from gram.y? -- greg -- 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] Making tab-complete.c easier to maintain
On Wed, Dec 09, 2015 at 08:49:22PM +0900, Michael Paquier wrote: > On Wed, Dec 9, 2015 at 8:17 PM, Thomas Munro > wrote: > > On Mon, Dec 7, 2015 at 8:41 PM, Michael Paquier > > wrote: > >> On Tue, Nov 17, 2015 at 12:19 AM, Alvaro Herrera > >> wrote: > >>> Thomas Munro wrote: > New version attached, merging recent changes. > >>> > >>> I wonder about the TailMatches and Matches macros --- wouldn't it be > >>> better to have a single one, renaming TailMatches to Matches and > >>> replacing the current Matches() with an initial token that corresponds > >>> to anchoring to start of command? Just wondering, not terribly attached > >>> to the idea. > >> > >> + /* TODO:TM -- begin temporary, not part of the patch! */ > >> + Assert(!word_matches(NULL, "")); > >> + [...] > >> + Assert(!word_matches("foo", "")); > >> + /* TODO:TM -- end temporary */ > >> > >> Be sure to not forget to remove that later. > > > > Thanks for looking at this Michael. It's probably not much fun to > > review! Here is a new version with that bit removed. More responses > > inline below. > > I had a hard time not sleeping when reading it... That's very mechanical. > > > I agree that would probably be better but Alvaro suggested following > > the existing logic in the first pass, which was mostly based on tails, > > and then considering simpler head-based patterns in a future pass. > > Fine with me. > > So what do we do now? There is your patch, which is already quite big, > but as well a second patch based on regexps, which is far bigger. And > at the end they provide a similar result: > > Here is for example what the regexp patch does for some complex > checks, like ALTER TABLE RENAME: > /* ALTER TABLE xxx RENAME yyy */ > -else if (pg_strcasecmp(prev4_wd, "TABLE") == 0 && > - pg_strcasecmp(prev2_wd, "RENAME") == 0 && > - pg_strcasecmp(prev_wd, "CONSTRAINT") != 0 && > - pg_strcasecmp(prev_wd, "TO") != 0) > +else if (MATCH("TABLE #id RENAME !CONSTRAINT|TO")) > > And what Thomas's patch does: > +else if (TailMatches5("ALTER", "TABLE", MatchAny, "RENAME", MatchAny) && > + !TailMatches1("CONSTRAINT|TO")) > > The regexp patch makes the negative checks somewhat easier to read > (there are 19 positions in tab-complete.c doing that), still inventing > a new langage and having a heavy refactoring just tab completion of > psql seems a bit too much IMO, so my heart balances in favor of > Thomas' stuff. Thoughts from others? Agreed that the "whole new language" aspect seems like way too big a hammer, given what it actually does. Cheers, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- 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] Making tab-complete.c easier to maintain
On Wed, Dec 9, 2015 at 8:17 PM, Thomas Munro wrote: > On Mon, Dec 7, 2015 at 8:41 PM, Michael Paquier > wrote: >> On Tue, Nov 17, 2015 at 12:19 AM, Alvaro Herrera >> wrote: >>> Thomas Munro wrote: New version attached, merging recent changes. >>> >>> I wonder about the TailMatches and Matches macros --- wouldn't it be >>> better to have a single one, renaming TailMatches to Matches and >>> replacing the current Matches() with an initial token that corresponds >>> to anchoring to start of command? Just wondering, not terribly attached >>> to the idea. >> >> + /* TODO:TM -- begin temporary, not part of the patch! */ >> + Assert(!word_matches(NULL, "")); >> + [...] >> + Assert(!word_matches("foo", "")); >> + /* TODO:TM -- end temporary */ >> >> Be sure to not forget to remove that later. > > Thanks for looking at this Michael. It's probably not much fun to > review! Here is a new version with that bit removed. More responses > inline below. I had a hard time not sleeping when reading it... That's very mechanical. > I agree that would probably be better but Alvaro suggested following > the existing logic in the first pass, which was mostly based on tails, > and then considering simpler head-based patterns in a future pass. Fine with me. So what do we do now? There is your patch, which is already quite big, but as well a second patch based on regexps, which is far bigger. And at the end they provide a similar result: Here is for example what the regexp patch does for some complex checks, like ALTER TABLE RENAME: /* ALTER TABLE xxx RENAME yyy */ -else if (pg_strcasecmp(prev4_wd, "TABLE") == 0 && - pg_strcasecmp(prev2_wd, "RENAME") == 0 && - pg_strcasecmp(prev_wd, "CONSTRAINT") != 0 && - pg_strcasecmp(prev_wd, "TO") != 0) +else if (MATCH("TABLE #id RENAME !CONSTRAINT|TO")) And what Thomas's patch does: +else if (TailMatches5("ALTER", "TABLE", MatchAny, "RENAME", MatchAny) && + !TailMatches1("CONSTRAINT|TO")) The regexp patch makes the negative checks somewhat easier to read (there are 19 positions in tab-complete.c doing that), still inventing a new langage and having a heavy refactoring just tab completion of psql seems a bit too much IMO, so my heart balances in favor of Thomas' stuff. Thoughts from others? -- Michael -- 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] Making tab-complete.c easier to maintain
On Mon, Dec 7, 2015 at 8:41 PM, Michael Paquier wrote: > On Tue, Nov 17, 2015 at 12:19 AM, Alvaro Herrera > wrote: >> Thomas Munro wrote: >>> New version attached, merging recent changes. >> >> I wonder about the TailMatches and Matches macros --- wouldn't it be >> better to have a single one, renaming TailMatches to Matches and >> replacing the current Matches() with an initial token that corresponds >> to anchoring to start of command? Just wondering, not terribly attached >> to the idea. > > + /* TODO:TM -- begin temporary, not part of the patch! */ > + Assert(!word_matches(NULL, "")); > + [...] > + Assert(!word_matches("foo", "")); > + /* TODO:TM -- end temporary */ > > Be sure to not forget to remove that later. Thanks for looking at this Michael. It's probably not much fun to review! Here is a new version with that bit removed. More responses inline below. > - else if (pg_strcasecmp(prev5_wd, "DEFAULT") == 0 && > -pg_strcasecmp(prev4_wd, "PRIVILEGES") == 0 && > -(pg_strcasecmp(prev3_wd, "FOR") == 0 || > - pg_strcasecmp(prev3_wd, "IN") == 0)) > - { > - static const char *const > list_ALTER_DEFAULT_PRIVILEGES_REST[] = > - {"GRANT", "REVOKE", NULL}; > - > - COMPLETE_WITH_LIST(list_ALTER_DEFAULT_PRIVILEGES_REST); > - } > + else if (TailMatches5("DEFAULT", "PRIVILEGES", "FOR", "ROLE", > MatchAny) || > +TailMatches5("DEFAULT", "PRIVILEGES", "IN", > "SCHEMA", MatchAny)) > + CompleteWithList2("GRANT", "REVOKE"); > For this chunk I think that you need to check for ROLE|USER and not only > ROLE. Right, done. > + else if (TailMatches4("ALTER", "DOMAIN", MatchAny, "RENAME")) > { > static const char *const list_ALTERDOMAIN[] = > {"CONSTRAINT", "TO", NULL}; > I think you should remove COMPLETE_WITH_LIST here for consistency with the > rest. Right, done. > - else if (pg_strcasecmp(prev5_wd, "DOMAIN") == 0 && > -pg_strcasecmp(prev3_wd, "RENAME") == 0 && > -pg_strcasecmp(prev2_wd, "CONSTRAINT") == 0) > + else if (TailMatches3("RENAME", "CONSTRAINT", MatchAny)) > COMPLETE_WITH_CONST("TO"); > Perhaps you are missing DOMAIN here? Right, done. > - else if (pg_strcasecmp(prev4_wd, "ALTER") == 0 && > -pg_strcasecmp(prev3_wd, "SEQUENCE") == 0 && > -pg_strcasecmp(prev_wd, "NO") == 0) > - { > - static const char *const list_ALTERSEQUENCE2[] = > - {"MINVALUE", "MAXVALUE", "CYCLE", NULL}; > - > - COMPLETE_WITH_LIST(list_ALTERSEQUENCE2); > - } > + else if (TailMatches4("ALTER", "SEQUEMCE", MatchAny, "NO")) > + CompleteWithList3("MINVALUE", "MAXVALUE", "CYCLE"); > Typo here: s/SEQUEMCE/SEQUENCE. Oops, fixed. > - else if (pg_strcasecmp(prev5_wd, "TABLE") == 0 && > -pg_strcasecmp(prev3_wd, "RENAME") == 0 && > -(pg_strcasecmp(prev2_wd, "COLUMN") == 0 || > - pg_strcasecmp(prev2_wd, "CONSTRAINT") == 0) && > -pg_strcasecmp(prev_wd, "TO") != 0) > + else if (TailMatches6("ALTER", "TABLE", MatchAny, "RENAME", > "COLUMN|CONSTRAINT", MatchAny) && > +!TailMatches1("TO")) > This should use TailMatches5 without ALTER for consistency with the existing > code? Ok, done. > - else if (pg_strcasecmp(prev_wd, "CLUSTER") == 0 && > -pg_strcasecmp(prev2_wd, "WITHOUT") != 0) > + else if (TailMatches1("CLUSTER") && !TailMatches2("WITHOUT", > "CLUSTER")) > Here removing CLUSTER should be fine. Ok. > - else if (pg_strcasecmp(prev2_wd, "CLUSTER") == 0 && > -pg_strcasecmp(prev_wd, "ON") != 0 && > -pg_strcasecmp(prev_wd, "VERBOSE") != 0) > - { > + else if (TailMatches2("CLUSTER", MatchAny) && > !TailMatches1("VERBOSE")) > Handling of ON has been forgotten. Right, fixed. > - else if (pg_strcasecmp(prev3_wd, "CREATE") == 0 && > -!(pg_strcasecmp(prev2_wd, "USER") == 0 && > pg_strcasecmp(prev_wd, "MAPPING") == 0) && > -(pg_strcasecmp(prev2_wd, "ROLE") == 0 || > - pg_strcasecmp(prev2_wd, "GROUP") == 0 || > pg_strcasecmp(prev2_wd, "USER") == 0)) > + else if (TailMatches3("CREATE", "ROLE|GROUP|USER", MatchAny) && > +!TailMatches3("CREATE", "USER", "MAPPING")) > !TailMatches2("USER", "MAPPING")? Ok. > - else if (pg_strcasecmp(prev4_wd, "CREATE") == 0 && > -pg_strcasecmp(prev3_wd, "MATERIALIZED") == 0 && > -pg_strcasecmp(prev2_wd, "VIEW") == 0) > + else if (TailMatches3("CREATE", "MATERIALIZED", "VIEW")) > Fo
Re: [HACKERS] Making tab-complete.c easier to maintain
Hello, At Tue, 8 Dec 2015 20:50:39 +0900, Michael Paquier wrote in > On Tue, Dec 8, 2015 at 6:31 PM, Kyotaro HORIGUCHI > wrote: > > > > > > > I believe I haven't ripped off any in CC: list or In-Reply-To and > > References in the previous post. (I read "top-post" as a post > > without these headers.) Could you let me know how the message > > was broken? > > 20151126.144512.10228250.horiguchi.kyot...@lab.ntt.co.jp just did that > when you sent this new patch series: > A: Because it reverses the logical flow of conversation. > Q: Why is top posting frowned upon? > https://www.freebsd.org/doc/en/articles/mailing-list-faq/etiquette.html#idp55416272 Thank you for the reference. I made it because I didn't regard it as a reply to (my) previous message but an update. I'll take care to avoid top-posting in any case, though. regards, -- Kyotaro Horiguchi NTT Open Source Software Center -- 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] Making tab-complete.c easier to maintain
On Tue, Dec 8, 2015 at 6:31 PM, Kyotaro HORIGUCHI wrote: > > Thank you for looking on this and the comment. > > At Mon, 7 Dec 2015 15:00:32 +0900, Michael Paquier > wrote in > > > On Thu, Nov 26, 2015 at 2:45 PM, Kyotaro HORIGUCHI > > wrote: > > > What do you think about this solution? > > > > > > I believe I haven't ripped off any in CC: list or In-Reply-To and > References in the previous post. (I read "top-post" as a post > without these headers.) Could you let me know how the message > was broken? 20151126.144512.10228250.horiguchi.kyot...@lab.ntt.co.jp just did that when you sent this new patch series: A: Because it reverses the logical flow of conversation. Q: Why is top posting frowned upon? https://www.freebsd.org/doc/en/articles/mailing-list-faq/etiquette.html#idp55416272 -- Michael -- 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] Making tab-complete.c easier to maintain
Thank you for looking on this and the comment. At Mon, 7 Dec 2015 15:00:32 +0900, Michael Paquier wrote in > On Thu, Nov 26, 2015 at 2:45 PM, Kyotaro HORIGUCHI > wrote: > > What do you think about this solution? > > I believe I haven't ripped off any in CC: list or In-Reply-To and References in the previous post. (I read "top-post" as a post without these headers.) Could you let me know how the message was broken? > This patch fails to compile on OSX: > Undefined symbols for architecture x86_64: > "_ExceptionalCondition", referenced from: > _pg_regexec in regexec.o > _cfind in regexec.o > _find in regexec.o > _newdfa in regexec.o > _cfindloop in regexec.o > _shortest in regexec.o > _cdissect in regexec.o > ... > So, to begin with, this may be better if replugged as a standalone > library, aka moving the regexp code into src/common for example or > similar. I agree to that. I'll consider doing so. (But my middle finger tip injury makes me further slower than usual..) > Also, per the comments on top of rcancelrequested, > rstacktoodeep and rcancelrequested, returning unconditionally 0 is not > a good idea for -DFRONTEND. Callbacks should be defined and made > available for callers. cancel_pressed is usable for the purpose and I'll add cancel_callback feeature to separate it from both frontend and backend. > - {"EVENT TRIGGER", NULL, NULL}, > + {"EVENT TRIGGER", Query_for_list_of_event_triggers, NULL}, > {"EXTENSION", Query_for_list_of_extensions}, > - {"FOREIGN DATA WRAPPER", NULL, NULL}, > + {"FOREIGN DATA WRAPPER", Query_for_list_of_fdws, NULL}, > {"FOREIGN TABLE", NULL, NULL}, > {"FUNCTION", NULL, &Query_for_list_of_functions}, > {"GROUP", Query_for_list_of_roles}, > {"LANGUAGE", Query_for_list_of_languages}, > {"INDEX", NULL, &Query_for_list_of_indexes}, > - {"MATERIALIZED VIEW", NULL, NULL}, > + {"MATERIALIZED VIEW", NULL, &Query_for_list_of_matviews}, > This has value as a separate patch. I carelessly merged it in the fourth (Merge mergable...) patch. I'll separate it. > The patch has many whitespaces, and unrelated diffs. Mmm, thanks for pointing it out. I haven't see such lines differ only in whitespaces or found unrelated diffs so far but I'll check it out. regards, -- Kyotaro Horiguchi NTT Open Source Software Center -- 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] Making tab-complete.c easier to maintain
On Tue, Nov 17, 2015 at 12:19 AM, Alvaro Herrera wrote: > Thomas Munro wrote: >> New version attached, merging recent changes. > > I wonder about the TailMatches and Matches macros --- wouldn't it be > better to have a single one, renaming TailMatches to Matches and > replacing the current Matches() with an initial token that corresponds > to anchoring to start of command? Just wondering, not terribly attached > to the idea. + /* TODO:TM -- begin temporary, not part of the patch! */ + Assert(!word_matches(NULL, "")); + [...] + Assert(!word_matches("foo", "")); + /* TODO:TM -- end temporary */ Be sure to not forget to remove that later. - else if (pg_strcasecmp(prev5_wd, "DEFAULT") == 0 && -pg_strcasecmp(prev4_wd, "PRIVILEGES") == 0 && -(pg_strcasecmp(prev3_wd, "FOR") == 0 || - pg_strcasecmp(prev3_wd, "IN") == 0)) - { - static const char *const list_ALTER_DEFAULT_PRIVILEGES_REST[] = - {"GRANT", "REVOKE", NULL}; - - COMPLETE_WITH_LIST(list_ALTER_DEFAULT_PRIVILEGES_REST); - } + else if (TailMatches5("DEFAULT", "PRIVILEGES", "FOR", "ROLE", MatchAny) || +TailMatches5("DEFAULT", "PRIVILEGES", "IN", "SCHEMA", MatchAny)) + CompleteWithList2("GRANT", "REVOKE"); For this chunk I think that you need to check for ROLE|USER and not only ROLE. + else if (TailMatches4("ALTER", "DOMAIN", MatchAny, "RENAME")) { static const char *const list_ALTERDOMAIN[] = {"CONSTRAINT", "TO", NULL}; I think you should remove COMPLETE_WITH_LIST here for consistency with the rest. - else if (pg_strcasecmp(prev5_wd, "DOMAIN") == 0 && -pg_strcasecmp(prev3_wd, "RENAME") == 0 && -pg_strcasecmp(prev2_wd, "CONSTRAINT") == 0) + else if (TailMatches3("RENAME", "CONSTRAINT", MatchAny)) COMPLETE_WITH_CONST("TO"); Perhaps you are missing DOMAIN here? - else if (pg_strcasecmp(prev4_wd, "ALTER") == 0 && -pg_strcasecmp(prev3_wd, "SEQUENCE") == 0 && -pg_strcasecmp(prev_wd, "NO") == 0) - { - static const char *const list_ALTERSEQUENCE2[] = - {"MINVALUE", "MAXVALUE", "CYCLE", NULL}; - - COMPLETE_WITH_LIST(list_ALTERSEQUENCE2); - } + else if (TailMatches4("ALTER", "SEQUEMCE", MatchAny, "NO")) + CompleteWithList3("MINVALUE", "MAXVALUE", "CYCLE"); Typo here: s/SEQUEMCE/SEQUENCE. - else if (pg_strcasecmp(prev5_wd, "TABLE") == 0 && -pg_strcasecmp(prev3_wd, "RENAME") == 0 && -(pg_strcasecmp(prev2_wd, "COLUMN") == 0 || - pg_strcasecmp(prev2_wd, "CONSTRAINT") == 0) && -pg_strcasecmp(prev_wd, "TO") != 0) + else if (TailMatches6("ALTER", "TABLE", MatchAny, "RENAME", "COLUMN|CONSTRAINT", MatchAny) && +!TailMatches1("TO")) This should use TailMatches5 without ALTER for consistency with the existing code? - else if (pg_strcasecmp(prev_wd, "CLUSTER") == 0 && -pg_strcasecmp(prev2_wd, "WITHOUT") != 0) + else if (TailMatches1("CLUSTER") && !TailMatches2("WITHOUT", "CLUSTER")) Here removing CLUSTER should be fine. - else if (pg_strcasecmp(prev2_wd, "CLUSTER") == 0 && -pg_strcasecmp(prev_wd, "ON") != 0 && -pg_strcasecmp(prev_wd, "VERBOSE") != 0) - { + else if (TailMatches2("CLUSTER", MatchAny) && !TailMatches1("VERBOSE")) Handling of ON has been forgotten. - else if (pg_strcasecmp(prev3_wd, "CREATE") == 0 && -!(pg_strcasecmp(prev2_wd, "USER") == 0 && pg_strcasecmp(prev_wd, "MAPPING") == 0) && -(pg_strcasecmp(prev2_wd, "ROLE") == 0 || - pg_strcasecmp(prev2_wd, "GROUP") == 0 || pg_strcasecmp(prev2_wd, "USER") == 0)) + else if (TailMatches3("CREATE", "ROLE|GROUP|USER", MatchAny) && +!TailMatches3("CREATE", "USER", "MAPPING")) !TailMatches2("USER", "MAPPING")? - else if (pg_strcasecmp(prev4_wd, "CREATE") == 0 && -pg_strcasecmp(prev3_wd, "MATERIALIZED") == 0 && -pg_strcasecmp(prev2_wd, "VIEW") == 0) + else if (TailMatches3("CREATE", "MATERIALIZED", "VIEW")) Forgot a MatchAny here? - else if (pg_strcasecmp(prev_wd, "DELETE") == 0 && -!(pg_strcasecmp(prev2_wd, "ON") == 0 || - pg_strcasecmp(prev2_wd, "GRANT") == 0 || - pg_strcasecmp(prev2_wd, "BEFORE") == 0 || - pg_strcasecmp(prev2_wd, "AFTER") == 0)) + else if (TailMatches1("DELETE") && !TailMatches2("ON|GRANT|BEFORE|AFTER", "DELETE")) COMPLETE_WITH_CONST("FR
Re: [HACKERS] Making tab-complete.c easier to maintain
On Thu, Nov 26, 2015 at 2:45 PM, Kyotaro HORIGUCHI wrote: > What do you think about this solution? This patch fails to compile on OSX: Undefined symbols for architecture x86_64: "_ExceptionalCondition", referenced from: _pg_regexec in regexec.o _cfind in regexec.o _find in regexec.o _newdfa in regexec.o _cfindloop in regexec.o _shortest in regexec.o _cdissect in regexec.o ... So, to begin with, this may be better if replugged as a standalone library, aka moving the regexp code into src/common for example or similar. Also, per the comments on top of rcancelrequested, rstacktoodeep and rcancelrequested, returning unconditionally 0 is not a good idea for -DFRONTEND. Callbacks should be defined and made available for callers. - {"EVENT TRIGGER", NULL, NULL}, + {"EVENT TRIGGER", Query_for_list_of_event_triggers, NULL}, {"EXTENSION", Query_for_list_of_extensions}, - {"FOREIGN DATA WRAPPER", NULL, NULL}, + {"FOREIGN DATA WRAPPER", Query_for_list_of_fdws, NULL}, {"FOREIGN TABLE", NULL, NULL}, {"FUNCTION", NULL, &Query_for_list_of_functions}, {"GROUP", Query_for_list_of_roles}, {"LANGUAGE", Query_for_list_of_languages}, {"INDEX", NULL, &Query_for_list_of_indexes}, - {"MATERIALIZED VIEW", NULL, NULL}, + {"MATERIALIZED VIEW", NULL, &Query_for_list_of_matviews}, This has value as a separate patch. The patch has many whitespaces, and unrelated diffs. -- Michael -- 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] Making tab-complete.c easier to maintain
Hello, I tried to implement the mini-language, which is a simplified reglar expression for this specific use. As a ultra-POC, the attached patch has very ad-hoc preprocess function and does on-the-fly preprocessing, compilation then execution of regular expression. And it is applied to only the first ten or so matchings in psql_completion(). The first attachment is the same with that of previous patchset. Every matching line looks like the following, > else if (RM("ALTER {AGGREGATE|FUNCTION} [#id](..")) > COMPLETE_WITH_FUNCTION_ARG(CAPTURE(1)); As a temporary desin, "{}" means grouping, "|" means alternatives, "[]" means capture and '#id' means any identifier. The largest problem of this would be its computation cost:( This in turn might be too slow if about three hundred matches run... I see no straight-forward way to preprocess these strings.. A possible solution would be extracting these strings then auto-generate a c-srouce to preprocess them. And when running, RM() retrieves the compiled regular expression using the string as the key. At Tue, 17 Nov 2015 15:35:43 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI wrote in <20151117.153543.183036803.horiguchi.kyot...@lab.ntt.co.jp> > At Mon, 16 Nov 2015 12:16:07 -0300, Alvaro Herrera > wrote in <20151116151606.GW614468@alvherre.pgsql> > > I don't think this is an improvement. It seems to me that a lot more > > work is required to maintain these regular expressions, which while > > straightforward are not entirely trivial (harder to read). > > > > If we could come up with a reasonable format that is pre-processed to a > > regexp, that might be better. I think Thomas' proposed patch is closer > > to that than Horiguchi-san's. > > I aimed that matching mechanism can handle optional elements in > syntexes more robustly. But as all you are saying, bare regular > expression is too complex here. At Tue, 17 Nov 2015 16:09:25 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI wrote in <20151117.160925.45883793.horiguchi.kyot...@lab.ntt.co.jp> > if (Match("^,ALTER,TABLE,\id,$") || > Match("^,ALTER,TABLE,IF,EXISTS,\id,$"))... > > Interpreting this kind of mini-language into regular expressions > could be doable.. regards, -- Kyotaro Horiguchi NTT Open Source Software Center >From cdc0b9cce43e38463af0b2b7ad4a0181f41995a2 Mon Sep 17 00:00:00 2001 From: Kyotaro Horiguchi Date: Fri, 30 Oct 2015 18:03:18 +0900 Subject: [PATCH 1/2] Allow regex module to be used outside server. To make regular expression available on frontend, enable pg_regex module and the files included from it to be detached from the features not available when used outside backend. --- src/backend/regex/regc_pg_locale.c | 7 +++ src/backend/regex/regcomp.c| 12 src/include/regex/regcustom.h | 6 ++ src/include/utils/pg_locale.h | 7 +-- 4 files changed, 30 insertions(+), 2 deletions(-) diff --git a/src/backend/regex/regc_pg_locale.c b/src/backend/regex/regc_pg_locale.c index b707b06..a631ac2 100644 --- a/src/backend/regex/regc_pg_locale.c +++ b/src/backend/regex/regc_pg_locale.c @@ -220,6 +220,13 @@ static const unsigned char pg_char_properties[128] = { }; +/* Get rid of using server-side feature elsewhere of server. */ +#ifdef FRONTEND +#define lc_ctype_is_c(col) (true) +#define GetDatabaseEncoding() PG_UTF8 +#define ereport(x,...) exit(1) +#endif + /* * pg_set_regex_collation: set collation for these functions to obey * diff --git a/src/backend/regex/regcomp.c b/src/backend/regex/regcomp.c index a165b3b..b35ccb4 100644 --- a/src/backend/regex/regcomp.c +++ b/src/backend/regex/regcomp.c @@ -2040,11 +2040,17 @@ rfree(regex_t *re) * The current implementation is Postgres-specific. If we ever get around * to splitting the regex code out as a standalone library, there will need * to be some API to let applications define a callback function for this. + * + * This check is available only on server side. */ static int rcancelrequested(void) { +#ifndef FRONTEND return InterruptPending && (QueryCancelPending || ProcDiePending); +#else + return 0; +#endif } /* @@ -2056,11 +2062,17 @@ rcancelrequested(void) * The current implementation is Postgres-specific. If we ever get around * to splitting the regex code out as a standalone library, there will need * to be some API to let applications define a callback function for this. + * + * This check is available only on server side. */ static int rstacktoodeep(void) { +#ifndef FRONTEND return stack_is_too_deep(); +#else + return 0; +#endif } #ifdef REG_DEBUG diff --git a/src/include/regex/regcustom.h b/src/include/regex/regcustom.h index dbb461a..ffc4031 100644 --- a/src/include/regex/regcustom.h +++ b/src/include/regex/regcustom.h @@ -29,7 +29,11 @@ */ /* headers if any */ +#ifdef FRONTEND +#include "postgres_fe.h" +#else #include "postgres.h" +#endif #include #include @@ -53,7 +57,9 @@ #define MALLOC(n) malloc(n) #define FREE
Re: [HACKERS] Making tab-complete.c easier to maintain
Hello, At Mon, 16 Nov 2015 12:19:23 -0300, Alvaro Herrera wrote in <20151116151923.GX614468@alvherre.pgsql> > Thomas Munro wrote: > > New version attached, merging recent changes. > > I wonder about the TailMatches and Matches macros --- wouldn't it be > better to have a single one, renaming TailMatches to Matches and > replacing the current Matches() with an initial token that corresponds > to anchoring to start of command? Just wondering, not terribly attached > to the idea. Does it looks like this? > if (Match(BOL, "ALTER", "TABLE", EOL)) ... It would be doable giving special meaning to word_matches(BOL/EOL, *_wd). And I give +1 to that than having so many similar macros. The following is my delusion.. It could develop to some mini-laguages like the following, which is a kind of subset of regular expressions, that is powerful than current mechanism but not meesier than regular expressions by narrowing its functionarity. Addition to that the custom minilang could have specilized functionarity for matching SQL statements. > if (Match("^ALTER TABLE \id$"))... It would be nice to have tokens to match to optional words. > if (Match("^ALTER TABLE(IF EXISTS) \id$"))... > if (Match("CREATE(OR REPLACE)FUNCTION \id (\CSL)$") Mmm. this might be another kind of complexity? This is also accomplished by multiple matching descriptions. > if (Match("^,ALTER,TABLE,\id,$") || > Match("^,ALTER,TABLE,IF,EXISTS,\id,$"))... Interpreting this kind of mini-language into regular expressions could be doable.. regards, -- Kyotaro Horiguchi NTT Open Source Software Center -- 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] Making tab-complete.c easier to maintain
Hello, thank you for many valuable opinions. I am convinced that bare regular expressions cannot be applied here:) At Mon, 16 Nov 2015 18:59:06 +1300, Thomas Munro wrote in > It's an interesting idea to use regular expressions, but it's a shame to > move the patterns so far away from the actions they trigger. Yes, I agree. RE is powerful tool but too complicated. Some intermediate expression would be needed but it also adds different kind of complexity. At Mon, 16 Nov 2015 23:09:52 +0900, Michael Paquier wrote in > This makes the situation messier. At least with Thomas' patch one can > immediately know the list of words that are being matched for a given > code path while with this patch we need to have a look to the regex > where they are list. And this would get more and more complicated with > new commands added. Hmm, so I named the enum items to reflect its pattern but even though I also think it is one of the problem of using regular expressions. At Mon, 16 Nov 2015 12:16:07 -0300, Alvaro Herrera wrote in <20151116151606.GW614468@alvherre.pgsql> > I don't think this is an improvement. It seems to me that a lot more > work is required to maintain these regular expressions, which while > straightforward are not entirely trivial (harder to read). > > If we could come up with a reasonable format that is pre-processed to a > regexp, that might be better. I think Thomas' proposed patch is closer > to that than Horiguchi-san's. I aimed that matching mechanism can handle optional elements in syntexes more robustly. But as all you are saying, bare regular expression is too complex here. regares, -- Kyotaro Horiguchi NTT Open Source Software Center -- 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] Making tab-complete.c easier to maintain
Thomas Munro wrote: > New version attached, merging recent changes. I wonder about the TailMatches and Matches macros --- wouldn't it be better to have a single one, renaming TailMatches to Matches and replacing the current Matches() with an initial token that corresponds to anchoring to start of command? Just wondering, not terribly attached to the idea. -- Álvaro Herrerahttp://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
Re: [HACKERS] Making tab-complete.c easier to maintain
Kyotaro HORIGUCHI wrote: > Auto-generating from grammer should be the ultimate solution but > I don't think it will be available. But still I found that the > word-splitting-then-match-word-by-word-for-each-matching is > terriblly unmaintainable and poorly capable. So, how about > regular expressions? I don't think this is an improvement. It seems to me that a lot more work is required to maintain these regular expressions, which while straightforward are not entirely trivial (harder to read). If we could come up with a reasonable format that is pre-processed to a regexp, that might be better. I think Thomas' proposed patch is closer to that than Horiguchi-san's. -- Álvaro Herrerahttp://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
Re: [HACKERS] Making tab-complete.c easier to maintain
On Thu, Nov 12, 2015 at 1:16 PM, Kyotaro HORIGUCHI wrote: > 1. 0001-Allow-regex-module-to-be-used-outside-server.patch > > This small change makes pg_regex possible to be used in > frontend. This is generic enough to live in src/common, then psql would directly reuse it using lpgcommon. > 2. 0002-Replace-previous-matching-rule-with-regexps.patch > > Simply replaces existing matching rules almost one-by-one with > regular expression matches. This makes the situation messier. At least with Thomas' patch one can immediately know the list of words that are being matched for a given code path while with this patch we need to have a look to the regex where they are list. And this would get more and more complicated with new commands added. -- Michael -- 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] Making tab-complete.c easier to maintain
On Thu, Nov 12, 2015 at 5:16 PM, Kyotaro HORIGUCHI < horiguchi.kyot...@lab.ntt.co.jp> wrote: > Hello. How about regular expressions? > > I've been thinking of better mechanism for tab-compltion for > these days since I found some bugs in it. > > At Fri, 23 Oct 2015 14:50:58 -0300, Alvaro Herrera < > alvhe...@2ndquadrant.com> wrote in <20151023175058.GA3391@alvherre.pgsql> > > Jeff Janes wrote: > > > > > For the bigger picture, I don't think we should not apply this patch > simply > > > because there is something even better we might theoretically do at > some > > > point in the future. > > > > Agreed. > > Auto-generating from grammer should be the ultimate solution but > I don't think it will be available. But still I found that the > word-splitting-then-match-word-by-word-for-each-matching is > terriblly unmaintainable and poorly capable. So, how about > regular expressions? > > I tried to use pg_regex in frontend and found that it is easily > doable. As a proof of the concept, the two patches attached to > this message does that changes. > > 1. 0001-Allow-regex-module-to-be-used-outside-server.patch > > This small change makes pg_regex possible to be used in > frontend. > > 2. 0002-Replace-previous-matching-rule-with-regexps.patch > > Simply replaces existing matching rules almost one-by-one with > regular expression matches. > > I made these patches not to change the behavior except inevitable > ones. > > We would have far powerful matching capability using regular > expressions and it makes tab-complete.c look simpler. On the > other hand, regular expressions - which are stashed away into new > file by this patch - is a chunk of complexity and (also) error > prone. For all that I think this is better than the current > situation in terms of maintainability and capability. > > This should look stupid because it really be replaced stupidly > and of course this can be more sane/effective/maintainable by > refactoring. But before that issue, I'm not confident at all that > this is really a alternative with *gigantic* improvement. > > Any opinions? > It's an interesting idea to use regular expressions, but it's a shame to move the patterns so far away from the actions they trigger. -- Thomas Munro http://www.enterprisedb.com
Re: [HACKERS] Making tab-complete.c easier to maintain
New version attached, merging recent changes. -- Thomas Munro http://www.enterprisedb.com tab-complete-v8.patch.gz Description: GNU Zip compressed data -- 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] Making tab-complete.c easier to maintain
On Thu, Oct 22, 2015 at 10:36 PM, Tom Lane wrote: > What I would like is to find a way to auto-generate basically this entire > file from gram.y. That would imply going over to something at least > somewhat parser-based, instead of the current way that is more or less > totally ad-hoc. That would be a very good thing though, because the > current way gives wrong answers not-infrequently, even discounting cases > that it's simply not been taught about. I always assumed the reason we didn't use the bison grammar table to generate completions was because the grammar is way too general and there would be way too many spurious completions that in practice nobody would ever be interested in. I assumed it was an intentional choice that it was more helpful to complete things we know people usually want rather than every theoretically possible next token. If that's not true then maybe I'll poke at this sometime. But I agree with the other part of this thread that that would be totally experimental and even if we had a working patch it would be a long time before the user experience was up to the same level as the current behaviour. I suspect it would involve sending the partial query to the server for parsing and asking for feedback on completions using the grammar parser table and the search_path object resolution rules in effect. -- greg -- 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] Making tab-complete.c easier to maintain
On Sat, Oct 24, 2015 at 6:19 AM, Jeff Janes wrote: > On Sun, Oct 18, 2015 at 9:12 PM, Thomas Munro > wrote: >> Thanks for taking a look at this! The word count returned by >> get_previous_words was incorrect. Here is a corrected version. > > I haven't looked at v6 yet, but in v5: > > "set work_mem TO" completes to "NULL" not to "DEFAULT" > > line 2665 of the patched tab complete file,, should be "DEFAULT", not "NULL" > as the completion string. Looks like a simple copy and paste error. Indeed. Thanks. Fixed in the attached. -- Thomas Munro http://www.enterprisedb.com tab-complete-macrology-v7.patch.gz Description: GNU Zip compressed data -- 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] Making tab-complete.c easier to maintain
Jeff Janes wrote: > For the bigger picture, I don't think we should not apply this patch simply > because there is something even better we might theoretically do at some > point in the future. Agreed. > Having used it a little bit, I do agree with Robert > that it is not a gigantic improvement over the current situation, as the > code it replaces is largely mechanical boilerplate. But I think it is > enough of an improvement that we should go ahead with it. To me this patch sounds much like 2eafcf68d563df8a1db80a. You could say that what was replaced was "largely mechanical", but it was so much easier to make mistakes with the original coding that it's not funny. -- Álvaro Herrerahttp://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
Re: [HACKERS] Making tab-complete.c easier to maintain
On Sun, Oct 18, 2015 at 9:12 PM, Thomas Munro wrote: > > Thanks for taking a look at this! The word count returned by > get_previous_words was incorrect. Here is a corrected version. > I haven't looked at v6 yet, but in v5: "set work_mem TO" completes to "NULL" not to "DEFAULT" line 2665 of the patched tab complete file,, should be "DEFAULT", not "NULL" as the completion string. Looks like a simple copy and paste error. For the bigger picture, I don't think we should not apply this patch simply because there is something even better we might theoretically do at some point in the future. Having used it a little bit, I do agree with Robert that it is not a gigantic improvement over the current situation, as the code it replaces is largely mechanical boilerplate. But I think it is enough of an improvement that we should go ahead with it. Cheers, Jeff
Re: [HACKERS] Making tab-complete.c easier to maintain
David Fetter writes: > Is it really on us as a community to go long distances out of our way > to assuage the baseless[1] paranoia of people who are by and large not > part of our community? While I personally don't care that much about the proprietary-psql-variant scenario, I do care whether people using Debian packages will have a good experience with psql. And frankly, sir, you do not have the standing to call their fears baseless. You can believe that they are, and you might even be right, but your opinion on such matters does not matter to them. 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] Making tab-complete.c easier to maintain
On Fri, Oct 23, 2015 at 12:15:01PM -0400, Robert Haas wrote: > On Fri, Oct 23, 2015 at 11:16 AM, David Fetter wrote: > > Proprietary, secret changes to the back end, sure, but the client? > > The most recent example I recall of that is Netezza, and I suspect > > that they just couldn't be bothered to publish the changes they > > made. At that time, the community psql client was not by any > > means as nice as it is now, so it's conceivable that they made > > substantive improvements, at least for talking to Netezza DBs. > > > >> Most have added backend features and I guess many of those have > >> in turn added support to psql for those features. Sure it'd > >> probably in reality be relatively harmless for them to release > >> these psql modifications, but I rather doubt their management > >> will generally see it that way. > > > > Is it really on us as a community to go long distances out of our > > way to assuage the baseless[1] paranoia of people who are by and > > large not part of our community? > > I was under the impression that I was part of this community, and I > have already said that my employer has added tab completion support, > and other psql features, related to the server features we have > added. > > Also, your statement that this is a long distance out of our way > does not seem to be justified by the facts. As I wrote in the part you cut, we can continue to pretend libedit is a viable alternative if it keeps some feathers unruffled. If libedit "support" gets to be a real drag, we can and should reconsider with a strong bias to dropping it. Cheers, David. P.S. With a little luck, our next license or other perceived legal conflict will be as easy to sort out as this one. I have the sinking feeling that anything we do with crypto or hashing more secure than MD5 will run afoul of someone's idea of what the law is in their country, and the impacts will be a lot nastier than this. We will need to deal with that separately, as needs arise. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- 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] Making tab-complete.c easier to maintain
On Fri, Oct 23, 2015 at 11:16 AM, David Fetter wrote: > Proprietary, secret changes to the back end, sure, but the client? > The most recent example I recall of that is Netezza, and I suspect > that they just couldn't be bothered to publish the changes they made. > At that time, the community psql client was not by any means as nice > as it is now, so it's conceivable that they made substantive > improvements, at least for talking to Netezza DBs. > >> Most have added backend features and I guess many of those have in >> turn added support to psql for those features. Sure it'd probably >> in reality be relatively harmless for them to release these psql >> modifications, but I rather doubt their management will generally >> see it that way. > > Is it really on us as a community to go long distances out of our way > to assuage the baseless[1] paranoia of people who are by and large not > part of our community? I was under the impression that I was part of this community, and I have already said that my employer has added tab completion support, and other psql features, related to the server features we have added. Also, your statement that this is a long distance out of our way does not seem to be justified by the facts. -- 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
Re: [HACKERS] Making tab-complete.c easier to maintain
On Fri, Oct 23, 2015 at 02:09:43AM +0200, Andres Freund wrote: > On 2015-10-22 16:26:10 -0700, David Fetter wrote: > > To be affective negatively by libreadline's viral license, an entity > > would need to fork the psql client in proprietary ways that they did > > not wish not to make available to end users, at the same time linking > > in libreadline. > > > Maybe I'm missing something big, but I really don't see people out > > there shipping a libreadline-enabled psql client, details of whose > > source they'd want to keep a deep, dark secret. > > Isn't that just about every proprietary fork of postgres? Proprietary versions of the psql client? I'm not sure I understand. Proprietary, secret changes to the back end, sure, but the client? The most recent example I recall of that is Netezza, and I suspect that they just couldn't be bothered to publish the changes they made. At that time, the community psql client was not by any means as nice as it is now, so it's conceivable that they made substantive improvements, at least for talking to Netezza DBs. > Most have added backend features and I guess many of those have in > turn added support to psql for those features. Sure it'd probably > in reality be relatively harmless for them to release these psql > modifications, but I rather doubt their management will generally > see it that way. Is it really on us as a community to go long distances out of our way to assuage the baseless[1] paranoia of people who are by and large not part of our community? As to our "support" of libedit, feel free to try working with a psql client that has libedit instead of libreadline for a few weeks. It would be a very long stretch, at least for the work flows I've seen so far, to call that client functional. The strongest praise I can come up with is that it's usually not quite as awful as working with a psql client entirely devoid of command line editing support. Of course, we can continue to pretend that there is a viable alternative to libreadline if that soothes down some feathers, but I don't see any reason to make substantive sacrifices in service of keeping that particular box checked. Cheers, David. [1] as far as I know, and I'll take this back in its entirety if someone shows me a reasonable basis -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- 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] Making tab-complete.c easier to maintain
* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: > Stephen Frost wrote: > > > The issue for Debian, at least, isn't really about libreadline or > > libedit, it's about the GPL and the OpenSSL license. From the Debian > > perspective, if we were able to build with GNUTLS or another SSL library > > other than OpenSSL (which I know we've made some progress on, thanks > > Heikki...) then Debian wouldn't have any problem shipping PG linked with > > libreadline. > > What if OpenSSL were to switch to APL2? > http://www.postgresql.org/message-id/20150801151410.ga28...@awork2.anarazel.de According to that post, it would depend what libreadline is licensed under. There may be other complications with GPL3 and other libraries we link against, though I can't think of any offhand and perhaps there aren't any. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Making tab-complete.c easier to maintain
Stephen Frost wrote: > The issue for Debian, at least, isn't really about libreadline or > libedit, it's about the GPL and the OpenSSL license. From the Debian > perspective, if we were able to build with GNUTLS or another SSL library > other than OpenSSL (which I know we've made some progress on, thanks > Heikki...) then Debian wouldn't have any problem shipping PG linked with > libreadline. What if OpenSSL were to switch to APL2? http://www.postgresql.org/message-id/20150801151410.ga28...@awork2.anarazel.de -- Álvaro Herrerahttp://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
Re: [HACKERS] Making tab-complete.c easier to maintain
* Tom Lane (t...@sss.pgh.pa.us) wrote: > David Fetter writes: > > On Thu, Oct 22, 2015 at 04:05:06PM -0700, Tom Lane wrote: > >> Given the license issues around GNU readline, requiring it seems like > >> probably a non-starter. > > > I should have made this more clear. I am not an IP attorney and don't > > play one on TV, but this is what I've gotten out of conversations with > > IP attorneys on this subject: > > I'm not an IP attorney either, and the ones I've talked to seem to agree > with you. (Red Hat's corporate attorneys, at least, certainly thought > that when I last asked them.) But the observed facts are that some > distros refuse to ship psql-linked-to-readline, and substitute libedit > instead, because they got different advice from their attorneys. The issue for Debian, at least, isn't really about libreadline or libedit, it's about the GPL and the OpenSSL license. From the Debian perspective, if we were able to build with GNUTLS or another SSL library other than OpenSSL (which I know we've made some progress on, thanks Heikki...) then Debian wouldn't have any problem shipping PG linked with libreadline. > If we desupport libedit, the end result is going to be these distros > shipping psql with no command-line-editing support at all. Our opinion, > or even our lawyers' opinion, is unlikely to prevent that outcome. > > While libedit certainly has got its bugs and deficiencies, it's way better > than nothing at all, so I think we'd better keep supporting it. I agree that it's probably not a good idea to desupport libedit. However, I don't believe supporting libedit necessairly prevents us from providing better support when libreadline is available. Debian-based distributions won't be able to reap the benefits of that better support until we remove the OpenSSL dependency, which is unfortunate, but other distributions would be able to. Also, if the PG project is agreeable to it, there's no reason why we couldn't provide full libreadline-enabled builds off of apt.postgresql.org, regardless of what Debian wants. That would add a bit of extra burden on our package maintainers, as there isn't any difference between packages built for Debian and packages built for apt.p.o currently, I don't believe, but it seems like it'd be a pretty minor change that would be well worth it if we get better libreadline integration. The one bit of all of this that worries me is that we would take libreadline for granted and the libedit support would end up broken. That seems a relatively minor concern though, as it sounds like there is a fair bit of exercise of the libedit path due to the commercial forks of PG which make use of it to avoid the GPL. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Making tab-complete.c easier to maintain
On Thu, Oct 22, 2015 at 8:09 PM, Andres Freund wrote: > On 2015-10-22 16:26:10 -0700, David Fetter wrote: >> To be affective negatively by libreadline's viral license, an entity >> would need to fork the psql client in proprietary ways that they did >> not wish not to make available to end users, at the same time linking >> in libreadline. > >> Maybe I'm missing something big, but I really don't see people out >> there shipping a libreadline-enabled psql client, details of whose >> source they'd want to keep a deep, dark secret. > > Isn't that just about every proprietary fork of postgres? Most have > added backend features and I guess many of those have in turn added > support to psql for those features. Sure it'd probably in reality be > relatively harmless for them to release these psql modifications, but I > rather doubt their management will generally see it that way. Yeah, exactly. EnterpriseDB have to keep libedit working even if the PostgreSQL community dropped support, so I hope we don't decide to do that. -- 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
Re: [HACKERS] Making tab-complete.c easier to maintain
On 2015-10-22 16:26:10 -0700, David Fetter wrote: > To be affective negatively by libreadline's viral license, an entity > would need to fork the psql client in proprietary ways that they did > not wish not to make available to end users, at the same time linking > in libreadline. > Maybe I'm missing something big, but I really don't see people out > there shipping a libreadline-enabled psql client, details of whose > source they'd want to keep a deep, dark secret. Isn't that just about every proprietary fork of postgres? Most have added backend features and I guess many of those have in turn added support to psql for those features. Sure it'd probably in reality be relatively harmless for them to release these psql modifications, but I rather doubt their management will generally see it that way. Greetings, Andres Freund -- 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] Making tab-complete.c easier to maintain
David Fetter writes: > On Thu, Oct 22, 2015 at 04:05:06PM -0700, Tom Lane wrote: >> Given the license issues around GNU readline, requiring it seems like >> probably a non-starter. > I should have made this more clear. I am not an IP attorney and don't > play one on TV, but this is what I've gotten out of conversations with > IP attorneys on this subject: I'm not an IP attorney either, and the ones I've talked to seem to agree with you. (Red Hat's corporate attorneys, at least, certainly thought that when I last asked them.) But the observed facts are that some distros refuse to ship psql-linked-to-readline, and substitute libedit instead, because they got different advice from their attorneys. If we desupport libedit, the end result is going to be these distros shipping psql with no command-line-editing support at all. Our opinion, or even our lawyers' opinion, is unlikely to prevent that outcome. While libedit certainly has got its bugs and deficiencies, it's way better than nothing at all, so I think we'd better keep supporting it. 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] Making tab-complete.c easier to maintain
On Thu, Oct 22, 2015 at 04:05:06PM -0700, Tom Lane wrote: > David Fetter writes: > > On Thu, Oct 22, 2015 at 02:36:53PM -0700, Tom Lane wrote: > >> I have no very good idea how to do that, though. Bison does have a > >> notion of which symbols are possible as the next symbol at any given > >> parse point, but it doesn't really make that accessible. There's a lack > >> of cooperation on the readline side too: we'd need to be able to see the > >> whole query buffer not just the current line. > > > This may be on point: > > > http://stackoverflow.com/questions/161495/is-there-a-nice-way-of-handling-multi-line-input-with-gnu-readline > > > I suspect we might have to stop pretending to support alternatives to > > libreadline if we went that direction, not that that would necessarily > > be a bad idea. > > Given the license issues around GNU readline, requiring it seems like > probably a non-starter. I should have made this more clear. I am not an IP attorney and don't play one on TV, but this is what I've gotten out of conversations with IP attorneys on this subject: - If we allow people to disable readline at compile time, the software is not GPL even if only libreadline would add the command line editing capability. - When people do compile with libreadline, the only software affected by libreadine's license is the binary to which it is linked, namely the psql client. To be affective negatively by libreadline's viral license, an entity would need to fork the psql client in proprietary ways that they did not wish not to make available to end users, at the same time linking in libreadline. Maybe I'm missing something big, but I really don't see people out there shipping a libreadline-enabled psql client, details of whose source they'd want to keep a deep, dark secret. If this gets to matter, we can probably get /pro bono/ work from IP attorneys specializing in just this kind of thing. > It strikes me though that maybe we don't need readline's > cooperation. I think it's already true that the previous lines of > the query buffer are stashed somewhere that psql knows about, so in > principle we could sew them together with the current line. That > might be a project worth tackling on its own, since we could make > the existing code smarter about multiline queries, whether or not we > ever get to a grammar-based solution. Great! Cheers, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- 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] Making tab-complete.c easier to maintain
On Thu, Oct 22, 2015 at 7:05 PM, Tom Lane wrote: > I think it's already true that the previous lines of the query buffer > are stashed somewhere that psql knows about, Just skimming but: """ \p or \print Print the current query buffer to the standard output. """ http://www.postgresql.org/docs/9.4/interactive/app-psql.html David J.
Re: [HACKERS] Making tab-complete.c easier to maintain
David Fetter writes: > On Thu, Oct 22, 2015 at 02:36:53PM -0700, Tom Lane wrote: >> I have no very good idea how to do that, though. Bison does have a >> notion of which symbols are possible as the next symbol at any given >> parse point, but it doesn't really make that accessible. There's a lack >> of cooperation on the readline side too: we'd need to be able to see the >> whole query buffer not just the current line. > This may be on point: > http://stackoverflow.com/questions/161495/is-there-a-nice-way-of-handling-multi-line-input-with-gnu-readline > I suspect we might have to stop pretending to support alternatives to > libreadline if we went that direction, not that that would necessarily > be a bad idea. Given the license issues around GNU readline, requiring it seems like probably a non-starter. It strikes me though that maybe we don't need readline's cooperation. I think it's already true that the previous lines of the query buffer are stashed somewhere that psql knows about, so in principle we could sew them together with the current line. That might be a project worth tackling on its own, since we could make the existing code smarter about multiline queries, whether or not we ever get to a grammar-based solution. 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] Making tab-complete.c easier to maintain
On Thu, Oct 22, 2015 at 02:36:53PM -0700, Tom Lane wrote: > Robert Haas writes: > > On Wed, Oct 21, 2015 at 8:54 PM, Thomas Munro > > wrote: > >> (Apologies for sending so many versions. tab-complete.c keeps moving > >> and I want to keep a version that applies on top of master out there, > >> for anyone interested in looking at this. As long as no one objects > >> and there is interest in the patch, I'll keep doing that.) > > > I don't want to rain on the parade since other people seem to like > > this, but I'm sort of unimpressed by this. Yes, it removes >1000 > > lines of code, and that's not nothing. But it's all mechanical code, > > so, not to be dismissive, but who really cares? Is it really worth > > replacing the existing notation that we all know with a new one that > > we have to learn? I'm not violently opposed if someone else wants to > > commit this, but I'm unexcited about it. > > What I would like is to find a way to auto-generate basically this entire > file from gram.y. I've been hoping we could use a principled approach for years. My fondest hope along that line would also involve catalog access, so it could correctly tab-complete user-defined things, but I have the impression that the catalog access variant is "much later" even if autogeneration from gram.y is merely "soon." I'd love to be wrong about that. > That would imply going over to something at least > somewhat parser-based, instead of the current way that is more or less > totally ad-hoc. That would be a very good thing though, because the > current way gives wrong answers not-infrequently, even discounting cases > that it's simply not been taught about. Indeed. > I have no very good idea how to do that, though. Bison does have a > notion of which symbols are possible as the next symbol at any given > parse point, but it doesn't really make that accessible. There's a lack > of cooperation on the readline side too: we'd need to be able to see the > whole query buffer not just the current line. This may be on point: http://stackoverflow.com/questions/161495/is-there-a-nice-way-of-handling-multi-line-input-with-gnu-readline I suspect we might have to stop pretending to support alternatives to libreadline if we went that direction, not that that would necessarily be a bad idea. Cheers, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- 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] Making tab-complete.c easier to maintain
Tom Lane wrote: > What I would like is to find a way to auto-generate basically this entire > file from gram.y. That would imply going over to something at least > somewhat parser-based, instead of the current way that is more or less > totally ad-hoc. That would be a very good thing though, because the > current way gives wrong answers not-infrequently, even discounting cases > that it's simply not been taught about. I did discuss exactly this topic with Thomas a month ago or so in private email, and our conclusion was that it would be a really neat project but a lot more effort than this patch. And after skimming over the patch back then, I think this is well worth the reduced maintenance effort. > I have no very good idea how to do that, though. Bison does have a > notion of which symbols are possible as the next symbol at any given > parse point, but it doesn't really make that accessible. There's a lack > of cooperation on the readline side too: we'd need to be able to see the > whole query buffer not just the current line. At the current pace, a project like this might take years to turn into a real patch. My own vote is for applying this for the time being. -- Álvaro Herrerahttp://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
Re: [HACKERS] Making tab-complete.c easier to maintain
Robert Haas writes: > On Wed, Oct 21, 2015 at 8:54 PM, Thomas Munro > wrote: >> (Apologies for sending so many versions. tab-complete.c keeps moving >> and I want to keep a version that applies on top of master out there, >> for anyone interested in looking at this. As long as no one objects >> and there is interest in the patch, I'll keep doing that.) > I don't want to rain on the parade since other people seem to like > this, but I'm sort of unimpressed by this. Yes, it removes >1000 > lines of code, and that's not nothing. But it's all mechanical code, > so, not to be dismissive, but who really cares? Is it really worth > replacing the existing notation that we all know with a new one that > we have to learn? I'm not violently opposed if someone else wants to > commit this, but I'm unexcited about it. What I would like is to find a way to auto-generate basically this entire file from gram.y. That would imply going over to something at least somewhat parser-based, instead of the current way that is more or less totally ad-hoc. That would be a very good thing though, because the current way gives wrong answers not-infrequently, even discounting cases that it's simply not been taught about. I have no very good idea how to do that, though. Bison does have a notion of which symbols are possible as the next symbol at any given parse point, but it doesn't really make that accessible. There's a lack of cooperation on the readline side too: we'd need to be able to see the whole query buffer not just the current line. 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] Making tab-complete.c easier to maintain
On Wed, Oct 21, 2015 at 8:54 PM, Thomas Munro wrote: > Here is a new version merging the recent CREATE EXTENSION ... VERSION > patch from master. > > (Apologies for sending so many versions. tab-complete.c keeps moving > and I want to keep a version that applies on top of master out there, > for anyone interested in looking at this. As long as no one objects > and there is interest in the patch, I'll keep doing that.) I don't want to rain on the parade since other people seem to like this, but I'm sort of unimpressed by this. Yes, it removes >1000 lines of code, and that's not nothing. But it's all mechanical code, so, not to be dismissive, but who really cares? Is it really worth replacing the existing notation that we all know with a new one that we have to learn? I'm not violently opposed if someone else wants to commit this, but I'm unexcited about it. -- 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
Re: [HACKERS] Making tab-complete.c easier to maintain
Here is a new version merging the recent CREATE EXTENSION ... VERSION patch from master. (Apologies for sending so many versions. tab-complete.c keeps moving and I want to keep a version that applies on top of master out there, for anyone interested in looking at this. As long as no one objects and there is interest in the patch, I'll keep doing that.) -- Thomas Munro http://www.enterprisedb.com tab-complete-macrology-v6.patch.gz Description: GNU Zip compressed data -- 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] Making tab-complete.c easier to maintain
On Mon, Oct 19, 2015 at 4:58 PM, Jeff Janes wrote: > On Sun, Oct 18, 2015 at 5:31 PM, Thomas Munro > wrote: >> >> Hi >> >> Here is a new version merging recent changes. > > > For reasons I do not understand, "SET work_mem " does not complete with > "TO". > > But if I change: > > else if (Matches2("SET", MatchAny)) > > to: > > else if (TailMatches2("SET", MatchAny)) > > Then it does. Thanks for taking a look at this! The word count returned by get_previous_words was incorrect. Here is a corrected version. -- Thomas Munro http://www.enterprisedb.com tab-complete-macrology-v5.patch.gz Description: GNU Zip compressed data -- 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] Making tab-complete.c easier to maintain
On Sun, Oct 18, 2015 at 5:31 PM, Thomas Munro wrote: > Hi > > Here is a new version merging recent changes. > For reasons I do not understand, "SET work_mem " does not complete with "TO". But if I change: else if (Matches2("SET", MatchAny)) to: else if (TailMatches2("SET", MatchAny)) Then it does. Cheers, Jeff
Re: [HACKERS] Making tab-complete.c easier to maintain
Hi Here is a new version merging recent changes. -- Thomas Munro http://www.enterprisedb.com tab-complete-macrology-v4.patch.gz Description: GNU Zip compressed data -- 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] Making tab-complete.c easier to maintain
On Tue, Sep 8, 2015 at 1:56 AM, Alvaro Herrera wrote: > Thomas Munro wrote: > > > Thanks, good point. Here's a version that uses NULL via a macro ANY. > > Aside from a few corrections it also now distinguishes between > > TAIL_MATCHESn (common) and MATCHESn (rarely used for now), for example: > > This looks pretty neat -- 100x neater than what we have, at any rate. I > would use your new MATCHESn() macros a bit more -- for instance the > completion for "ALTER but not ALTER after ALTER TABLE" could be > rephrased as simply MATCHES1("ALTER"), i.e. have it match at start of > command only. Maybe that's just a matter of going over the new code > after the initial run, so that we can have a first patch that's mostly > mechanical and a second pass in which more semantically relevant changes > are applied. Seems easier to review ... > +1 > I would use "ANY" as a keyword here. Sounds way too generic to me. > Maybe "CompleteAny" or something like that. > MatchAny would make more sense to me. Stylistically, I find there's too much uppercasing here. Maybe rename the > macros like this instead: > > > + else if (TailMatches4("ALL", "IN", "TABLESPACE", ANY)) > > + CompleteWithList2("SET TABLESPACE", "OWNED BY"); > > Not totally sure about this part TBH. > Ok, here's a rebased version that uses the style you suggested. It also adds HeadMatchesN macros, so we can do this: * Complete "GRANT/REVOKE ... TO/FROM" with username, PUBLIC, * CURRENT_USER, or SESSION_USER. */ - else if (((pg_strcasecmp(prev9_wd, "GRANT") == 0 || - pg_strcasecmp(prev8_wd, "GRANT") == 0 || - pg_strcasecmp(prev7_wd, "GRANT") == 0 || - pg_strcasecmp(prev6_wd, "GRANT") == 0 || - pg_strcasecmp(prev5_wd, "GRANT") == 0) && - pg_strcasecmp(prev_wd, "TO") == 0) || -((pg_strcasecmp(prev9_wd, "REVOKE") == 0 || - pg_strcasecmp(prev8_wd, "REVOKE") == 0 || - pg_strcasecmp(prev7_wd, "REVOKE") == 0 || - pg_strcasecmp(prev6_wd, "REVOKE") == 0 || - pg_strcasecmp(prev5_wd, "REVOKE") == 0) && - pg_strcasecmp(prev_wd, "FROM") == 0)) + else if ((HeadMatches1("GRANT") && TailMatches1("TO")) || +(HeadMatches1("REVOKE") && TailMatches1("FROM"))) COMPLETE_WITH_QUERY(Query_for_list_of_grant_roles); So to recap: MatchesN(...) -- matches the whole expression (up to lookback buffer size) HeadMatchesN(...) -- matches the start of the expression (ditto) TailMatchesN(...) -- matches the end of the expression MatchAny -- placeholder It would be nice to get rid of those numbers in the macro names, and I understand that we can't use variadic macros. Should we use varargs functions instead of macros? Then we could lose the numbers, but we'd need to introduce global variables to keep the notation short and sweet (or pass in the previous_words and previous_words_count, which would be ugly boilerplate worse than the numbers). -- Thomas Munro http://www.enterprisedb.com tab-complete-macrology-v3.patch.gz Description: GNU Zip compressed data -- 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] Making tab-complete.c easier to maintain
Thomas Munro wrote: > Thanks, good point. Here's a version that uses NULL via a macro ANY. > Aside from a few corrections it also now distinguishes between > TAIL_MATCHESn (common) and MATCHESn (rarely used for now), for example: This looks pretty neat -- 100x neater than what we have, at any rate. I would use your new MATCHESn() macros a bit more -- for instance the completion for "ALTER but not ALTER after ALTER TABLE" could be rephrased as simply MATCHES1("ALTER"), i.e. have it match at start of command only. Maybe that's just a matter of going over the new code after the initial run, so that we can have a first patch that's mostly mechanical and a second pass in which more semantically relevant changes are applied. Seems easier to review ... I would use "ANY" as a keyword here. Sounds way too generic to me. Maybe "CompleteAny" or something like that. Stylistically, I find there's too much uppercasing here. Maybe rename the macros like this instead: > + else if (TailMatches4("ALL", "IN", "TABLESPACE", ANY)) > + CompleteWithList2("SET TABLESPACE", "OWNED BY"); Not totally sure about this part TBH. -- Álvaro Herrerahttp://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
Re: [HACKERS] Making tab-complete.c easier to maintain
Thomas Munro writes: > See attached a proof-of-concept patch. It reduces the size of > tab-complete.c by a bit over a thousand lines. I realise that changing so > many lines just to refactor code may may be a difficult sell! But I think > this would make it easier to improve the tab completion code in future, and > although it's large it's a superficial and mechanical change. I really dislike the magical "<" business. Maybe you could use NULL instead of having to reserve a class of real strings as placeholders. (That would require finding another way to denote the end of the list, but a separate count argument seems like a possible answer.) Or perhaps use empty strings as placeholders? 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