Обсуждение: Suggested fix for \p and \r in psql

  Hi,

I've noticed two issues with the query buffer post-commit e984ef5
(Support \if ... \elif ... \else ... \endif in psql scripting):

1. \p ignores the "previous buffer". Example:

postgres=# select 1;
 ?column?
----------
    1
(1 row)

postgres=# \p
Query buffer is empty.

That doesn't match the pre-commit behavior, and is not
consistent with \e or \w


2. \r keeps the "previous buffer". I think it should clear it. Example:

postgres=# select 1;
 ?column?
----------
    1
(1 row)

postgres=# select 2 \r
Query buffer reset (cleared).
postgres=# \w /tmp/buffer
postgres=# \! cat /tmp/buffer
select 1;

I suggest the attached fix, with a few new regression tests.


Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

"Daniel Verite" <daniel@manitou-mail.org> writes:
> I've noticed two issues with the query buffer post-commit e984ef5
> (Support \if ... \elif ... \else ... \endif in psql scripting):

> 1. \p ignores the "previous buffer". Example:

Yeah, I did that intentionally, thinking that the old behavior was
confusing.  We can certainly discuss it though.  I'd tend to agree
with your point that \p and \w should print the same thing, but
maybe neither of them should look at the previous_buf.

> 2. \r keeps the "previous buffer". I think it should clear it.

I don't really agree with this.  The fact that it used to clear both
buffers was an implementation accident that probably nobody had even
understood clearly.  ISTM that loses functionality because you can't
do \g anymore.
        regards, tom lane

I wrote:
> "Daniel Verite" <daniel@manitou-mail.org> writes:
>> 1. \p ignores the "previous buffer". Example:

> Yeah, I did that intentionally, thinking that the old behavior was
> confusing.  We can certainly discuss it though.  I'd tend to agree
> with your point that \p and \w should print the same thing, but
> maybe neither of them should look at the previous_buf.

After a bit more thought, it seems like the definition we want for
these is "print what \g would execute, but don't actually change
the buffer state".  So \w is doing the right thing, \p is not, and
that half of your patch is correct.  (I'd be inclined to document
that spec in a comment in each place, though.)

>> 2. \r keeps the "previous buffer". I think it should clear it.

> I don't really agree with this.  The fact that it used to clear both
> buffers was an implementation accident that probably nobody had even
> understood clearly.  ISTM that loses functionality because you can't
> do \g anymore.

Still not sure about this.  The actual behavior of \r under the old
code was to clear query_buf if it was nonempty and otherwise clear
previous_buf.  It's hard for me to credit that that was intentional,
but maybe it was, or at least had behavior natural enough that nobody
complained about it.  Your patch does strictly more than that, and
I think it's too much.  What I committed does strictly less; is it
too little?
        regards, tom lane

>> 1. \p ignores the "previous buffer". Example:
>
> Yeah, I did that intentionally, thinking that the old behavior was
> confusing.  We can certainly discuss it though.  I'd tend to agree
> with your point that \p and \w should print the same thing, but
> maybe neither of them should look at the previous_buf.

After some testing:

ISTM that \p should print what \g would execute, otherwise there is no 
consistent way to look at what \g would do.

Currently \e allows to look at this (previous) executed buffer by editing 
it, but I would find it more consistent if \p is in sync with that, and \e 
also coldly executes the command on return if it ends with ";".

>> 2. \r keeps the "previous buffer". I think it should clear it.
>
> I don't really agree with this.  The fact that it used to clear both
> buffers was an implementation accident that probably nobody had even
> understood clearly.  ISTM that loses functionality because you can't
> do \g anymore.

I agree on this one.

-- 
Fabien.

    Tom Lane wrote:

> > 1. \p ignores the "previous buffer". Example:
>
> Yeah, I did that intentionally, thinking that the old behavior was
> confusing.  We can certainly discuss it though.  I'd tend to agree
> with your point that \p and \w should print the same thing, but
> maybe neither of them should look at the previous_buf.
>
> > 2. \r keeps the "previous buffer". I think it should clear it.
>
> I don't really agree with this.  The fact that it used to clear both
> buffers was an implementation accident that probably nobody had even
> understood clearly.  ISTM that loses functionality because you can't
> do \g anymore.

I don't have a strong opinion on \r. As a user I tend to use Ctrl+C
rather than \r for the edit in progress.
The documentation over-simplifies things as if there
was only one query buffer, instead of two of them.
 \r or \reset     Resets (clears) the query buffer.

From just that reading, the behavior doesn't seem right when
we "clear the query buffer", and then print it, and it doesn't
come out as empty.

About \p alone, if it doesn't output what \g is about to run, I
think that's a problem because ISTM that it's the main use
case of \p

Following the chain of consistency, the starting point seems to be
that \g sends the previous query if the edit-in-progress input
buffer is empty, instead of saying, empty buffer = empty query,
so nothing to send.
Presumably the usage of issuing  \g alone to repeat the last
query is well established, so we can't change it and need
to adjust the other commands to be as less surprising
as possible with our two buffers.


Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite