Re: 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. Macs uses 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 iteration scheme to an index based loop (see code and comments), which seemed to be the only working option for both readline and libedit. In any case, i have tested and compiled this on MacOX 10.9.3 and Linux. Windows doesn’t have the pager in 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 your history 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 this go into …./regress/sql/psql.sql at all? I am not sure on that one.

Regards, Stepan

Stepan Rutz <stepan(dot)rutz(at)gmx(dot)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 production server 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(at)sss(dot)pgh(dot)pa(dot)us>:

> Stepan Rutz <stepan(dot)rutz(at)gmx(dot)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(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

Stepan Rutz <stepan(dot)rutz(at)gmx(dot)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

I wrote:
> Stepan Rutz <stepan(dot)rutz(at)gmx(dot)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(dot)rutz(at)gmx(dot)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(at)leadboat(dot)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

On Mon, Sep 01, 2014 at 10:22:57PM -0400, Tom Lane wrote:
> Noah Misch <noah(at)leadboat(dot)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(at)leadboat(dot)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(at)leadboat(dot)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(at)leadboat(dot)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(at)leadboat(dot)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 all on mac-osx. so nothing to lose for the mac psql users.

regards,
stepan

Am 03.09.2014 um 07:45 schrieb Noah Misch <noah(at)leadboat(dot)com>:

> On Tue, Sep 02, 2014 at 09:49:56AM -0400, Tom Lane wrote:
>> Noah Misch <noah(at)leadboat(dot)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(at)postgresql(dot)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(at)leadboat(dot)com> wrote:
> On Tue, Sep 02, 2014 at 01:56:34AM -0400, Tom Lane wrote:
>> Noah Misch <noah(at)leadboat(dot)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(at)leadboat(dot)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(at)leadboat(dot)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: each broken 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(at)leadboat(dot)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

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 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?

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

Quite so.

Thanks,
nm

Noah Misch <noah(at)leadboat(dot)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