Code review for psql multiline history patch(es). Fix memory leak,

failure to enter commands in history if canceled by control-C, other
infelicities.

src/bin/psql/command.c

@@ -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);
 		}
 

src/bin/psql/help.c

@@ -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"

src/bin/psql/input.c

@@ -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");

src/bin/psql/input.h

@@ -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 */

src/bin/psql/mainloop.c

@@ -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)

src/bin/psql/prompt.c

@@ -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"
 

src/bin/psql/tab-complete.c

@@ -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"