Re: Patch for psql History Display on MacOSX - Mailing list pgsql-hackers

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.