Thread: psql: backslash fix
Hi all,
Recently, I reported on -hackers a strange case where psql wouldn't
reset it's query buffer properly on a malformed backslash command, which
would mean that the next query wouldn't be parsed properly. For
instance:
nconway=> select foo\\bar;
Invalid command \. Try \? for help.
nconway-> select 1;
ERROR: parser: parse error at or near "select"
I've attached a patch which fixes this. I'm not sure if it is the
"right" way to fix it, but it resolves the situation discribed above, at
least. The new behavior is:
nconway=> select foo\\bar;
Invalid command \. Try \? for help.
ERROR: Attribute 'foo' not found
nconway=> select 1;
?column?
----------
1
(1 row)
Unless anyone sees any problems, please apply.
Cheers,
Neil
--
Neil Conway <neilconway@rogers.com>
PGP Key ID: DB3C29FC
Attachment
Neil Conway writes: > Recently, I reported on -hackers a strange case where psql wouldn't > reset it's query buffer properly on a malformed backslash command, which > would mean that the next query wouldn't be parsed properly. For > instance: > > nconway=> select foo\\bar; > Invalid command \. Try \? for help. > nconway-> select 1; > ERROR: parser: parse error at or near "select" This is the correct behavior. > I've attached a patch which fixes this. I'm not sure if it is the > "right" way to fix it, but it resolves the situation discribed above, at > least. The new behavior is: > > nconway=> select foo\\bar; > Invalid command \. Try \? for help. > ERROR: Attribute 'foo' not found > nconway=> select 1; > ?column? > ---------- > 1 The semicolon belongs to the backslash command. Where did this get the idea that the query buffer should be executed? You could clear the query buffer on a failed backslash command, but I wouldn't like that. -- Peter Eisentraut peter_e@gmx.net
On Mon, 2002-03-11 at 17:08, Peter Eisentraut wrote: > Neil Conway writes: > > nconway=> select foo\\bar; > > Invalid command \. Try \? for help. > > nconway-> select 1; > > ERROR: parser: parse error at or near "select" > > This is the correct behavior. It would be nice if you could have told me that on -hackers ;-) > > nconway=> select foo\\bar; > > Invalid command \. Try \? for help. > > ERROR: Attribute 'foo' not found > > nconway=> select 1; > > ?column? > > ---------- > > 1 > > The semicolon belongs to the backslash command. Where did this get the > idea that the query buffer should be executed? You could clear the query > buffer on a failed backslash command, but I wouldn't like that. Well, I won't pretend to understand the psql command parsing code (hand parsing w/o flex: yuck!) -- but it seems to me that when it returns the "invalid command" error, it should reset the environment: the previous command failed, so the user should get an error and a chance to enter a new command. With the current code, this isn't the case: they get an error, but it's very likely that their next command will not be parsed properly either. As for the semicolon, AFAICT that's not part of the problem. In current sources, this still fails: nconway=> select foo\\bar Invalid command \. Try \? for help. nconway-> select 1; ERROR: parser: parse error at or near "select" Whether my fix is correct or not, I think that the current behavior is definately wrong. Cheers, Neil -- Neil Conway <neilconway@rogers.com> PGP Key ID: DB3C29FC
Is this change required:
&end_of_cmd);
! success = slashCmdStatus != CMD_ERROR;
if ((slashCmdStatus == CMD_SEND || slashCmdStatus == CMD_NEWEDIT) &&
query_buf->len == 0)
--- 467,473 ----
query_buf->len > 0 ? query_buf : previous_buf,
&end_of_cmd);
! success = (slashCmdStatus != CMD_ERROR);
I thought != was done before =, and my associativity chart shows that:
Operator Associativity
-----------------------------------------------
() [] -> . left to right
! ~ ++ -- - (type) * & sizeof right to left
* / % left to right
+ - left to right
<< >> left to right
< <= > >= left to right
== != left to right
& left to right
^ left to right
| left to right
&& left to right
|| left to right
?: right to left
= += -= etc. right to left
, left to right
---------------------------------------------------------------------------
Neil Conway wrote:
> Hi all,
>
> Recently, I reported on -hackers a strange case where psql wouldn't
> reset it's query buffer properly on a malformed backslash command, which
> would mean that the next query wouldn't be parsed properly. For
> instance:
>
> nconway=> select foo\\bar;
> Invalid command \. Try \? for help.
> nconway-> select 1;
> ERROR: parser: parse error at or near "select"
>
> I've attached a patch which fixes this. I'm not sure if it is the
> "right" way to fix it, but it resolves the situation discribed above, at
> least. The new behavior is:
>
> nconway=> select foo\\bar;
> Invalid command \. Try \? for help.
> ERROR: Attribute 'foo' not found
> nconway=> select 1;
> ?column?
> ----------
> 1
> (1 row)
>
> Unless anyone sees any problems, please apply.
>
> Cheers,
>
> Neil
>
> --
> Neil Conway <neilconway@rogers.com>
> PGP Key ID: DB3C29FC
[ Attachment, skipping... ]
>
> ---------------------------(end of broadcast)---------------------------
> TIP 2: you can get off all lists at once with the unregister command
> (send "unregister YourEmailAddressHere" to majordomo@postgresql.org)
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026
On Mon, 2002-03-11 at 17:44, Bruce Momjian wrote:
>
> Is this change required:
>
>
> &end_of_cmd);
>
> ! success = slashCmdStatus != CMD_ERROR;
>
> if ((slashCmdStatus == CMD_SEND || slashCmdStatus == CMD_NEWEDIT) &&
> query_buf->len == 0)
> --- 467,473 ----
> query_buf->len > 0 ? query_buf : previous_buf,
>
> &end_of_cmd);
>
> ! success = (slashCmdStatus != CMD_ERROR);
>
>
> I thought != was done before =, and my associativity chart shows that:
Yes, that is correct. That change was just for readability (IMHO, it's
silly to depend on operator precedence when a pair of brackets makes the
intent of the code a lot clearer).
The actual functional change is the second part of the patch:
*** 476,482 ****
appendPQExpBufferStr(query_buf,
previous_buf->data);
}
! if (slashCmdStatus == CMD_SEND)
{
success =
SendQuery(query_buf->data);
query_start = i + thislen;
--- 476,482 ----
appendPQExpBufferStr(query_buf,
previous_buf->data);
}
! if (slashCmdStatus == CMD_SEND ||
slashCmdStatus == CMD_ERROR)
{
success =
SendQuery(query_buf->data);
query_start = i + thislen;
Cheers,
Neil
--
Neil Conway <neilconway@rogers.com>
PGP Key ID: DB3C29FC
Oh, just checking. Thank you.
---------------------------------------------------------------------------
Neil Conway wrote:
> On Mon, 2002-03-11 at 17:44, Bruce Momjian wrote:
> >
> > Is this change required:
> >
> >
> > &end_of_cmd);
> >
> > ! success = slashCmdStatus != CMD_ERROR;
> >
> > if ((slashCmdStatus == CMD_SEND || slashCmdStatus == CMD_NEWEDIT) &&
> > query_buf->len == 0)
> > --- 467,473 ----
> > query_buf->len > 0 ? query_buf : previous_buf,
> >
> > &end_of_cmd);
> >
> > ! success = (slashCmdStatus != CMD_ERROR);
> >
> >
> > I thought != was done before =, and my associativity chart shows that:
>
> Yes, that is correct. That change was just for readability (IMHO, it's
> silly to depend on operator precedence when a pair of brackets makes the
> intent of the code a lot clearer).
>
> The actual functional change is the second part of the patch:
>
> *** 476,482 ****
> appendPQExpBufferStr(query_buf,
> previous_buf->data);
> }
>
> ! if (slashCmdStatus == CMD_SEND)
> {
> success =
> SendQuery(query_buf->data);
> query_start = i + thislen;
> --- 476,482 ----
> appendPQExpBufferStr(query_buf,
> previous_buf->data);
> }
>
> ! if (slashCmdStatus == CMD_SEND ||
> slashCmdStatus == CMD_ERROR)
> {
> success =
> SendQuery(query_buf->data);
> query_start = i + thislen;
>
> Cheers,
>
> Neil
>
> --
> Neil Conway <neilconway@rogers.com>
> PGP Key ID: DB3C29FC
>
>
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Neil Conway writes: > Well, I won't pretend to understand the psql command parsing code (hand > parsing w/o flex: yuck!) -- but it seems to me that when it returns the > "invalid command" error, it should reset the environment: the previous > command failed, so the user should get an error and a chance to enter a > new command. The backslash commands are separate from the SQL commands. The reason is that the backslash commands may actually operate on the buffer where the SQL commands are being entered. For instance, say I start entering a command and then decide not to do it and reset the buffer. delete from table1 \R Instead of \r I entered \R, as a typo. There is no \R command, so what do you do? 1. What you proposed: Execute the command as is and reset the buffer. -- I think not. 2. Clear the buffer. This might be reasonable, but I find it totally unnecessary. Maybe I wanted to type \p or \e because my buffer is already 23 lines long. 3. Ignore the failed backslash command and keep going. This is what it's doing. Think of psql as an editor and (some of) the backslash commands as editor commands. When you enter a wrong command in your editor, what does it do? -- Peter Eisentraut peter_e@gmx.net
On Mon, 2002-03-11 at 18:24, Peter Eisentraut wrote:
> The backslash commands are separate from the SQL commands.
This distinction isn't very obvious to the user. Sure, the backslash
commands look very different from SQL -- but you're still entering input
into psql. A lot of backslash commands (e.g. \d ) are just short-cuts
for the equivalent SQL. I'd say that having two different methods of
handling errors is needless and confusing.
This is part of the confusion in this case: a malformed SQL command
resets the buffer, a malformed backslash command does not. I'd vote to
make these consistent, or at least make the differing behavior a lot
more obvious to the user.
> For instance, say I start entering a command and then decide not to do it
> and reset the buffer.
>
> delete from table1
> \R
>
> Instead of \r I entered \R, as a typo. There is no \R command, so what do
> you do?
Give the user an error. If they want to retry the command, let them use
the up-arrow, or even copy-and-paste. I think psql is being too fancy in
this case and trying to read the user's mind: if they typed in malformed
input, let them know "your input was malformed" and let them start over
again.
> 2. Clear the buffer. This might be reasonable, but I find it totally
> unnecessary. Maybe I wanted to type \p or \e because my buffer is
> already 23 lines long.
As I said, they can get back their previous buffer using the up-arrow.
> 3. Ignore the failed backslash command and keep going. This is what it's
> doing.
Even if we want to continue with this behavior, I think the current
implementation is confusing: psql should recall the previous buffer, set
the prompt to "=>", and allow the user to continue editing the command.
For example:
nconway=> select foo\\bar;
Invalid command \. Try \? for help.
nconway=> select foo
^^^^^^^^^^ recalled by psql, the user can continue
typing after this point
But as I said before, I'd prefer that we make the behavior consistent
with the backend.
> Think of psql as an editor and (some of) the backslash commands as editor
> commands. When you enter a wrong command in your editor, what does it do?
It gives me an error. For instance, in vim:
":set nooooexpandtab" -> "Unknown option: nooooexpandtab"
And it returns me to my previous mode. It does _not_ recall ":set ..."
Cheers,
Neil
--
Neil Conway <neilconway@rogers.com>
PGP Key ID: DB3C29FC
Neil Conway writes: > > 3. Ignore the failed backslash command and keep going. This is what it's > > doing. > > Even if we want to continue with this behavior, I think the current > implementation is confusing: psql should recall the previous buffer, set > the prompt to "=>", and allow the user to continue editing the command. > For example: > > nconway=> select foo\\bar; > Invalid command \. Try \? for help. > nconway=> select foo > ^^^^^^^^^^ recalled by psql, the user can continue > typing after this point That's an interesting idea. I'm sure readline can do this, but I haven't looked. > > Think of psql as an editor and (some of) the backslash commands as editor > > commands. When you enter a wrong command in your editor, what does it do? > > It gives me an error. For instance, in vim: > > ":set nooooexpandtab" -> "Unknown option: nooooexpandtab" > > And it returns me to my previous mode. It does _not_ recall ":set ..." Yes, but note that it doesn't completely clear the editing area or save the file or compile it. -- Peter Eisentraut peter_e@gmx.net
Neil, You don't actually _use_ psql much, do you? Yes, it's a different mode of operation than a straight interpreter. That's actually a good thing. The change you've suggested 'for consistencies sake' strike me as making psql a lot more difficult to use, in particular, clearing the buffer on error. One of the big differences is that multiline input is handled differently - if you've got a long buffer, it's in multiple lines. Perhaps that should be changed to more like bash's handling of multiline input, bug throwing it away is wrong. Your comparison with vi is wrong: put vi in linemode (i.e. use ex) and then thow an error: does it clear your edit buffer? I don't think so. One change I _would_ like to see is after invoking the external editor, dump the editted command into the history, as well as sending it to the backend. This would probably need the multiline capability mentioned above, however. On Mon, Mar 11, 2002 at 06:41:08PM -0500, Neil Conway wrote: > On Mon, 2002-03-11 at 18:24, Peter Eisentraut wrote: > > The backslash commands are separate from the SQL commands. > > This distinction isn't very obvious to the user. Sure, the backslash > commands look very different from SQL -- but you're still entering input > into psql. A lot of backslash commands (e.g. \d ) are just short-cuts > for the equivalent SQL. I'd say that having two different methods of > handling errors is needless and confusing. > > This is part of the confusion in this case: a malformed SQL command > resets the buffer, a malformed backslash command does not. I'd vote to > make these consistent, or at least make the differing behavior a lot > more obvious to the user. > > > For instance, say I start entering a command and then decide not to do it > > and reset the buffer. > > > > delete from table1 > > \R > > > > Instead of \r I entered \R, as a typo. There is no \R command, so what do > > you do? > > Give the user an error. If they want to retry the command, let them use > the up-arrow, or even copy-and-paste. I think psql is being too fancy in > this case and trying to read the user's mind: if they typed in malformed > input, let them know "your input was malformed" and let them start over > again. > > > 2. Clear the buffer. This might be reasonable, but I find it totally > > unnecessary. Maybe I wanted to type \p or \e because my buffer is > > already 23 lines long. > > As I said, they can get back their previous buffer using the up-arrow. > > > 3. Ignore the failed backslash command and keep going. This is what it's > > doing. > > Even if we want to continue with this behavior, I think the current > implementation is confusing: psql should recall the previous buffer, set > the prompt to "=>", and allow the user to continue editing the command. > For example: > > nconway=> select foo\\bar; > Invalid command \. Try \? for help. > nconway=> select foo > ^^^^^^^^^^ recalled by psql, the user can continue > typing after this point > > But as I said before, I'd prefer that we make the behavior consistent > with the backend. > > > Think of psql as an editor and (some of) the backslash commands as editor > > commands. When you enter a wrong command in your editor, what does it do? > > It gives me an error. For instance, in vim: > > ":set nooooexpandtab" -> "Unknown option: nooooexpandtab" > > And it returns me to my previous mode. It does _not_ recall ":set ..." > > Cheers, > > Neil > > -- > Neil Conway <neilconway@rogers.com> > PGP Key ID: DB3C29FC > > > ---------------------------(end of broadcast)--------------------------- > TIP 4: Don't 'kill -9' the postmaster
On Tue, 2002-03-12 at 13:21, Ross J. Reedstrom wrote: > Neil, > You don't actually _use_ psql much, do you? I do use it a fair amount -- but I find the accusatory tone a bit unnecessary. I'm trying to improve psql here -- perhaps our views differ, but so what? I'm willing to be convinced... > Yes, it's a different mode of > operation than a straight interpreter. That's actually a good thing. The > change you've suggested 'for consistencies sake' strike me as making > psql a lot more difficult to use, in particular, clearing the buffer > on error. One of the big differences is that multiline input is handled > differently - if you've got a long buffer, it's in multiple lines. Perhaps > that should be changed to more like bash's handling of multiline input, > bug throwing it away is wrong. Well sure, don't clear the buffer on error -- recall the last line of input and put the cursor where the error occured. That's another one of the suggestions I made, and I think it satisfies your concerns. Peter said he'd look into implementing it. Regardless, I do think the current behavior should be improved. In the case I ran into (a simple SQL statement with a malformed backslash command) psql behaves in an extremely unintuitive fashion: the previous buffer is silently prepended to the current one, and the only indication of this is that the prompt changes to a "->". Whether this is improved by clearing the buffer (and thus returning to "=>"), or by recalling the previous buffer and printing it to the screen (allowing the user to continue editing it), I don't really mind. Cheers, Neil -- Neil Conway <neilconway@rogers.com> PGP Key ID: DB3C29FC
On Tue, Mar 12, 2002 at 01:53:20PM -0500, Neil Conway wrote: > On Tue, 2002-03-12 at 13:21, Ross J. Reedstrom wrote: > > Neil, > > You don't actually _use_ psql much, do you? > > I do use it a fair amount -- but I find the accusatory tone a bit > unnecessary. I'm trying to improve psql here -- perhaps our views > differ, but so what? I'm willing to be convinced... Sorry about that - I needed lunch, and my brain gets a bit wonky sometimes when it's short of glucose. I think it was the isuggestion of losing functionality "for consistency's sake" (or equivalant verbage) that touched a nerve for me. > > > Yes, it's a different mode of > > operation than a straight interpreter. That's actually a good thing. The > > change you've suggested 'for consistencies sake' strike me as making > > psql a lot more difficult to use, in particular, clearing the buffer > > on error. One of the big differences is that multiline input is handled > > differently - if you've got a long buffer, it's in multiple lines. Perhaps > > that should be changed to more like bash's handling of multiline input, > > bug throwing it away is wrong. > > Well sure, don't clear the buffer on error -- recall the last line of > input and put the cursor where the error occured. That's another one of > the suggestions I made, and I think it satisfies your concerns. Peter > said he'd look into implementing it. > Agreed. Much better. Although, as I mention above, long expression handling is less than perfect, in any case: having to 'up arrow' for each line in a multi-line command is wonky. Bash actually handles this pretty well: all the lines of a multiline command are available asa single history item. Although, displaying and editing such history items are the best way to find out what's broken in your current terminfo entry. > Regardless, I do think the current behavior should be improved. In the > case I ran into (a simple SQL statement with a malformed backslash > command) psql behaves in an extremely unintuitive fashion: the previous > buffer is silently prepended to the current one, and the only indication > of this is that the prompt changes to a "->". That's pretty standard shell commandline multiline practice: the 'secondary prompt' shows up for an incomplete command. > > Whether this is improved by clearing the buffer (and thus returning to > "=>"), or by recalling the previous buffer and printing it to the screen > (allowing the user to continue editing it), I don't really mind. If the entire previous command were just an uparrow away, I'd even agree to resetting the current buffer. Otherwise, reprinting the current line's worth, as you suggest, might be a good choice. Ross