Thread: TODO item -- Improve psql's handling of multi-line queries
Hello All,
I'm proposing the small patch for the TODO item -- Improve psql's handling
of multi-line queries. With this patch the multi-line queries are saved
by readline as whole and not line by line.
This is my first patch for Postgres but it seems to work and to not break
anything.
I'm waiting for review, comments, objections, etc...
With Best Regards,
Sergey
*****************************************************
Sergey E. Koposov
Max Planck Institute for Astronomy
Web: http://lnfm1.sai.msu.ru/~math
E-mail: math@sai.msu.ru
Attachment
This has been saved for the 8.2 release:
http://momjian.postgresql.org/cgi-bin/pgpatches_hold
---------------------------------------------------------------------------
Sergey E. Koposov wrote:
> Hello All,
>
> I'm proposing the small patch for the TODO item -- Improve psql's handling
> of multi-line queries. With this patch the multi-line queries are saved
> by readline as whole and not line by line.
>
> This is my first patch for Postgres but it seems to work and to not break
> anything.
>
> I'm waiting for review, comments, objections, etc...
>
> With Best Regards,
>
> Sergey
>
> *****************************************************
> Sergey E. Koposov
> Max Planck Institute for Astronomy
> Web: http://lnfm1.sai.msu.ru/~math
> E-mail: math@sai.msu.ru
>
Content-Description:
[ Attachment, skipping... ]
>
> ---------------------------(end of broadcast)---------------------------
> TIP 5: don't forget to increase your free space map settings
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073
Sergey E. Koposov writes: > I'm proposing the small patch for the TODO item -- Improve psql's handling > of multi-line queries. With this patch the multi-line queries are saved > by readline as whole and not line by line. I like it already! > This is my first patch for Postgres but it seems to work and to not break > anything. > > I'm waiting for review, comments, objections, etc... Did you consider its interaction with \e? Editing the query_buffer with \e will leave that query prefixed with \e in the history. That wasn't the case before your patch. Also, using \e several times on a query without sending it (i.e. without a semicolon) will yield a history entry of a concatenation of old query buffers. Thanks, Andreas
Where are we on this patch? Was it submitted? Applied? Just an idea? --------------------------------------------------------------------------- Andreas Seltenreich wrote: > Sergey E. Koposov writes: > > > I'm proposing the small patch for the TODO item -- Improve psql's handling > > of multi-line queries. With this patch the multi-line queries are saved > > by readline as whole and not line by line. > > I like it already! > > > This is my first patch for Postgres but it seems to work and to not break > > anything. > > > > I'm waiting for review, comments, objections, etc... > > Did you consider its interaction with \e? Editing the query_buffer > with \e will leave that query prefixed with \e in the history. That > wasn't the case before your patch. > > Also, using \e several times on a query without sending it (i.e. > without a semicolon) will yield a history entry of a concatenation of > old query buffers. > > Thanks, > Andreas > > ---------------------------(end of broadcast)--------------------------- > TIP 4: Have you searched our list archives? > > http://archives.postgresql.org > -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
Bruce Momjian writes: > Where are we on this patch? Was it submitted? Applied? Just an > idea? | This has been saved for the 8.2 release: | | http://momjian.postgresql.org/cgi-bin/pgpatches_hold I tested it with 8.1RC1 and noticed the inconsistency with the \edit command. I guess one would at least need to change do_edit to modify the newly introduced history_buffer instead of the history itself. But I'm not sure why there is a need for a history_buffer at all, couldn't one use the query_buffer instead? (Sergey?) regards, Andreas --
Hello,
Sorry for not quick problem fixing, I was quite busy last time...
I submit the new version of my patch (against the CVS tip), correcting the
problem with \edit (pointed by Andreas). So now everything works fine.
With Best Regards,
Sergey
On Thu, 1 Dec 2005, Bruce Momjian wrote:
>
> Where are we on this patch? Was it submitted? Applied? Just an idea?
>
> ---------------------------------------------------------------------------
>
> Andreas Seltenreich wrote:
> > Sergey E. Koposov writes:
> >
> > > I'm proposing the small patch for the TODO item -- Improve psql's handling
> > > of multi-line queries. With this patch the multi-line queries are saved
> > > by readline as whole and not line by line.
> >
> > I like it already!
> >
> > > This is my first patch for Postgres but it seems to work and to not break
> > > anything.
> > >
> > > I'm waiting for review, comments, objections, etc...
> >
> > Did you consider its interaction with \e? Editing the query_buffer
> > with \e will leave that query prefixed with \e in the history. That
> > wasn't the case before your patch.
> >
> > Also, using \e several times on a query without sending it (i.e.
> > without a semicolon) will yield a history entry of a concatenation of
> > old query buffers.
> >
> > Thanks,
> > Andreas
> >
> > ---------------------------(end of broadcast)---------------------------
> > TIP 4: Have you searched our list archives?
> >
> > http://archives.postgresql.org
> >
>
>
*****************************************************
Sergey E. Koposov
Max-Planck Institut for Astronomy
Web: http://lnfm1.sai.msu.ru/~math
E-mail: math@sai.msu.ru
Attachment
Can I have this patch in diff -c? The format you used isn't reliable for patching. Thanks. --------------------------------------------------------------------------- Sergey E. Koposov wrote: > Hello, > > Sorry for not quick problem fixing, I was quite busy last time... > > I submit the new version of my patch (against the CVS tip), correcting the > problem with \edit (pointed by Andreas). So now everything works fine. > > With Best Regards, > > Sergey > > > > On Thu, 1 Dec 2005, Bruce Momjian wrote: > > > > > Where are we on this patch? Was it submitted? Applied? Just an idea? > > > > --------------------------------------------------------------------------- > > > > Andreas Seltenreich wrote: > > > Sergey E. Koposov writes: > > > > > > > I'm proposing the small patch for the TODO item -- Improve psql's handling > > > > of multi-line queries. With this patch the multi-line queries are saved > > > > by readline as whole and not line by line. > > > > > > I like it already! > > > > > > > This is my first patch for Postgres but it seems to work and to not break > > > > anything. > > > > > > > > I'm waiting for review, comments, objections, etc... > > > > > > Did you consider its interaction with \e? Editing the query_buffer > > > with \e will leave that query prefixed with \e in the history. That > > > wasn't the case before your patch. > > > > > > Also, using \e several times on a query without sending it (i.e. > > > without a semicolon) will yield a history entry of a concatenation of > > > old query buffers. > > > > > > Thanks, > > > Andreas > > > > > > ---------------------------(end of broadcast)--------------------------- > > > TIP 4: Have you searched our list archives? > > > > > > http://archives.postgresql.org > > > > > > > > > > ***************************************************** > Sergey E. Koposov > Max-Planck Institut for Astronomy > Web: http://lnfm1.sai.msu.ru/~math > E-mail: math@sai.msu.ru > Content-Description: [ Attachment, skipping... ] -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
On Sun, 4 Dec 2005, Bruce Momjian wrote:
>
> Can I have this patch in diff -c? The format you used isn't reliable
> for patching. Thanks.
Sorry, I didn't know that the patch should be done with "-c".
Ok, now I'm sending it diff -c
Regards,
Sergey
>
> ---------------------------------------------------------------------------
>
> Sergey E. Koposov wrote:
> > Hello,
> >
> > Sorry for not quick problem fixing, I was quite busy last time...
> >
> > I submit the new version of my patch (against the CVS tip), correcting the
> > problem with \edit (pointed by Andreas). So now everything works fine.
> >
> > With Best Regards,
> >
> > Sergey
> >
> >
> >
> > On Thu, 1 Dec 2005, Bruce Momjian wrote:
> >
> > >
> > > Where are we on this patch? Was it submitted? Applied? Just an idea?
> > >
> > > ---------------------------------------------------------------------------
> > >
> > > Andreas Seltenreich wrote:
> > > > Sergey E. Koposov writes:
> > > >
> > > > > I'm proposing the small patch for the TODO item -- Improve psql's handling
> > > > > of multi-line queries. With this patch the multi-line queries are saved
> > > > > by readline as whole and not line by line.
> > > >
> > > > I like it already!
> > > >
> > > > > This is my first patch for Postgres but it seems to work and to not break
> > > > > anything.
> > > > >
> > > > > I'm waiting for review, comments, objections, etc...
> > > >
> > > > Did you consider its interaction with \e? Editing the query_buffer
> > > > with \e will leave that query prefixed with \e in the history. That
> > > > wasn't the case before your patch.
> > > >
> > > > Also, using \e several times on a query without sending it (i.e.
> > > > without a semicolon) will yield a history entry of a concatenation of
> > > > old query buffers.
> > > >
> > > > Thanks,
> > > > Andreas
> > > >
> > > > ---------------------------(end of broadcast)---------------------------
> > > > TIP 4: Have you searched our list archives?
> > > >
> > > > http://archives.postgresql.org
> > > >
> > >
> > >
> >
> >
> > *****************************************************
> > Sergey E. Koposov
> > Max-Planck Institut for Astronomy
> > Web: http://lnfm1.sai.msu.ru/~math
> > E-mail: math@sai.msu.ru
> >
>
> Content-Description:
>
> [ Attachment, skipping... ]
>
>
*****************************************************
Sergey E. Koposov
Max-Planck Institut for Astronomy
Web: http://lnfm1.sai.msu.ru/~math
E-mail: math@sai.msu.ru
Attachment
Sergey E. Koposov schrob:
> I submit the new version of my patch (against the CVS tip), correcting the
> problem with \edit (pointed by Andreas). So now everything works fine.
I think there's a pgflush_history() call missing somewhere, since the
buffer isn't flushed on a control-c. The fresh query is appended to
the aborted one in the history:
--8<---------------cut here---------------start------------->8---
nnpg=# select
nnpg-# something_i_would_rather_not_submit_yet
nnpg-# -- <control-c>
nnpg=# select 1;
?column?
----------
1
(1 row)
nnpg=# -- <control-p> will now yield the following history entry
nnpg=# select
something_i_would_rather_not_submit_yet
select 1;
--8<---------------cut here---------------end--------------->8---
Some more comments:
Wouldn't it be more elegant to use the facilities in libpq's
pqexpbuffer.h for the history buffer instead of passing the
buffer-length around as a separate function argument and doing custom
string operations?
The multi-line history entries are not preserved between psql
invocations. Bash does solve this by folding multi-line commands into
a single line where possible. But I have to admit, this could be sold
as another TODO item :-)
regards,
Andreas
On Wed, 7 Dec 2005, Andreas Seltenreich wrote:
> Sergey E. Koposov schrob:
>
> > I submit the new version of my patch (against the CVS tip), correcting the
> > problem with \edit (pointed by Andreas). So now everything works fine.
>
> I think there's a pgflush_history() call missing somewhere, since the
> buffer isn't flushed on a control-c. The fresh query is appended to
> the aborted one in the history:
> Wouldn't it be more elegant to use the facilities in libpq's
> pqexpbuffer.h for the history buffer instead of passing the
> buffer-length around as a separate function argument and doing custom
> string operations?
Thank you, Andreas. I corrected the bug and also switched to pqexpbuffer
instead of my own work with the strings.
The new patch is attached.
>
> The multi-line history entries are not preserved between psql
> invocations. Bash does solve this by folding multi-line commands into
> a single line where possible. But I have to admit, this could be sold
> as another TODO item :-)
>
I also wanted this feature for a long time.
And I agree that it is rather simple to just remove the \n when writing to the
readline history file, but I don't think that's what everybody wants. I
think that people want to preserve the original formatting...
So in that case the solution can be just putting some symbol instead of \n
in the history file, and during the loading of that file replace it back
(that symbol can be zero byte for example). But I'm not sure that people
will like that solution too.
Regards,
Sergey
*****************************************************
Sergey E. Koposov
Max-Planck Institut for Astronomy
Web: http://lnfm1.sai.msu.ru/~math
E-mail: math@sai.msu.ru
Attachment
[ Previous version removed.]
Your patch has been added to the PostgreSQL unapplied patches list at:
http://momjian.postgresql.org/cgi-bin/pgpatches
It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.
---------------------------------------------------------------------------
Sergey E. Koposov wrote:
> On Wed, 7 Dec 2005, Andreas Seltenreich wrote:
>
> > Sergey E. Koposov schrob:
> >
> > > I submit the new version of my patch (against the CVS tip), correcting the
> > > problem with \edit (pointed by Andreas). So now everything works fine.
> >
> > I think there's a pgflush_history() call missing somewhere, since the
> > buffer isn't flushed on a control-c. The fresh query is appended to
> > the aborted one in the history:
>
> > Wouldn't it be more elegant to use the facilities in libpq's
> > pqexpbuffer.h for the history buffer instead of passing the
> > buffer-length around as a separate function argument and doing custom
> > string operations?
>
>
> Thank you, Andreas. I corrected the bug and also switched to pqexpbuffer
> instead of my own work with the strings.
>
> The new patch is attached.
>
> >
> > The multi-line history entries are not preserved between psql
> > invocations. Bash does solve this by folding multi-line commands into
> > a single line where possible. But I have to admit, this could be sold
> > as another TODO item :-)
> >
>
> I also wanted this feature for a long time.
> And I agree that it is rather simple to just remove the \n when writing to the
> readline history file, but I don't think that's what everybody wants. I
> think that people want to preserve the original formatting...
>
> So in that case the solution can be just putting some symbol instead of \n
> in the history file, and during the loading of that file replace it back
> (that symbol can be zero byte for example). But I'm not sure that people
> will like that solution too.
>
>
> Regards,
> Sergey
>
> *****************************************************
> Sergey E. Koposov
> Max-Planck Institut for Astronomy
> Web: http://lnfm1.sai.msu.ru/~math
> E-mail: math@sai.msu.ru
Content-Description:
[ Attachment, skipping... ]
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073
Sergey E. Koposov wrote: >I also wanted this feature for a long time. >And I agree that it is rather simple to just remove the \n when writing to the >readline history file, but I don't think that's what everybody wants. I >think that people want to preserve the original formatting... > >So in that case the solution can be just putting some symbol instead of \n >in the history file, and during the loading of that file replace it back >(that symbol can be zero byte for example). But I'm not sure that people >will like that solution too. > > > > A zero byte is probably a pretty bad choice. Some other low valued byte (e.g. \x01 ) would probably work better. And I agree that format preservation is highly desirable. cheers andrew
On Wed, 7 Dec 2005, Andrew Dunstan wrote:
> Sergey E. Koposov wrote:
>
> >I also wanted this feature for a long time.
> >And I agree that it is rather simple to just remove the \n when writing to the
> >readline history file, but I don't think that's what everybody wants. I
> >think that people want to preserve the original formatting...
> >
> >So in that case the solution can be just putting some symbol instead of \n
> >in the history file, and during the loading of that file replace it back
> >(that symbol can be zero byte for example). But I'm not sure that people
> >will like that solution too.
> >
> A zero byte is probably a pretty bad choice. Some other low valued byte
> (e.g. \x01 ) would probably work better.
>
> And I agree that format preservation is highly desirable.
If actually the idea with just replacing '\n' in the history with other
byte is not rejected I'm
sending the new version of the psql patch including also the saving of the
multiline queries in the history (even after exiting the psql).
Currently I replace '\n' with the '\x01' as Andrew suggested.
Regards,
Sergey
*****************************************************
Sergey E. Koposov
Max-Planck Institut for Astronomy
Web: http://lnfm1.sai.msu.ru/~math
E-mail: math@sai.msu.ru
Attachment
"Sergey E. Koposov" <math@sai.msu.ru> writes:
> On Wed, 7 Dec 2005, Andrew Dunstan wrote:
>> A zero byte is probably a pretty bad choice. Some other low valued byte
>> (e.g. \x01 ) would probably work better.
> Currently I replace '\n' with the '\x01' as Andrew suggested.
Won't this get confused by some of the Far Eastern encodings we support?
The zero-byte approach is at least proof against that. But what we need
to ask is whether we can expect readline to cope with either.
The patch *looks* pretty ugly: random insertions of blank space,
general failure to conform to the project's code layout conventions,
etc. (Some of this would get cleaned up by pgindent, but I'm not sure
how much.) Also I get the impression that the patch enforces a lot of
history maintenance overhead even in the non-USE_READLINE case, which is
surely useless.
regards, tom lane
Modified patch attached and applied. Thanks. I adjusted based on Tom's comments to use a zero byte, and to clean up the formatting. I didn't see any extra non-readline overhead, just calls to functions that are no-ops in non-readline cases. --------------------------------------------------------------------------- Tom Lane wrote: > "Sergey E. Koposov" <math@sai.msu.ru> writes: > > On Wed, 7 Dec 2005, Andrew Dunstan wrote: > >> A zero byte is probably a pretty bad choice. Some other low valued byte > >> (e.g. \x01 ) would probably work better. > > > Currently I replace '\n' with the '\x01' as Andrew suggested. > > Won't this get confused by some of the Far Eastern encodings we support? > The zero-byte approach is at least proof against that. But what we need > to ask is whether we can expect readline to cope with either. > > The patch *looks* pretty ugly: random insertions of blank space, > general failure to conform to the project's code layout conventions, > etc. (Some of this would get cleaned up by pgindent, but I'm not sure > how much.) Also I get the impression that the patch enforces a lot of > history maintenance overhead even in the non-USE_READLINE case, which is > surely useless. > > regards, tom lane > > ---------------------------(end of broadcast)--------------------------- > TIP 5: don't forget to increase your free space map settings > -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073 Index: src/bin/psql/help.c =================================================================== RCS file: /cvsroot/pgsql/src/bin/psql/help.c,v retrieving revision 1.106 diff -c -c -r1.106 help.c *** src/bin/psql/help.c 15 Oct 2005 02:49:40 -0000 1.106 --- src/bin/psql/help.c 11 Feb 2006 21:52:55 -0000 *************** *** 7,12 **** --- 7,13 ---- */ #include "postgres_fe.h" #include "common.h" + #include "pqexpbuffer.h" #include "input.h" #include "print.h" #include "help.h" Index: src/bin/psql/input.c =================================================================== RCS file: /cvsroot/pgsql/src/bin/psql/input.c,v retrieving revision 1.46 diff -c -c -r1.46 input.c *** src/bin/psql/input.c 15 Oct 2005 02:49:40 -0000 1.46 --- src/bin/psql/input.c 11 Feb 2006 21:52:56 -0000 *************** *** 7,14 **** */ #include "postgres_fe.h" - #include "input.h" #include "pqexpbuffer.h" #include "settings.h" #include "tab-complete.h" #include "common.h" --- 7,14 ---- */ #include "postgres_fe.h" #include "pqexpbuffer.h" + #include "input.h" #include "settings.h" #include "tab-complete.h" #include "common.h" *************** *** 90,107 **** #ifdef USE_READLINE char *s; - static char *prev_hist = NULL; - if (useReadline) /* On some platforms, readline is declared as readline(char *) */ s = readline((char *) prompt); else s = gets_basic(prompt); ! if (useHistory && s && s[0]) { ! enum histcontrol HC; HC = GetHistControlConfig(); if (((HC & hctl_ignorespace) && s[0] == ' ') || --- 90,144 ---- #ifdef USE_READLINE char *s; if (useReadline) /* On some platforms, readline is declared as readline(char *) */ s = readline((char *) prompt); else s = gets_basic(prompt); ! return s; ! #else ! return gets_basic(prompt); ! #endif ! } ! ! ! /* Put the line in the history buffer and also add the trailing \n */ ! void pgadd_history(char *s, PQExpBuffer history_buf) ! { ! #ifdef USE_READLINE ! ! int slen; ! if (useReadline && useHistory && s && s[0]) { ! slen = strlen(s); ! if (s[slen-1] == '\n') ! appendPQExpBufferStr(history_buf, s); ! else ! { ! appendPQExpBufferStr(history_buf, s); ! appendPQExpBufferChar(history_buf, '\n'); ! } ! } ! #endif ! } ! + /* Feed the contents of the history buffer to readline */ + void pgflush_history(PQExpBuffer history_buf) + { + #ifdef USE_READLINE + char *s; + static char *prev_hist; + int slen, i; + + if (useReadline && useHistory ) + { + enum histcontrol HC; + + s = history_buf->data; + prev_hist = NULL; + HC = GetHistControlConfig(); if (((HC & hctl_ignorespace) && s[0] == ' ') || *************** *** 112,128 **** else { free(prev_hist); prev_hist = pg_strdup(s); add_history(s); } } - - return s; - #else - return gets_basic(prompt); #endif } /* --- 149,175 ---- else { free(prev_hist); + slen = strlen(s); + /* Trim the trailing \n's */ + for (i = slen-1; i >= 0 && s[i] == '\n'; i--) + ; + s[i + 1] = '\0'; prev_hist = pg_strdup(s); add_history(s); } + + resetPQExpBuffer(history_buf); } #endif } + void pgclear_history(PQExpBuffer history_buf) + { + #ifdef USE_READLINE + if (useReadline && useHistory) + resetPQExpBuffer(history_buf); + #endif + } /* *************** *** 157,162 **** --- 204,233 ---- } + static void encode_history() + { + HIST_ENTRY *cur_hist; + char *cur_ptr; + + for (history_set_pos(0), cur_hist = current_history(); + cur_hist; cur_hist = next_history()) + for (cur_ptr = cur_hist->line; *cur_ptr; cur_ptr++) + if (*cur_ptr == '\n') + *cur_ptr = '\0'; + } + + static void decode_history() + { + HIST_ENTRY *cur_hist; + char *cur_ptr; + + for (history_set_pos(0), cur_hist = current_history(); + cur_hist; cur_hist = next_history()) + for (cur_ptr = cur_hist->line; *cur_ptr; cur_ptr++) + if (*cur_ptr == '\0') + *cur_ptr = '\n'; + } + /* * Put any startup stuff related to input in here. It's good to maintain *************** *** 197,202 **** --- 268,275 ---- if (psql_history) read_history(psql_history); + + decode_history(); } #endif *************** *** 215,220 **** --- 288,294 ---- #ifdef USE_READLINE if (useHistory && fname) { + encode_history(); if (write_history(fname) == 0) return true; Index: src/bin/psql/input.h =================================================================== RCS file: /cvsroot/pgsql/src/bin/psql/input.h,v retrieving revision 1.23 diff -c -c -r1.23 input.h *** src/bin/psql/input.h 1 Jan 2005 05:43:08 -0000 1.23 --- src/bin/psql/input.h 11 Feb 2006 21:52:56 -0000 *************** *** 39,42 **** --- 39,47 ---- void initializeInput(int flags); bool saveHistory(char *fname); + void pgadd_history(char *s, PQExpBuffer history_buf); + void pgclear_history(PQExpBuffer history_buf); + void pgflush_history(PQExpBuffer history_buf); + + #endif /* INPUT_H */ Index: src/bin/psql/mainloop.c =================================================================== RCS file: /cvsroot/pgsql/src/bin/psql/mainloop.c,v retrieving revision 1.69 diff -c -c -r1.69 mainloop.c *** src/bin/psql/mainloop.c 18 Dec 2005 02:17:16 -0000 1.69 --- src/bin/psql/mainloop.c 11 Feb 2006 21:52:56 -0000 *************** *** 37,42 **** --- 37,43 ---- PQExpBuffer query_buf; /* buffer for query being accumulated */ PQExpBuffer previous_buf; /* if there isn't anything in the new buffer * yet, use this one for \e, etc. */ + PQExpBuffer history_buf; char *line; /* current line of input */ int added_nl_pos; bool success; *************** *** 66,72 **** query_buf = createPQExpBuffer(); previous_buf = createPQExpBuffer(); ! if (!query_buf || !previous_buf) { psql_error("out of memory\n"); exit(EXIT_FAILURE); --- 67,75 ---- query_buf = createPQExpBuffer(); previous_buf = createPQExpBuffer(); ! history_buf = createPQExpBuffer(); ! ! if (!query_buf || !previous_buf || !history_buf) { psql_error("out of memory\n"); exit(EXIT_FAILURE); *************** *** 90,96 **** successResult = EXIT_USER; break; } ! cancel_pressed = false; } --- 93,99 ---- successResult = EXIT_USER; break; } ! pgclear_history(history_buf); cancel_pressed = false; } *************** *** 106,111 **** --- 109,116 ---- count_eof = 0; slashCmdStatus = PSQL_CMD_UNKNOWN; prompt_status = PROMPT_READY; + if (pset.cur_cmd_interactive) + pgclear_history(history_buf); if (pset.cur_cmd_interactive) putc('\n', stdout); *************** *** 138,148 **** psql_scan_reset(scan_state); slashCmdStatus = PSQL_CMD_UNKNOWN; prompt_status = PROMPT_READY; } ! ! /* ! * otherwise, get another line ! */ else if (pset.cur_cmd_interactive) { /* May need to reset prompt, eg after \r command */ --- 143,157 ---- psql_scan_reset(scan_state); slashCmdStatus = PSQL_CMD_UNKNOWN; prompt_status = PROMPT_READY; + + if (pset.cur_cmd_interactive) + /* + * Pass all the contents of history_buf to readline + * and free the history buffer. + */ + pgflush_history(history_buf); } ! /* otherwise, get another line */ else if (pset.cur_cmd_interactive) { /* May need to reset prompt, eg after \r command */ *************** *** 212,218 **** */ psql_scan_setup(scan_state, line, strlen(line)); success = true; ! while (success || !die_on_error) { PsqlScanResult scan_result; --- 221,231 ---- */ psql_scan_setup(scan_state, line, strlen(line)); success = true; ! ! if (pset.cur_cmd_interactive) ! /* Put current line in the history buffer */ ! pgadd_history(line, history_buf); ! while (success || !die_on_error) { PsqlScanResult scan_result; *************** *** 287,292 **** --- 300,312 ---- scan_result == PSCAN_EOL) break; } + + if (pset.cur_cmd_interactive && prompt_status != PROMPT_CONTINUE) + /* + * Pass all the contents of history_buf to readline + * and free the history buffer. + */ + pgflush_history(history_buf); psql_scan_finish(scan_state); free(line); *************** *** 333,338 **** --- 353,359 ---- destroyPQExpBuffer(query_buf); destroyPQExpBuffer(previous_buf); + destroyPQExpBuffer(history_buf); psql_scan_destroy(scan_state); Index: src/bin/psql/prompt.c =================================================================== RCS file: /cvsroot/pgsql/src/bin/psql/prompt.c,v retrieving revision 1.41 diff -c -c -r1.41 prompt.c *** src/bin/psql/prompt.c 3 Jan 2006 23:32:30 -0000 1.41 --- src/bin/psql/prompt.c 11 Feb 2006 21:52:56 -0000 *************** *** 12,17 **** --- 12,18 ---- #include "settings.h" #include "common.h" + #include "pqexpbuffer.h" #include "input.h" #include "variables.h" Index: src/bin/psql/tab-complete.c =================================================================== RCS file: /cvsroot/pgsql/src/bin/psql/tab-complete.c,v retrieving revision 1.144 diff -c -c -r1.144 tab-complete.c *** src/bin/psql/tab-complete.c 11 Jan 2006 08:43:12 -0000 1.144 --- src/bin/psql/tab-complete.c 11 Feb 2006 21:52:59 -0000 *************** *** 43,48 **** --- 43,49 ---- #include "postgres_fe.h" #include "tab-complete.h" + #include "pqexpbuffer.h" #include "input.h" /* If we don't have this, we might as well forget about the whole thing: */ *************** *** 50,56 **** #include <ctype.h> #include "libpq-fe.h" - #include "pqexpbuffer.h" #include "common.h" #include "settings.h" --- 51,56 ----
On Sat, 11 Feb 2006, Bruce Momjian wrote:
>
> Modified patch attached and applied. Thanks.
>
> I adjusted based on Tom's comments to use a zero byte, and to clean up
> the formatting. I didn't see any extra non-readline overhead, just
> calls to functions that are no-ops in non-readline cases.
Thank you, Bruce for modifying and applying the patch (during 2 months I
didn't find time to do that formatting corrections by myself).
> Tom Lane wrote:
> > "Sergey E. Koposov" <math@sai.msu.ru> writes:
> > > On Wed, 7 Dec 2005, Andrew Dunstan wrote:
> > >> A zero byte is probably a pretty bad choice. Some other low valued byte
> > >> (e.g. \x01 ) would probably work better.
> >
> > > Currently I replace '\n' with the '\x01' as Andrew suggested.
> >
> > Won't this get confused by some of the Far Eastern encodings we support?
> > The zero-byte approach is at least proof against that. But what we need
> > to ask is whether we can expect readline to cope with either.
But concerning to your zero byte change, it currently just broke
everything (as I thought, and that's why I didn't implemented it). The
problem with using zero byte is that it breaks all the readline functions
read_history and write_history. Those functions deal with usual C
strings, so putting zero byte inside them will just truncate everything.
(that's exactly what occur with the psql from CVS).
So, I don't know. There are two alternatives. One is to use 0x01 byte
instead: (at least I don't really agree with Tom's comments about possible
problems with using 0x01 with some exotic encodings) (for example I did a
test like this
cat ./pgsql/src/backend/utils/mb/Unicode/*.map |grep 01
cat ./pgsql/src/backend/utils/mb/Unicode/*.map |grep '0x1'
and it didn't produce any output, so it seems to me that 0x01 byte is not
used by any encoding... and UTF encodings are not using 0x01 byte for
certain)
The second alternative is to write our own implementations of read_history
and write_history functions instead of standart readline implementations to
deal with zero bytes in the strings. But it seems to be a rather bad
solution....
Regards,
Sergey
*****************************************************
Sergey E. Koposov
Max Planck Institute for Astronomy
Web: http://lnfm1.sai.msu.ru/~math
E-mail: math@sai.msu.ru
"Sergey E. Koposov" <math@sai.msu.ru> writes:
> But concerning to your zero byte change, it currently just broke
> everything (as I thought, and that's why I didn't implemented it). The
> problem with using zero byte is that it breaks all the readline functions
> read_history and write_history. Those functions deal with usual C
> strings, so putting zero byte inside them will just truncate everything.
> (that's exactly what occur with the psql from CVS).
If CVS tip is actually broken, we'd better revert this patch and
rethink the approach.
> So, I don't know. There are two alternatives. One is to use 0x01 byte
> instead: (at least I don't really agree with Tom's comments about possible
> problems with using 0x01 with some exotic encodings)
Just because you don't use far eastern encodings doesn't mean there's
not a large contingent who do.
I don't understand why any of these shenanigans are needed. If \e is
able to stick a multiline entry into the history, why can't the other
code do it?
regards, tom lane
Oh, seems like a serious problem. I don't think all our encodings avoid bytes after the first multibyte being non-control characters. Some of the Chinese encodings come to mind. Here is a comment from copy.c: * Multi-byte encodings: all supported client-side encodings encode multi-byte * characters by having the first byte's high bit set. Subsequent bytes of the * character can have the high bit not set. When scanning data in such an * encoding to look for a match to a single-byte (ie ASCII) character, we must * use the full pg_encoding_mblen() machinery to skip over multibyte * characters, else we might find a false match to a trailing byte. In * supported server encodings, there is no possibility of a false match, and * it's faster to make useless comparisons to trailing bytes than it is to * invoke pg_encoding_mblen() to skip over them. encoding_embeds_ascii is TRUE * when we have to do it the hard way. Consider that the client-side encoding can have ASCII characters as the non-first byte in multi-byte encodings. I think that is the problem, and you can see how copy.c uses pg_encoding_mblen() to skip over any control characters embedded in the multi-byte sequence. I don't think there is any safe byte value in every multi-byte case except NUL. FYI, I see it broken now if I exit psql and restart it and look at the history. Can we use 0x01 and prefix the history with some kind of tag which indicates if 0x01 appeared in the original string and supress \n conversion? --------------------------------------------------------------------------- Sergey E. Koposov wrote: > On Sat, 11 Feb 2006, Bruce Momjian wrote: > > > > Modified patch attached and applied. Thanks. > > > > I adjusted based on Tom's comments to use a zero byte, and to clean up > > the formatting. I didn't see any extra non-readline overhead, just > > calls to functions that are no-ops in non-readline cases. > > Thank you, Bruce for modifying and applying the patch (during 2 months I > didn't find time to do that formatting corrections by myself). > > > Tom Lane wrote: > > > "Sergey E. Koposov" <math@sai.msu.ru> writes: > > > > On Wed, 7 Dec 2005, Andrew Dunstan wrote: > > > >> A zero byte is probably a pretty bad choice. Some other low valued byte > > > >> (e.g. \x01 ) would probably work better. > > > > > > > Currently I replace '\n' with the '\x01' as Andrew suggested. > > > > > > Won't this get confused by some of the Far Eastern encodings we support? > > > The zero-byte approach is at least proof against that. But what we need > > > to ask is whether we can expect readline to cope with either. > > But concerning to your zero byte change, it currently just broke > everything (as I thought, and that's why I didn't implemented it). The > problem with using zero byte is that it breaks all the readline functions > read_history and write_history. Those functions deal with usual C > strings, so putting zero byte inside them will just truncate everything. > (that's exactly what occur with the psql from CVS). > > So, I don't know. There are two alternatives. One is to use 0x01 byte > instead: (at least I don't really agree with Tom's comments about possible > problems with using 0x01 with some exotic encodings) (for example I did a > test like this > cat ./pgsql/src/backend/utils/mb/Unicode/*.map |grep 01 > cat ./pgsql/src/backend/utils/mb/Unicode/*.map |grep '0x1' > and it didn't produce any output, so it seems to me that 0x01 byte is not > used by any encoding... and UTF encodings are not using 0x01 byte for > certain) > The second alternative is to write our own implementations of read_history > and write_history functions instead of standart readline implementations to > deal with zero bytes in the strings. But it seems to be a rather bad > solution.... > > Regards, > Sergey > > ***************************************************** > Sergey E. Koposov > Max Planck Institute for Astronomy > Web: http://lnfm1.sai.msu.ru/~math > E-mail: math@sai.msu.ru > > > ---------------------------(end of broadcast)--------------------------- > TIP 5: don't forget to increase your free space map settings > -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
On Sat, 11 Feb 2006, Tom Lane wrote: > "Sergey E. Koposov" <math@sai.msu.ru> writes: > > But concerning to your zero byte change, it currently just broke > > everything (as I thought, and that's why I didn't implemented it). The > > problem with using zero byte is that it breaks all the readline functions > > read_history and write_history. Those functions deal with usual C > > strings, so putting zero byte inside them will just truncate everything. > > (that's exactly what occur with the psql from CVS). > > If CVS tip is actually broken, we'd better revert this patch and > rethink the approach. > > > So, I don't know. There are two alternatives. One is to use 0x01 byte > > instead: (at least I don't really agree with Tom's comments about possible > > problems with using 0x01 with some exotic encodings) > > Just because you don't use far eastern encodings doesn't mean there's > not a large contingent who do. > I have said the phrase that I don't agree only after at least some checking of the encodings: 1) First I greped the map files pgsql/src/backend/utils/mb/Unicode/*.map and there is no 0x01 byte in any encoding. 2) UCS, UTF don't use 0x01 inside the multibyte chars. 3) I looked on the most problematic encodings like BIG5, JIS, SJIS, ISO-2022-JP http://en.wikipedia.org/wiki/Big5 http://lfw.org/text/jp.html and they certainly don't use the 0x01 byte. So myself I'm rather convinced that the 0x01 byte is safe. Probably that's not true, but I have no evidence for that. > I don't understand why any of these shenanigans are needed. If \e is > able to stick a multiline entry into the history, why can't the other > code do it? > The problem is in saving those multiline queries to the disk and loading them again as multiline on next psql session and not with putting the queries into the history for one psql session (that thing works with that patch perfectly fine). Regards, Sergey ***************************************************** Sergey E. Koposov Max Planck Institute for Astronomy Web: http://lnfm1.sai.msu.ru/~math E-mail: math@sai.msu.ru
Sergey E. Koposov wrote: > On Sat, 11 Feb 2006, Tom Lane wrote: > > > "Sergey E. Koposov" <math@sai.msu.ru> writes: > > > But concerning to your zero byte change, it currently just broke > > > everything (as I thought, and that's why I didn't implemented it). The > > > problem with using zero byte is that it breaks all the readline functions > > > read_history and write_history. Those functions deal with usual C > > > strings, so putting zero byte inside them will just truncate everything. > > > (that's exactly what occur with the psql from CVS). > > > > If CVS tip is actually broken, we'd better revert this patch and > > rethink the approach. > > > > > So, I don't know. There are two alternatives. One is to use 0x01 byte > > > instead: (at least I don't really agree with Tom's comments about possible > > > problems with using 0x01 with some exotic encodings) > > > > Just because you don't use far eastern encodings doesn't mean there's > > not a large contingent who do. > > > > I have said the phrase that I don't agree only after at least some > checking of the encodings: > 1) First I greped the map files > pgsql/src/backend/utils/mb/Unicode/*.map and there is no 0x01 byte in any > encoding. > 2) UCS, UTF don't use 0x01 inside the multibyte chars. > 3) I looked on the most problematic encodings like BIG5, JIS, SJIS, > ISO-2022-JP > http://en.wikipedia.org/wiki/Big5 > http://lfw.org/text/jp.html > and they certainly don't use the 0x01 byte. > So myself I'm rather convinced that the 0x01 byte is safe. Probably that's > not true, but I have no evidence for that. > OK, seems you did your homework. I will modify the code to use 0x01 unless someone can find an encoding we support that uses 0x01. I will use a macro for 0x01 so it is clearer. > > I don't understand why any of these shenanigans are needed. If \e is > > able to stick a multiline entry into the history, why can't the other > > code do it? > > > > The problem is in saving those multiline queries to the disk and > loading them again as multiline on next psql session and not with > putting the queries into the history for one psql session (that thing > works with that patch perfectly fine). Right, I tested that. Even your patch, if someone does \e and edits a query, and then exits psql and restarts it, the query is in lines, right, so \e really doesn't work even without your patch. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
Modified to use a macro with value 0x01. Applied. --------------------------------------------------------------------------- Bruce Momjian wrote: > Sergey E. Koposov wrote: > > On Sat, 11 Feb 2006, Tom Lane wrote: > > > > > "Sergey E. Koposov" <math@sai.msu.ru> writes: > > > > But concerning to your zero byte change, it currently just broke > > > > everything (as I thought, and that's why I didn't implemented it). The > > > > problem with using zero byte is that it breaks all the readline functions > > > > read_history and write_history. Those functions deal with usual C > > > > strings, so putting zero byte inside them will just truncate everything. > > > > (that's exactly what occur with the psql from CVS). > > > > > > If CVS tip is actually broken, we'd better revert this patch and > > > rethink the approach. > > > > > > > So, I don't know. There are two alternatives. One is to use 0x01 byte > > > > instead: (at least I don't really agree with Tom's comments about possible > > > > problems with using 0x01 with some exotic encodings) > > > > > > Just because you don't use far eastern encodings doesn't mean there's > > > not a large contingent who do. > > > > > > > I have said the phrase that I don't agree only after at least some > > checking of the encodings: > > 1) First I greped the map files > > pgsql/src/backend/utils/mb/Unicode/*.map and there is no 0x01 byte in any > > encoding. > > 2) UCS, UTF don't use 0x01 inside the multibyte chars. > > 3) I looked on the most problematic encodings like BIG5, JIS, SJIS, > > ISO-2022-JP > > http://en.wikipedia.org/wiki/Big5 > > http://lfw.org/text/jp.html > > and they certainly don't use the 0x01 byte. > > So myself I'm rather convinced that the 0x01 byte is safe. Probably that's > > not true, but I have no evidence for that. > > > > OK, seems you did your homework. I will modify the code to use 0x01 > unless someone can find an encoding we support that uses 0x01. I will > use a macro for 0x01 so it is clearer. > > > > I don't understand why any of these shenanigans are needed. If \e is > > > able to stick a multiline entry into the history, why can't the other > > > code do it? > > > > > > > The problem is in saving those multiline queries to the disk and > > loading them again as multiline on next psql session and not with > > putting the queries into the history for one psql session (that thing > > works with that patch perfectly fine). > > Right, I tested that. Even your patch, if someone does \e and edits a > query, and then exits psql and restarts it, the query is in lines, > right, so \e really doesn't work even without your patch. > > -- > Bruce Momjian | http://candle.pha.pa.us > pgman@candle.pha.pa.us | (610) 359-1001 > + If your life is a hard drive, | 13 Roberts Road > + Christ can be your backup. | Newtown Square, Pennsylvania 19073 > > ---------------------------(end of broadcast)--------------------------- > TIP 2: Don't 'kill -9' the postmaster > -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073 Index: src/bin/psql/input.c =================================================================== RCS file: /cvsroot/pgsql/src/bin/psql/input.c,v retrieving revision 1.47 diff -c -c -r1.47 input.c *** src/bin/psql/input.c 11 Feb 2006 21:55:35 -0000 1.47 --- src/bin/psql/input.c 12 Feb 2006 05:21:55 -0000 *************** *** 26,31 **** --- 26,40 ---- static bool useHistory; char *psql_history; + /* + * Preserve newlines in saved queries by mapping '\n' to NL_IN_HISTORY + * + * It is assumed NL_IN_HISTORY will never be entered by the user + * nor appear inside a multi-byte string. 0x00 is not properly + * handled by the readline routines so it can not be used + * for this purpose. + */ + #define NL_IN_HISTORY 0x01 enum histcontrol { *************** *** 213,219 **** cur_hist; cur_hist = next_history()) for (cur_ptr = cur_hist->line; *cur_ptr; cur_ptr++) if (*cur_ptr == '\n') ! *cur_ptr = '\0'; } static void decode_history() --- 222,228 ---- cur_hist; cur_hist = next_history()) for (cur_ptr = cur_hist->line; *cur_ptr; cur_ptr++) if (*cur_ptr == '\n') ! *cur_ptr = NL_IN_HISTORY; } static void decode_history() *************** *** 224,230 **** for (history_set_pos(0), cur_hist = current_history(); cur_hist; cur_hist = next_history()) for (cur_ptr = cur_hist->line; *cur_ptr; cur_ptr++) ! if (*cur_ptr == '\0') *cur_ptr = '\n'; } --- 233,239 ---- for (history_set_pos(0), cur_hist = current_history(); cur_hist; cur_hist = next_history()) for (cur_ptr = cur_hist->line; *cur_ptr; cur_ptr++) ! if (*cur_ptr == NL_IN_HISTORY) *cur_ptr = '\n'; }