Thread: Patch for psql History Display on MacOSX
Hi everbody, My first mail to this one, so please be mild. I fired up the debugger to get this item going, which is also on the Todo List. Attached is a very trivial patch as a basis for discussion that at least makes \s (show history) work in psql on Macs. Macsuses libedit, which has a libreadline interface. A short investigation showed that the way psql iterates over the history does not work with libedit. I changed the iterationscheme to an index based loop (see code and comments), which seemed to be the only working option for both readlineand libedit. In any case, i have tested and compiled this on MacOX 10.9.3 and Linux. Windows doesn’t have the pagerin the first place. As noted in the todo I have made this code pay attention to the pager configuration from psql. The odd part is when yourhistory opens in less you see the top part rather then the bottom part, but the bottom is just a single keystroke away.If pager is disabled history is just printed fine. Please note that this didn’t work at all on Mac before. Could thisgo into …./regress/sql/psql.sql at all? I am not sure on that one. Regards, Stepan
Attachment
Stepan Rutz <stepan.rutz@gmx.de> writes:
> Attached is a very trivial patch as a basis for discussion that at least makes \s (show history) work in psql on
Macs.Macs uses libedit, which has a libreadline interface.
Hm. The $64 question here is whether we can assume that history_get()
exists and works compatibly in every interesting version of libreadline
and libedit.
I poked into the oldest version of GNU readline I could find, 4.0
(released in 1999), and that has it. The oldest libedit I have around
is the one that came with OSX 10.4 (the CVS marker in readline.h from
that says 2004/01/17). That has it too. So that looks pretty good.
The readline code says that the argument ranges from "history_base"
up, not from 1 up as this patch assumes. And it looks like history_base
can change once the max number of stored lines is exceeded, so we can't
assume that 1 is good enough. Fortunately, the global variable
history_base also exists in both libraries (though it looks like it
never changes from 1 in libedit).
Functionally this seems like a clear win over what we had, especially
since it supports using the pager. I'm inclined to think we should
not only apply this change but back-patch it.
One thing worth thinking about: should we use a history_get() loop
like this for *all* \s commands, even when the target file is a
regular file not /dev/tty? libedit's version of write_history does
not write the history "in the clear" exactly, which you would think
is the behavior wanted when saving a command history for any purpose
other than updating ~/.psql_history. Such a change would break a
workflow that involves doing \s to some random file and then copying
that file to ~/.psql_history, but I find it hard to fathom why anyone
would do that.
There are a couple other minor bugs and some cosmetic things I don't like
in this patch, but I'm willing to fix it up and commit it if there
are not objections.
regards, tom lane
Thanks Tom. This would help the poor mac-osx guys like me. I guess this is not that important because no one runs a productionserver on OS-X. Back patching to 9.3 won’t work as is, some minor conflict was there. Anyway, I am sure the iteration used in encode_history and decode_history in input.c does not work on libedit. Regards from cologne, Stepan Am 01.09.2014 um 20:05 schrieb Tom Lane <tgl@sss.pgh.pa.us>: > Stepan Rutz <stepan.rutz@gmx.de> writes: >> Attached is a very trivial patch as a basis for discussion that at least makes \s (show history) work in psql on Macs.Macs uses libedit, which has a libreadline interface. > > Hm. The $64 question here is whether we can assume that history_get() > exists and works compatibly in every interesting version of libreadline > and libedit. > > I poked into the oldest version of GNU readline I could find, 4.0 > (released in 1999), and that has it. The oldest libedit I have around > is the one that came with OSX 10.4 (the CVS marker in readline.h from > that says 2004/01/17). That has it too. So that looks pretty good. > > The readline code says that the argument ranges from "history_base" > up, not from 1 up as this patch assumes. And it looks like history_base > can change once the max number of stored lines is exceeded, so we can't > assume that 1 is good enough. Fortunately, the global variable > history_base also exists in both libraries (though it looks like it > never changes from 1 in libedit). > > Functionally this seems like a clear win over what we had, especially > since it supports using the pager. I'm inclined to think we should > not only apply this change but back-patch it. > > One thing worth thinking about: should we use a history_get() loop > like this for *all* \s commands, even when the target file is a > regular file not /dev/tty? libedit's version of write_history does > not write the history "in the clear" exactly, which you would think > is the behavior wanted when saving a command history for any purpose > other than updating ~/.psql_history. Such a change would break a > workflow that involves doing \s to some random file and then copying > that file to ~/.psql_history, but I find it hard to fathom why anyone > would do that. > > There are a couple other minor bugs and some cosmetic things I don't like > in this patch, but I'm willing to fix it up and commit it if there > are not objections. > > regards, tom lane > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers
Stepan Rutz <stepan.rutz@gmx.de> writes:
> Anyway, I am sure the iteration used in encode_history and decode_history in input.c does not work on libedit.
Yeah, I noticed your comment about that. That seems odd; a look at the
Apple libedit sources suggests it should work. I was just about to trace
through the logic and try to see what's happening.
The reason nobody noticed is possibly that libedit doesn't actually need
the newline-encoding hack. However, we should probably fix the loops if
they aren't working as expected on libedit, just in case somebody tries
to copy the logic for some other purpose.
Meanwhile, attached is a draft patch that uses the history_get loop for
all \s operations, and simplifies saveHistory in consequence.
regards, tom lane
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index e16b4d5..e1949d8 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..f43e4a2 100644
*** a/src/bin/psql/input.c
--- b/src/bin/psql/input.c
*************** 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
--- 319,333 ----
/*
! * 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
--- 337,351 ----
* 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;
--- 359,364 ----
*************** saveHistory(char *fname, int max_lines,
*** 387,394 ****
if (errno == 0)
return true;
}
! else
! #endif
{
/* truncate what we have ... */
if (max_lines >= 0)
--- 383,389 ----
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)
{
--- 394,467 ----
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;
+ int i;
+ HIST_ENTRY *curr;
+
+ if (!useHistory)
+ return false;
+
+ if (fname == NULL)
+ {
+ /* use pager if appropriate when printing to console */
+ output = PageOutput(history_length, 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;
+ }
+
+ for (i = history_base; (curr = history_get(i)) != NULL; i++)
+ {
+ fprintf(output, "%s\n", curr->line);
+ }
+
+ 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;
}
--- 471,477 ----
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);
I wrote:
> Stepan Rutz <stepan.rutz@gmx.de> writes:
>> Anyway, I am sure the iteration used in encode_history and decode_history in input.c does not work on libedit.
> Yeah, I noticed your comment about that. That seems odd; a look at the
> Apple libedit sources suggests it should work. I was just about to trace
> through the logic and try to see what's happening.
Sigh ... the answer is that libedit has the direction of traversal
backwards compared to libreadline. If you replace next_history() by
previous_history() in those loops, then it works as expected.
> The reason nobody noticed is possibly that libedit doesn't actually need
> the newline-encoding hack.
Indeed, that's the reason.
> However, we should probably fix the loops if
> they aren't working as expected on libedit, just in case somebody tries
> to copy the logic for some other purpose.
We should either do that, or document what's actually going on here.
A disadvantage of fixing this is that psql versions containing the fix
would be incompatible with versions without (since writing out a history
file containing ^A in place of ^J, and not reversing that encoding upon
reload, would lead to messed-up history data). However, I have a feeling
that we'd better proceed with a fix. Sooner or later, somebody is going
to point out to the libedit guys that they've emulated libreadline
incorrectly. If they fix that, we'll have a situation where psql's using
different libedit versions are incompatible, which would be even more of
a mess.
regards, tom lane
I wrote:
>> Stepan Rutz <stepan.rutz@gmx.de> writes:
>>> Anyway, I am sure the iteration used in encode_history and decode_history in input.c does not work on libedit.
>> Yeah, I noticed your comment about that. That seems odd; a look at the
>> Apple libedit sources suggests it should work. I was just about to trace
>> through the logic and try to see what's happening.
> Sigh ... the answer is that libedit has the direction of traversal
> backwards compared to libreadline. If you replace next_history() by
> previous_history() in those loops, then it works as expected.
Oh, even *more* interesting: the existing coding seems to work as designed
in OS X Tiger. I duplicated your result that it's broken on Mavericks
(that was what you were testing, no?). I have a couple intermediate
Mac versions laying about, but no time to test them right now.
So apparently what we've got here is another episode in Apple's
nearly-unblemished record of shipping broken versions of libedit.
Sigh. Either they have astonishingly bad luck at choosing when to
pull from the upstream sources, or the upstream sources are broken
most of the time.
regards, tom lane
On Mon, Sep 01, 2014 at 02:05:37PM -0400, Tom Lane wrote: > Functionally this seems like a clear win over what we had, especially > since it supports using the pager. I'm inclined to think we should > not only apply this change but back-patch it. > > One thing worth thinking about: should we use a history_get() loop > like this for *all* \s commands, even when the target file is a > regular file not /dev/tty? +1 for printing the same bytes regardless of destination. > libedit's version of write_history does > not write the history "in the clear" exactly, which you would think > is the behavior wanted when saving a command history for any purpose > other than updating ~/.psql_history. Such a change would break a > workflow that involves doing \s to some random file and then copying > that file to ~/.psql_history, but I find it hard to fathom why anyone > would do that. I've not used \s apart from verifying that certain patches didn't break it. (That "less ~/.psql_history" beats dumping thousands of lines to the tty was a factor.) "\s fname" is theoretically useful as an OS-independent alternative to "cp ~/.psql_history fname". I see too little certainty of net benefit to justify a minor-release change to this. On Mon, Sep 01, 2014 at 04:27:57PM -0400, Tom Lane wrote: [history encoding change discussion] > A disadvantage of fixing this is that psql versions containing the fix > would be incompatible with versions without (since writing out a history > file containing ^A in place of ^J, and not reversing that encoding upon > reload, would lead to messed-up history data). However, I have a feeling > that we'd better proceed with a fix. Sooner or later, somebody is going > to point out to the libedit guys that they've emulated libreadline > incorrectly. If they fix that, we'll have a situation where psql's using > different libedit versions are incompatible, which would be even more of > a mess. Yikes. It's already painful to see libedit and GNU readline trash each other's .psql_history files; let's not add a third format. Long-term, psql should cease relying on the history library to serialize and deserialize its history file. psql can then understand both formats, rewrite in the original format, and use GNU format for new files. Thanks, nm
Noah Misch <noah@leadboat.com> writes: > On Mon, Sep 01, 2014 at 02:05:37PM -0400, Tom Lane wrote: >> Functionally this seems like a clear win over what we had, especially >> since it supports using the pager. I'm inclined to think we should >> not only apply this change but back-patch it. > I've not used \s apart from verifying that certain patches didn't break it. > (That "less ~/.psql_history" beats dumping thousands of lines to the tty was a > factor.) "\s fname" is theoretically useful as an OS-independent alternative > to "cp ~/.psql_history fname". I see too little certainty of net benefit to > justify a minor-release change to this. I disagree. \s to the tty is *completely broken* on all but quite old libedit releases, cf http://www.postgresql.org/message-id/17435.1408719984@sss.pgh.pa.us That seems to me to be a bug worthy of back-patching a fix for. Also, as best I can tell, .psql_history files from older libedit versions are not forward-compatible to current libedit versions because of the failure of the decode_history() loop to reach all lines of the file when using current libedit. That is also a back-patchable bug fix IMO. (Closer investigation suggests this is a bug or definitional change in libedit's history_set_pos, not so much in next_history vs previous_history. But whatever it is, it behooves us to work around it.) You could certainly argue that the introduction of pager support is a feature addition not a bug fix, but I can't really see the point of leaving out that part of the patch in the back branches. The lack of pager support in \s has been an acknowledged bug since forever, and I don't think the pager calls in the new code are the riskiest part of it. > Yikes. It's already painful to see libedit and GNU readline trash each > other's .psql_history files; let's not add a third format. There's no third format involved in this patch, though we'd need one if we stopped using the underlying libraries' read/write functions, since both those formats suck for different reasons. I agree that it might be best if we did that, but designing and testing such a change seems well beyond the scope of a back-patchable fix. regards, tom lane
I've confirmed that the attached patches work as expected in both the
oldest and newest readline and libedit versions available to me.
Barring further objections, I plan to commit and back-patch these
changes.
regards, tom lane
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 74d4618..6033dfd 100644
*** a/doc/src/sgml/ref/psql-ref.sgml
--- b/doc/src/sgml/ref/psql-ref.sgml
*************** EOF
*** 277,283 ****
<term><option>--no-readline</></term>
<listitem>
<para>
! Do not use <application>readline</application> for line editing and do not use the history.
This can be useful to turn off tab expansion when cutting and pasting.
</para>
</listitem>
--- 277,284 ----
<term><option>--no-readline</></term>
<listitem>
<para>
! 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>
*************** lo_import 152801
*** 2357,2368 ****
<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.
</para>
</listitem>
</varlistentry>
--- 2358,2370 ----
<term><literal>\s [ <replaceable class="parameter">filename</replaceable> ]</literal></term>
<listitem>
<para>
! 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
index e16b4d5..e1949d8 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..2e01eb1 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"
*************** 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
--- 320,334 ----
/*
! * 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
--- 338,352 ----
* 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;
--- 360,365 ----
*************** saveHistory(char *fname, int max_lines,
*** 387,394 ****
if (errno == 0)
return true;
}
! else
! #endif
{
/* truncate what we have ... */
if (max_lines >= 0)
--- 384,390 ----
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)
{
--- 395,468 ----
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;
+ int i;
+ HIST_ENTRY *cur_hist;
+
+ 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;
+ }
+
+ for (i = history_base; (cur_hist = history_get(i)) != NULL; i++)
+ {
+ fprintf(output, "%s\n", cur_hist->line);
+ }
+
+ 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;
}
--- 472,478 ----
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);
diff --git a/src/bin/psql/input.c b/src/bin/psql/input.c
index 2e01eb1..f3193a2 100644
*** a/src/bin/psql/input.c
--- b/src/bin/psql/input.c
*************** gets_fromFile(FILE *source)
*** 229,244 ****
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;
}
}
--- 229,246 ----
static void
encode_history(void)
{
+ int i;
HIST_ENTRY *cur_hist;
char *cur_ptr;
! for (i = history_base; (cur_hist = history_get(i)) != NULL; i++)
{
/* 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;
+ }
}
}
*************** encode_history(void)
*** 248,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 */
--- 250,267 ----
static void
decode_history(void)
{
+ int i;
HIST_ENTRY *cur_hist;
char *cur_ptr;
! for (i = history_base; (cur_hist = history_get(i)) != NULL; i++)
{
/* 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 */
On Mon, Sep 01, 2014 at 10:22:57PM -0400, Tom Lane wrote: > Noah Misch <noah@leadboat.com> writes: > > On Mon, Sep 01, 2014 at 02:05:37PM -0400, Tom Lane wrote: > >> Functionally this seems like a clear win over what we had, especially > >> since it supports using the pager. I'm inclined to think we should > >> not only apply this change but back-patch it. > > > I've not used \s apart from verifying that certain patches didn't break it. > > (That "less ~/.psql_history" beats dumping thousands of lines to the tty was a > > factor.) "\s fname" is theoretically useful as an OS-independent alternative > > to "cp ~/.psql_history fname". I see too little certainty of net benefit to > > justify a minor-release change to this. > > I disagree. \s to the tty is *completely broken* on all but quite old > libedit releases, cf > http://www.postgresql.org/message-id/17435.1408719984@sss.pgh.pa.us > That seems to me to be a bug worthy of back-patching a fix for. I'm with you that far. Given a patch that does not change "\s /tmp/foo" and that makes "\s" equivalent to "\s /tmp/foo" + "\! cat /tmp/foo >/dev/tty", back-patch by all means. No patch posted on this thread is so surgical, hence my objection. In particular, your latest patch revision changes "\s /tmp/foo" to match the novel output the patch introduces for plain "\s". "\s /tmp/foo" would no longer write data that libedit can reload as a history file. I'm cautiously optimistic that nobody relies on today's "\s /tmp/foo" behavior, but I'm confident that folks can wait for 9.5 to get the envisaged benefits. > Also, as best I can tell, .psql_history files from older libedit versions > are not forward-compatible to current libedit versions because of the > failure of the decode_history() loop to reach all lines of the file > when using current libedit. That is also a back-patchable bug fix IMO. > (Closer investigation suggests this is a bug or definitional change in > libedit's history_set_pos, not so much in next_history vs > previous_history. But whatever it is, it behooves us to work around it.) I haven't studied this part of the topic other than to read what you have written. All other things being equal, I agree. If fixing this will make psql-9.3.6 w/ libedit-20141001 write history files that confuse psql-9.3.5 w/ libedit-20141001, that changes the calculus. Will it? > You could certainly argue that the introduction of pager support is a > feature addition not a bug fix, but I can't really see the point of > leaving out that part of the patch in the back branches. The lack of > pager support in \s has been an acknowledged bug since forever, and I > don't think the pager calls in the new code are the riskiest part of it. Agreed; if the pager support were the only debatable aspect, I would not have commented.
Noah Misch <noah@leadboat.com> writes:
> On Mon, Sep 01, 2014 at 10:22:57PM -0400, Tom Lane wrote:
>> Also, as best I can tell, .psql_history files from older libedit versions
>> are not forward-compatible to current libedit versions because of the
>> failure of the decode_history() loop to reach all lines of the file
>> when using current libedit. That is also a back-patchable bug fix IMO.
>> (Closer investigation suggests this is a bug or definitional change in
>> libedit's history_set_pos, not so much in next_history vs
>> previous_history. But whatever it is, it behooves us to work around it.)
> I haven't studied this part of the topic other than to read what you have
> written. All other things being equal, I agree. If fixing this will make
> psql-9.3.6 w/ libedit-20141001 write history files that confuse psql-9.3.5 w/
> libedit-20141001, that changes the calculus. Will it?
I'm not sure exactly when things changed, but I have verified that the
existing loops in decode/encode_history visit all lines of the history
when using OS X Tiger's libedit library. On OS X Mavericks, the loops
visit only the oldest history entry, as Stepan reported. This means that
there may be libedit-style ~/.psql_history files out there in which ^A has
been substituted for ^J (in lines after the oldest), which will not be
correctly reloaded by psql versions using newer libedit.
It's certainly arguable whether this is an issue warranting a back-patch,
since we've not heard field complaints about it AFAIR. But I think we
ought to do so. I think "psql N produces files that psql N+1 can't read"
is worse than the reverse case, and that's exactly what we're debating
here.
regards, tom lane
Noah Misch <noah@leadboat.com> writes:
> I'm with you that far. Given a patch that does not change "\s /tmp/foo" and
> that makes "\s" equivalent to "\s /tmp/foo" + "\! cat /tmp/foo >/dev/tty",
> back-patch by all means. No patch posted on this thread is so surgical, hence
> my objection. In particular, your latest patch revision changes "\s /tmp/foo"
> to match the novel output the patch introduces for plain "\s". "\s /tmp/foo"
> would no longer write data that libedit can reload as a history file.
BTW, I failed last night to produce a coherent argument against that
particular point, but consider this. What are the main use-cases for
\s to a file? I argue that they are
1. Create a human-readable record of what you did.2. Create the starting point for a SQL script file.
I do not deny it's possible that somebody out there is also using \s for
3. Create a file that I can overwrite ~/.psql_history with later.
But if this is being done in the field at all, surely it is miles behind
the applications listed above.
Now, if you are using libreadline, the output of \s has always been
perfectly fit for purposes 1 and 2, because it's plain text of the
history entries. Moreover, it is *not* particularly fit for purpose 3,
because intra-command newlines aren't encoded. Yes, you could get
libreadline to read the file, but multiline SQL commands will be seen
as multiple history entries which is very far from convenient to use.
(This adds to my suspicion that nobody is doing #3 in practice.)
On the other hand, if you are using libedit, purpose 3 works great
but the output is utterly unfit for either purpose 1 or 2. Here
are the first few lines of ~/.psql_history on one of my Macs:
_HiStOrY_V2_
explain\040verbose\^A\040\040select\0401\^Aunion\^A\040\040select\0402;
\\q
select\0404;
explain\040verbose\^A\040\040select\0401\^Aunion\^A\040\040select\0402;
select\04044;
\\q
\\s
\\s\040foobar
\\q
What the proposed patch does is ensure that \s produces plain text
regardless of which history library you are using. I think arguing
that we shouldn't do that is stretching the concept of backwards
compatibility well past the breaking point. Moreover, output like
the above doesn't satisfy the existing description of \s, namely
that it prints your history.
regards, tom lane
On Tue, Sep 02, 2014 at 01:56:34AM -0400, Tom Lane wrote: > Noah Misch <noah@leadboat.com> writes: > > On Mon, Sep 01, 2014 at 10:22:57PM -0400, Tom Lane wrote: > >> Also, as best I can tell, .psql_history files from older libedit versions > >> are not forward-compatible to current libedit versions because of the > >> failure of the decode_history() loop to reach all lines of the file > >> when using current libedit. That is also a back-patchable bug fix IMO. > >> (Closer investigation suggests this is a bug or definitional change in > >> libedit's history_set_pos, not so much in next_history vs > >> previous_history. But whatever it is, it behooves us to work around it.) > > > I haven't studied this part of the topic other than to read what you have > > written. All other things being equal, I agree. If fixing this will make > > psql-9.3.6 w/ libedit-20141001 write history files that confuse psql-9.3.5 w/ > > libedit-20141001, that changes the calculus. Will it? > > I'm not sure exactly when things changed, but I have verified that the > existing loops in decode/encode_history visit all lines of the history > when using OS X Tiger's libedit library. On OS X Mavericks, the loops > visit only the oldest history entry, as Stepan reported. This means that > there may be libedit-style ~/.psql_history files out there in which ^A has > been substituted for ^J (in lines after the oldest), which will not be > correctly reloaded by psql versions using newer libedit. > > It's certainly arguable whether this is an issue warranting a back-patch, > since we've not heard field complaints about it AFAIR. But I think we > ought to do so. I think "psql N produces files that psql N+1 can't read" > is worse than the reverse case, and that's exactly what we're debating > here. I tried your patches against libedit-28. Wherever a command contains a newline, unpatched psql writes the three bytes "\^A" to the history file, and patched psql writes the four bytes "\012". Unpatched psql correctly reads either form of the history file. Patched psql misinterprets a history file created by unpatched psql, placing 0x01 bytes in the recalled command where it should have newlines. That's a worrisome compatibility break.
On Tue, Sep 02, 2014 at 09:49:56AM -0400, Tom Lane wrote: > Noah Misch <noah@leadboat.com> writes: > > I'm with you that far. Given a patch that does not change "\s /tmp/foo" and > > that makes "\s" equivalent to "\s /tmp/foo" + "\! cat /tmp/foo >/dev/tty", > > back-patch by all means. No patch posted on this thread is so surgical, hence > > my objection. In particular, your latest patch revision changes "\s /tmp/foo" > > to match the novel output the patch introduces for plain "\s". "\s /tmp/foo" > > would no longer write data that libedit can reload as a history file. > > BTW, I failed last night to produce a coherent argument against that > particular point, but consider this. What are the main use-cases for > \s to a file? I argue that they are > > 1. Create a human-readable record of what you did. > 2. Create the starting point for a SQL script file. > > I do not deny it's possible that somebody out there is also using \s for > > 3. Create a file that I can overwrite ~/.psql_history with later. > > But if this is being done in the field at all, surely it is miles behind > the applications listed above. I'm unprepared to speculate about the relative prevalence of those use cases. > Now, if you are using libreadline, the output of \s has always been > perfectly fit for purposes 1 and 2, because it's plain text of the > history entries. Moreover, it is *not* particularly fit for purpose 3, > because intra-command newlines aren't encoded. Yes, you could get > libreadline to read the file, but multiline SQL commands will be seen > as multiple history entries which is very far from convenient to use. > (This adds to my suspicion that nobody is doing #3 in practice.) > > On the other hand, if you are using libedit, purpose 3 works great > but the output is utterly unfit for either purpose 1 or 2. Here > are the first few lines of ~/.psql_history on one of my Macs: > > _HiStOrY_V2_ > explain\040verbose\^A\040\040select\0401\^Aunion\^A\040\040select\0402; > \\q > select\0404; > explain\040verbose\^A\040\040select\0401\^Aunion\^A\040\040select\0402; > select\04044; > \\q > \\s > \\s\040foobar > \\q > > What the proposed patch does is ensure that \s produces plain text > regardless of which history library you are using. I think arguing > that we shouldn't do that is stretching the concept of backwards > compatibility well past the breaking point. Given the negligible urgency to improve \s, the slightest compatibility hazard justifies punting this work from back-patch to master-only.
Hello again, just my thoughts… in psql \s without a file is nice for me iff going through less (e.g. pager), but for the most part it doesn't work at allon mac-osx. so nothing to lose for the mac psql users. regards, stepan Am 03.09.2014 um 07:45 schrieb Noah Misch <noah@leadboat.com>: > On Tue, Sep 02, 2014 at 09:49:56AM -0400, Tom Lane wrote: >> Noah Misch <noah@leadboat.com> writes: >>> I'm with you that far. Given a patch that does not change "\s /tmp/foo" and >>> that makes "\s" equivalent to "\s /tmp/foo" + "\! cat /tmp/foo >/dev/tty", >>> back-patch by all means. No patch posted on this thread is so surgical, hence >>> my objection. In particular, your latest patch revision changes "\s /tmp/foo" >>> to match the novel output the patch introduces for plain "\s". "\s /tmp/foo" >>> would no longer write data that libedit can reload as a history file. >> >> BTW, I failed last night to produce a coherent argument against that >> particular point, but consider this. What are the main use-cases for >> \s to a file? I argue that they are >> >> 1. Create a human-readable record of what you did. >> 2. Create the starting point for a SQL script file. >> >> I do not deny it's possible that somebody out there is also using \s for >> >> 3. Create a file that I can overwrite ~/.psql_history with later. >> >> But if this is being done in the field at all, surely it is miles behind >> the applications listed above. > > I'm unprepared to speculate about the relative prevalence of those use cases. > >> Now, if you are using libreadline, the output of \s has always been >> perfectly fit for purposes 1 and 2, because it's plain text of the >> history entries. Moreover, it is *not* particularly fit for purpose 3, >> because intra-command newlines aren't encoded. Yes, you could get >> libreadline to read the file, but multiline SQL commands will be seen >> as multiple history entries which is very far from convenient to use. >> (This adds to my suspicion that nobody is doing #3 in practice.) >> >> On the other hand, if you are using libedit, purpose 3 works great >> but the output is utterly unfit for either purpose 1 or 2. Here >> are the first few lines of ~/.psql_history on one of my Macs: >> >> _HiStOrY_V2_ >> explain\040verbose\^A\040\040select\0401\^Aunion\^A\040\040select\0402; >> \\q >> select\0404; >> explain\040verbose\^A\040\040select\0401\^Aunion\^A\040\040select\0402; >> select\04044; >> \\q >> \\s >> \\s\040foobar >> \\q >> >> What the proposed patch does is ensure that \s produces plain text >> regardless of which history library you are using. I think arguing >> that we shouldn't do that is stretching the concept of backwards >> compatibility well past the breaking point. > > Given the negligible urgency to improve \s, the slightest compatibility hazard > justifies punting this work from back-patch to master-only. > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Sep 3, 2014 at 12:35 AM, Noah Misch <noah@leadboat.com> wrote: > On Tue, Sep 02, 2014 at 01:56:34AM -0400, Tom Lane wrote: >> Noah Misch <noah@leadboat.com> writes: >> > On Mon, Sep 01, 2014 at 10:22:57PM -0400, Tom Lane wrote: >> >> Also, as best I can tell, .psql_history files from older libedit versions >> >> are not forward-compatible to current libedit versions because of the >> >> failure of the decode_history() loop to reach all lines of the file >> >> when using current libedit. That is also a back-patchable bug fix IMO. >> >> (Closer investigation suggests this is a bug or definitional change in >> >> libedit's history_set_pos, not so much in next_history vs >> >> previous_history. But whatever it is, it behooves us to work around it.) >> >> > I haven't studied this part of the topic other than to read what you have >> > written. All other things being equal, I agree. If fixing this will make >> > psql-9.3.6 w/ libedit-20141001 write history files that confuse psql-9.3.5 w/ >> > libedit-20141001, that changes the calculus. Will it? >> >> I'm not sure exactly when things changed, but I have verified that the >> existing loops in decode/encode_history visit all lines of the history >> when using OS X Tiger's libedit library. On OS X Mavericks, the loops >> visit only the oldest history entry, as Stepan reported. This means that >> there may be libedit-style ~/.psql_history files out there in which ^A has >> been substituted for ^J (in lines after the oldest), which will not be >> correctly reloaded by psql versions using newer libedit. >> >> It's certainly arguable whether this is an issue warranting a back-patch, >> since we've not heard field complaints about it AFAIR. But I think we >> ought to do so. I think "psql N produces files that psql N+1 can't read" >> is worse than the reverse case, and that's exactly what we're debating >> here. > > I tried your patches against libedit-28. Wherever a command contains a > newline, unpatched psql writes the three bytes "\^A" to the history file, and > patched psql writes the four bytes "\012". Unpatched psql correctly reads > either form of the history file. Patched psql misinterprets a history file > created by unpatched psql, placing 0x01 bytes in the recalled command where it > should have newlines. That's a worrisome compatibility break. Worrisome seems like a strong word, but certainly irritating. FWIW, my Mac has psql linked to /usr/lib/libedit.3.dylib, is running 10.8.5, and has history file lines that look like this: select\0401\040union\040select\0401; (You may wonder whether I actually get paid to craft such exciting SQL commands. Turns out I do.) One point to note is that not back-patching this doesn't really fix anything. Will a user be annoyed when .psql_history fails to reload properly on a new minor release, but utterly indifferent to whether it reloads in a new major release? What if they run multiple major releases of PostgreSQL on the same machine, using the psql executable for each version when talking to that version? (Yeah, I know it's backward compatible, but not everyone may realize that, or care.) Given that, if we're going to do it this way at all, I favor back-patching: at least then the newest releases of all supported branches will be compatible with each other. But I'm still fuzzy on why we need to give up the ability to read the old format in the first place. Can't we just fix that and be done with this? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Noah Misch <noah@leadboat.com> writes:
> I tried your patches against libedit-28. Wherever a command contains a
> newline, unpatched psql writes the three bytes "\^A" to the history file, and
> patched psql writes the four bytes "\012". Unpatched psql correctly reads
> either form of the history file. Patched psql misinterprets a history file
> created by unpatched psql, placing 0x01 bytes in the recalled command where it
> should have newlines. That's a worrisome compatibility break.
I think you got the test cases backwards, or maybe neglected the aspect
about how unpatched psql will only translate ^J to ^A in the oldest
(or maybe the newest? too pressed for time to recheck right now) history
entry.
The issue is that a patched psql, or a psql with a sufficient old libedit,
will apply ^J -> ^A to all entries when saving, and the reverse when
loading. Without the patch, only the oldest entry gets transformed.
Failure to reverse the encoding in all lines is what creates a
user-visible problem. If we do not fix this, that's what we risk.
We do not escape a problem by refusing to fix it.
regards, tom lane
On Thu, Sep 04, 2014 at 09:54:37AM -0400, Robert Haas wrote: > One point to note is that not back-patching this doesn't really fix > anything. Will a user be annoyed when .psql_history fails to reload > properly on a new minor release, but utterly indifferent to whether it > reloads in a new major release? Users won't be utterly indifferent, but they will be less alarmed. We frequently use a major-version debut for bug fixes posing compatibility hazards. About half the items listed in the "Migration to Version 9.4" release notes section fit that description. > What if they run multiple major > releases of PostgreSQL on the same machine, using the psql executable > for each version when talking to that version? (Yeah, I know it's > backward compatible, but not everyone may realize that, or care.) Sure. Had I authored the patch, I probably would have withdrawn it pending development of a thorough plan for minimizing these problems. I don't care to sell that level of conservatism to the rest of you. If Tom is unconcerned about these problems and wants to move forward with the current patch for 9.5, that works for me. > Given that, if we're going to do it this way at all, I favor > back-patching: at least then the newest releases of all supported > branches will be compatible with each other. That's a fair point. A back-patch is better for hackers, who occasionally run each supported branch but rarely run outdated back-branch code. (When I built PostgreSQL on OS X, I used GNU readline. I suppose some hackers do use libedit, though.) Not sure about ordinary users, though. > But I'm still fuzzy on > why we need to give up the ability to read the old format in the first > place. Can't we just fix that and be done with this? Sort of. I see no free-lunch fix, but there are alternatives to the current proposed patch that resolve the compromises differently.
On Thu, Sep 04, 2014 at 07:51:03AM -0700, Tom Lane wrote: > Noah Misch <noah@leadboat.com> writes: > > I tried your patches against libedit-28. Wherever a command contains a > > newline, unpatched psql writes the three bytes "\^A" to the history file, and > > patched psql writes the four bytes "\012". Unpatched psql correctly reads > > either form of the history file. Patched psql misinterprets a history file > > created by unpatched psql, placing 0x01 bytes in the recalled command where it > > should have newlines. That's a worrisome compatibility break. > > I think you got the test cases backwards, or maybe neglected the aspect > about how unpatched psql will only translate ^J to ^A in the oldest > (or maybe the newest? too pressed for time to recheck right now) history > entry. I, too, had more-productive uses for this time, but morbid curiosity prevailed. It was the latter: I was testing a one-command history file. Under libedit-28, unpatched psql writes "^A" for newlines in the oldest command and "\012" for newlines in subsequent commands. Patched psql writes "\012" for newlines in the oldest command and "^A" for newlines in subsequent commands. (Surely a comment is in order if that's intentional. Wasn't the point to discontinue making the oldest command a special case?) Here's the matrix of behaviors when recalling history under libedit-28: master using master-written history: oldest command: ok rest: ok patched using master-written history: oldest command:broken if it contained a newline rest: ok master using patched-written history oldest command: ok rest: eachbroken if it contained a newline patched using patched-written history oldest command: ok rest: ok Corrupting just the oldest history entry, only when it happens to contain a newline, is acceptable. If one assumes that users who deploy multiple major releases use a consistent vintage of minor release, the compatibility problems after back-patching this change are negligible. That assumption has moderate credibility. > We do not escape a problem by refusing to fix it. I have not recommended a general refusal of fixes for this bug.
Noah Misch <noah@leadboat.com> writes: > On Thu, Sep 04, 2014 at 07:51:03AM -0700, Tom Lane wrote: >> I think you got the test cases backwards, or maybe neglected the aspect >> about how unpatched psql will only translate ^J to ^A in the oldest >> (or maybe the newest? too pressed for time to recheck right now) history >> entry. > I, too, had more-productive uses for this time, but morbid curiosity > prevailed. It was the latter: I was testing a one-command history file. > Under libedit-28, unpatched psql writes "^A" for newlines in the oldest > command and "\012" for newlines in subsequent commands. Patched psql writes > "\012" for newlines in the oldest command and "^A" for newlines in subsequent > commands. (Surely a comment is in order if that's intentional. Wasn't the > point to discontinue making the oldest command a special case?) Un-frickin-believable. Comparing the sources for history_get() at http://www.opensource.apple.com/source/libedit/libedit-28/src/readline.c vs http://www.opensource.apple.com/source/libedit/libedit-39/src/readline.c shows that -39 counts entries from history_base, as expected, but -28 counts them from zero (even though it exports history_base as one). So that's why the patched loop is misbehaving for you and not for me. 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. We could start the loop at zero unconditionally, but in a situation where libreadline had increased history_base much beyond one, that would be a bit wasteful. In any case, it now appears that we'd better test on more than just the oldest and newest libedits :-(. My confidence in the competence of libedit's authors has fallen another notch. regards, tom lane
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);
On Sat, Sep 06, 2014 at 11:40:02PM -0400, Tom Lane wrote: > 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. I ran libedit-history-fixes-v3.patch through my previous libedit-28 test. Now, patched psql writes ^A for newlines in any command. Here's the new matrix of behaviors when recalling history: master using master-written history: oldest command: ok rest: ok v3-patched using master-written history: oldest command:ok rest: ok master using v3-patched-written history oldest command: ok rest: each broken if it contained anewline v3-patched using v3-patched-written history oldest command: ok rest: ok That's probably the same result you saw. How does it compare to the compatibility effects for other libedit versions you tested? > [ wanders away wondering how it is that libedit has any following > whatsoever ... ] Quite so. Thanks, nm
Noah Misch <noah@leadboat.com> writes:
> I ran libedit-history-fixes-v3.patch through my previous libedit-28 test.
> Now, patched psql writes ^A for newlines in any command. Here's the new
> matrix of behaviors when recalling history:
> master using master-written history:
> oldest command: ok
> rest: ok
> v3-patched using master-written history:
> oldest command: ok
> rest: ok
> master using v3-patched-written history
> oldest command: ok
> rest: each broken if it contained a newline
> v3-patched using v3-patched-written history
> oldest command: ok
> rest: ok
> That's probably the same result you saw. How does it compare to the
> compatibility effects for other libedit versions you tested?
Yeah, this is the behavior I'm expecting; libedit-13 and up all seem to
work like this.
It seems that in very old libedit versions (5.1, Tiger era) both the
existing loop and the patched version are able to iterate over more than
just the oldest command, though not necessarily the entire history ---
I've not figured out exactly what happens, and am not sure it's worth
bothering. This means that in a ~/.psql_history made with such a version,
at least some commands besides the oldest might've had newlines converted
to ^A. Such history files are currently broken when forward-ported to any
later libedit version; with the proposed patch, though, we would read them
correctly. This gain makes me think the patch is worth the
backwards-compatibility loss you mention.
regards, tom lane