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
Attachments:
psql-query-buffer.difftext/x-patch; name=psql-query-buffer.diffDownload
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 94a3cfc..6554268 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -106,14 +106,14 @@ static backslashResult exec_command_lo(PsqlScanState scan_state, bool active_bra
const char *cmd);
static backslashResult exec_command_out(PsqlScanState scan_state, bool active_branch);
static backslashResult exec_command_print(PsqlScanState scan_state, bool active_branch,
- PQExpBuffer query_buf);
+ PQExpBuffer query_buf, PQExpBuffer previous_buf);
static backslashResult exec_command_password(PsqlScanState scan_state, bool active_branch);
static backslashResult exec_command_prompt(PsqlScanState scan_state, bool active_branch,
const char *cmd);
static backslashResult exec_command_pset(PsqlScanState scan_state, bool active_branch);
static backslashResult exec_command_quit(PsqlScanState scan_state, bool active_branch);
static backslashResult exec_command_reset(PsqlScanState scan_state, bool active_branch,
- PQExpBuffer query_buf);
+ PQExpBuffer query_buf, PQExpBuffer previous_buf);
static backslashResult exec_command_s(PsqlScanState scan_state, bool active_branch);
static backslashResult exec_command_set(PsqlScanState scan_state, bool active_branch);
static backslashResult exec_command_setenv(PsqlScanState scan_state, bool active_branch,
@@ -192,8 +192,8 @@ static void checkWin32Codepage(void);
* execution of the backslash command (for example, \r clears it).
*
* previous_buf contains the query most recently sent to the server
- * (empty if none yet). This should not be modified here, but some
- * commands copy its content into query_buf.
+ * (empty if none yet). This should not be modified here (except by
+ * \r), but some commands copy its content into query_buf.
*
* query_buf and previous_buf will be NULL when executing a "-c"
* command-line option.
@@ -362,7 +362,8 @@ exec_command(const char *cmd,
else if (strcmp(cmd, "o") == 0 || strcmp(cmd, "out") == 0)
status = exec_command_out(scan_state, active_branch);
else if (strcmp(cmd, "p") == 0 || strcmp(cmd, "print") == 0)
- status = exec_command_print(scan_state, active_branch, query_buf);
+ status = exec_command_print(scan_state, active_branch,
+ query_buf, previous_buf);
else if (strcmp(cmd, "password") == 0)
status = exec_command_password(scan_state, active_branch);
else if (strcmp(cmd, "prompt") == 0)
@@ -372,7 +373,8 @@ exec_command(const char *cmd,
else if (strcmp(cmd, "q") == 0 || strcmp(cmd, "quit") == 0)
status = exec_command_quit(scan_state, active_branch);
else if (strcmp(cmd, "r") == 0 || strcmp(cmd, "reset") == 0)
- status = exec_command_reset(scan_state, active_branch, query_buf);
+ status = exec_command_reset(scan_state, active_branch,
+ query_buf, previous_buf);
else if (strcmp(cmd, "s") == 0)
status = exec_command_s(scan_state, active_branch);
else if (strcmp(cmd, "set") == 0)
@@ -1827,12 +1829,15 @@ exec_command_out(PsqlScanState scan_state, bool active_branch)
*/
static backslashResult
exec_command_print(PsqlScanState scan_state, bool active_branch,
- PQExpBuffer query_buf)
+ PQExpBuffer query_buf, PQExpBuffer previous_buf)
{
if (active_branch)
{
if (query_buf && query_buf->len > 0)
puts(query_buf->data);
+ /* Applies to previous query if current buffer is empty */
+ else if (previous_buf && previous_buf->len > 0)
+ puts(previous_buf->data);
else if (!pset.quiet)
puts(_("Query buffer is empty."));
fflush(stdout);
@@ -2056,10 +2061,11 @@ exec_command_quit(PsqlScanState scan_state, bool active_branch)
*/
static backslashResult
exec_command_reset(PsqlScanState scan_state, bool active_branch,
- PQExpBuffer query_buf)
+ PQExpBuffer query_buf, PQExpBuffer previous_buf)
{
if (active_branch)
{
+ resetPQExpBuffer(previous_buf);
resetPQExpBuffer(query_buf);
psql_scan_reset(scan_state);
if (!pset.quiet)
diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out
index 8aa914f..f1c516a 100644
--- a/src/test/regress/expected/psql.out
+++ b/src/test/regress/expected/psql.out
@@ -2932,3 +2932,30 @@ NOTICE: foo
CONTEXT: PL/pgSQL function inline_code_block line 3 at RAISE
ERROR: bar
CONTEXT: PL/pgSQL function inline_code_block line 4 at RAISE
+-- test printing and clearing the query buffer
+SELECT 1;
+ ?column?
+----------
+ 1
+(1 row)
+
+\p
+SELECT 1;
+SELECT 2 \r
+\p
+SELECT 3 \p
+SELECT 3
+UNION SELECT 4 \p
+SELECT 3
+UNION SELECT 4
+UNION SELECT 5
+ORDER BY 1;
+ ?column?
+----------
+ 3
+ 4
+ 5
+(3 rows)
+
+\r
+\p
diff --git a/src/test/regress/sql/psql.sql b/src/test/regress/sql/psql.sql
index 0ae4dd8..b56a05f 100644
--- a/src/test/regress/sql/psql.sql
+++ b/src/test/regress/sql/psql.sql
@@ -548,3 +548,15 @@ begin
raise notice 'foo';
raise exception 'bar';
end $$;
+
+-- test printing and clearing the query buffer
+SELECT 1;
+\p
+SELECT 2 \r
+\p
+SELECT 3 \p
+UNION SELECT 4 \p
+UNION SELECT 5
+ORDER BY 1;
+\r
+\p
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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
"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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
"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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
"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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers