Обсуждение: fix for readline terminal size problems when window is resized with open pager

Hello,

The following patch deals with a long standing gripe of mine that the
terminal frequently gets garbled so that when typing.  I guess this
problem is entirely dependent on pager settings and your interaction
patterns with the window (in particular, if you tend to resize the
window when the pager is open).  Experimenting with the problem, it
became pretty clear: libreadline for whatever reason does not get the
signal from the kernal telling it that the bounds have changed.  This
problem does not manifest 100% of the time, you may have to get the
pager to open, resize the window, close the pager, and recall a
previous long line (or type a new one) several times to get the
problem to occur.  Nevertheless, the attached seems to end the
problem.

This adds a dependency to print.c on input.h for the readline macro
and the readline header.

merlin

diff --git a/src/bin/psql/print.c b/src/bin/psql/print.c
index 655850b..ede736e 100644
--- a/src/bin/psql/print.c
+++ b/src/bin/psql/print.c
@@ -27,6 +27,7 @@#include "common.h"#include "mbprint.h"#include "print.h"
+#include "input.h"
/* * We define the cancel_pressed flag in this file, rather than common.c where
@@ -2247,6 +2248,13 @@ ClosePager(FILE *pagerpipe)#ifndef WIN32       pqsignal(SIGPIPE, SIG_DFL);#endif
+#ifdef USE_READLINE
+       /*
+        * Force libreadline to recheck the terminal size in case pager may
+        * have handled any terminal resize events.
+        */
+       rl_resize_terminal();
+#endif   }}

Merlin Moncure <mmoncure@gmail.com> writes:
> The following patch deals with a long standing gripe of mine that the
> terminal frequently gets garbled so that when typing.

Hm.  I wonder whether rl_resize_terminal() exists in every iteration
of libreadline and libedit.
        regards, tom lane

I wrote:
> Merlin Moncure <mmoncure@gmail.com> writes:
>> The following patch deals with a long standing gripe of mine that the
>> terminal frequently gets garbled so that when typing.

> Hm.  I wonder whether rl_resize_terminal() exists in every iteration
> of libreadline and libedit.

Quick followup: rl_resize_terminal() exists in GNU readline at least as
far back as 4.0 (released Feb 1999).  However, it doesn't seem to be there
at all in libedit; I don't see it in OS X Yosemite's headers, anyway.
So we'd need a configure test for this.
        regards, tom lane

On Tue, Dec 8, 2015 at 2:02 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
>> Merlin Moncure <mmoncure@gmail.com> writes:
>>> The following patch deals with a long standing gripe of mine that the
>>> terminal frequently gets garbled so that when typing.
>
>> Hm.  I wonder whether rl_resize_terminal() exists in every iteration
>> of libreadline and libedit.
>
> Quick followup: rl_resize_terminal() exists in GNU readline at least as
> far back as 4.0 (released Feb 1999).  However, it doesn't seem to be there
> at all in libedit; I don't see it in OS X Yosemite's headers, anyway.
> So we'd need a configure test for this.

All right, I guess this answers the question I was thinking, 'can this
be backpatched?' (basically, now).  I'll work up a configure test and
submit it to the appropriate commit fest.

merlin

On Tue, Dec 8, 2015 at 2:33 PM, Merlin Moncure <mmoncure@gmail.com> wrote:
> On Tue, Dec 8, 2015 at 2:02 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I wrote:
>>> Merlin Moncure <mmoncure@gmail.com> writes:
>>>> The following patch deals with a long standing gripe of mine that the
>>>> terminal frequently gets garbled so that when typing.
>>
>>> Hm.  I wonder whether rl_resize_terminal() exists in every iteration
>>> of libreadline and libedit.
>>
>> Quick followup: rl_resize_terminal() exists in GNU readline at least as
>> far back as 4.0 (released Feb 1999).  However, it doesn't seem to be there
>> at all in libedit; I don't see it in OS X Yosemite's headers, anyway.
>> So we'd need a configure test for this.
>
> All right, I guess this answers the question I was thinking, 'can this
> be backpatched?' (basically, now).

er, meant to say, 'no'.

> merlin

On Tue, Dec 8, 2015 at 2:02 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
>> Merlin Moncure <mmoncure@gmail.com> writes:
>>> The following patch deals with a long standing gripe of mine that the
>>> terminal frequently gets garbled so that when typing.
>
>> Hm.  I wonder whether rl_resize_terminal() exists in every iteration
>> of libreadline and libedit.
>
> Quick followup: rl_resize_terminal() exists in GNU readline at least as
> far back as 4.0 (released Feb 1999).  However, it doesn't seem to be there
> at all in libedit; I don't see it in OS X Yosemite's headers, anyway.
> So we'd need a configure test for this.

I thought that maybe raising the signal (SIGWINCH) manually might be a
more portable way of doing things but this appears not to work
reliably with readline.  Poking around the code and reading posts like
this (https://www.sourceware.org/ml/gdb-patches/2009-11/msg00597.html)
suggests maybe readline's signal handling is too smart for its own
good.  Oh well.

merlin

Merlin Moncure wrote:
> On Tue, Dec 8, 2015 at 2:33 PM, Merlin Moncure <mmoncure@gmail.com> wrote:
> > On Tue, Dec 8, 2015 at 2:02 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> I wrote:
> >>> Merlin Moncure <mmoncure@gmail.com> writes:
> >>>> The following patch deals with a long standing gripe of mine that the
> >>>> terminal frequently gets garbled so that when typing.
> >>
> >>> Hm.  I wonder whether rl_resize_terminal() exists in every iteration
> >>> of libreadline and libedit.
> >>
> >> Quick followup: rl_resize_terminal() exists in GNU readline at least as
> >> far back as 4.0 (released Feb 1999).  However, it doesn't seem to be there
> >> at all in libedit; I don't see it in OS X Yosemite's headers, anyway.
> >> So we'd need a configure test for this.
> >
> > All right, I guess this answers the question I was thinking, 'can this
> > be backpatched?' (basically, now).
> 
> er, meant to say, 'no'.

Why not?  We don't forbid adding configure tests in minor releases, do
we?

I've been troubled by this many times, so thanks for finding the problem
and fixing.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Tom Lane wrote:
> I wrote:
> > Merlin Moncure <mmoncure@gmail.com> writes:
> >> The following patch deals with a long standing gripe of mine that the
> >> terminal frequently gets garbled so that when typing.
> 
> > Hm.  I wonder whether rl_resize_terminal() exists in every iteration
> > of libreadline and libedit.
> 
> Quick followup: rl_resize_terminal() exists in GNU readline at least as
> far back as 4.0 (released Feb 1999).  However, it doesn't seem to be there
> at all in libedit; I don't see it in OS X Yosemite's headers, anyway.
> So we'd need a configure test for this.

In libedit (NetBSD's at least) there is el_resize() which seems to do
the same thing.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Tom Lane wrote:
>> Quick followup: rl_resize_terminal() exists in GNU readline at least as
>> far back as 4.0 (released Feb 1999).  However, it doesn't seem to be there
>> at all in libedit; I don't see it in OS X Yosemite's headers, anyway.
>> So we'd need a configure test for this.

> In libedit (NetBSD's at least) there is el_resize() which seems to do
> the same thing.

Hmm.  I see this in OS X's histedit.h:

void                el_resize(EditLine *);

but it appears that this is part of a completely separate API with
essentially nothing in common with GNU readline.  Not sure if we have
the motivation to try to support that API in parallel with readline's.
I sure don't.
        regards, tom lane

On Mon, Dec 14, 2015 at 2:50 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
>> Tom Lane wrote:
>>> Quick followup: rl_resize_terminal() exists in GNU readline at least as
>>> far back as 4.0 (released Feb 1999).  However, it doesn't seem to be there
>>> at all in libedit; I don't see it in OS X Yosemite's headers, anyway.
>>> So we'd need a configure test for this.
>
>> In libedit (NetBSD's at least) there is el_resize() which seems to do
>> the same thing.
>
> Hmm.  I see this in OS X's histedit.h:
>
> void                el_resize(EditLine *);
>
> but it appears that this is part of a completely separate API with
> essentially nothing in common with GNU readline.  Not sure if we have
> the motivation to try to support that API in parallel with readline's.
> I sure don't.

This may be moot; some testing demonstrated that libedit was not
impacted so it really comes down to having the right readline api call
available.  Looking at the code ISTM that libedit resets the terminal
on every prompt.

Also, after some experimentation I had better luck with
rl_reset_screen_size() (vs rl_resize_terminal()) that seemed to give
more regular behavior with the prompt.  So the consolidated patch with
the configure check is attached.

I'll leave it to the powers that be in terms of how and when to apply
this.  My gut is that it could probably be patched in now but I'd feel
comfortable with more testing then my own perfunctory probing.

merlin

On 12/14/2015 01:57 PM, Merlin Moncure wrote:
> This may be moot; some testing demonstrated that libedit was not
> impacted so it really comes down to having the right readline api call
> available.  Looking at the code ISTM that libedit resets the terminal
> on every prompt.

Did you manage to figure out why one was better than the other? The 
differences between the functions seem rather subtle.

rl_reset_screen_size()

Does not respect the COLUMNS and ROWS environment variables.

_rl_sigwinch_resize_terminal()

Internal callback, does the same thing as
rl_reset_screen_size() except that it also respects COLUMNS and ROWS.

rl_resize_terminal()

Respects COLUMNS and ROWS and also has some logic when echo mode is 
turned on which I have not managed to understand yet.

Btw, really nice to see someone working at this. It has bugged me a long 
time.

Andreas

Andreas Karlsson <andreas@proxel.se> writes:
> Did you manage to figure out why one was better than the other? The 
> differences between the functions seem rather subtle.

I'm a bit suspicious of Merlin's recommendation as well.  Looking at
the readline 6.3 sources, it is rl_resize_terminal() not the other
one that is called when an actual SIGWINCH is handled.

Another issue is that rl_reset_screen_size() doesn't exist in older copies
of readline (I don't see it in 4.0 nor 4.2a, which is what I've got laying
around in my archives).  So even if we agree that that's the one to use
when available, we'd need configure logic to fall back to
rl_resize_terminal() otherwise.

> rl_resize_terminal()
> Respects COLUMNS and ROWS and also has some logic when echo mode is 
> turned on which I have not managed to understand yet.

It looks to me like what it's doing is repainting the current line
on the theory that it might be messed up.  Since we are, at this
point, presumably *not* in the middle of accepting a command line,
that should be unnecessary but also harmless.  If there is any
visible difference in behavior, I should think it would be
rl_resize_terminal() that produces more consistent results, because
it does have the repaint logic which the other lacks.
        regards, tom lane

Hi,

First off: Glad that you investigated, this has been bugging me.

On 2015-12-14 15:57:22 -0600, Merlin Moncure wrote:
> Also, after some experimentation I had better luck with
> rl_reset_screen_size() (vs rl_resize_terminal()) that seemed to give
> more regular behavior with the prompt.  So the consolidated patch with
> the configure check is attached.

Hm. rl_reset_screen_size() works well for me. rl_resize_terminal() not
so much.  Apparently the reason for that is that rl_reset_screen_size()
doesn't set ignore_env to to true when calling
_rl_get_screen_size(). I've verified that just toggling that parameter
changes the behaviour.

What bugs me about that a bit is that _rl_sigwinch_resize_terminal()
sets ignore_env to true.

Greetings,

Andres Freund

On 2015-12-16 11:21:53 -0500, Tom Lane wrote:
> It looks to me like what it's doing is repainting the current line
> on the theory that it might be messed up.  Since we are, at this
> point, presumably *not* in the middle of accepting a command line,
> that should be unnecessary but also harmless.  If there is any
> visible difference in behavior, I should think it would be
> rl_resize_terminal() that produces more consistent results, because
> it does have the repaint logic which the other lacks.

The repainting actually causes trouble here, if I have \timing enabled.
Even without an actual resize even it leads to the Time: line to be
displayed in the same line as the query, after the pager was
active. Looks like
postgres[8444][1]=# SELECT * .. \n ... \nUNION ALLSELECT 1;Time: 2.311 ms

So something isn't entirely right after a redraw...

Andres Freund <andres@anarazel.de> writes:
> Hm. rl_reset_screen_size() works well for me. rl_resize_terminal() not
> so much.  Apparently the reason for that is that rl_reset_screen_size()
> doesn't set ignore_env to to true when calling
> _rl_get_screen_size(). I've verified that just toggling that parameter
> changes the behaviour.

[ squint... ]  What readline version are you using, and do you have
LINES/COLUMNS set in your terminal environment?

As far as I can see in the 6.3 sources, the very first call of
_rl_get_screen_size will cause LINES/COLUMNS to become set from the
results of ioctl(TIOCGWINSZ), assuming that that succeeds, because we
do nothing to alter the default values of rl_prefer_env_winsize (0)
or rl_change_environment (1).  After that, it really doesn't matter
whether ignore_env is true or not; this code will track the ioctl
as long as rl_prefer_env_winsize stays 0.

It may be that the echo stuff is not good, but I'm pretty dubious
about ignore_env being the issue.
        regards, tom lane

On 2015-12-16 12:02:26 -0500, Tom Lane wrote:
> [ squint... ]  What readline version are you using, and do you have
> LINES/COLUMNS set in your terminal environment?

libreadline-dev: Installed: 6.3-8+b4

Both are set - I think bash does that.

> It may be that the echo stuff is not good, but I'm pretty dubious
> about ignore_env being the issue.

Indeed it's not, I'm not entirely sure whether I managed to repeatedly
run with the wrong version of psql (doubtful, same terminal), or what
strange state I observed.

But    _rl_get_screen_size (fileno (rl_instream), 0);    if (RL_ISSTATE(RL_STATE_REDISPLAYING) == 0)
_rl_redisplay_after_sigwinch();
 
in ClosePager() shows the problem independent of ignore_env or not.

I think the difference here is actually that redrawing shows the query
after the pager, without it we don't show it again.

If you configure less to automatically quit if the query is below one
screen (-F) and use \pset pager always the difference is clearly
visible:

resize (just _rl_get_screen_size()):

postgres[12561][1]=# SELECT 1;
┌──────────┐
│ ?column? │
├──────────┤
│        1 │
└──────────┘
(1 row)

Time: 0.797 ms
postgres[12561][1]=# 

resize & redraw (_rl_get_screen_size() & _rl_redisplay_after_sigwinch):

postgres[12393][1]=# SELECT 1;
┌──────────┐
│ ?column? │
├──────────┤
│        1 │
└──────────┘
(1 row)

postgres[12393][1]=# SELECT 1;Time: 0.555 ms

based on that I'm inclined to just go with resize - redisplaying the
query after the pager might be desirable, but I think it's an actual
behavioural change.

Andres Freund <andres@anarazel.de> writes:
> ... based on that I'm inclined to just go with resize - redisplaying the
> query after the pager might be desirable, but I think it's an actual
> behavioural change.

Hmm ... given that we've not printed "Time:" yet (in \timing mode),
I think you're right that we don't want anything printed at this point.
It may be that we can't fix this in readline versions that precede the
introduction of the resize function.  Let me go experiment on my pet
dinosaurs.
        regards, tom lane

On 2015-12-16 12:23:28 -0500, Tom Lane wrote:
> It may be that we can't fix this in readline versions that precede the
> introduction of the resize function.  Let me go experiment on my pet
> dinosaurs.

I'm not particularly bothered by not supporting old readline versions
here. If we really want to we could basically directly use
_rl_get_screen_size() - which seems to have been present from before
4.0. It's not declared static...

extern void _rl_get_screen_size (int tty, int ignore_env);
and then   _rl_get_screen_size (fileno (rl_instream), 0);
in ClosePager() seems to do the trick.

Andres

Andres Freund <andres@anarazel.de> writes:
> On 2015-12-16 12:23:28 -0500, Tom Lane wrote:
>> It may be that we can't fix this in readline versions that precede the
>> introduction of the resize function.  Let me go experiment on my pet
>> dinosaurs.

> I'm not particularly bothered by not supporting old readline versions
> here.

I'm not either, just wanted to know if it was possible.  (Answer: no.
rl_resize_terminal definitely does not do what we want, neither in
current releases nor in 4.2.  It looks to me like it's only intended
to be called while interactive input is active, which is the only
time that libreadline has its signal handler in place.)

> If we really want to we could basically directly use
> _rl_get_screen_size() - which seems to have been present from before
> 4.0. It's not declared static...

Nah, I don't think we should rely on calling undocumented internal
readline functions.  We've lived with this behavior for long enough
that I don't think it's a catastrophe if we don't fix it for ancient
readline releases.

One issue I do have with the patch as proposed is that it will call
rl_reset_screen_size even with -n, which does not seem like a good
idea.  There's no guarantee that rl_reset_screen_size will behave
nicely if libreadline hasn't been initialized properly, and even
if it does, it will have side-effects on the LINES/COLUMNS environment
variables which are potentially user-visible and should not happen
with -n.  I think the most reasonable way to handle this is to put the
call into a new function exported from input.c, where it can be
made conditional on useReadline.

Except for that minor rearrangement, I think this is a good patch
and we should accept it.  Does anyone object to back-patching it?
        regards, tom lane

On 2015-12-16 13:02:25 -0500, Tom Lane wrote:
> > If we really want to we could basically directly use
> > _rl_get_screen_size() - which seems to have been present from before
> > 4.0. It's not declared static...
> 
> Nah, I don't think we should rely on calling undocumented internal
> readline functions.

Agreed.


> We've lived with this behavior for long enough
> that I don't think it's a catastrophe if we don't fix it for ancient
> readline releases.

Yes.


> I think the most reasonable way to handle this is to put the
> call into a new function exported from input.c, where it can be
> made conditional on useReadline.

Sounds good.


> Does anyone object to back-patching it?

Not here.

On Wed, Dec 16, 2015 at 12:06 PM, Andres Freund <andres@anarazel.de> wrote:
> On 2015-12-16 13:02:25 -0500, Tom Lane wrote:
>> I think the most reasonable way to handle this is to put the
>> call into a new function exported from input.c, where it can be
>> made conditional on useReadline.
>
> Sounds good.

I'll right, I'll follow up with that shortly.

For posterity, I think you guys arrived at the right conclusion, but
the difference between the two public screen reset calls is that one
(the original patch) forces a re-display of the prompt and the newer
one, rl_reset_screen_size(), does not.  With the older patch the best
I could come up with was to call rl_crlf() so that the prompt display
did not amend to the end of \timing's output.  However, that still
modified the output relative to what it was pre-patch, which would be
impolite to our users in a backpatch scenario.

merlin

I did some more experimentation and concluded that actually, this problem
has nothing whatsoever to do with pager invocations.  What seems to really
be happening is that libreadline activates its SIGWINCH handler only while
it's being called to collect input, which is fine in itself, but *it does
not notice screen resizes that happen when the handler is not active*.

You can prove this by doing, say, "select pg_sleep(10);" and resizing
the window before the backend comes back.  Readline never notices the
resize, even though no pager invocation happens at all.

So I now think that print.c shouldn't be involved at all, and the right
thing to do is just have gets_interactive() invoke the resize function
immediately before calling readline().  That will still leave a window
for SIGWINCH to be missed, but it's a pretty tiny one.

And somebody oughta report this as a libreadline bug ...
        regards, tom lane

On Wed, Dec 16, 2015 at 1:29 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I did some more experimentation and concluded that actually, this problem
> has nothing whatsoever to do with pager invocations.  What seems to really
> be happening is that libreadline activates its SIGWINCH handler only while
> it's being called to collect input, which is fine in itself, but *it does
> not notice screen resizes that happen when the handler is not active*.
>
> You can prove this by doing, say, "select pg_sleep(10);" and resizing
> the window before the backend comes back.  Readline never notices the
> resize, even though no pager invocation happens at all.
>
> So I now think that print.c shouldn't be involved at all, and the right
> thing to do is just have gets_interactive() invoke the resize function
> immediately before calling readline().  That will still leave a window
> for SIGWINCH to be missed, but it's a pretty tiny one.

right -- agreed.

> And somebody oughta report this as a libreadline bug ...

See https://bugs.python.org/issue23735 for background.  Apparently
this is expected behavior (and we are far from the only ones
complaining about it):

"And so we reach where we are. If a SIGWINCH arrives while readline is
not active, and the application using callback mode does not catch it and
tell readline about the updated screen dimensions (rl_set_screen_size()
exists for this purpose), readline will not know about the new size."

merlin

Tom Lane wrote:
> I did some more experimentation and concluded that actually, this problem
> has nothing whatsoever to do with pager invocations.  What seems to really
> be happening is that libreadline activates its SIGWINCH handler only while
> it's being called to collect input, which is fine in itself, but *it does
> not notice screen resizes that happen when the handler is not active*.

I wonder if we're doing the proper things.  According to their manual,

http://git.savannah.gnu.org/gitweb/?p=readline.git;a=blob_plain;f=doc/readline.html;h=9b7dd842764c81ad496c38a2794361cad964ee90;hb=7628b745a813aac53586b640da056a975f1c443e#SEC44
I think we should be setting rl_catch_signal and/or rl_catch_sigwinch
and then perhaps call rl_set_signals(), but I don't think we're doing
anything of the sort.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera wrote:

> I wonder if we're doing the proper things.  According to their manual,
>
http://git.savannah.gnu.org/gitweb/?p=readline.git;a=blob_plain;f=doc/readline.html;h=9b7dd842764c81ad496c38a2794361cad964ee90;hb=7628b745a813aac53586b640da056a975f1c443e#SEC44

Note: the above link documents readline 6.5 according to git tags.  This
one is for readline 4.2:

http://git.savannah.gnu.org/gitweb/?p=readline.git;a=blob_plain;f=doc/readline.html;hb=abde3125f6228a63e22de708b9edaef62cab0ac3#SEC43

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
>> I did some more experimentation and concluded that actually, this problem
>> has nothing whatsoever to do with pager invocations.  What seems to really
>> be happening is that libreadline activates its SIGWINCH handler only while
>> it's being called to collect input, which is fine in itself, but *it does
>> not notice screen resizes that happen when the handler is not active*.

> I wonder if we're doing the proper things.  According to their manual,
>
http://git.savannah.gnu.org/gitweb/?p=readline.git;a=blob_plain;f=doc/readline.html;h=9b7dd842764c81ad496c38a2794361cad964ee90;hb=7628b745a813aac53586b640da056a975f1c443e#SEC44
> I think we should be setting rl_catch_signal and/or rl_catch_sigwinch
> and then perhaps call rl_set_signals(), but I don't think we're doing
> anything of the sort.

According to the info page I have here, those are all enabled by default,
and we would only need to do something special if we wanted to prevent
readline from catching signals.

It's possible that 6.3 has made fundamental, incompatible changes in
this area and not bothered to update the documentation, but it would be
pretty shoddy work on their part.
        regards, tom lane

Merlin Moncure <mmoncure@gmail.com> writes:
> On Wed, Dec 16, 2015 at 1:29 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> So I now think that print.c shouldn't be involved at all, and the right
>> thing to do is just have gets_interactive() invoke the resize function
>> immediately before calling readline().  That will still leave a window
>> for SIGWINCH to be missed, but it's a pretty tiny one.

> right -- agreed.

I tried that and found out that the first call dumps core because readline
hasn't been initialized yet.  To fix that, we need an explicit call to
rl_initialize() instead of depending on readline() to do it for us.
So I arrive at the attached modified patch.

>> And somebody oughta report this as a libreadline bug ...

> See https://bugs.python.org/issue23735 for background.  Apparently
> this is expected behavior (and we are far from the only ones
> complaining about it):

> "And so we reach where we are. If a SIGWINCH arrives while readline is
> not active, and the application using callback mode does not catch it and
> tell readline about the updated screen dimensions (rl_set_screen_size()
> exists for this purpose), readline will not know about the new size."

Meh.  At the very least, this is a serious documentation failure on their
part, because their docs about interrupt handling look exactly the same
as before, to wit saying exactly the opposite of this.

            regards, tom lane

diff --git a/configure b/configure
index 8c61390..5772d0e 100755
*** a/configure
--- b/configure
*************** if test x"$pgac_cv_var_rl_completion_app
*** 13232,13238 ****
  $as_echo "#define HAVE_RL_COMPLETION_APPEND_CHARACTER 1" >>confdefs.h

  fi
!   for ac_func in rl_completion_matches rl_filename_completion_function
  do :
    as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
  ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var"
--- 13232,13238 ----
  $as_echo "#define HAVE_RL_COMPLETION_APPEND_CHARACTER 1" >>confdefs.h

  fi
!   for ac_func in rl_completion_matches rl_filename_completion_function rl_reset_screen_size
  do :
    as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
  ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var"
diff --git a/configure.in b/configure.in
index b5868b0..44f832f 100644
*** a/configure.in
--- b/configure.in
*************** LIBS="$LIBS_including_readline"
*** 1635,1641 ****

  if test "$with_readline" = yes; then
    PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER
!   AC_CHECK_FUNCS([rl_completion_matches rl_filename_completion_function])
    AC_CHECK_FUNCS([append_history history_truncate_file])
  fi

--- 1635,1641 ----

  if test "$with_readline" = yes; then
    PGAC_VAR_RL_COMPLETION_APPEND_CHARACTER
!   AC_CHECK_FUNCS([rl_completion_matches rl_filename_completion_function rl_reset_screen_size])
    AC_CHECK_FUNCS([append_history history_truncate_file])
  fi

diff --git a/src/bin/psql/input.c b/src/bin/psql/input.c
index e377104..c0c5524 100644
*** a/src/bin/psql/input.c
--- b/src/bin/psql/input.c
*************** gets_interactive(const char *prompt)
*** 65,70 ****
--- 65,81 ----
      {
          char       *result;

+         /*
+          * Some versions of readline don't notice SIGWINCH signals that arrive
+          * when not actively reading input.  The simplest fix is to always
+          * re-read the terminal size.  This leaves a window for SIGWINCH to be
+          * missed between here and where readline() enables libreadline's
+          * signal handler, but that's probably short enough to be ignored.
+          */
+ #ifdef HAVE_RL_RESET_SCREEN_SIZE
+         rl_reset_screen_size();
+ #endif
+
          /* Enable SIGINT to longjmp to sigint_interrupt_jmp */
          sigint_interrupt_enabled = true;

*************** initializeInput(int flags)
*** 330,335 ****
--- 341,347 ----
          char        home[MAXPGPATH];

          useReadline = true;
+         rl_initialize();
          initialize_readline();

          useHistory = true;
diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in
index a20e0cd..16a272e 100644
*** a/src/include/pg_config.h.in
--- b/src/include/pg_config.h.in
***************
*** 428,433 ****
--- 428,436 ----
  /* Define to 1 if you have the `rl_filename_completion_function' function. */
  #undef HAVE_RL_FILENAME_COMPLETION_FUNCTION

+ /* Define to 1 if you have the `rl_reset_screen_size' function. */
+ #undef HAVE_RL_RESET_SCREEN_SIZE
+
  /* Define to 1 if you have the <security/pam_appl.h> header file. */
  #undef HAVE_SECURITY_PAM_APPL_H

I wrote:
> Merlin Moncure <mmoncure@gmail.com> writes:
>> See https://bugs.python.org/issue23735 for background.  Apparently
>> this is expected behavior (and we are far from the only ones
>> complaining about it):

>> "And so we reach where we are. If a SIGWINCH arrives while readline is
>> not active, and the application using callback mode does not catch it and
>> tell readline about the updated screen dimensions (rl_set_screen_size()
>> exists for this purpose), readline will not know about the new size."

> Meh.  At the very least, this is a serious documentation failure on their
> part, because their docs about interrupt handling look exactly the same
> as before, to wit saying exactly the opposite of this.

Hm ... actually, the claim that this is somehow new behavior in
libreadline 6.3 or thereabouts is purest bilge, because 4.2a (released
Nov 2001) acts exactly the same.
        regards, tom lane

On Wed, Dec 16, 2015 at 2:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Merlin Moncure <mmoncure@gmail.com> writes:
>> On Wed, Dec 16, 2015 at 1:29 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> So I now think that print.c shouldn't be involved at all, and the right
>>> thing to do is just have gets_interactive() invoke the resize function
>>> immediately before calling readline().  That will still leave a window
>>> for SIGWINCH to be missed, but it's a pretty tiny one.
>
>> right -- agreed.
>
> I tried that and found out that the first call dumps core because readline
> hasn't been initialized yet.  To fix that, we need an explicit call to
> rl_initialize() instead of depending on readline() to do it for us.
> So I arrive at the attached modified patch.

This is working great.  Is there anything left for me to do here?

merlin

Merlin Moncure <mmoncure@gmail.com> writes:
> This is working great.  Is there anything left for me to do here?

Nope, it's committed.
        regards, tom lane