Horton

Thread

Commits

  1. Doc: clarify behavior of OT_WHOLE_LINE and OT_FILEPIPE psql slash commands.

  2. Document psql's behavior of recalling the previously executed query.

  3. Fix behavior of psql's \p to agree with \g, \w, etc.

  4. Support \if ... \elif ... \else ... \endif in psql scripting.

  1. Suggested fix for \p and \r in psql

    Daniel Verite <daniel@manitou-mail.org> — 2017-04-02T13:22:04Z

      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

  2. Re: Suggested fix for \p and \r in psql

    Fabien COELHO <coelho@cri.ensmp.fr> — 2017-04-02T14:58:38Z

    Hello Daniel,

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

I thought that Tom's changes were somehow intentional, in order to 
simplify the code.

> 1. \p ignores the "previous buffer". Example:
>
> postgres=# select 1;
> ?column?
> ----------
> 	1
> (1 row)
>
> postgres=# \p
> Query buffer is empty.

Yep. Note that it still works with:

   SELECT 1 \g

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

Indeed, there is definitely an issue because \g executes something where 
\p prints nothing...

   10devel> SELECT 1; -- 1
   10devel> \p -- <empty>
   10devel> \g -- 1!

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

I'm not that sure, \r clears the current buffer and restores the previous 
one, so that two \r are needed to fully clear:

   9.6> SELECT 1; -- 1
   9.6> SELECT 2 \r -- first clear
   9.6> \g -- 1 (redo SELECT 1)
   9.6> \r -- second clear
   9.6> \g -- <nothing>


Patch applies, make check ok. However:

I tend to agree that 1 above is a regression, but ISTM that 2 is somehow 
the expected behavior, i.e. \r should not clear the previous buffer, just 
the current one, so that two \r are needed for a "full" clear. \r should 
clear the current buffer and restores the previous one.

-- 
Fabien.

  3. Re: Suggested fix for \p and \r in psql

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-02T15:18:09Z

    "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

  4. Re: Suggested fix for \p and \r in psql

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-02T15:41:04Z

    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

  5. Re: Suggested fix for \p and \r in psql

    Fabien COELHO <coelho@cri.ensmp.fr> — 2017-04-02T15:44:45Z

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

  6. Re: Suggested fix for \p and \r in psql

    Daniel Verite <daniel@manitou-mail.org> — 2017-04-02T16:31:52Z

    	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

  7. Re: Suggested fix for \p and \r in psql

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-02T16:54:20Z

    "Daniel Verite" <daniel@manitou-mail.org> writes:
> The documentation over-simplifies things as if there
> was only one query buffer, instead of two of them.

Yeah, there's a lot of oversimplification in the docs for slash commands
--- for instance, I was just noticing yesterday that there's no mention
of the variant argument-parsing behavior of slash commands that use
OT_WHOLE_LINE or OT_FILEPIPE parsing.  It would be good to make some
effort to improve that.  It seems like a separate question from what
the code should do, though.

My first thought about how to document the query-buffer behavior is
to continue to speak as though there is only one query buffer, but
to add, for example, for the \g command "If the query buffer is empty
then the most recent command is re-executed".

If we do phrase it like that, I think that the most natural behavior
for \r is the way I have it in HEAD --- you'd expect it to clear
the query buffer, but you would not expect it to forget the most
recent command.

			regards, tom lane

  8. Re: Suggested fix for \p and \r in psql

    Daniel Verite <daniel@manitou-mail.org> — 2017-04-02T17:41:02Z

    	Tom Lane wrote:

> If we do phrase it like that, I think that the most natural behavior
> for \r is the way I have it in HEAD --- you'd expect it to clear
> the query buffer, but you would not expect it to forget the most
> recent command.

Okay, leaving out \r as it is and changing only \p works for me.

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

  9. Re: Suggested fix for \p and \r in psql

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-02T20:02:24Z

    "Daniel Verite" <daniel@manitou-mail.org> writes:
> 	Tom Lane wrote:
>> If we do phrase it like that, I think that the most natural behavior
>> for \r is the way I have it in HEAD --- you'd expect it to clear
>> the query buffer, but you would not expect it to forget the most
>> recent command.

> Okay, leaving out \r as it is and changing only \p works for me.

Sold, I'll adjust your patch and push it.

			regards, tom lane