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
}
}--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
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:
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
--
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, 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
--
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, 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
--
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, 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
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, 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
Attachments:
psql_readline_fix.patchtext/x-patch; charset=US-ASCII; name=psql_readline_fix.patchDownload
diff --git a/configure b/configure
index 660aa57..2f0fee6 100755
--- a/configure
+++ b/configure
@@ -12415,7 +12415,7 @@ if test x"$pgac_cv_var_rl_completion_append_character" = x"yes"; then
$as_echo "#define HAVE_RL_COMPLETION_APPEND_CHARACTER 1" >>confdefs.h
fi
- for ac_func in rl_completion_matches rl_filename_completion_function
+ 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 419e3d3..f3d926d 100644
--- a/configure.in
+++ b/configure.in
@@ -1545,7 +1545,7 @@ LIBS="$LIBS_including_readline"
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([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/print.c b/src/bin/psql/print.c
index 655850b..0f17826 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 +2251,13 @@ ClosePager(FILE *pagerpipe)
#ifndef WIN32
pqsignal(SIGPIPE, SIG_DFL);
#endif
+#ifdef HAVE_RL_RESET_SCREEN_SIZE
+ /*
+ * Force libreadline to recheck the terminal size in case the pager
+ * may have handled any terminal resize events.
+ */
+ rl_reset_screen_size();
+#endif
}
}
diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in
index 6ce5907..8aa182c 100644
--- a/src/include/pg_config.h.in
+++ b/src/include/pg_config.h.in
@@ -406,6 +406,9 @@
/* 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
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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 ... \n
UNION ALL
SELECT 1;Time: 2.311 ms
So something isn't entirely right after a redraw...
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
Attachments:
readline-resize-fix.patchtext/x-diff; charset=us-ascii; name=readline-resize-fix.patchDownload
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers