Re: Patch for psql History Display on MacOSX

I wrote:
> What I'm inclined to do based on this info is to start the loop at
> history_base - 1, and ignore NULL returns until we're past history_base.

I poked at that for awhile and decided it was a bad approach.  It emerges
that libedit's history_get() is just as full of version-specific
misbehaviors as the next_history() approach.  While we could possibly
work around all those bugs, it's true in all libedit versions that
history_get(N) is O(N) because it iterates down the history list.  So
looping over the whole history list with it is O(N^2) in the number of
history entries, which could well get painful with long history lists.

What seems better is to stick with the history_set_pos/next_history
approach, but adapt it to use previous_history when required.  This is
O(N) overall in both libreadline and libedit.  I therefore propose the
attached patch.

Experimenting with this, it seems to work as expected in Apple's
libedit-13 and up (corresponding to OS X Snow Leopard and newer).  The
fact that it works in pre-Mavericks releases is a bit accidental, because
history_set_pos() is in fact partially broken in those releases, per
comments in the patch.  And it doesn't work very well in libedit-5.1 (OS X
Tiger) because history_set_pos() is seemingly *completely* broken in that
release: it never succeeds, and we end up iterating over a subset of the
history list that does not seem to have any rhyme or reason to it.
However I don't think that this patch makes things any worse than they
were before with that release.

I only tried this directly on Tiger, Snow Leopard, and Mavericks.  I
tested libedit-28 by compiling from source on a RHEL machine, so it's
possible that there's some difference between what I tested and what
Apple's really shipping.  If anyone wants to try it on other platforms,
feel free.

[ wanders away wondering how it is that libedit has any following
whatsoever ... ]

            regards, tom lane

diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index a66093a..39b5777 100644
*** a/src/bin/psql/command.c
--- b/src/bin/psql/command.c
*************** exec_command(const char *cmd,
*** 1088,1107 ****
          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);
          if (success && !pset.quiet && fname)
              printf(_("Wrote history to file \"%s\".\n"), fname);
          if (!fname)
--- 1088,1095 ----
          char       *fname = psql_scan_slash_option(scan_state,
                                                     OT_NORMAL, NULL, true);

          expand_tilde(&fname);
!         success = printHistory(fname, pset.popt.topt.pager);
          if (success && !pset.quiet && fname)
              printf(_("Wrote history to file \"%s\".\n"), fname);
          if (!fname)
diff --git a/src/bin/psql/input.c b/src/bin/psql/input.c
index aa32a3f..136767f 100644
*** a/src/bin/psql/input.c
--- b/src/bin/psql/input.c
***************
*** 11,16 ****
--- 11,17 ----
  #include <unistd.h>
  #endif
  #include <fcntl.h>
+ #include <limits.h>

  #include "input.h"
  #include "settings.h"
*************** gets_fromFile(FILE *source)
*** 222,244 ****


  #ifdef USE_READLINE
  /*
   * 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())
      {
          /* 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;
      }
  }

  /*
--- 223,285 ----


  #ifdef USE_READLINE
+
+ /*
+  * Macros to iterate over each element of the history list
+  *
+  * You would think this would be simple enough, but in its inimitable fashion
+  * libedit has managed to break it: in all but very old libedit releases it is
+  * necessary to iterate using previous_history(), whereas in libreadline the
+  * call to use is next_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.
+  *
+  * 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.
+  */
+ #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)
  {
!     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();
  }

  /*
*************** encode_history(void)
*** 247,263 ****
  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())
      {
          /* 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';
      }
  }
  #endif   /* USE_READLINE */

--- 288,305 ----
  static void
  decode_history(void)
  {
!     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 */

*************** initializeInput(int flags)
*** 319,340 ****


  /*
!  * This function saves the readline history when user
!  * runs \s command or 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)
  {
  #ifdef USE_READLINE

--- 361,375 ----


  /*
!  * 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.
   */
! static bool
! saveHistory(char *fname, int max_lines)
  {
  #ifdef USE_READLINE

*************** saveHistory(char *fname, int max_lines,
*** 344,354 ****
       * where write_history will fail because it tries to chmod the target
       * file.
       */
!     if (useHistory && fname &&
!         strcmp(fname, DEVNULL) != 0)
      {
!         if (encodeFlag)
!             encode_history();

          /*
           * On newer versions of libreadline, truncate the history file as
--- 379,393 ----
       * where write_history will fail because it tries to chmod the target
       * file.
       */
!     if (strcmp(fname, DEVNULL) != 0)
      {
!         /*
!          * 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
*************** saveHistory(char *fname, int max_lines,
*** 362,368 ****
           * 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;
--- 401,406 ----
*************** saveHistory(char *fname, int max_lines,
*** 387,394 ****
              if (errno == 0)
                  return true;
          }
!         else
! #endif
          {
              /* truncate what we have ... */
              if (max_lines >= 0)
--- 425,431 ----
              if (errno == 0)
                  return true;
          }
! #else                            /* don't have append support */
          {
              /* truncate what we have ... */
              if (max_lines >= 0)
*************** saveHistory(char *fname, int max_lines,
*** 399,417 ****
              if (errno == 0)
                  return true;
          }

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


  static void
  finishInput(void)
  {
--- 436,508 ----
              if (errno == 0)
                  return true;
          }
+ #endif

          psql_error("could not save history to file \"%s\": %s\n",
                     fname, strerror(errno));
      }
  #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
  finishInput(void)
  {
*************** finishInput(void)
*** 421,427 ****
          int            hist_size;

          hist_size = GetVariableNum(pset.vars, "HISTSIZE", 500, -1, true);
!         saveHistory(psql_history, hist_size, true, true);
          free(psql_history);
          psql_history = NULL;
      }
--- 512,518 ----
          int            hist_size;

          hist_size = GetVariableNum(pset.vars, "HISTSIZE", 500, -1, 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
index 1d10a22..1b22399 100644
*** a/src/bin/psql/input.h
--- b/src/bin/psql/input.h
*************** char       *gets_interactive(const char *pr
*** 42,48 ****
  char       *gets_fromFile(FILE *source);

  void        initializeInput(int flags);
! bool        saveHistory(char *fname, int max_lines, bool appendFlag, bool encodeFlag);

  void        pg_append_history(const char *s, PQExpBuffer history_buf);
  void        pg_send_history(PQExpBuffer history_buf);
--- 42,49 ----
  char       *gets_fromFile(FILE *source);

  void        initializeInput(int flags);
!
! 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);