Thread: Getting psql to redisplay command after \e

The attached patch teaches psql to redisplay any not-yet-executed
query text after editing with \e.  The fact that you don't get to
see what you're about to execute has been complained of before,
most recently at bug #16034 [1].  In that thread I complained that
we needed some probably-not-very-portable readline functionality
to make this work.  However, after experimenting with trying to
shove text back into readline's buffer, I realized that there's
not really any need to do that: we just need to print the waiting
text and then collect another line.  (As a bonus, it works the
same even if you turned off readline with -n.)

It also seems like to make this not confusing, we need to regurgitate
a prompt before the query text.  As an example, if I do

regression=# \e

and then put this into the edited file:

select 1,
2,
3

what I see after exiting the editor is now

regression=# select 1,
2,
3
regression-#

Without the initial prompt it looks (to me anyway) like output
from the command, rather than something I've sort of automagically
typed.

In the cited bug, Pavlo argued that we should also print any
completed commands that get sent to the backend immediately
after \e.  It'd be possible to do that by extending this patch
(basically, dump about-to-be-executed commands if need_redisplay
is still true), but on the whole I think that that would be overly
chatty, so I didn't do it.

This could stand some review and testing (e.g. does it interact
badly with any other psql features), so I'll add it to the
upcoming CF.

            regards, tom lane

[1] https://www.postgresql.org/message-id/flat/16034-a7ebf0622970a1dd%40postgresql.org

diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 7789fc6..c8f9cb7 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1831,9 +1831,10 @@ testdb=>
         the normal rules of <application>psql</application>, treating the
         whole buffer as a single line.  Any complete queries are immediately
         executed; that is, if the query buffer contains or ends with a
-        semicolon, everything up to that point is executed.  Whatever remains
-        will wait in the query buffer; type semicolon or <literal>\g</literal> to
-        send it, or <literal>\r</literal> to cancel it by clearing the query buffer.
+        semicolon, everything up to that point is executed and removed from
+        the query buffer.  Whatever remains in the query buffer is
+        redisplayed.  Type semicolon or <literal>\g</literal> to send it,
+        or <literal>\r</literal> to cancel it by clearing the query buffer.
         Treating the buffer as a single line primarily affects meta-commands:
         whatever is in the buffer after a meta-command will be taken as
         argument(s) to the meta-command, even if it spans multiple lines.
@@ -1893,7 +1894,8 @@ Tue Oct 26 21:40:57 CEST 1999
          in the form of a <command>CREATE OR REPLACE FUNCTION</command> or
          <command>CREATE OR REPLACE PROCEDURE</command> command.
          Editing is done in the same way as for <literal>\edit</literal>.
-         After the editor exits, the updated command waits in the query buffer;
+         After the editor exits, the updated command is executed immediately
+         if you added a semicolon to it.  Otherwise it is redisplayed;
          type semicolon or <literal>\g</literal> to send it, or <literal>\r</literal>
          to cancel.
         </para>
@@ -1969,7 +1971,8 @@ Tue Oct 26 21:40:57 CEST 1999
          This command fetches and edits the definition of the named view,
          in the form of a <command>CREATE OR REPLACE VIEW</command> command.
          Editing is done in the same way as for <literal>\edit</literal>.
-         After the editor exits, the updated command waits in the query buffer;
+         After the editor exits, the updated command is executed immediately
+         if you added a semicolon to it.  Otherwise it is redisplayed;
          type semicolon or <literal>\g</literal> to send it, or <literal>\r</literal>
          to cancel.
         </para>
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index b981ae8..7f57da8 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -3508,7 +3508,8 @@ do_edit(const char *filename_arg, PQExpBuffer query_buf,
         {
             unsigned int ql = query_buf->len;

-            if (ql == 0 || query_buf->data[ql - 1] != '\n')
+            /* force newline-termination of what we send to editor */
+            if (ql > 0 && query_buf->data[ql - 1] != '\n')
             {
                 appendPQExpBufferChar(query_buf, '\n');
                 ql++;
diff --git a/src/bin/psql/mainloop.c b/src/bin/psql/mainloop.c
index b3a8407..f7b1b94 100644
--- a/src/bin/psql/mainloop.c
+++ b/src/bin/psql/mainloop.c
@@ -47,6 +47,7 @@ MainLoop(FILE *source)
     volatile int successResult = EXIT_SUCCESS;
     volatile backslashResult slashCmdStatus = PSQL_CMD_UNKNOWN;
     volatile promptStatus_t prompt_status = PROMPT_READY;
+    volatile bool need_redisplay = false;
     volatile int count_eof = 0;
     volatile bool die_on_error = false;
     FILE       *prev_cmd_source;
@@ -118,6 +119,7 @@ MainLoop(FILE *source)
             count_eof = 0;
             slashCmdStatus = PSQL_CMD_UNKNOWN;
             prompt_status = PROMPT_READY;
+            need_redisplay = false;
             pset.stmt_lineno = 1;
             cancel_pressed = false;

@@ -152,6 +154,18 @@ MainLoop(FILE *source)
             /* May need to reset prompt, eg after \r command */
             if (query_buf->len == 0)
                 prompt_status = PROMPT_READY;
+            /* If query buffer came from \e, redisplay it with a prompt */
+            if (need_redisplay)
+            {
+                if (query_buf->len > 0)
+                {
+                    fputs(get_prompt(PROMPT_READY, cond_stack), stdout);
+                    fputs(query_buf->data, stdout);
+                    fflush(stdout);
+                }
+                need_redisplay = false;
+            }
+            /* Now we can fetch a line */
             line = gets_interactive(get_prompt(prompt_status, cond_stack),
                                     query_buf);
         }
@@ -518,6 +532,10 @@ MainLoop(FILE *source)
                 {
                     /* should not see this in inactive branch */
                     Assert(conditional_active(cond_stack));
+                    /* ensure what came back from editing ends in a newline */
+                    if (query_buf->len > 0 &&
+                        query_buf->data[query_buf->len - 1] != '\n')
+                        appendPQExpBufferChar(query_buf, '\n');
                     /* rescan query_buf as new input */
                     psql_scan_finish(scan_state);
                     free(line);
@@ -529,6 +547,8 @@ MainLoop(FILE *source)
                                     pset.encoding, standard_strings());
                     line_saved_in_history = false;
                     prompt_status = PROMPT_READY;
+                    /* we'll want to redisplay after parsing what we have */
+                    need_redisplay = true;
                 }
                 else if (slashCmdStatus == PSQL_CMD_TERMINATE)
                     break;

Hello Tom,

> The attached patch teaches psql to redisplay any not-yet-executed
> query text after editing with \e. 
>
> [...]

I've tested this patch. Although I agree that it is an improvement, I'm a 
little at odd with the feature as is:

   psql=> \e
   # select 1...

then:

   psql=> select 1...
   psql-> <prompt>

I cannot move back with readline to edit further, I'm stuck there, which 
is strange. I would prefer a simpler:

   psql=> select 1...<prompt>

that would also be readline-aware, so that I know I'm there and ready to 
nl but also to edit directly if I want that.

That would suggest to remove the ending newline rather than appending it, 
and possibly to discuss a little bit with readline as well so that the 
display line is also the current line for its point of view, so that it 
can be edited further?

-- 
Fabien.

Fabien COELHO <coelho@cri.ensmp.fr> writes:
>> The attached patch teaches psql to redisplay any not-yet-executed
>> query text after editing with \e. 

> I've tested this patch. Although I agree that it is an improvement, I'm a 
> little at odd with the feature as is:

>    psql=> \e
>    # select 1...

> then:

>    psql=> select 1...
>    psql-> <prompt>

> I cannot move back with readline to edit further, I'm stuck there, which 
> is strange.

I don't follow.  readline doesn't allow you to edit already-entered lines
today, that is, after typing "select 1<return>" you see

regression=# select 1
regression-# 

and there isn't any way to move back and edit the already-entered line
within readline.  I agree it might be nicer if you could do that, but
that's *far* beyond the scope of this patch.  It would take entirely
fundamental rethinking of our use of libreadline, if indeed it's possible
at all.  I also don't see how we could have syntax-aware per-line prompts
if we were allowing readline to treat the whole query as one line.

In the larger picture, tinkering with how that works would affect
every psql user at the level of "muscle memory" editing habits,
and I suspect that their reactions would not be uniformly positive.
What I propose here doesn't affect anyone who doesn't use \e at all.
Even for \e users it doesn't have any effect on what you need to type.

            regards, tom lane

On Mon, 2019-10-28 at 23:00 -0400, Tom Lane wrote:
> The attached patch teaches psql to redisplay any not-yet-executed
> query text after editing with \e.  The fact that you don't get to
> see what you're about to execute has been complained of before,
> most recently at bug #16034 [1].  In that thread I complained that
> we needed some probably-not-very-portable readline functionality
> to make this work.  However, after experimenting with trying to
> shove text back into readline's buffer, I realized that there's
> not really any need to do that: we just need to print the waiting
> text and then collect another line.  (As a bonus, it works the
> same even if you turned off readline with -n.)

This is a nice improvement.

I tried to torture it with a hex editor, but couldn't get it to break.

There were some weird carriage returns in the patch, but after I
removed them, it applied fine.

Yours,
Laurenz Albe

Hello Tom,

>>    psql=> select 1...
>>    psql-> <prompt>
>
>> I cannot move back with readline to edit further, I'm stuck there, which
>> is strange.
>
> I don't follow.  readline doesn't allow you to edit already-entered lines
> today, that is, after typing "select 1<return>" you see
>
> regression=# select 1
> regression-#
>
> and there isn't any way to move back and edit the already-entered line
> within readline.

Yep.

My point is to possibly not implicitely <return> at the end of \e, but to 
behave as if we were moving in history, which allows editing the lines, so 
that you would get

   psql=> select 1<cursor>

Instead of the above.

> I agree it might be nicer if you could do that, but that's *far* beyond 
> the scope of this patch.  It would take entirely fundamental rethinking 
> of our use of libreadline, if indeed it's possible at all.  I also don't 
> see how we could have syntax-aware per-line prompts if we were allowing 
> readline to treat the whole query as one line.

I was suggesting something much simpler than rethinking readline handling. 
Does not mean that it is a good idea, but while testing the patch I would 
have liked the unfinished line to be in the current editing buffer, 
basically as if I had not typed <nl>.

ISTM more natural that \e behaves like history when coming back from 
editing, i.e. the \e-edited line is set as the current buffer for 
readline.

> In the larger picture, tinkering with how that works would affect
> every psql user at the level of "muscle memory" editing habits,
> and I suspect that their reactions would not be uniformly positive.
> What I propose here doesn't affect anyone who doesn't use \e at all.
> Even for \e users it doesn't have any effect on what you need to type.

-- 
Fabien.

Fabien COELHO <coelho@cri.ensmp.fr> writes:
> My point is to possibly not implicitely <return> at the end of \e, but to 
> behave as if we were moving in history, which allows editing the lines, so 
> that you would get
>    psql=> select 1<cursor>
> Instead of the above.

>> I agree it might be nicer if you could do that, but that's *far* beyond 
>> the scope of this patch.  It would take entirely fundamental rethinking 
>> of our use of libreadline, if indeed it's possible at all.  I also don't 
>> see how we could have syntax-aware per-line prompts if we were allowing 
>> readline to treat the whole query as one line.

> I was suggesting something much simpler than rethinking readline handling. 
> Does not mean that it is a good idea, but while testing the patch I would 
> have liked the unfinished line to be in the current editing buffer, 
> basically as if I had not typed <nl>.

I did experiment with trying to do that, but I couldn't get it to work,
even with the single version of libreadline I had at hand.  It appears
to me that readline() starts by clearing the internal buffer.  Even if
we could persuade it to work in a particular readline version, I think
the odds of making it portable across all the libreadline and libedit
versions that are out there aren't very good.  And there's definitely
no chance of being remotely compatible with that behavior when using the
bare tty drivers (psql -n).

In practice, if you decide that you don't like what you're looking at,
you're probably going to go back into the editor to fix it, ie issue
another \e.  So I'm not sure that it's worth such pushups to get the
data into readline's buffer.

            regards, tom lane

Hello Tom,

>> I was suggesting something much simpler than rethinking readline handling.
>> Does not mean that it is a good idea, but while testing the patch I would
>> have liked the unfinished line to be in the current editing buffer,
>> basically as if I had not typed <nl>.
>
> I did experiment with trying to do that, but I couldn't get it to work, 
> even with the single version of libreadline I had at hand.  It appears 
> to me that readline() starts by clearing the internal buffer.  Even if 
> we could persuade it to work in a particular readline version, I think 
> the odds of making it portable across all the libreadline and libedit 
> versions that are out there aren't very good.  And there's definitely no 
> chance of being remotely compatible with that behavior when using the 
> bare tty drivers (psql -n).

Argh, too bad.

This suggests that readline cannot be used to edit simply a known string? 
:-( "rl_insert_text" looked promising, although probably not portable, and 
I tried to make it work without much success anyway. Maybe I'll try to 
investigate more deeply later.

Note that replacing the current buffer is exactly what history does. So 
maybe that could be exploited by appending the edited line into history 
(add_history) and tell readline to move there (could not find how to do 
that automatically, though)? Or some other history handling…

> In practice, if you decide that you don't like what you're looking at,
> you're probably going to go back into the editor to fix it, ie issue
> another \e.  So I'm not sure that it's worth such pushups to get the
> data into readline's buffer.

For me \e should mean edit, not edit-and-execute, so I should be back to 
prompt, which is the crux of my unease with how the feature behaves, 
because it combines two functions that IMO shouldn't.

Anyway the submitted patch is an improvement to the current status.

-- 
Fabien.

Fabien COELHO <coelho@cri.ensmp.fr> writes:
>> I did experiment with trying to do that, but I couldn't get it to work, 
>> even with the single version of libreadline I had at hand.  It appears 
>> to me that readline() starts by clearing the internal buffer.  Even if 
>> we could persuade it to work in a particular readline version, I think 
>> the odds of making it portable across all the libreadline and libedit 
>> versions that are out there aren't very good.  And there's definitely no 
>> chance of being remotely compatible with that behavior when using the 
>> bare tty drivers (psql -n).

> This suggests that readline cannot be used to edit simply a known string? 
> :-( "rl_insert_text" looked promising, although probably not portable, and 
> I tried to make it work without much success anyway. Maybe I'll try to 
> investigate more deeply later.

I think that rl_insert_text and friends can probably only be used from
readline callback functions.  So in principle maybe you could make it
work by having an rl_startup_hook that injects text if there is any
to inject.  There would remain the issues of (a) is it portable across
a wide range of readline and libedit versions, (b) will the prompting
behavior be nice, and (c) do we really want this to work fundamentally
differently when readline is turned off?

(Pavel's code cited nearby seems to me to be a fine example of what
we do *not* want to do.  Getting in bed with libreadline to that
extent is inevitably going to misbehave in some places.)

>> In practice, if you decide that you don't like what you're looking at,
>> you're probably going to go back into the editor to fix it, ie issue
>> another \e.  So I'm not sure that it's worth such pushups to get the
>> data into readline's buffer.

> For me \e should mean edit, not edit-and-execute, so I should be back to 
> prompt, which is the crux of my unease with how the feature behaves, 
> because it combines two functions that IMO shouldn't.

I don't understand that complaint at all.  My proposed patch does not
change the behavior to force execution, and it does display a prompt ---
one that reflects whether you've given a complete command.

            regards, tom lane

I wrote:
> Fabien COELHO <coelho@cri.ensmp.fr> writes:
>> This suggests that readline cannot be used to edit simply a known string? 
>> :-( "rl_insert_text" looked promising, although probably not portable, and 
>> I tried to make it work without much success anyway. Maybe I'll try to 
>> investigate more deeply later.

> I think that rl_insert_text and friends can probably only be used from
> readline callback functions.  So in principle maybe you could make it
> work by having an rl_startup_hook that injects text if there is any
> to inject.  There would remain the issues of (a) is it portable across
> a wide range of readline and libedit versions, (b) will the prompting
> behavior be nice, and (c) do we really want this to work fundamentally
> differently when readline is turned off?

I thought maybe you were going to work on this right away, but since
you haven't, I went ahead and pushed what I had.  There's certainly
plenty of time to reconsider if you find a better answer.

            regards, tom lane

>> I think that rl_insert_text and friends can probably only be used from
>> readline callback functions.  So in principle maybe you could make it
>> work by having an rl_startup_hook that injects text if there is any
>> to inject.  There would remain the issues of (a) is it portable across
>> a wide range of readline and libedit versions, (b) will the prompting
>> behavior be nice, and (c) do we really want this to work fundamentally
>> differently when readline is turned off?
>
> I thought maybe you were going to work on this right away, but since
> you haven't, I went ahead and pushed what I had.  There's certainly
> plenty of time to reconsider if you find a better answer.

Indeeed.

I started to play with the startup hook, it kind of worked but not exactly 
as I wished, and I do not have much time available this round.

-- 
Fabien.