psql: backslash fix

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 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