Re: Patch for psql History Display on MacOSX

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