Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtp (Exim 4.80) (envelope-from ) id 1aAR4P-0006ZW-Ne for pgsql-hackers@arkaria.postgresql.org; Sat, 19 Dec 2015 23:37:49 +0000 Received: from localhost ([127.0.0.1] helo=postgresql.org) by malur.postgresql.org with smtp (Exim 4.84) (envelope-from ) id 1aAR4O-0006rJ-Lq for pgsql-hackers@arkaria.postgresql.org; Sat, 19 Dec 2015 23:37:48 +0000 Received: from makus.postgresql.org ([2001:4800:1501:1::229]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.84) (envelope-from ) id 1aAR33-0005P9-Kc for pgsql-hackers@postgresql.org; Sat, 19 Dec 2015 23:36:25 +0000 Received: from sss.pgh.pa.us ([66.207.139.130]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.84) (envelope-from ) id 1aAR2w-0003mK-Gy for pgsql-hackers@postgresql.org; Sat, 19 Dec 2015 23:36:24 +0000 Received: from sss1.sss.pgh.pa.us (localhost [127.0.0.1]) by sss.pgh.pa.us (8.14.4/8.14.4) with ESMTP id tBJNaAMO014996; Sat, 19 Dec 2015 18:36:10 -0500 From: Tom Lane To: Michael Paquier cc: Thomas Munro , Kyotaro HORIGUCHI , Alvaro Herrera , Jeff Janes , PostgreSQL mailing lists Subject: Re: Making tab-complete.c easier to maintain In-reply-to: <21460.1450560248@sss.pgh.pa.us> References: <20151217.150633.216859242.horiguchi.kyotaro@lab.ntt.co.jp> <1851.1450471370@sss.pgh.pa.us> <21460.1450560248@sss.pgh.pa.us> Comments: In-reply-to Tom Lane message dated "Sat, 19 Dec 2015 16:24:08 -0500" MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0" Content-ID: <14974.1450568148.0@sss.pgh.pa.us> Date: Sat, 19 Dec 2015 18:36:10 -0500 Message-ID: <14995.1450568170@sss.pgh.pa.us> X-Pg-Spam-Score: -1.9 (-) List-Archive: List-Help: List-ID: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: X-Mailing-List: pgsql-hackers Precedence: bulk Sender: pgsql-hackers-owner@postgresql.org ------- =_aaaaaaaaaa0 Content-Type: text/plain; charset="us-ascii" Content-ID: <14974.1450568148.1@sss.pgh.pa.us> 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 ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="multiline-tab-completion.patch"; charset="us-ascii" Content-ID: <14974.1450568148.2@sss.pgh.pa.us> Content-Description: multiline-tab-completion.patch Content-Transfer-Encoding: quoted-printable 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 completi= on) + * * 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 =20=20 + /* Make current query_buf available to tab completion callback */ + tab_completion_query_buf =3D query_buf; +=20 /* Enable SIGINT to longjmp to sigint_interrupt_jmp */ sigint_interrupt_enabled =3D true; =20=20 *************** gets_interactive(const char *prompt) *** 85,90 **** --- 93,101 ---- /* Disable SIGINT again */ sigint_interrupt_enabled =3D false; =20=20 + /* Pure neatnik-ism */ + tab_completion_query_buf =3D NULL; +=20 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" =20=20 =20=20 ! char *gets_interactive(const char *prompt); char *gets_fromFile(FILE *source); =20=20 void initializeInput(int flags); --- 38,44 ---- #include "pqexpbuffer.h" =20=20 =20=20 ! char *gets_interactive(const char *prompt, PQExpBuffer query_buf); char *gets_fromFile(FILE *source); =20=20 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 =3D=3D 0) prompt_status =3D PROMPT_READY; ! line =3D gets_interactive(get_prompt(prompt_status)); } else { --- 133,139 ---- /* May need to reset prompt, eg after \r command */ if (query_buf->len =3D=3D 0) prompt_status =3D PROMPT_READY; ! line =3D 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@$><=3D;|&{() " =20=20 /* + * Since readline doesn't let us pass any state through to the tab comple= tion + * callback, we have to use this global variable to let get_previous_word= s() + * get at the previous lines of the current command. Ick. + */ + PQExpBuffer tab_completion_query_buf =3D NULL; +=20 + /* * 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); =20=20 ! static int get_previous_words(int point, char **previous_words, int nword= s); =20=20 static char *get_guctype(const char *varname); =20=20 --- 930,936 ---- static char *escape_string(const char *text); static PGresult *exec_query(const char *query); =20=20 ! static char **get_previous_words(int point, char **buffer, int *nwords); =20=20 static char *get_guctype(const char *varname); =20=20 *************** psql_completion(const char *text, int st *** 1027,1039 **** /* This is the variable we'll return. */ char **matches =3D NULL; =20=20 ! /* This array will contain some scannage of the input line. */ ! char *previous_words[9]; =20=20 /* The number of words found on the input line. */ int previous_words_count; =20=20 ! /* For compactness, we use these macros to reference previous_words[]. */ #define prev_wd (previous_words[0]) #define prev2_wd (previous_words[1]) #define prev3_wd (previous_words[2]) --- 1034,1055 ---- /* This is the variable we'll return. */ char **matches =3D NULL; =20=20 ! /* Workspace for parsed words. */ ! char *words_buffer; !=20 ! /* This array will contain pointers to parsed words. */ ! char **previous_words; =20=20 /* The number of words found on the input line. */ int previous_words_count; =20=20 ! /* ! * For compactness, we use these macros to reference previous_words[]. ! * Caution: do not access a previous_words[] entry without having checked ! * previous_words_count to be sure it's valid. In most cases below, that ! * check is implicit in a TailMatches() or similar macro, but in some ! * places we have to check it explicitly. ! */ #define prev_wd (previous_words[0]) #define prev2_wd (previous_words[1]) #define prev3_wd (previous_words[2]) *************** psql_completion(const char *text, int st *** 1133,1141 **** =20=20 /* * Macros for matching N words at the start of the line, regardless of ! * what is after them. These are pretty broken because they can only lo= ok ! * back lengthof(previous_words) words, but we'll worry about improving ! * their implementation some other day. */ #define HeadMatches1(p1) \ (previous_words_count >=3D 1 && \ --- 1149,1155 ---- =20=20 /* * Macros for matching N words at the start of the line, regardless of ! * what is after them. */ #define HeadMatches1(p1) \ (previous_words_count >=3D 1 && \ *************** psql_completion(const char *text, int st *** 1194,1206 **** completion_info_charp2 =3D NULL; =20=20 /* ! * Scan the input line before our current position for the last few word= s. * According to those we'll make some smart decisions on what the user is * probably intending to type. */ ! previous_words_count =3D get_previous_words(start, ! previous_words, ! lengthof(previous_words)); =20=20 /* If current word is a backslash command, offer completions for that */ if (text[0] =3D=3D '\\') --- 1208,1220 ---- completion_info_charp2 =3D NULL; =20=20 /* ! * Scan the input line to extract the words before our current position. * According to those we'll make some smart decisions on what the user is * probably intending to type. */ ! previous_words =3D get_previous_words(start, ! &words_buffer, ! &previous_words_count); =20=20 /* If current word is a backslash command, offer completions for that */ if (text[0] =3D=3D '\\') *************** psql_completion(const char *text, int st *** 1692,1698 **** =20=20 COMPLETE_WITH_LIST(list_TABLEOPTIONS); } ! else if (TailMatches4("REPLICA", "IDENTITY", "USING", "INDEX")) { completion_info_charp =3D prev5_wd; COMPLETE_WITH_QUERY(Query_for_index_of_table); --- 1706,1712 ---- =20=20 COMPLETE_WITH_LIST(list_TABLEOPTIONS); } ! else if (TailMatches5(MatchAny, "REPLICA", "IDENTITY", "USING", "INDEX")) { completion_info_charp =3D prev5_wd; COMPLETE_WITH_QUERY(Query_for_index_of_table); *************** psql_completion(const char *text, int st *** 2806,2812 **** if (!recognized_connection_string(text)) COMPLETE_WITH_QUERY(Query_for_list_of_databases); } ! else if (strcmp(prev2_wd, "\\connect") =3D=3D 0 || strcmp(prev2_wd, "\\c= ") =3D=3D 0) { if (!recognized_connection_string(prev_wd)) COMPLETE_WITH_QUERY(Query_for_list_of_roles); --- 2820,2828 ---- if (!recognized_connection_string(text)) COMPLETE_WITH_QUERY(Query_for_list_of_databases); } ! else if (previous_words_count >=3D 2 && ! (strcmp(prev2_wd, "\\connect") =3D=3D 0 || ! strcmp(prev2_wd, "\\c") =3D=3D 0)) { if (!recognized_connection_string(prev_wd)) COMPLETE_WITH_QUERY(Query_for_list_of_roles); *************** psql_completion(const char *text, int st *** 2891,2897 **** =20=20 COMPLETE_WITH_LIST_CS(my_list); } ! else if (strcmp(prev2_wd, "\\pset") =3D=3D 0) { if (strcmp(prev_wd, "format") =3D=3D 0) { --- 2907,2914 ---- =20=20 COMPLETE_WITH_LIST_CS(my_list); } ! else if (previous_words_count >=3D 2 && ! strcmp(prev2_wd, "\\pset") =3D=3D 0) { if (strcmp(prev_wd, "format") =3D=3D 0) { *************** psql_completion(const char *text, int st *** 2927,2933 **** { matches =3D complete_from_variables(text, "", "", false); } ! else if (strcmp(prev2_wd, "\\set") =3D=3D 0) { static const char *const boolean_value_list[] =3D {"on", "off", NULL}; --- 2944,2951 ---- { matches =3D complete_from_variables(text, "", "", false); } ! else if (previous_words_count >=3D 2 && ! strcmp(prev2_wd, "\\set") =3D=3D 0) { static const char *const boolean_value_list[] =3D {"on", "off", NULL}; *************** psql_completion(const char *text, int st *** 3048,3059 **** } =20=20 /* free storage */ ! { ! int i; !=20 ! for (i =3D 0; i < lengthof(previous_words); i++) ! free(previous_words[i]); ! } =20=20 /* Return our Grand List O' Matches */ return matches; --- 3066,3073 ---- } =20=20 /* free storage */ ! free(previous_words); ! free(words_buffer); =20=20 /* Return our Grand List O' Matches */ return matches; *************** exec_query(const char *query) *** 3632,3661 **** =20=20 =20=20 /* ! * Return the nwords word(s) before point. Words are returned right to l= eft, ! * that is, previous_words[0] gets the last word before point. ! * If we run out of words, remaining array elements are set to empty stri= ngs. ! * Each array element is filled with a malloc'd string. ! * Returns the number of words that were actually found (up to nwords). */ ! static int ! get_previous_words(int point, char **previous_words, int nwords) { ! const char *buf =3D rl_line_buffer; /* alias */ int words_found =3D 0; int i; =20=20 ! /* first we look for a non-word char before the current point */ for (i =3D point - 1; i >=3D 0; i--) if (strchr(WORD_BREAKS, buf[i])) break; point =3D i; =20=20 ! while (nwords-- > 0) { int start, end; ! char *s; =20=20 /* now find the first non-space which then constitutes the end */ end =3D -1; --- 3646,3725 ---- =20=20 =20=20 /* ! * Parse all the word(s) before point. ! * ! * Returns a malloc'd array of character pointers that point into the mal= loc'd ! * data array returned to *buffer; caller must free() both of these when = done. ! * *nwords receives the number of words found, ie, the valid length of the ! * return array. ! * ! * Words are returned right to left, that is, previous_words[0] gets the = last ! * word before point, previous_words[1] the next-to-last, etc. */ ! static char ** ! get_previous_words(int point, char **buffer, int *nwords) { ! char **previous_words; ! char *buf; ! int buflen; int words_found =3D 0; int i; =20=20 ! /* ! * Construct a writable buffer including both preceding and current lines ! * of the query, up to "point" which is where the currently completable ! * word begins. Because our definition of "word" is such that a non-word ! * character must end each word, we can use this buffer to return the wo= rd ! * data as-is, by placing a '\0' after each word. ! */ ! buflen =3D point + 1; ! if (tab_completion_query_buf) ! buflen +=3D tab_completion_query_buf->len + 1; ! *buffer =3D buf =3D pg_malloc(buflen); ! i =3D 0; ! if (tab_completion_query_buf) ! { ! memcpy(buf, tab_completion_query_buf->data, ! tab_completion_query_buf->len); ! i +=3D tab_completion_query_buf->len; ! buf[i++] =3D '\n'; ! } ! memcpy(buf + i, rl_line_buffer, point); ! i +=3D point; ! buf[i] =3D '\0'; !=20 ! /* Readjust point to reference appropriate offset in buf */ ! point =3D i; !=20 ! /* ! * Allocate array of word start points. There can be at most length/2 += 1 ! * words in the buffer. ! */ ! previous_words =3D (char **) pg_malloc((point / 2 + 1) * sizeof(char *)); !=20 ! /* ! * First we look for a non-word char before the current point. (This is ! * probably useless, if readline is on the same page as we are about what ! * is a word, but if so it's cheap.) ! */ for (i =3D point - 1; i >=3D 0; i--) + { if (strchr(WORD_BREAKS, buf[i])) break; + } point =3D i; =20=20 ! /* ! * Now parse words, working backwards, until we hit start of line. The ! * backwards scan has some interesting but intentional properties ! * concerning parenthesis handling. ! */ ! while (point >=3D 0) { int start, end; ! bool inquotes =3D false; ! int parentheses =3D 0; =20=20 /* now find the first non-space which then constitutes the end */ end =3D -1; *************** get_previous_words(int point, char **pre *** 3667,3725 **** break; } } =20=20 /* ! * If no end found we return an empty string, because there is no word ! * before the point */ ! if (end < 0) ! { ! point =3D end; ! s =3D pg_strdup(""); ! } ! else { ! /* ! * Otherwise we now look for the start. The start is either the ! * last character before any word-break character going backwards ! * from the end, or it's simply character 0. We also handle open ! * quotes and parentheses. ! */ ! bool inquotes =3D false; ! int parentheses =3D 0; !=20 ! for (start =3D end; start > 0; start--) { ! if (buf[start] =3D=3D '"') ! inquotes =3D !inquotes; ! if (!inquotes) { ! if (buf[start] =3D=3D ')') ! parentheses++; ! else if (buf[start] =3D=3D '(') ! { ! if (--parentheses <=3D 0) ! break; ! } ! else if (parentheses =3D=3D 0 && ! strchr(WORD_BREAKS, buf[start - 1])) break; } } -=20 - point =3D start - 1; -=20 - /* make a copy of chars from start to end inclusive */ - s =3D pg_malloc(end - start + 2); - strlcpy(s, &buf[start], end - start + 2); -=20 - words_found++; } =20=20 ! *previous_words++ =3D s; } =20=20 ! return words_found; } =20=20 /* --- 3731,3776 ---- break; } } + /* if no end found, we're done */ + if (end < 0) + break; =20=20 /* ! * Otherwise we now look for the start. The start is either the last ! * character before any word-break character going backwards from the ! * end, or it's simply character 0. We also handle open quotes and ! * parentheses. */ ! for (start =3D end; start > 0; start--) { ! if (buf[start] =3D=3D '"') ! inquotes =3D !inquotes; ! if (!inquotes) { ! if (buf[start] =3D=3D ')') ! parentheses++; ! else if (buf[start] =3D=3D '(') { ! if (--parentheses <=3D 0) break; } + else if (parentheses =3D=3D 0 && + strchr(WORD_BREAKS, buf[start - 1])) + break; } } =20=20 ! /* Return the word located at start to end inclusive */ ! previous_words[words_found] =3D &buf[start]; ! buf[end + 1] =3D '\0'; ! words_found++; !=20 ! /* Continue searching */ ! point =3D start - 1; } =20=20 ! *nwords =3D words_found; ! return previous_words; } =20=20 /* diff --git a/src/bin/psql/tab-complete.h b/src/bin/psql/tab-complete.h index 9dcd7e7..0e9a430 100644 *** a/src/bin/psql/tab-complete.h --- b/src/bin/psql/tab-complete.h *************** *** 8,15 **** #ifndef TAB_COMPLETE_H #define TAB_COMPLETE_H =20=20 ! #include "postgres_fe.h" =20=20 ! void initialize_readline(void); =20=20 ! #endif --- 8,17 ---- #ifndef TAB_COMPLETE_H #define TAB_COMPLETE_H =20=20 ! #include "pqexpbuffer.h" =20=20 ! extern PQExpBuffer tab_completion_query_buf; =20=20 ! extern void initialize_readline(void); !=20 ! #endif /* TAB_COMPLETE_H */ ------- =_aaaaaaaaaa0 Content-Type: text/plain Content-Disposition: inline Content-Transfer-Encoding: 8bit MIME-Version: 1.0 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers ------- =_aaaaaaaaaa0--