Fix psql \s to work with recent libedit, and add pager support.

tglsfdc · tglsfdc · commit 440fcc568263 · 2014-09-08T16:10:01.000-04:00
psql's \s (print command history) doesn't work at all with recent libedit
versions when printing to the terminal, because libedit tries to do an
fchmod() on the target file which will fail if the target is /dev/tty.
(We'd already noted this in the context of the target being /dev/null.)
Even before that, it didn't work pleasantly, because libedit likes to
encode the command history file (to ensure successful reloading), which
renders it nigh unreadable, not to mention significantly different-looking
depending on exactly which libedit version you have.  So let's forget using
write_history() for this purpose, and instead print the data ourselves,
using logic similar to that used to iterate over the history for newline
encoding/decoding purposes.

While we're at it, insert the ability to use the pager when \s is printing
to the terminal.  This has been an acknowledged shortcoming of \s for many
years, so while you could argue it's not exactly a back-patchable bug fix
it still seems like a good improvement.  Anyone who's seriously annoyed
at this can use "\s /dev/tty" or local equivalent to get the old behavior.

Experimentation with this showed that the history iteration logic was
actually rather broken when used with libedit.  It turns out that with
libedit you have to use previous_history() not next_history() to advance
to more recent history entries.  The easiest and most robust fix for this
seems to be to make a run-time test to verify which function to call.
We had not noticed this because libedit doesn't really need the newline
encoding logic: its own encoding ensures that command entries containing
newlines are reloaded correctly (unlike libreadline).  So the effective
behavior with recent libedits was that only the oldest history entry got
newline-encoded or newline-decoded.  However, because of yet other bugs in
history_set_pos(), some old versions of libedit allowed the existing loop
logic to reach entries besides the oldest, which means there may be libedit
~/.psql_history files out there containing encoded newlines in more than
just the oldest entry.  To ensure we can reload such files, it seems
appropriate to back-patch this fix, even though that will result in some
incompatibility with older psql versions (ie, multiline history entries
written by a psql with this fix will look corrupted to a psql without it,
if its libedit is reasonably up to date).

Stepan Rutz and Tom Lane
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
@@ -261,7 +261,8 @@ EOF
       <term><option>--no-readline</></term>
       <listitem>
       <para>
-       Do not use readline for line editing and do not use the history.
+       Do not use <application>Readline</application> for line editing and do
+       not use the command history.
        This can be useful to turn off tab expansion when cutting and pasting.
       </para>
       </listitem>
@@ -2173,12 +2174,13 @@ lo_import 152801
         <term><literal>\s [ <replaceable class="parameter">filename</replaceable> ]</literal></term>
         <listitem>
         <para>
-        Print or save the command line history to <replaceable
-        class="parameter">filename</replaceable>. If <replaceable
-        class="parameter">filename</replaceable> is omitted, the history
-        is written to the standard output. This option is only available
-        if <application>psql</application> is configured to use the
-        <acronym>GNU</acronym> <application>Readline</application> library.
+        Print <application>psql</application>'s command line history
+        to <replaceable class="parameter">filename</replaceable>.
+        If <replaceable class="parameter">filename</replaceable> is omitted,
+        the history is written to the standard output (using the pager if
+        appropriate).  This command is not available
+        if <application>psql</application> was built
+        without <application>Readline</application> support.
         </para>
         </listitem>
       </varlistentry>
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
@@ -1046,20 +1046,8 @@ exec_command(const char *cmd,
 		char	   *fname = psql_scan_slash_option(scan_state,
 												   OT_NORMAL, NULL, true);
 
-#if defined(WIN32) && !defined(__CYGWIN__)
-
-		/*
-		 * XXX This does not work for all terminal environments or for output
-		 * containing non-ASCII characters; see comments in simple_prompt().
-		 */
-#define DEVTTY	"con"
-#else
-#define DEVTTY	"/dev/tty"
-#endif
-
 		expand_tilde(&fname);
-		/* This scrolls off the screen when using /dev/tty */
-		success = saveHistory(fname ? fname : DEVTTY, -1, false, false);
+		success = printHistory(fname, pset.popt.topt.pager);
 		if (success && !pset.quiet && fname)
 			printf(gettext("Wrote history to file \"%s/%s\".\n"),
 				   pset.dirname ? pset.dirname : ".", fname);
diff --git a/src/bin/psql/input.c b/src/bin/psql/input.c
@@ -11,6 +11,7 @@
 #include <unistd.h>
 #endif
 #include <fcntl.h>
+#include <limits.h>
 
 #include "input.h"
 #include "settings.h"
@@ -227,23 +228,73 @@ gets_fromFile(FILE *source)
 
 
 #ifdef USE_READLINE
+
+/*
+ * Macros to iterate over each element of the history list in order
+ *
+ * You would think this would be simple enough, but in its inimitable fashion
+ * libedit has managed to break it: in libreadline we must use next_history()
+ * to go from oldest to newest, but in libedit we must use previous_history().
+ * To detect what to do, we make a trial call of previous_history(): if it
+ * fails, then either next_history() is what to use, or there's zero or one
+ * history entry so that it doesn't matter which direction we go.
+ *
+ * In case that wasn't disgusting enough: the code below is not as obvious as
+ * it might appear.  In some libedit releases history_set_pos(0) fails until
+ * at least one add_history() call has been done.  This is not an issue for
+ * printHistory() or encode_history(), which cannot be invoked before that has
+ * happened.  In decode_history(), that's not so, and what actually happens is
+ * that we are sitting on the newest entry to start with, previous_history()
+ * fails, and we iterate over all the entries using next_history().  So the
+ * decode_history() loop iterates over the entries in the wrong order when
+ * using such a libedit release, and if there were another attempt to use
+ * BEGIN_ITERATE_HISTORY() before some add_history() call had happened, it
+ * wouldn't work.  Fortunately we don't care about either of those things.
+ *
+ * Usage pattern is:
+ *
+ *		BEGIN_ITERATE_HISTORY(varname);
+ *		{
+ *			loop body referencing varname->line;
+ *		}
+ *		END_ITERATE_HISTORY();
+ */
+#define BEGIN_ITERATE_HISTORY(VARNAME) \
+	do { \
+		HIST_ENTRY *VARNAME; \
+		bool		use_prev_; \
+		\
+		history_set_pos(0); \
+		use_prev_ = (previous_history() != NULL); \
+		history_set_pos(0); \
+		for (VARNAME = current_history(); VARNAME != NULL; \
+			 VARNAME = use_prev_ ? previous_history() : next_history()) \
+		{ \
+			(void) 0
+
+#define END_ITERATE_HISTORY() \
+		} \
+	} while(0)
+
+
 /*
  * Convert newlines to NL_IN_HISTORY for safe saving in readline history file
  */
 static void
 encode_history(void)
 {
-	HIST_ENTRY *cur_hist;
-	char	   *cur_ptr;
-
-	history_set_pos(0);
-	for (cur_hist = current_history(); cur_hist; cur_hist = next_history())
+	BEGIN_ITERATE_HISTORY(cur_hist);
 	{
+		char	   *cur_ptr;
+
 		/* some platforms declare HIST_ENTRY.line as const char * */
 		for (cur_ptr = (char *) cur_hist->line; *cur_ptr; cur_ptr++)
+		{
 			if (*cur_ptr == '\n')
 				*cur_ptr = NL_IN_HISTORY;
+		}
 	}
+	END_ITERATE_HISTORY();
 }
 
 /*
@@ -252,17 +303,18 @@ encode_history(void)
 static void
 decode_history(void)
 {
-	HIST_ENTRY *cur_hist;
-	char	   *cur_ptr;
-
-	history_set_pos(0);
-	for (cur_hist = current_history(); cur_hist; cur_hist = next_history())
+	BEGIN_ITERATE_HISTORY(cur_hist);
 	{
+		char	   *cur_ptr;
+
 		/* some platforms declare HIST_ENTRY.line as const char * */
 		for (cur_ptr = (char *) cur_hist->line; *cur_ptr; cur_ptr++)
+		{
 			if (*cur_ptr == NL_IN_HISTORY)
 				*cur_ptr = '\n';
+		}
 	}
+	END_ITERATE_HISTORY();
 }
 #endif   /* USE_READLINE */
 
@@ -322,22 +374,15 @@ initializeInput(int flags)
 
 
 /*
- * This function saves the readline history when user
- * runs \s command or when psql exits.
+ * This function saves the readline history when psql exits.
  *
  * fname: pathname of history file.  (Should really be "const char *",
  * but some ancient versions of readline omit the const-decoration.)
  *
  * max_lines: if >= 0, limit history file to that many entries.
- *
- * appendFlag: if true, try to append just our new lines to the file.
- * If false, write the whole available history.
- *
- * encodeFlag: whether to encode \n as \x01.  For \s calls we don't wish
- * to do that, but must do so when saving the final history file.
  */
-bool
-saveHistory(char *fname, int max_lines, bool appendFlag, bool encodeFlag)
+static bool
+saveHistory(char *fname, int max_lines)
 {
 #ifdef USE_READLINE
 
@@ -347,11 +392,15 @@ saveHistory(char *fname, int max_lines, bool appendFlag, bool encodeFlag)
 	 * where write_history will fail because it tries to chmod the target
 	 * file.
 	 */
-	if (useHistory && fname &&
-		strcmp(fname, DEVNULL) != 0)
+	if (strcmp(fname, DEVNULL) != 0)
 	{
-		if (encodeFlag)
-			encode_history();
+		/*
+		 * Encode \n, since otherwise readline will reload multiline history
+		 * entries as separate lines.  (libedit doesn't really need this, but
+		 * we do it anyway since it's too hard to tell which implementation we
+		 * are using.)
+		 */
+		encode_history();
 
 		/*
 		 * On newer versions of libreadline, truncate the history file as
@@ -365,7 +414,6 @@ saveHistory(char *fname, int max_lines, bool appendFlag, bool encodeFlag)
 		 * see if the write failed.  Similarly for append_history.
 		 */
 #if defined(HAVE_HISTORY_TRUNCATE_FILE) && defined(HAVE_APPEND_HISTORY)
-		if (appendFlag)
 		{
 			int			nlines;
 			int			fd;
@@ -390,8 +438,7 @@ saveHistory(char *fname, int max_lines, bool appendFlag, bool encodeFlag)
 			if (errno == 0)
 				return true;
 		}
-		else
-#endif
+#else							/* don't have append support */
 		{
 			/* truncate what we have ... */
 			if (max_lines >= 0)
@@ -402,19 +449,73 @@ saveHistory(char *fname, int max_lines, bool appendFlag, bool encodeFlag)
 			if (errno == 0)
 				return true;
 		}
+#endif
 
 		psql_error("could not save history to file \"%s\": %s\n",
 				   fname, strerror(errno));
 	}
-#else
-	/* only get here in \s case, so complain */
-	psql_error("history is not supported by this installation\n");
 #endif
 
 	return false;
 }
 
 
+/*
+ * Print history to the specified file, or to the console if fname is NULL
+ * (psql \s command)
+ *
+ * We used to use saveHistory() for this purpose, but that doesn't permit
+ * use of a pager; moreover libedit's implementation behaves incompatibly
+ * (preferring to encode its output) and may fail outright when the target
+ * file is specified as /dev/tty.
+ */
+bool
+printHistory(const char *fname, unsigned short int pager)
+{
+#ifdef USE_READLINE
+	FILE	   *output;
+	bool		is_pager;
+
+	if (!useHistory)
+		return false;
+
+	if (fname == NULL)
+	{
+		/* use pager, if enabled, when printing to console */
+		output = PageOutput(INT_MAX, pager);
+		is_pager = true;
+	}
+	else
+	{
+		output = fopen(fname, "w");
+		if (output == NULL)
+		{
+			psql_error("could not save history to file \"%s\": %s\n",
+					   fname, strerror(errno));
+			return false;
+		}
+		is_pager = false;
+	}
+
+	BEGIN_ITERATE_HISTORY(cur_hist);
+	{
+		fprintf(output, "%s\n", cur_hist->line);
+	}
+	END_ITERATE_HISTORY();
+
+	if (is_pager)
+		ClosePager(output);
+	else
+		fclose(output);
+
+	return true;
+#else
+	psql_error("history is not supported by this installation\n");
+	return false;
+#endif
+}
+
+
 static void
 #ifdef HAVE_ATEXIT
 finishInput(void)
@@ -428,7 +529,7 @@ finishInput(int exitstatus, void *arg)
 		int			hist_size;
 
 		hist_size = GetVariableNum(pset.vars, "HISTSIZE", 500, -1, true);
-		saveHistory(psql_history, hist_size, true, true);
+		(void) saveHistory(psql_history, hist_size);
 		free(psql_history);
 		psql_history = NULL;
 	}
diff --git a/src/bin/psql/input.h b/src/bin/psql/input.h
@@ -42,7 +42,8 @@ char	   *gets_interactive(const char *prompt);
 char	   *gets_fromFile(FILE *source);
 
 void		initializeInput(int flags);
-bool		saveHistory(char *fname, int max_lines, bool appendFlag, bool encodeFlag);
+
+bool		printHistory(const char *fname, unsigned short int pager);
 
 void		pg_append_history(const char *s, PQExpBuffer history_buf);
 void		pg_send_history(PQExpBuffer history_buf);
