Patch for psql History Display on MacOSX

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

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@sss.pgh.pa.us>:

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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
/messages/by-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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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@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
/messages/by-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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

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@leadboat.com>:

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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: 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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers