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+13-2
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