Re: Patch for psql History Display on MacOSX - Mailing list pgsql-hackers
From | Tom Lane |
---|---|
Subject | Re: Patch for psql History Display on MacOSX |
Date | |
Msg-id | 12176.1410061202@sss.pgh.pa.us Whole thread Raw |
In response to | Re: Patch for psql History Display on MacOSX (Tom Lane <tgl@sss.pgh.pa.us>) |
Responses |
Re: Patch for psql History Display on MacOSX
|
List | pgsql-hackers |
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);
pgsql-hackers by date: