Thread: Re [HACKERS]: Still not happy with psql's multiline history behavior
Hello,
I'm sending the patch fixing the \r bug:
On Wed, 31 May 2006, Tom Lane wrote:
> The example that seriously sucks is:
>
> regression=# select foo ...
> regression-# \r
> Query buffer reset (cleared).
> now control-P brings back:
> regression=# select foo ...
> \r
In my fix the currently-written query is completely erased if \r is
invoked. I think that's the most logic behavior.
So, the example of work:
wsdb=# select 1;
?column?
----------
1
(1 row)
wsdb=# select
wsdb-# \r
Query buffer reset (cleared).
wsdb=#
Control-P now brings back:
wsdb=# \r
Another Control-P brings back:
wsdb=# select 1;
> Also, \e is seriously broken: after you edit the text and exit the
> editor, the text is not reloaded into the visible display (although it
> does seem to still be "behind the scenes" somewhere).
For that part, I currently do not see the possible fix, and I don't even
think that it should be fixed, because there with current multi-line
implementation there is no need for that.
For example, when we do
wsdb=# select 1+
wsdb-#
And press Control-P , the string "select 1+" is not in the history, and
there is no need for that.
And in the case of \e editing, if in the external editor we have typed
the incomplete query like "select 1+", the situation will be exactly the
same and for the same reason...
So, I don't think, that this require fixing. In principal, probably it is
possible to fix that in a different way:
Suppose we executed the \e command and in the external editor we typed
"select 1 +". And after the exit from the editor, psql produce
wsdb=# select 1 +
(and put cursor in the end of the line).
That can be reasonable (I'm not sure that it's possible to do that with
readline, but should be).
But I would consider that as a new feature (probably not very useful), not
as a bug fix.
Regards,
Sergey
*******************************************************************
Sergey E. Koposov
Max Planck Institute for Astronomy/Sternberg Astronomical Institute
Tel: +49-6221-528-349 Web: http://lnfm1.sai.msu.ru/~math
E-mail: math@sai.msu.ru
Attachment
Also, in this line from psql/mainloop.c:
312 if (scan_result == PSCAN_BACKSLASH && history_buf->len != 0)
Is the "history_buf->len != 0" test necessary?
---------------------------------------------------------------------------
pgman wrote:
> Sergey E. Koposov wrote:
> > On Sun, 4 Jun 2006, Bruce Momjian wrote:
> >
> > > Sergey E. Koposov wrote:
> > >> Hello,
> > >>
> > >> I'm sending the patch fixing the \r bug:
> > >
> > > Funny, I just applied a simpler fix for the \r bug. Please review it.
> > > Thanks.
> > >
> >
> > It is simpler, because it behaves differently.
> > Your patch for the case:
> > wsdb=# select
> > wsdb-# \r
> > Query buffer reset (cleared).
> >
> > puts in the history two elements ("\r" and "select")
> >
> > Mine puts only "\r", just because
> > <quote> \r reset (clear) the query buffer</quote>
> > and I think that's reasonable, but I don't insist...
>
> I think I like the fact there two separate entries, so we are OK.
>
> > > I am confused by the \e bug. I just tried 8.1.X and it seems to behave
> > > the same as CVS HEAD. What exactly should it do?
> > >
> >
> > It is not actually a bug, I think it's a Tom's habit.
> > In 8.1.x if you type for example in the editor "select 1+" and exit from
> > it, the "select 1+" string will be already in the history. But in CVS HEAD
> > it will be saved internally, but it won't be shown in the history until the
> > user finishes the command with ';'. And there is really no need for
> > correcting that, I think.
>
> Thanks, that was clear. The attached patch fixes it. It is a little
> larger than usual because I changed the name of the boolean and reversed
> its usage. It now works like it did in 8.1.X.
>
> --
> Bruce Momjian http://candle.pha.pa.us
> EnterpriseDB http://www.enterprisedb.com
>
> + If your life is a hard drive, Christ can be your backup. +
[ text/x-diff is unsupported, treating like TEXT/PLAIN ]
> Index: src/bin/psql/mainloop.c
> ===================================================================
> RCS file: /cvsroot/pgsql/src/bin/psql/mainloop.c,v
> retrieving revision 1.75
> diff -c -c -r1.75 mainloop.c
> *** src/bin/psql/mainloop.c 4 Jun 2006 04:35:55 -0000 1.75
> --- src/bin/psql/mainloop.c 5 Jun 2006 03:50:11 -0000
> ***************
> *** 41,47 ****
> char *line; /* current line of input */
> int added_nl_pos;
> bool success;
> ! bool first_query_scan;
>
> volatile int successResult = EXIT_SUCCESS;
> volatile backslashResult slashCmdStatus = PSQL_CMD_UNKNOWN;
> --- 41,47 ----
> char *line; /* current line of input */
> int added_nl_pos;
> bool success;
> ! bool line_saved_in_history;
>
> volatile int successResult = EXIT_SUCCESS;
> volatile backslashResult slashCmdStatus = PSQL_CMD_UNKNOWN;
> ***************
> *** 80,85 ****
> --- 80,87 ----
> /* main loop to get queries and execute them */
> while (successResult == EXIT_SUCCESS)
> {
> + line_saved_in_history = false;
> +
> /*
> * Welcome code for Control-C
> */
> ***************
> *** 154,159 ****
> --- 156,163 ----
> */
> pg_write_history(history_buf->data);
> pg_clear_history(history_buf);
> + pg_write_history(line);
> + line_saved_in_history = true;
> }
> }
> /* otherwise, get another line */
> ***************
> *** 226,232 ****
> */
> psql_scan_setup(scan_state, line, strlen(line));
> success = true;
> - first_query_scan = true;
>
> while (success || !die_on_error)
> {
> --- 230,235 ----
> ***************
> *** 303,319 ****
> * down here so we can check for \g and other 'execute'
> * backslash commands, which should be appended.
> */
> ! if (first_query_scan && pset.cur_cmd_interactive)
> {
> /* Sending a command (PSQL_CMD_SEND) zeros the length */
> if (scan_result == PSCAN_BACKSLASH && history_buf->len != 0)
> pg_write_history(line);
> else
> pg_append_history(line, history_buf);
> }
>
> - first_query_scan = false;
> -
> /* fall out of loop on \q or if lexer reached EOL */
> if (slashCmdStatus == PSQL_CMD_TERMINATE ||
> scan_result == PSCAN_INCOMPLETE ||
> --- 306,321 ----
> * down here so we can check for \g and other 'execute'
> * backslash commands, which should be appended.
> */
> ! if (!line_saved_in_history && pset.cur_cmd_interactive)
> {
> /* Sending a command (PSQL_CMD_SEND) zeros the length */
> if (scan_result == PSCAN_BACKSLASH && history_buf->len != 0)
> pg_write_history(line);
> else
> pg_append_history(line, history_buf);
> + line_saved_in_history = true;
> }
>
> /* fall out of loop on \q or if lexer reached EOL */
> if (slashCmdStatus == PSQL_CMD_TERMINATE ||
> scan_result == PSCAN_INCOMPLETE ||
--
Bruce Momjian http://candle.pha.pa.us
EnterpriseDB http://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Conditional history length test removed.
---------------------------------------------------------------------------
Bruce Momjian wrote:
>
> Also, in this line from psql/mainloop.c:
>
> 312 if (scan_result == PSCAN_BACKSLASH && history_buf->len != 0)
>
> Is the "history_buf->len != 0" test necessary?
>
> ---------------------------------------------------------------------------
>
> pgman wrote:
> > Sergey E. Koposov wrote:
> > > On Sun, 4 Jun 2006, Bruce Momjian wrote:
> > >
> > > > Sergey E. Koposov wrote:
> > > >> Hello,
> > > >>
> > > >> I'm sending the patch fixing the \r bug:
> > > >
> > > > Funny, I just applied a simpler fix for the \r bug. Please review it.
> > > > Thanks.
> > > >
> > >
> > > It is simpler, because it behaves differently.
> > > Your patch for the case:
> > > wsdb=# select
> > > wsdb-# \r
> > > Query buffer reset (cleared).
> > >
> > > puts in the history two elements ("\r" and "select")
> > >
> > > Mine puts only "\r", just because
> > > <quote> \r reset (clear) the query buffer</quote>
> > > and I think that's reasonable, but I don't insist...
> >
> > I think I like the fact there two separate entries, so we are OK.
> >
> > > > I am confused by the \e bug. I just tried 8.1.X and it seems to behave
> > > > the same as CVS HEAD. What exactly should it do?
> > > >
> > >
> > > It is not actually a bug, I think it's a Tom's habit.
> > > In 8.1.x if you type for example in the editor "select 1+" and exit from
> > > it, the "select 1+" string will be already in the history. But in CVS HEAD
> > > it will be saved internally, but it won't be shown in the history until the
> > > user finishes the command with ';'. And there is really no need for
> > > correcting that, I think.
> >
> > Thanks, that was clear. The attached patch fixes it. It is a little
> > larger than usual because I changed the name of the boolean and reversed
> > its usage. It now works like it did in 8.1.X.
> >
> > --
> > Bruce Momjian http://candle.pha.pa.us
> > EnterpriseDB http://www.enterprisedb.com
> >
> > + If your life is a hard drive, Christ can be your backup. +
>
> [ text/x-diff is unsupported, treating like TEXT/PLAIN ]
>
> > Index: src/bin/psql/mainloop.c
> > ===================================================================
> > RCS file: /cvsroot/pgsql/src/bin/psql/mainloop.c,v
> > retrieving revision 1.75
> > diff -c -c -r1.75 mainloop.c
> > *** src/bin/psql/mainloop.c 4 Jun 2006 04:35:55 -0000 1.75
> > --- src/bin/psql/mainloop.c 5 Jun 2006 03:50:11 -0000
> > ***************
> > *** 41,47 ****
> > char *line; /* current line of input */
> > int added_nl_pos;
> > bool success;
> > ! bool first_query_scan;
> >
> > volatile int successResult = EXIT_SUCCESS;
> > volatile backslashResult slashCmdStatus = PSQL_CMD_UNKNOWN;
> > --- 41,47 ----
> > char *line; /* current line of input */
> > int added_nl_pos;
> > bool success;
> > ! bool line_saved_in_history;
> >
> > volatile int successResult = EXIT_SUCCESS;
> > volatile backslashResult slashCmdStatus = PSQL_CMD_UNKNOWN;
> > ***************
> > *** 80,85 ****
> > --- 80,87 ----
> > /* main loop to get queries and execute them */
> > while (successResult == EXIT_SUCCESS)
> > {
> > + line_saved_in_history = false;
> > +
> > /*
> > * Welcome code for Control-C
> > */
> > ***************
> > *** 154,159 ****
> > --- 156,163 ----
> > */
> > pg_write_history(history_buf->data);
> > pg_clear_history(history_buf);
> > + pg_write_history(line);
> > + line_saved_in_history = true;
> > }
> > }
> > /* otherwise, get another line */
> > ***************
> > *** 226,232 ****
> > */
> > psql_scan_setup(scan_state, line, strlen(line));
> > success = true;
> > - first_query_scan = true;
> >
> > while (success || !die_on_error)
> > {
> > --- 230,235 ----
> > ***************
> > *** 303,319 ****
> > * down here so we can check for \g and other 'execute'
> > * backslash commands, which should be appended.
> > */
> > ! if (first_query_scan && pset.cur_cmd_interactive)
> > {
> > /* Sending a command (PSQL_CMD_SEND) zeros the length */
> > if (scan_result == PSCAN_BACKSLASH && history_buf->len != 0)
> > pg_write_history(line);
> > else
> > pg_append_history(line, history_buf);
> > }
> >
> > - first_query_scan = false;
> > -
> > /* fall out of loop on \q or if lexer reached EOL */
> > if (slashCmdStatus == PSQL_CMD_TERMINATE ||
> > scan_result == PSCAN_INCOMPLETE ||
> > --- 306,321 ----
> > * down here so we can check for \g and other 'execute'
> > * backslash commands, which should be appended.
> > */
> > ! if (!line_saved_in_history && pset.cur_cmd_interactive)
> > {
> > /* Sending a command (PSQL_CMD_SEND) zeros the length */
> > if (scan_result == PSCAN_BACKSLASH && history_buf->len != 0)
> > pg_write_history(line);
> > else
> > pg_append_history(line, history_buf);
> > + line_saved_in_history = true;
> > }
> >
> > /* fall out of loop on \q or if lexer reached EOL */
> > if (slashCmdStatus == PSQL_CMD_TERMINATE ||
> > scan_result == PSCAN_INCOMPLETE ||
>
> --
> Bruce Momjian http://candle.pha.pa.us
> EnterpriseDB http://www.enterprisedb.com
>
> + If your life is a hard drive, Christ can be your backup. +
>
--
Bruce Momjian http://candle.pha.pa.us
EnterpriseDB http://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +