Code review for psql multiline history patch(es). Fix memory leak,
authorTom Lane <tgl@sss.pgh.pa.us>
Sun, 11 Jun 2006 23:06:00 +0000 (23:06 +0000)
committerTom Lane <tgl@sss.pgh.pa.us>
Sun, 11 Jun 2006 23:06:00 +0000 (23:06 +0000)
failure to enter commands in history if canceled by control-C, other
infelicities.

src/bin/psql/command.c
src/bin/psql/help.c
src/bin/psql/input.c
src/bin/psql/input.h
src/bin/psql/mainloop.c
src/bin/psql/prompt.c
src/bin/psql/tab-complete.c

index 3c5bd6740f101b9d5ca1e65919fb1e759f9f1736..a9d8a07f8350e83b6f8567fdd8719b4acdb6dc0e 100644 (file)
@@ -3,7 +3,7 @@
  *
  * Copyright (c) 2000-2006, PostgreSQL Global Development Group
  *
- * $PostgreSQL: pgsql/src/bin/psql/command.c,v 1.169 2006/06/07 22:24:45 momjian Exp $
+ * $PostgreSQL: pgsql/src/bin/psql/command.c,v 1.170 2006/06/11 23:06:00 tgl Exp $
  */
 #include "postgres_fe.h"
 #include "command.h"
@@ -1361,7 +1361,7 @@ do_edit(const char *filename_arg, PQExpBuffer query_buf)
        }
        else
        {
-           /* read file back in */
+           /* read file back into query_buf */
            char        line[1024];
 
            resetPQExpBuffer(query_buf);
@@ -1374,14 +1374,6 @@ do_edit(const char *filename_arg, PQExpBuffer query_buf)
                error = true;
            }
 
-#ifdef USE_READLINE
-#ifdef HAVE_REPLACE_HISTORY_ENTRY
-
-           replace_history_entry(where_history(), query_buf->data, NULL);
-#else
-           add_history(query_buf->data);
-#endif
-#endif
            fclose(stream);
        }
 
index dd3bdef0434c5098006298fc039449e427b058a4..6bc0b73d08e297c5f2854a5eaa1bfff6ad12426e 100644 (file)
@@ -3,11 +3,10 @@
  *
  * Copyright (c) 2000-2006, PostgreSQL Global Development Group
  *
- * $PostgreSQL: pgsql/src/bin/psql/help.c,v 1.110 2006/03/05 15:58:51 momjian Exp $
+ * $PostgreSQL: pgsql/src/bin/psql/help.c,v 1.111 2006/06/11 23:06:00 tgl Exp $
  */
 #include "postgres_fe.h"
 #include "common.h"
-#include "pqexpbuffer.h"
 #include "input.h"
 #include "print.h"
 #include "help.h"
index 1dc36d9dbc514d1d262e8a396c0d1b1f8665263e..b970175b805bb62376c7fb179406074079cc0376 100644 (file)
@@ -3,12 +3,12 @@
  *
  * Copyright (c) 2000-2006, PostgreSQL Global Development Group
  *
- * $PostgreSQL: pgsql/src/bin/psql/input.c,v 1.53 2006/03/21 13:38:12 momjian Exp $
+ * $PostgreSQL: pgsql/src/bin/psql/input.c,v 1.54 2006/06/11 23:06:00 tgl Exp $
  */
 #include "postgres_fe.h"
 
-#include "pqexpbuffer.h"
 #include "input.h"
+#include "pqexpbuffer.h"
 #include "settings.h"
 #include "tab-complete.h"
 #include "common.h"
@@ -78,110 +78,89 @@ GetHistControlConfig(void)
 #endif
 
 
-static char *
-gets_basic(const char prompt[])
-{
-   fputs(prompt, stdout);
-   fflush(stdout);
-   return gets_fromFile(stdin);
-}
-
-
 /*
  * gets_interactive()
  *
- * Gets a line of interactive input, using readline of desired.
+ * Gets a line of interactive input, using readline if desired.
  * The result is malloc'ed.
  */
 char *
 gets_interactive(const char *prompt)
 {
 #ifdef USE_READLINE
-   char       *s;
-
    if (useReadline)
+   {
        /* On some platforms, readline is declared as readline(char *) */
-       s = readline((char *) prompt);
-   else
-       s = gets_basic(prompt);
-
-   return s;
-#else
-   return gets_basic(prompt);
+       return readline((char *) prompt);
+   }
 #endif
+
+   fputs(prompt, stdout);
+   fflush(stdout);
+   return gets_fromFile(stdin);
 }
 
 
-/* Put the line in the history buffer and also add the trailing \n */
+/*
+ * Append the line to the history buffer, making sure there is a trailing '\n'
+ */
 void
-pg_append_history(char *s, PQExpBuffer history_buf)
+pg_append_history(const char *s, PQExpBuffer history_buf)
 {
 #ifdef USE_READLINE
-
-   int slen;
-   if (useReadline && useHistory && s && s[0])
+   if (useHistory && s && s[0])
    {
-       slen = strlen(s);
-       if (s[slen-1] == '\n')
-           appendPQExpBufferStr(history_buf, s);
-       else
-       {
-           appendPQExpBufferStr(history_buf, s);
+       appendPQExpBufferStr(history_buf, s);
+       if (s[strlen(s) - 1] != '\n')
            appendPQExpBufferChar(history_buf, '\n');
-       }
    }   
 #endif 
 }
 
 
 /*
- * Feed the string to readline
+ * Emit accumulated history entry to readline's history mechanism,
+ * then reset the buffer to empty.
+ *
+ * Note: we write nothing if history_buf is empty, so extra calls to this
+ * function don't hurt.  There must have been at least one line added by
+ * pg_append_history before we'll do anything.
  */
 void
-pg_write_history(char *s)
+pg_send_history(PQExpBuffer history_buf)
 {
 #ifdef USE_READLINE
-   static char *prev_hist;
-   int slen, i;
-   
-   if (useReadline && useHistory )
+   static char *prev_hist = NULL;
+
+   char   *s = history_buf->data;
+
+   if (useHistory && s[0])
    {
-       enum histcontrol HC;
-       
-       /* Flushing of empty buffer should do nothing */
-       if  (*s == 0)
-           return;
-       
-       prev_hist = NULL;
-           
-       HC = GetHistControlConfig();
+       enum histcontrol HC = GetHistControlConfig();
 
        if (((HC & hctl_ignorespace) && s[0] == ' ') ||
-         ((HC & hctl_ignoredups) && prev_hist && strcmp(s, prev_hist) == 0))
+           ((HC & hctl_ignoredups) && prev_hist && strcmp(s, prev_hist) == 0))
        {
            /* Ignore this line as far as history is concerned */
        }
        else
        {
-           free(prev_hist);
-           slen = strlen(s);
-           /* Trim the trailing \n's */
-           for (i = slen-1; i >= 0 && s[i] == '\n'; i--)
+           int     i;
+
+           /* Trim any trailing \n's (OK to scribble on history_buf) */
+           for (i = strlen(s)-1; i >= 0 && s[i] == '\n'; i--)
                ;
            s[i + 1] = '\0';
+           /* Save each previous line for ignoredups processing */
+           if (prev_hist)
+               free(prev_hist);
            prev_hist = pg_strdup(s);
+           /* And send it to readline */
            add_history(s);
        }
    }
-#endif
-}
 
-void
-pg_clear_history(PQExpBuffer history_buf)
-{
-#ifdef USE_READLINE    
-   if (useReadline && useHistory)
-       resetPQExpBuffer(history_buf);
+   resetPQExpBuffer(history_buf);
 #endif
 }
 
@@ -219,6 +198,9 @@ gets_fromFile(FILE *source)
 
 
 #ifdef USE_READLINE
+/*
+ * Convert newlines to NL_IN_HISTORY for safe saving in readline history file
+ */
 static void
 encode_history(void)
 {
@@ -232,6 +214,9 @@ encode_history(void)
                *cur_ptr = NL_IN_HISTORY;
 }
 
+/*
+ * Reverse the above encoding
+ */
 static void
 decode_history(void)
 {
@@ -285,9 +270,10 @@ initializeInput(int flags)
        }
 
        if (psql_history)
+       {
            read_history(psql_history);
-           
-       decode_history();
+           decode_history();
+       }
    }
 #endif
 
@@ -299,11 +285,13 @@ initializeInput(int flags)
 }
 
 
-/* This function is designed for saving the readline history when user 
- * run \s command or when psql finishes. 
- * We have an argument named encodeFlag to handle those cases differently
- * In that case of call via \s we don't really need to encode \n as \x01,
- * but when we save history for Readline we must do that conversion
+/*
+ * This function is for saving the readline history when user 
+ * runs \s command or when psql finishes. 
+ *
+ * We have an argument named encodeFlag to handle the cases differently.
+ * In case of call via \s we don't really need to encode \n as \x01,
+ * but when we save history for Readline we must do that conversion.
  */
 bool
 saveHistory(char *fname, bool encodeFlag)
@@ -316,7 +304,8 @@ saveHistory(char *fname, bool encodeFlag)
        if (write_history(fname) == 0)
            return true;
 
-       psql_error("could not save history to file \"%s\": %s\n", fname, strerror(errno));
+       psql_error("could not save history to file \"%s\": %s\n",
+                  fname, strerror(errno));
    }
 #else
    psql_error("history is not supported by this installation\n");
index 4fbc86079b4faf2526f79c5ff97d955c1e8662f6..0c6ede0cd5acc0559776d3715e76b263af40da82 100644 (file)
@@ -3,7 +3,7 @@
  *
  * Copyright (c) 2000-2006, PostgreSQL Global Development Group
  *
- * $PostgreSQL: pgsql/src/bin/psql/input.h,v 1.27 2006/03/21 13:38:12 momjian Exp $
+ * $PostgreSQL: pgsql/src/bin/psql/input.h,v 1.28 2006/06/11 23:06:00 tgl Exp $
  */
 #ifndef INPUT_H
 #define INPUT_H
@@ -32,6 +32,8 @@
 #endif
 #endif
 
+#include "pqexpbuffer.h"
+
 
 char      *gets_interactive(const char *prompt);
 char      *gets_fromFile(FILE *source);
@@ -39,9 +41,7 @@ char     *gets_fromFile(FILE *source);
 void       initializeInput(int flags);
 bool       saveHistory(char *fname, bool encodeFlag);
 
-void pg_append_history(char *s, PQExpBuffer history_buf);
-void pg_clear_history(PQExpBuffer history_buf);
-void pg_write_history(char *s);
-
+void       pg_append_history(const char *s, PQExpBuffer history_buf);
+void       pg_send_history(PQExpBuffer history_buf);
 
 #endif   /* INPUT_H */
index 6c02ba1ccaec5aad68017b5b0b5e55ff8084ef87..af297ae2a91c5a13a9520f1b1ab5c416ad40c8c0 100644 (file)
@@ -3,7 +3,7 @@
  *
  * Copyright (c) 2000-2006, PostgreSQL Global Development Group
  *
- * $PostgreSQL: pgsql/src/bin/psql/mainloop.c,v 1.78 2006/06/07 13:18:37 momjian Exp $
+ * $PostgreSQL: pgsql/src/bin/psql/mainloop.c,v 1.79 2006/06/11 23:06:00 tgl Exp $
  */
 #include "postgres_fe.h"
 #include "mainloop.h"
@@ -37,12 +37,12 @@ MainLoop(FILE *source)
    PQExpBuffer query_buf;      /* buffer for query being accumulated */
    PQExpBuffer previous_buf;   /* if there isn't anything in the new buffer
                                 * yet, use this one for \e, etc. */
-   PQExpBuffer history_buf;
+   PQExpBuffer history_buf;    /* earlier lines of a multi-line command,
+                                * not yet saved to readline history */
    char       *line;           /* current line of input */
    int         added_nl_pos;
    bool        success;
-
-   volatile bool       line_saved_in_history;
+   bool        line_saved_in_history;
    volatile int successResult = EXIT_SUCCESS;
    volatile backslashResult slashCmdStatus = PSQL_CMD_UNKNOWN;
    volatile promptStatus_t prompt_status = PROMPT_READY;
@@ -70,7 +70,6 @@ MainLoop(FILE *source)
    query_buf = createPQExpBuffer();
    previous_buf = createPQExpBuffer();
    history_buf = createPQExpBuffer();
-
    if (!query_buf || !previous_buf || !history_buf)
    {
        psql_error("out of memory\n");
@@ -80,8 +79,6 @@ MainLoop(FILE *source)
    /* main loop to get queries and execute them */
    while (successResult == EXIT_SUCCESS)
    {
-       line_saved_in_history = false;
-
        /*
         * Welcome code for Control-C
         */
@@ -97,7 +94,7 @@ MainLoop(FILE *source)
                successResult = EXIT_USER;
                break;
            }
-           pg_clear_history(history_buf);          
+
            cancel_pressed = false;
        }
 
@@ -113,8 +110,6 @@ MainLoop(FILE *source)
            count_eof = 0;
            slashCmdStatus = PSQL_CMD_UNKNOWN;
            prompt_status = PROMPT_READY;
-           if (pset.cur_cmd_interactive)
-               pg_write_history(history_buf->data);
 
            if (pset.cur_cmd_interactive)
                putc('\n', stdout);
@@ -135,33 +130,10 @@ MainLoop(FILE *source)
 
        fflush(stdout);
 
-       if (slashCmdStatus == PSQL_CMD_NEWEDIT)
-       {
-           /*
-            * just returned from editing the line? then just copy to the
-            * input buffer
-            */
-           line = pg_strdup(query_buf->data);
-           /* reset parsing state since we are rescanning whole line */
-           resetPQExpBuffer(query_buf);
-           psql_scan_reset(scan_state);
-           slashCmdStatus = PSQL_CMD_UNKNOWN;
-           prompt_status = PROMPT_READY;
-           
-           if (pset.cur_cmd_interactive)
-           {
-               /*
-                *  Pass all the contents of history_buf to readline
-                *  and free the history buffer.
-                */
-               pg_write_history(history_buf->data);
-               pg_clear_history(history_buf);
-               pg_write_history(line);
-               line_saved_in_history = true;
-           }
-       }
-       /* otherwise, get another line */
-       else if (pset.cur_cmd_interactive)
+       /*
+        * get another line
+        */
+       if (pset.cur_cmd_interactive)
        {
            /* May need to reset prompt, eg after \r command */
            if (query_buf->len == 0)
@@ -230,7 +202,8 @@ MainLoop(FILE *source)
         */
        psql_scan_setup(scan_state, line, strlen(line));
        success = true;
-       
+       line_saved_in_history = false;
+
        while (success || !die_on_error)
        {
            PsqlScanResult scan_result;
@@ -247,6 +220,17 @@ MainLoop(FILE *source)
                (scan_result == PSCAN_EOL &&
                 GetVariableBool(pset.vars, "SINGLELINE")))
            {
+               /*
+                * Save query in history.  We use history_buf to accumulate
+                * multi-line queries into a single history entry.
+                */
+               if (pset.cur_cmd_interactive && !line_saved_in_history)
+               {
+                   pg_append_history(line, history_buf);
+                   pg_send_history(history_buf);
+                   line_saved_in_history = true;
+               }
+
                /* execute query */
                success = SendQuery(query_buf->data);
                slashCmdStatus = success ? PSQL_CMD_SEND : PSQL_CMD_ERROR;
@@ -265,12 +249,27 @@ MainLoop(FILE *source)
                 * If we added a newline to query_buf, and nothing else has
                 * been inserted in query_buf by the lexer, then strip off the
                 * newline again.  This avoids any change to query_buf when a
-                * line contains only a backslash command.
+                * line contains only a backslash command.  Also, in this
+                * situation we force out any previous lines as a separate
+                * history entry; we don't want SQL and backslash commands
+                * intermixed in history if at all possible.
                 */
                if (query_buf->len == added_nl_pos)
+               {
                    query_buf->data[--query_buf->len] = '\0';
+                   pg_send_history(history_buf);
+               }
                added_nl_pos = -1;
 
+               /* save backslash command in history */
+               if (pset.cur_cmd_interactive && !line_saved_in_history)
+               {
+                   pg_append_history(line, history_buf);
+                   pg_send_history(history_buf);
+                   line_saved_in_history = true;
+               }
+
+               /* execute backslash command */
                slashCmdStatus = HandleSlashCmds(scan_state,
                                                 query_buf->len > 0 ?
                                                 query_buf : previous_buf);
@@ -295,44 +294,32 @@ MainLoop(FILE *source)
                    /* flush any paren nesting info after forced send */
                    psql_scan_reset(scan_state);
                }
+               else if (slashCmdStatus == PSQL_CMD_NEWEDIT)
+               {
+                   /* rescan query_buf as new input */
+                   psql_scan_finish(scan_state);
+                   free(line);
+                   line = pg_strdup(query_buf->data);
+                   resetPQExpBuffer(query_buf);
+                   /* reset parsing state since we are rescanning whole line */
+                   psql_scan_reset(scan_state);
+                   psql_scan_setup(scan_state, line, strlen(line));
+                   line_saved_in_history = false;
+                   prompt_status = PROMPT_READY;
+               }
+               else if (slashCmdStatus == PSQL_CMD_TERMINATE)
+                   break;
            }
 
-           /*
-            *  If we append to history a backslash command that is inside
-            *  a multi-line query, then when we recall the history, the
-            *  backslash command will make the query invalid, so we write
-            *  backslash commands immediately rather than keeping them
-            *  as part of the current multi-line query.  We do the test
-            *  down here so we can check for \g and other 'execute'
-            *  backslash commands, which should be appended.
-            */
-           if (!line_saved_in_history && pset.cur_cmd_interactive)
-           {
-               /* Sending a command (PSQL_CMD_SEND) zeros the length */
-               if (scan_result == PSCAN_BACKSLASH)
-                   pg_write_history(line);
-               else
-                   pg_append_history(line, history_buf);
-               line_saved_in_history = true;
-           }
-
-           /* fall out of loop on \q or if lexer reached EOL */
-           if (slashCmdStatus == PSQL_CMD_TERMINATE ||
-               scan_result == PSCAN_INCOMPLETE ||
+           /* fall out of loop if lexer reached EOL */
+           if (scan_result == PSCAN_INCOMPLETE ||
                scan_result == PSCAN_EOL)
                break;
        }
 
-       if ((pset.cur_cmd_interactive && prompt_status == PROMPT_READY) ||
-           (GetVariableBool(pset.vars, "SINGLELINE") && prompt_status == PROMPT_CONTINUE))
-       {
-           /*
-            *  Pass all the contents of history_buf to readline
-            *  and free the history buffer.
-            */
-           pg_write_history(history_buf->data);
-           pg_clear_history(history_buf);
-       }
+       /* Add line to pending history if we didn't execute anything yet */
+       if (pset.cur_cmd_interactive && !line_saved_in_history)
+           pg_append_history(line, history_buf);
 
        psql_scan_finish(scan_state);
        free(line);
@@ -359,6 +346,11 @@ MainLoop(FILE *source)
    if (query_buf->len > 0 && !pset.cur_cmd_interactive &&
        successResult == EXIT_SUCCESS)
    {
+       /* save query in history */
+       if (pset.cur_cmd_interactive)
+           pg_send_history(history_buf);
+
+       /* execute query */
        success = SendQuery(query_buf->data);
 
        if (!success && die_on_error)
index f7d591b054570f329d13a748b478d084992500a0..07917c534502fca30f4eedf9b4a43c8f6f20aac7 100644 (file)
@@ -3,7 +3,7 @@
  *
  * Copyright (c) 2000-2006, PostgreSQL Global Development Group
  *
- * $PostgreSQL: pgsql/src/bin/psql/prompt.c,v 1.44 2006/04/19 16:02:17 tgl Exp $
+ * $PostgreSQL: pgsql/src/bin/psql/prompt.c,v 1.45 2006/06/11 23:06:00 tgl Exp $
  */
 #include "postgres_fe.h"
 #include "prompt.h"
@@ -12,7 +12,6 @@
 
 #include "settings.h"
 #include "common.h"
-#include "pqexpbuffer.h"
 #include "input.h"
 #include "variables.h"
 
index 653e57023d8d6d3553c681d57ceb9ff88166a54d..36d2ff2202026ee98c19dcacc39c16f544257076 100644 (file)
@@ -3,7 +3,7 @@
  *
  * Copyright (c) 2000-2006, PostgreSQL Global Development Group
  *
- * $PostgreSQL: pgsql/src/bin/psql/tab-complete.c,v 1.152 2006/05/28 02:27:08 alvherre Exp $
+ * $PostgreSQL: pgsql/src/bin/psql/tab-complete.c,v 1.153 2006/06/11 23:06:00 tgl Exp $
  */
 
 /*----------------------------------------------------------------------
@@ -43,7 +43,6 @@
 
 #include "postgres_fe.h"
 #include "tab-complete.h"
-#include "pqexpbuffer.h"
 #include "input.h"
 
 /* If we don't have this, we might as well forget about the whole thing: */
@@ -51,6 +50,7 @@
 
 #include <ctype.h>
 #include "libpq-fe.h"
+#include "pqexpbuffer.h"
 #include "common.h"
 #include "settings.h"