Re: psql - add SHOW_ALL_RESULTS option - pg_regress output
Hi, On 2022-04-06 10:37:29 +0200, Peter Eisentraut wrote: > On 06.04.22 04:06, Andres Freund wrote: > > On 2022-04-04 23:32:50 +0200, Peter Eisentraut wrote: > > > This has been committed. > > > > It's somewhat annoying that made pg_regress even more verbose than before: > > > > == removing existing temp instance== > > == creating temporary instance== > > == initializing database system == > > == starting postmaster== > > running on port 51696 with PID 2203449 > > == creating database "regression" == > > CREATE DATABASE > > ALTER DATABASE > > ALTER DATABASE > > ALTER DATABASE > > ALTER DATABASE > > ALTER DATABASE > > ALTER DATABASE > > == running regression test queries== > > > > Unfortunately it appears that neither can CREATE DATABASE set GUCs, nor can > > ALTER DATABASE set multiple GUCs in one statement. > > > > Perhaps we can just set SHOW_ALL_RESULTS off for that psql command? > > Do you mean the extra "ALTER DATABASE" lines? Couldn't we just turn all of > those off? AFAICT, no one likes them. Yea. Previously there was just CREATE DATABASE. And yes, it seems like we should use -q in psql_start_command(). Daniel has a patch to shrink pg_regress output overall, but it came too late for 15. It'd still be good to avoid further increasing the size till then IMO. Greetings, Andres Freund
Re: psql - add SHOW_ALL_RESULTS option - pg_regress output
On 06.04.22 04:06, Andres Freund wrote: On 2022-04-04 23:32:50 +0200, Peter Eisentraut wrote: This has been committed. It's somewhat annoying that made pg_regress even more verbose than before: == removing existing temp instance== == creating temporary instance== == initializing database system == == starting postmaster== running on port 51696 with PID 2203449 == creating database "regression" == CREATE DATABASE ALTER DATABASE ALTER DATABASE ALTER DATABASE ALTER DATABASE ALTER DATABASE ALTER DATABASE == running regression test queries== Unfortunately it appears that neither can CREATE DATABASE set GUCs, nor can ALTER DATABASE set multiple GUCs in one statement. Perhaps we can just set SHOW_ALL_RESULTS off for that psql command? Do you mean the extra "ALTER DATABASE" lines? Couldn't we just turn all of those off? AFAICT, no one likes them.
Re: psql - add SHOW_ALL_RESULTS option - pg_regress output
Hi, On 2022-04-04 23:32:50 +0200, Peter Eisentraut wrote: > This has been committed. It's somewhat annoying that made pg_regress even more verbose than before: == removing existing temp instance== == creating temporary instance== == initializing database system == == starting postmaster== running on port 51696 with PID 2203449 == creating database "regression" == CREATE DATABASE ALTER DATABASE ALTER DATABASE ALTER DATABASE ALTER DATABASE ALTER DATABASE ALTER DATABASE == running regression test queries== Unfortunately it appears that neither can CREATE DATABASE set GUCs, nor can ALTER DATABASE set multiple GUCs in one statement. Perhaps we can just set SHOW_ALL_RESULTS off for that psql command? - Andres
Re: psql - add SHOW_ALL_RESULTS option
On 02.04.22 15:26, Fabien COELHO wrote: Again, after the SendQuery refactoring extraction. I'm doing this locally, so don't feel obliged to send more of these. ;-) Good for me :-) This has been committed. I reduced some of your stylistic changes in order to keep the surface area of this complicated patch small. We can apply some of those later if you are interested. Right now, let's let it settle a bit.
Re: psql - add SHOW_ALL_RESULTS option
Again, after the SendQuery refactoring extraction. I'm doing this locally, so don't feel obliged to send more of these. ;-) Good for me :-) -- Fabien.
Re: psql - add SHOW_ALL_RESULTS option
On 01.04.22 07:46, Fabien COELHO wrote: Attached a rebase. Again, after the SendQuery refactoring extraction. I'm doing this locally, so don't feel obliged to send more of these. ;-) I've started committing this now, in pieces.
Re: psql - add SHOW_ALL_RESULTS option
Attached a rebase.
Again, after the SendQuery refactoring extraction.
--
Fabien.diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out
index e0abe34bb6..8f7f93172a 100644
--- a/contrib/pg_stat_statements/expected/pg_stat_statements.out
+++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out
@@ -50,8 +50,28 @@ BEGIN \;
SELECT 2.0 AS "float" \;
SELECT 'world' AS "text" \;
COMMIT;
+ float
+---
+ 2.0
+(1 row)
+
+ text
+---
+ world
+(1 row)
+
-- compound with empty statements and spurious leading spacing
\;\; SELECT 3 + 3 \;\;\; SELECT ' ' || ' !' \;\; SELECT 1 + 4 \;;
+ ?column?
+--
+6
+(1 row)
+
+ ?column?
+--
+ !
+(1 row)
+
?column?
--
5
@@ -61,6 +81,11 @@ COMMIT;
SELECT 1 + 1 + 1 AS "add" \gset
SELECT :add + 1 + 1 AS "add" \;
SELECT :add + 1 + 1 AS "add" \gset
+ add
+-
+ 5
+(1 row)
+
-- set operator
SELECT 1 AS i UNION SELECT 2 ORDER BY i;
i
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index caabb06c53..f01adb1fd2 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -127,18 +127,11 @@ echo '\x \\ SELECT * FROM foo;' | psql
commands included in the string to divide it into multiple
transactions. (See
for more details about how the server handles multi-query strings.)
- Also, psql only prints the
- result of the last SQL command in the string.
- This is different from the behavior when the same string is read from
- a file or fed to psql's standard input,
- because then psql sends
- each SQL command separately.
- Because of this behavior, putting more than one SQL command in a
- single -c string often has unexpected results.
- It's better to use repeated -c commands or feed
- multiple commands to psql's standard input,
+ If having several commands executed in one transaction is not desired,
+ use repeated -c commands or feed multiple commands to
+ psql's standard input,
either using echo as illustrated above, or
via a shell here-document, for example:
@@ -3570,10 +3563,6 @@ select 1\; select 2\; select 3;
commands included in the string to divide it into multiple
transactions. (See
for more details about how the server handles multi-query strings.)
-psql prints only the last query result
-it receives for each request; in this example, although all
-three SELECTs are indeed executed, psql
-only prints the 3.
@@ -4160,6 +4149,18 @@ bar
+SHOW_ALL_RESULTS
+
+
+When this variable is set to off, only the last
+result of a combined query (\;) is shown instead of
+all of them. The default is on. The off behavior
+is for compatibility with older versions of psql.
+
+
+
+
+
SHOW_CONTEXT
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index c9847c8f9a..c84688e89c 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -32,9 +32,10 @@
static bool DescribeQuery(const char *query, double *elapsed_msec);
static bool ExecQueryUsingCursor(const char *query, double *elapsed_msec);
-static bool ExecQueryAndProcessResult(const char *query, double *elapsed_msec, bool *svpt_gone_p);
static bool command_no_begin(const char *query);
static bool is_select_command(const char *query);
+static int SendQueryAndProcessResults(const char *query, double *pelapsed_msec,
+ bool is_watch, const printQueryOpt *opt, FILE *printQueryFout, bool *tx_ended);
/*
@@ -355,7 +356,7 @@ CheckConnection(void)
* Returns true for valid result, false for error state.
*/
static bool
-AcceptResult(const PGresult *result)
+AcceptResult(const PGresult *result, bool show_error)
{
bool OK;
@@ -386,7 +387,7 @@ AcceptResult(const PGresult *result)
break;
}
- if (!OK)
+ if (!OK && show_error)
{
const char *error = PQerrorMessage(pset.db);
@@ -474,6 +475,18 @@ ClearOrSaveResult(PGresult *result)
}
}
+/*
+ * Consume all results
+ */
+static void
+ClearOrSaveAllResults()
+{
+ PGresult *result;
+
+ while ((result = PQgetResult(pset.db)) != NULL)
+ ClearOrSaveResult(result);
+}
+
/*
* Print microtiming output. Always print raw milliseconds; if the interval
@@ -574,7 +587,7 @@ PSQLexec(const char *query)
ResetCancelConn();
- if (!AcceptResult(res))
+ if (!AcceptResult(res, true))
{
ClearOrSaveResult(res);
res = NULL;
@@ -597,11 +610,8 @@ int
PSQLexecWatch(const char *query, const printQueryOpt *opt, FILE *printQueryFout)
{
bool timing = pset.timing;
- PGresult *res;
double elapsed_msec = 0;
- instr_time before;
- instr_time after;
- FILE *fout;
+ int res;
if (!pset.db)
{
@@
Re: psql - add SHOW_ALL_RESULTS option
Attached a rebase.
--
Fabien.diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out
index e0abe34bb6..8f7f93172a 100644
--- a/contrib/pg_stat_statements/expected/pg_stat_statements.out
+++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out
@@ -50,8 +50,28 @@ BEGIN \;
SELECT 2.0 AS "float" \;
SELECT 'world' AS "text" \;
COMMIT;
+ float
+---
+ 2.0
+(1 row)
+
+ text
+---
+ world
+(1 row)
+
-- compound with empty statements and spurious leading spacing
\;\; SELECT 3 + 3 \;\;\; SELECT ' ' || ' !' \;\; SELECT 1 + 4 \;;
+ ?column?
+--
+6
+(1 row)
+
+ ?column?
+--
+ !
+(1 row)
+
?column?
--
5
@@ -61,6 +81,11 @@ COMMIT;
SELECT 1 + 1 + 1 AS "add" \gset
SELECT :add + 1 + 1 AS "add" \;
SELECT :add + 1 + 1 AS "add" \gset
+ add
+-
+ 5
+(1 row)
+
-- set operator
SELECT 1 AS i UNION SELECT 2 ORDER BY i;
i
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index caabb06c53..f01adb1fd2 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -127,18 +127,11 @@ echo '\x \\ SELECT * FROM foo;' | psql
commands included in the string to divide it into multiple
transactions. (See
for more details about how the server handles multi-query strings.)
- Also, psql only prints the
- result of the last SQL command in the string.
- This is different from the behavior when the same string is read from
- a file or fed to psql's standard input,
- because then psql sends
- each SQL command separately.
- Because of this behavior, putting more than one SQL command in a
- single -c string often has unexpected results.
- It's better to use repeated -c commands or feed
- multiple commands to psql's standard input,
+ If having several commands executed in one transaction is not desired,
+ use repeated -c commands or feed multiple commands to
+ psql's standard input,
either using echo as illustrated above, or
via a shell here-document, for example:
@@ -3570,10 +3563,6 @@ select 1\; select 2\; select 3;
commands included in the string to divide it into multiple
transactions. (See
for more details about how the server handles multi-query strings.)
-psql prints only the last query result
-it receives for each request; in this example, although all
-three SELECTs are indeed executed, psql
-only prints the 3.
@@ -4160,6 +4149,18 @@ bar
+SHOW_ALL_RESULTS
+
+
+When this variable is set to off, only the last
+result of a combined query (\;) is shown instead of
+all of them. The default is on. The off behavior
+is for compatibility with older versions of psql.
+
+
+
+
+
SHOW_CONTEXT
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index d65b9a124f..c84688e89c 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -34,6 +34,8 @@ static bool DescribeQuery(const char *query, double *elapsed_msec);
static bool ExecQueryUsingCursor(const char *query, double *elapsed_msec);
static bool command_no_begin(const char *query);
static bool is_select_command(const char *query);
+static int SendQueryAndProcessResults(const char *query, double *pelapsed_msec,
+ bool is_watch, const printQueryOpt *opt, FILE *printQueryFout, bool *tx_ended);
/*
@@ -354,7 +356,7 @@ CheckConnection(void)
* Returns true for valid result, false for error state.
*/
static bool
-AcceptResult(const PGresult *result)
+AcceptResult(const PGresult *result, bool show_error)
{
bool OK;
@@ -385,7 +387,7 @@ AcceptResult(const PGresult *result)
break;
}
- if (!OK)
+ if (!OK && show_error)
{
const char *error = PQerrorMessage(pset.db);
@@ -473,6 +475,18 @@ ClearOrSaveResult(PGresult *result)
}
}
+/*
+ * Consume all results
+ */
+static void
+ClearOrSaveAllResults()
+{
+ PGresult *result;
+
+ while ((result = PQgetResult(pset.db)) != NULL)
+ ClearOrSaveResult(result);
+}
+
/*
* Print microtiming output. Always print raw milliseconds; if the interval
@@ -573,7 +587,7 @@ PSQLexec(const char *query)
ResetCancelConn();
- if (!AcceptResult(res))
+ if (!AcceptResult(res, true))
{
ClearOrSaveResult(res);
res = NULL;
@@ -596,11 +610,8 @@ int
PSQLexecWatch(const char *query, const printQueryOpt *opt, FILE *printQueryFout)
{
bool timing = pset.timing;
- PGresult *res;
double elapsed_msec = 0;
- instr_time before;
- instr_time after;
- FILE *fout;
+ int res;
if (!pset.db)
{
@@ -609,77 +620,14 @@ PSQLexecWatch(const char *query, const printQueryOpt *opt, FILE *printQueryFout)
}
SetCancelConn(pset.db);
-
- if (timing)
-
Re: psql - add SHOW_ALL_RESULTS option
Hello Peter,
As Tom said earlier, wasn't this fixed by 618c16707? If not, is there any
other discussion on the specifics of this issue? I'm not aware of one.
This answer is that I had kept psql's calls to PQerrorMessage which
reports errors from the connection, whereas it needed to change to
PQresultErrorMessage to benefit from the libpq improvement.
I made the added autocommit/on_error_rollback tests at the end really
focus on multi-statement queries (\;), as was more or less intended.
I updated the tap test.
--
Fabien.diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out
index e0abe34bb6..8f7f93172a 100644
--- a/contrib/pg_stat_statements/expected/pg_stat_statements.out
+++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out
@@ -50,8 +50,28 @@ BEGIN \;
SELECT 2.0 AS "float" \;
SELECT 'world' AS "text" \;
COMMIT;
+ float
+---
+ 2.0
+(1 row)
+
+ text
+---
+ world
+(1 row)
+
-- compound with empty statements and spurious leading spacing
\;\; SELECT 3 + 3 \;\;\; SELECT ' ' || ' !' \;\; SELECT 1 + 4 \;;
+ ?column?
+--
+6
+(1 row)
+
+ ?column?
+--
+ !
+(1 row)
+
?column?
--
5
@@ -61,6 +81,11 @@ COMMIT;
SELECT 1 + 1 + 1 AS "add" \gset
SELECT :add + 1 + 1 AS "add" \;
SELECT :add + 1 + 1 AS "add" \gset
+ add
+-
+ 5
+(1 row)
+
-- set operator
SELECT 1 AS i UNION SELECT 2 ORDER BY i;
i
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index caabb06c53..f01adb1fd2 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -127,18 +127,11 @@ echo '\x \\ SELECT * FROM foo;' | psql
commands included in the string to divide it into multiple
transactions. (See
for more details about how the server handles multi-query strings.)
- Also, psql only prints the
- result of the last SQL command in the string.
- This is different from the behavior when the same string is read from
- a file or fed to psql's standard input,
- because then psql sends
- each SQL command separately.
- Because of this behavior, putting more than one SQL command in a
- single -c string often has unexpected results.
- It's better to use repeated -c commands or feed
- multiple commands to psql's standard input,
+ If having several commands executed in one transaction is not desired,
+ use repeated -c commands or feed multiple commands to
+ psql's standard input,
either using echo as illustrated above, or
via a shell here-document, for example:
@@ -3570,10 +3563,6 @@ select 1\; select 2\; select 3;
commands included in the string to divide it into multiple
transactions. (See
for more details about how the server handles multi-query strings.)
-psql prints only the last query result
-it receives for each request; in this example, although all
-three SELECTs are indeed executed, psql
-only prints the 3.
@@ -4160,6 +4149,18 @@ bar
+SHOW_ALL_RESULTS
+
+
+When this variable is set to off, only the last
+result of a combined query (\;) is shown instead of
+all of them. The default is on. The off behavior
+is for compatibility with older versions of psql.
+
+
+
+
+
SHOW_CONTEXT
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index d65b9a124f..c84688e89c 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -34,6 +34,8 @@ static bool DescribeQuery(const char *query, double *elapsed_msec);
static bool ExecQueryUsingCursor(const char *query, double *elapsed_msec);
static bool command_no_begin(const char *query);
static bool is_select_command(const char *query);
+static int SendQueryAndProcessResults(const char *query, double *pelapsed_msec,
+ bool is_watch, const printQueryOpt *opt, FILE *printQueryFout, bool *tx_ended);
/*
@@ -354,7 +356,7 @@ CheckConnection(void)
* Returns true for valid result, false for error state.
*/
static bool
-AcceptResult(const PGresult *result)
+AcceptResult(const PGresult *result, bool show_error)
{
bool OK;
@@ -385,7 +387,7 @@ AcceptResult(const PGresult *result)
break;
}
- if (!OK)
+ if (!OK && show_error)
{
const char *error = PQerrorMessage(pset.db);
@@ -473,6 +475,18 @@ ClearOrSaveResult(PGresult *result)
}
}
+/*
+ * Consume all results
+ */
+static void
+ClearOrSaveAllResults()
+{
+ PGresult *result;
+
+ while ((result = PQgetResult(pset.db)) != NULL)
+ ClearOrSaveResult(result);
+}
+
/*
* Print microtiming output. Always print raw milliseconds; if the interval
@@ -573,7 +587,7 @@ PSQLexec(const char *query)
ResetCancelConn();
- if (!AcceptResult(res))
+ if
Re: psql - add SHOW_ALL_RESULTS option
As Tom said earlier, wasn't this fixed by 618c16707? If not, is there any other discussion on the specifics of this issue? I'm not aware of one. Hmmm… I'll try to understand why the doubled message seems to be still there. -- Fabien.
Re: psql - add SHOW_ALL_RESULTS option
On 23.03.22 13:58, Fabien COELHO wrote: If you want to wait for libpq to provide a solution for this corner case, I'm afraid that "never" is the likely result, especially as no test case exercices this path to show that there is a problem somewhere, so nobody should care to fix it. I'm not sure it is even worth it given the highly special situation which triggers the issue, which is not such an actual problem (ok, the user is told twice that there was a connection loss, no big deal). As Tom said earlier, wasn't this fixed by 618c16707? If not, is there any other discussion on the specifics of this issue? I'm not aware of one.
Re: psql - add SHOW_ALL_RESULTS option
Hello Peter, Attached v17 is another try. The point is to record the current status, whatever it is, buggy or not, and to update the test when libpq fixes things, whenever this is done. [...] The expected output (which passes) contains this line twice: psql::2: FATAL: terminating connection due to administrator command psql::2: FATAL: terminating connection due to administrator command If I paste this test case into current master without your patch, I only get this line once. So your patch is changing this output. The whole point of the libpq fixes was to not have this duplicate output. So I think something is still wrong somewhere. Hmmm. Yes and no:-) The previous path inside libpq silently ignores intermediate results, it skips all results to keep only the last one. The new approach does not discard resultss silently, hence the duplicated output, because they are actually there and have always been there in the first place, they were just ignored: The previous "good" result is really a side effect of a bad implementation in a corner case, which just becomes apparent when opening the list of results. So my opinion is still to dissociate the libpq "bug/behavior" fix from this feature, as they are only loosely connected, because it is a very corner case anyway. An alternative would be to remove the test case, but I'd prefer that it is kept. If you want to wait for libpq to provide a solution for this corner case, I'm afraid that "never" is the likely result, especially as no test case exercices this path to show that there is a problem somewhere, so nobody should care to fix it. I'm not sure it is even worth it given the highly special situation which triggers the issue, which is not such an actual problem (ok, the user is told twice that there was a connection loss, no big deal). -- Fabien.
Re: psql - add SHOW_ALL_RESULTS option
On 17.03.22 19:04, Fabien COELHO wrote: Indeed! The missing part was put back in v17. Some unrelated notes on this v17 patch: -use Test::More; +use Test::More tests => 41; The test counts are not needed/wanted anymore. + +\set ECHO none + This seems inappropriate. +-- +-- autocommit +-- +-- +-- test ON_ERROR_ROLLBACK +-- This test file already contains tests for autocommit and ON_ERROR_ROLLBACK. If you want to change those, please add yours into the existing sections, not make new ones. I'm not sure if your tests add any new coverage, or if it is just duplicate.
Re: psql - add SHOW_ALL_RESULTS option
On 17.03.22 19:04, Fabien COELHO wrote: Hello Peter, See attached v16 which removes the libpq workaround. I suppose this depends on https://www.postgresql.org/message-id/flat/ab4288f8-be5c-57fb-2400-e3e857f53e46%40enterprisedb.com getting committed, because right now this makes the psql TAP tests fail because of the duplicate error message. How should we handle that? Ok, it seems I got the patch wrong. Attached v17 is another try. The point is to record the current status, whatever it is, buggy or not, and to update the test when libpq fixes things, whenever this is done. Your patch contains this test case: +# Test voluntary crash +my ($ret, $out, $err) = $node->psql( + 'postgres', + "SELECT 'before' AS running;\n" . + "SELECT pg_terminate_backend(pg_backend_pid());\n" . + "SELECT 'AFTER' AS not_running;\n"); + +is($ret, 2, "server stopped"); +like($out, qr/before/, "output before crash"); +ok($out !~ qr/AFTER/, "no output after crash"); +is($err, 'psql::2: FATAL: terminating connection due to administrator command +psql::2: FATAL: terminating connection due to administrator command +server closed the connection unexpectedly + This probably means the server terminated abnormally + before or while processing the request. +psql::2: fatal: connection to server was lost', "expected error message"); The expected output (which passes) contains this line twice: psql::2: FATAL: terminating connection due to administrator command psql::2: FATAL: terminating connection due to administrator command If I paste this test case into current master without your patch, I only get this line once. So your patch is changing this output. The whole point of the libpq fixes was to not have this duplicate output. So I think something is still wrong somewhere.
Re: psql - add SHOW_ALL_RESULTS option
Hello Peter,
See attached v16 which removes the libpq workaround.
I suppose this depends on
https://www.postgresql.org/message-id/flat/ab4288f8-be5c-57fb-2400-e3e857f53e46%40enterprisedb.com
getting committed, because right now this makes the psql TAP tests fail
because of the duplicate error message.
How should we handle that?
Ok, it seems I got the patch wrong.
Attached v17 is another try. The point is to record the current status,
whatever it is, buggy or not, and to update the test when libpq fixes
things, whenever this is done.
In this part of the patch, there seems to be part of a sentence missing:
Indeed! The missing part was put back in v17.
--
Fabien.diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out
index e0abe34bb6..8f7f93172a 100644
--- a/contrib/pg_stat_statements/expected/pg_stat_statements.out
+++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out
@@ -50,8 +50,28 @@ BEGIN \;
SELECT 2.0 AS "float" \;
SELECT 'world' AS "text" \;
COMMIT;
+ float
+---
+ 2.0
+(1 row)
+
+ text
+---
+ world
+(1 row)
+
-- compound with empty statements and spurious leading spacing
\;\; SELECT 3 + 3 \;\;\; SELECT ' ' || ' !' \;\; SELECT 1 + 4 \;;
+ ?column?
+--
+6
+(1 row)
+
+ ?column?
+--
+ !
+(1 row)
+
?column?
--
5
@@ -61,6 +81,11 @@ COMMIT;
SELECT 1 + 1 + 1 AS "add" \gset
SELECT :add + 1 + 1 AS "add" \;
SELECT :add + 1 + 1 AS "add" \gset
+ add
+-
+ 5
+(1 row)
+
-- set operator
SELECT 1 AS i UNION SELECT 2 ORDER BY i;
i
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index caabb06c53..f01adb1fd2 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -127,18 +127,11 @@ echo '\x \\ SELECT * FROM foo;' | psql
commands included in the string to divide it into multiple
transactions. (See
for more details about how the server handles multi-query strings.)
- Also, psql only prints the
- result of the last SQL command in the string.
- This is different from the behavior when the same string is read from
- a file or fed to psql's standard input,
- because then psql sends
- each SQL command separately.
- Because of this behavior, putting more than one SQL command in a
- single -c string often has unexpected results.
- It's better to use repeated -c commands or feed
- multiple commands to psql's standard input,
+ If having several commands executed in one transaction is not desired,
+ use repeated -c commands or feed multiple commands to
+ psql's standard input,
either using echo as illustrated above, or
via a shell here-document, for example:
@@ -3570,10 +3563,6 @@ select 1\; select 2\; select 3;
commands included in the string to divide it into multiple
transactions. (See
for more details about how the server handles multi-query strings.)
-psql prints only the last query result
-it receives for each request; in this example, although all
-three SELECTs are indeed executed, psql
-only prints the 3.
@@ -4160,6 +4149,18 @@ bar
+SHOW_ALL_RESULTS
+
+
+When this variable is set to off, only the last
+result of a combined query (\;) is shown instead of
+all of them. The default is on. The off behavior
+is for compatibility with older versions of psql.
+
+
+
+
+
SHOW_CONTEXT
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index d65b9a124f..2f3dd91602 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -34,6 +34,8 @@ static bool DescribeQuery(const char *query, double *elapsed_msec);
static bool ExecQueryUsingCursor(const char *query, double *elapsed_msec);
static bool command_no_begin(const char *query);
static bool is_select_command(const char *query);
+static int SendQueryAndProcessResults(const char *query, double *pelapsed_msec,
+ bool is_watch, const printQueryOpt *opt, FILE *printQueryFout, bool *tx_ended);
/*
@@ -354,7 +356,7 @@ CheckConnection(void)
* Returns true for valid result, false for error state.
*/
static bool
-AcceptResult(const PGresult *result)
+AcceptResult(const PGresult *result, bool show_error)
{
bool OK;
@@ -385,7 +387,7 @@ AcceptResult(const PGresult *result)
break;
}
- if (!OK)
+ if (!OK && show_error)
{
const char *error = PQerrorMessage(pset.db);
@@ -473,6 +475,18 @@ ClearOrSaveResult(PGresult *result)
}
}
+/*
+ * Consume all results
+ */
+static void
+ClearOrSaveAllResults()
+{
+ PGresult *result;
+
+ while ((result = PQgetResult(pset.db)) != NULL)
+ ClearOrSaveResult(result);
+}
+
/*
* Print microtiming output. Always
Re: psql - add SHOW_ALL_RESULTS option
Peter Eisentraut writes: > I suppose this depends on > https://www.postgresql.org/message-id/flat/ab4288f8-be5c-57fb-2400-e3e857f53e46%40enterprisedb.com > getting committed, because right now this makes the psql TAP tests fail > because of the duplicate error message. Umm ... wasn't 618c16707 what you need here? regards, tom lane
Re: psql - add SHOW_ALL_RESULTS option
On 12.03.22 17:27, Fabien COELHO wrote: Are you planning to send a rebased patch for this commit fest? Argh, I did it in a reply in another thread:-( Attached v15. So as to help moves things forward, I'd suggest that we should not to care too much about corner case repetition of some error messages which are due to libpq internals, so I could remove the ugly buffer reset from the patch and have the repetition, and if/when the issue is fixed later in libpq then the repetition will be removed, fine! The issue is that we just expose the strange behavior of libpq, which is libpq to solve, not psql. See attached v16 which removes the libpq workaround. I suppose this depends on https://www.postgresql.org/message-id/flat/ab4288f8-be5c-57fb-2400-e3e857f53e46%40enterprisedb.com getting committed, because right now this makes the psql TAP tests fail because of the duplicate error message. How should we handle that? In this part of the patch, there seems to be part of a sentence missing: + * Marshal the COPY data. Either subroutine will get the + * connection out of its COPY state, then + * once and report any error. Return whether all was ok.
Re: psql - add SHOW_ALL_RESULTS option
Are you planning to send a rebased patch for this commit fest?
Argh, I did it in a reply in another thread:-( Attached v15.
So as to help moves things forward, I'd suggest that we should not to care
too much about corner case repetition of some error messages which are due to
libpq internals, so I could remove the ugly buffer reset from the patch and
have the repetition, and if/when the issue is fixed later in libpq then the
repetition will be removed, fine! The issue is that we just expose the
strange behavior of libpq, which is libpq to solve, not psql.
See attached v16 which removes the libpq workaround.
--
Fabien.diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out
index e0abe34bb6..8f7f93172a 100644
--- a/contrib/pg_stat_statements/expected/pg_stat_statements.out
+++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out
@@ -50,8 +50,28 @@ BEGIN \;
SELECT 2.0 AS "float" \;
SELECT 'world' AS "text" \;
COMMIT;
+ float
+---
+ 2.0
+(1 row)
+
+ text
+---
+ world
+(1 row)
+
-- compound with empty statements and spurious leading spacing
\;\; SELECT 3 + 3 \;\;\; SELECT ' ' || ' !' \;\; SELECT 1 + 4 \;;
+ ?column?
+--
+6
+(1 row)
+
+ ?column?
+--
+ !
+(1 row)
+
?column?
--
5
@@ -61,6 +81,11 @@ COMMIT;
SELECT 1 + 1 + 1 AS "add" \gset
SELECT :add + 1 + 1 AS "add" \;
SELECT :add + 1 + 1 AS "add" \gset
+ add
+-
+ 5
+(1 row)
+
-- set operator
SELECT 1 AS i UNION SELECT 2 ORDER BY i;
i
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index caabb06c53..f01adb1fd2 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -127,18 +127,11 @@ echo '\x \\ SELECT * FROM foo;' | psql
commands included in the string to divide it into multiple
transactions. (See
for more details about how the server handles multi-query strings.)
- Also, psql only prints the
- result of the last SQL command in the string.
- This is different from the behavior when the same string is read from
- a file or fed to psql's standard input,
- because then psql sends
- each SQL command separately.
- Because of this behavior, putting more than one SQL command in a
- single -c string often has unexpected results.
- It's better to use repeated -c commands or feed
- multiple commands to psql's standard input,
+ If having several commands executed in one transaction is not desired,
+ use repeated -c commands or feed multiple commands to
+ psql's standard input,
either using echo as illustrated above, or
via a shell here-document, for example:
@@ -3570,10 +3563,6 @@ select 1\; select 2\; select 3;
commands included in the string to divide it into multiple
transactions. (See
for more details about how the server handles multi-query strings.)
-psql prints only the last query result
-it receives for each request; in this example, although all
-three SELECTs are indeed executed, psql
-only prints the 3.
@@ -4160,6 +4149,18 @@ bar
+SHOW_ALL_RESULTS
+
+
+When this variable is set to off, only the last
+result of a combined query (\;) is shown instead of
+all of them. The default is on. The off behavior
+is for compatibility with older versions of psql.
+
+
+
+
+
SHOW_CONTEXT
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index d65b9a124f..7903075975 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -34,6 +34,8 @@ static bool DescribeQuery(const char *query, double *elapsed_msec);
static bool ExecQueryUsingCursor(const char *query, double *elapsed_msec);
static bool command_no_begin(const char *query);
static bool is_select_command(const char *query);
+static int SendQueryAndProcessResults(const char *query, double *pelapsed_msec,
+ bool is_watch, const printQueryOpt *opt, FILE *printQueryFout, bool *tx_ended);
/*
@@ -354,7 +356,7 @@ CheckConnection(void)
* Returns true for valid result, false for error state.
*/
static bool
-AcceptResult(const PGresult *result)
+AcceptResult(const PGresult *result, bool show_error)
{
bool OK;
@@ -385,7 +387,7 @@ AcceptResult(const PGresult *result)
break;
}
- if (!OK)
+ if (!OK && show_error)
{
const char *error = PQerrorMessage(pset.db);
@@ -473,6 +475,18 @@ ClearOrSaveResult(PGresult *result)
}
}
+/*
+ * Consume all results
+ */
+static void
+ClearOrSaveAllResults()
+{
+ PGresult *result;
+
+ while ((result = PQgetResult(pset.db)) != NULL)
+ ClearOrSaveResult(result);
+}
+
/*
* Print microtiming output. Always print raw milliseconds; if the interval
@@ -573,7
Re: psql - add SHOW_ALL_RESULTS option
Are you planning to send a rebased patch for this commit fest?
Argh, I did it in a reply in another thread:-( Attached v15.
So as to help moves things forward, I'd suggest that we should not to care
too much about corner case repetition of some error messages which are due
to libpq internals, so I could remove the ugly buffer reset from the patch
and have the repetition, and if/when the issue is fixed later in libpq
then the repetition will be removed, fine! The issue is that we just
expose the strange behavior of libpq, which is libpq to solve, not psql.
What do you think?
--
Fabien.diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out
index e0abe34bb6..8f7f93172a 100644
--- a/contrib/pg_stat_statements/expected/pg_stat_statements.out
+++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out
@@ -50,8 +50,28 @@ BEGIN \;
SELECT 2.0 AS "float" \;
SELECT 'world' AS "text" \;
COMMIT;
+ float
+---
+ 2.0
+(1 row)
+
+ text
+---
+ world
+(1 row)
+
-- compound with empty statements and spurious leading spacing
\;\; SELECT 3 + 3 \;\;\; SELECT ' ' || ' !' \;\; SELECT 1 + 4 \;;
+ ?column?
+--
+6
+(1 row)
+
+ ?column?
+--
+ !
+(1 row)
+
?column?
--
5
@@ -61,6 +81,11 @@ COMMIT;
SELECT 1 + 1 + 1 AS "add" \gset
SELECT :add + 1 + 1 AS "add" \;
SELECT :add + 1 + 1 AS "add" \gset
+ add
+-
+ 5
+(1 row)
+
-- set operator
SELECT 1 AS i UNION SELECT 2 ORDER BY i;
i
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index caabb06c53..f01adb1fd2 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -127,18 +127,11 @@ echo '\x \\ SELECT * FROM foo;' | psql
commands included in the string to divide it into multiple
transactions. (See
for more details about how the server handles multi-query strings.)
- Also, psql only prints the
- result of the last SQL command in the string.
- This is different from the behavior when the same string is read from
- a file or fed to psql's standard input,
- because then psql sends
- each SQL command separately.
- Because of this behavior, putting more than one SQL command in a
- single -c string often has unexpected results.
- It's better to use repeated -c commands or feed
- multiple commands to psql's standard input,
+ If having several commands executed in one transaction is not desired,
+ use repeated -c commands or feed multiple commands to
+ psql's standard input,
either using echo as illustrated above, or
via a shell here-document, for example:
@@ -3570,10 +3563,6 @@ select 1\; select 2\; select 3;
commands included in the string to divide it into multiple
transactions. (See
for more details about how the server handles multi-query strings.)
-psql prints only the last query result
-it receives for each request; in this example, although all
-three SELECTs are indeed executed, psql
-only prints the 3.
@@ -4160,6 +4149,18 @@ bar
+SHOW_ALL_RESULTS
+
+
+When this variable is set to off, only the last
+result of a combined query (\;) is shown instead of
+all of them. The default is on. The off behavior
+is for compatibility with older versions of psql.
+
+
+
+
+
SHOW_CONTEXT
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index d65b9a124f..3b2f6305b4 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -34,6 +34,8 @@ static bool DescribeQuery(const char *query, double *elapsed_msec);
static bool ExecQueryUsingCursor(const char *query, double *elapsed_msec);
static bool command_no_begin(const char *query);
static bool is_select_command(const char *query);
+static int SendQueryAndProcessResults(const char *query, double *pelapsed_msec,
+ bool is_watch, const printQueryOpt *opt, FILE *printQueryFout, bool *tx_ended);
/*
@@ -354,7 +356,7 @@ CheckConnection(void)
* Returns true for valid result, false for error state.
*/
static bool
-AcceptResult(const PGresult *result)
+AcceptResult(const PGresult *result, bool show_error)
{
bool OK;
@@ -385,7 +387,7 @@ AcceptResult(const PGresult *result)
break;
}
- if (!OK)
+ if (!OK && show_error)
{
const char *error = PQerrorMessage(pset.db);
@@ -473,6 +475,18 @@ ClearOrSaveResult(PGresult *result)
}
}
+/*
+ * Consume all results
+ */
+static void
+ClearOrSaveAllResults()
+{
+ PGresult *result;
+
+ while ((result = PQgetResult(pset.db)) != NULL)
+ ClearOrSaveResult(result);
+}
+
/*
* Print microtiming output. Always print raw milliseconds; if the interval
@@ -573,7 +587,7 @@ PSQLexec(const char
Re: psql - add SHOW_ALL_RESULTS option
On 15.01.22 10:00, Fabien COELHO wrote: The reason this test constantly fails on cfbot windows is a use-after-free bug. Indeed! Thanks a lot for the catch and the debug! The ClearOrSaveResult function is quite annoying because it may or may not clear the result as a side effect. Attached v14 moves the status extraction before the possible clear. I've added a couple of results = NULL after such calls in the code. Are you planning to send a rebased patch for this commit fest?
Re: psql - add SHOW_ALL_RESULTS option
I wrote a few more small tests for psql to address uncovered territory
in SendQuery() especially:
- \timing
- client encoding handling
- notifications
What's still missing is:
- \watch
- pagers
For \watch, I think one would need something like the current cancel
test (since you need to get the psql pid to send a signal to stop the
watch). It would work in principle, but it will require more work to
refactor the cancel test.
For pagers, I don't know. I would be pretty easy to write a simple
script that acts as a pass-through pager and check that it is called.
There were some discussions earlier in the thread that some version of
some patch had broken some use of pagers. Does anyone remember details?
Anything worth testing specifically?From 6e75c1bec73f2128b94131305e6a37b97257f7c3 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Tue, 22 Feb 2022 13:42:38 +0100
Subject: [PATCH 1/2] Improve some psql test code
Split psql_like() into two functions psql_like() and psql_fails_like()
and make them mirror the existing command_like() and
command_fails_like() more closely. In particular, follow the
universal convention that the test name is the last argument.
---
src/bin/psql/t/001_basic.pl | 59 ++---
1 file changed, 29 insertions(+), 30 deletions(-)
diff --git a/src/bin/psql/t/001_basic.pl b/src/bin/psql/t/001_basic.pl
index ba3dd846ba..f416e0ab5e 100644
--- a/src/bin/psql/t/001_basic.pl
+++ b/src/bin/psql/t/001_basic.pl
@@ -12,40 +12,36 @@
program_version_ok('psql');
program_options_handling_ok('psql');
-my ($stdout, $stderr);
-my $result;
-
-# Execute a psql command and check its result patterns.
+# Execute a psql command and check its output.
sub psql_like
{
local $Test::Builder::Level = $Test::Builder::Level + 1;
- my $node= shift;
- my $test_name = shift;
- my $query = shift;
- my $expected_stdout = shift;
- my $expected_stderr = shift;
+ my ($node, $sql, $expected_stdout, $test_name) = @_;
+
+ my ($ret, $stdout, $stderr) = $node->psql('postgres', $sql);
+
+ is($ret,0, "$test_name: exit code 0");
+ is($stderr, '', "$test_name: no stderr");
+ like($stdout, $expected_stdout, "$test_name: matches");
+
+ return;
+}
+
+# Execute a psql command and check that it fails and check the stderr.
+sub psql_fails_like
+{
+ local $Test::Builder::Level = $Test::Builder::Level + 1;
- die "cannot specify both expected stdout and stderr here"
- if (defined($expected_stdout) && defined($expected_stderr));
+ my ($node, $sql, $expected_stderr, $test_name) = @_;
# Use the context of a WAL sender, some of the tests rely on that.
my ($ret, $stdout, $stderr) = $node->psql(
- 'postgres', $query,
- on_error_die => 0,
+ 'postgres', $sql,
replication => 'database');
- if (defined($expected_stdout))
- {
- is($ret,0, "$test_name: expected result code");
- is($stderr, '', "$test_name: no stderr");
- like($stdout, $expected_stdout, "$test_name: stdout matches");
- }
- if (defined($expected_stderr))
- {
- isnt($ret, 0, "$test_name: expected result code");
- like($stderr, $expected_stderr, "$test_name: stderr matches");
- }
+ isnt($ret, 0, "$test_name: exit code not 0");
+ like($stderr, $expected_stderr, "$test_name: matches");
return;
}
@@ -53,6 +49,9 @@ sub psql_like
# test --help=foo, analogous to program_help_ok()
foreach my $arg (qw(commands variables))
{
+ my ($stdout, $stderr);
+ my $result;
+
$result = IPC::Run::run [ 'psql', "--help=$arg" ], '>', \$stdout, '2>',
\$stderr;
ok($result, "psql --help=$arg exit code 0");
@@ -70,15 +69,15 @@ sub psql_like
});
$node->start;
-psql_like($node, '\copyright', '\copyright', qr/Copyright/, undef);
-psql_like($node, '\help without arguments', '\help', qr/ALTER/, undef);
-psql_like($node, '\help with argument', '\help SELECT', qr/SELECT/, undef);
+psql_like($node, '\copyright', qr/Copyright/, '\copyright');
+psql_like($node, '\help', qr/ALTER/, '\help without arguments');
+psql_like($node, '\help SELECT', qr/SELECT/, '\help with argument');
# Test clean handling of unsupported replication command responses
-psql_like(
+psql_fails_like(
$node,
- 'handling of unexpected PQresultStatus',
'START_REPLICATION 0/0',
- undef, qr/unexpected PQresultStatus: 8$/);
+ qr/unexpected PQresultStatus: 8$/,
+ 'handling of unexpected PQresultStatus');
done_testing();
--
2.35.1
From 189e977e47c505230195551833e0a61ec71dced3 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Tue, 22 Feb 2022 14:20:05 +0100
Subject: [PATCH 2/2] psql: Additional tests
Add a few TAP tests for things that happen while a user query is
Re: psql - add SHOW_ALL_RESULTS option
On 29.01.22 15:40, Fabien COELHO wrote: With this approach results are not available till the last one has been returned? If so, it loses the nice asynchronous property of getting results as they come when they come? This might or might not be desirable depending on the use case. For "psql", ISTM that we should want immediate and asynchronous anyway?? Well, I'm not sure. I'm thinking about this in terms of the dynamic result sets from stored procedures feature. That is typically used for small result sets. The interesting feature there is that the result sets can have different shapes. But of course people can use it differently. What is your motivation for this feature, and what is your experience how people would use it?
Re: psql - add SHOW_ALL_RESULTS option
Hello Peter, It would be better to do without. Also, it makes one wonder how others are supposed to use this multiple-results API properly, if even psql can't do it without extending libpq. Needs more thought. Fine with me! Obviously I'm okay if libpq is repaired instead of writing strange code on the client to deal with strange behavior. I have a new thought on this, as long as we are looking into libpq. Why can't libpq provide a variant of PQexec() that returns all results, instead of just the last one. It has all the information, all it has to do is return the results instead of throwing them away. Then the changes in psql would be very limited, and we don't have to re-invent PQexec() from its pieces in psql. And this would also make it easier for other clients and user code to make use of this functionality more easily. Attached is a rough draft of what this could look like. It basically works. Thoughts? My 0.02€: With this approach results are not available till the last one has been returned? If so, it loses the nice asynchronous property of getting results as they come when they come? This might or might not be desirable depending on the use case. For "psql", ISTM that we should want immediate and asynchronous anyway?? I'm unclear about what happens wrt to client-side data buffering if several large results are returned? COPY?? Also, I guess the user must free the returned array on top of closing all results? -- Fabien.
Re: psql - add SHOW_ALL_RESULTS option
On 23.01.22 18:17, Fabien COELHO wrote:
But I think if we are adding a libpq API function to work around a
misbehavior in libpq, we might as well fix the misbehavior in libpq to
begin with. Adding a new public libpq function is a significant step,
needs documentation, etc.
I'm not so sure.
The choice is (1) change the behavior of an existing function or (2) add
a new function. Whatever the existing function does, the usual anwer to
API changes is "someone is going to complain because it breaks their
code", so "Returned with feedback", hence I did not even try. The
advantage of (2) is that it does not harm anyone to have a new function
that they just do not need to use.
It would be better to do without. Also, it makes one wonder how
others are supposed to use this multiple-results API properly, if even
psql can't do it without extending libpq. Needs more thought.
Fine with me! Obviously I'm okay if libpq is repaired instead of writing
strange code on the client to deal with strange behavior.
I have a new thought on this, as long as we are looking into libpq. Why
can't libpq provide a variant of PQexec() that returns all results,
instead of just the last one. It has all the information, all it has to
do is return the results instead of throwing them away. Then the
changes in psql would be very limited, and we don't have to re-invent
PQexec() from its pieces in psql. And this would also make it easier
for other clients and user code to make use of this functionality more
easily.
Attached is a rough draft of what this could look like. It basically
works. Thoughts?From 947d9d98507d6c93b547ac17fef41c1870c0d577 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Thu, 27 Jan 2022 14:09:52 +0100
Subject: [PATCH] WIP: PQexecMulti
---
src/bin/psql/common.c| 44 +++-
src/interfaces/libpq/exports.txt | 1 +
src/interfaces/libpq/fe-exec.c | 37 +++
src/interfaces/libpq/libpq-fe.h | 1 +
4 files changed, 65 insertions(+), 18 deletions(-)
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 3503605a7d..deaaa54f10 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -1195,7 +1195,6 @@ bool
SendQuery(const char *query)
{
booltiming = pset.timing;
- PGresult *results;
PGTransactionStatusType transaction_status;
double elapsed_msec = 0;
boolOK = false;
@@ -1247,15 +1246,17 @@ SendQuery(const char *query)
!pset.autocommit &&
!command_no_begin(query))
{
- results = PQexec(pset.db, "BEGIN");
- if (PQresultStatus(results) != PGRES_COMMAND_OK)
+ PGresult *result;
+
+ result = PQexec(pset.db, "BEGIN");
+ if (PQresultStatus(result) != PGRES_COMMAND_OK)
{
pg_log_info("%s", PQerrorMessage(pset.db));
- ClearOrSaveResult(results);
+ ClearOrSaveResult(result);
ResetCancelConn();
goto sendquery_cleanup;
}
- ClearOrSaveResult(results);
+ ClearOrSaveResult(result);
transaction_status = PQtransactionStatus(pset.db);
}
@@ -1264,15 +1265,17 @@ SendQuery(const char *query)
(pset.cur_cmd_interactive ||
pset.on_error_rollback == PSQL_ERROR_ROLLBACK_ON))
{
- results = PQexec(pset.db, "SAVEPOINT
pg_psql_temporary_savepoint");
- if (PQresultStatus(results) != PGRES_COMMAND_OK)
+ PGresult *result;
+
+ result = PQexec(pset.db, "SAVEPOINT
pg_psql_temporary_savepoint");
+ if (PQresultStatus(result) != PGRES_COMMAND_OK)
{
pg_log_info("%s", PQerrorMessage(pset.db));
- ClearOrSaveResult(results);
+ ClearOrSaveResult(result);
ResetCancelConn();
goto sendquery_cleanup;
}
- ClearOrSaveResult(results);
+ ClearOrSaveResult(result);
on_error_rollback_savepoint = true;
}
@@ -1281,7 +1284,6 @@ SendQuery(const char *query)
/* Describe query's result columns, without executing it */
OK = DescribeQuery(query, _msec);
ResetCancelConn();
- results = NULL; /* PQclear(NULL) does nothing */
}
else if (pset.fetch_count <= 0 || pset.gexec_flag ||
pset.crosstab_flag || !is_select_command(query))
@@ -1289,15 +1291,19 @@ SendQuery(const char *query)
/* Default fetch-it-all-and-print mode */
instr_time before,
after;
+ PGresult **results;
+
Re: psql - add SHOW_ALL_RESULTS option
Hallo Peter, Attached v14 moves the status extraction before the possible clear. I've added a couple of results = NULL after such calls in the code. In the psql.sql test file, the test I previously added concluded with \set ECHO none, which was a mistake that I have now fixed. As a result, the tests that you added after that point didn't show their input lines, which was weird and not intentional. So the tests will now show a different output. Ok. I notice that this patch has recently gained a new libpq function. I gather that this is to work around the misbehaviors in libpq that we have discussed. Indeed. But I think if we are adding a libpq API function to work around a misbehavior in libpq, we might as well fix the misbehavior in libpq to begin with. Adding a new public libpq function is a significant step, needs documentation, etc. I'm not so sure. The choice is (1) change the behavior of an existing function or (2) add a new function. Whatever the existing function does, the usual anwer to API changes is "someone is going to complain because it breaks their code", so "Returned with feedback", hence I did not even try. The advantage of (2) is that it does not harm anyone to have a new function that they just do not need to use. It would be better to do without. Also, it makes one wonder how others are supposed to use this multiple-results API properly, if even psql can't do it without extending libpq. Needs more thought. Fine with me! Obviously I'm okay if libpq is repaired instead of writing strange code on the client to deal with strange behavior. -- Fabien.
Re: psql - add SHOW_ALL_RESULTS option
On 15.01.22 10:00, Fabien COELHO wrote: The reason this test constantly fails on cfbot windows is a use-after-free bug. Indeed! Thanks a lot for the catch and the debug! The ClearOrSaveResult function is quite annoying because it may or may not clear the result as a side effect. Attached v14 moves the status extraction before the possible clear. I've added a couple of results = NULL after such calls in the code. In the psql.sql test file, the test I previously added concluded with \set ECHO none, which was a mistake that I have now fixed. As a result, the tests that you added after that point didn't show their input lines, which was weird and not intentional. So the tests will now show a different output. I notice that this patch has recently gained a new libpq function. I gather that this is to work around the misbehaviors in libpq that we have discussed. But I think if we are adding a libpq API function to work around a misbehavior in libpq, we might as well fix the misbehavior in libpq to begin with. Adding a new public libpq function is a significant step, needs documentation, etc. It would be better to do without. Also, it makes one wonder how others are supposed to use this multiple-results API properly, if even psql can't do it without extending libpq. Needs more thought.
Re: psql - add SHOW_ALL_RESULTS option
Hello Andres,
The reason this test constantly fails on cfbot windows is a use-after-free
bug.
Indeed! Thanks a lot for the catch and the debug!
The ClearOrSaveResult function is quite annoying because it may or may not
clear the result as a side effect.
Attached v14 moves the status extraction before the possible clear. I've
added a couple of results = NULL after such calls in the code.
--
Fabien.diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out
index e0abe34bb6..8f7f93172a 100644
--- a/contrib/pg_stat_statements/expected/pg_stat_statements.out
+++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out
@@ -50,8 +50,28 @@ BEGIN \;
SELECT 2.0 AS "float" \;
SELECT 'world' AS "text" \;
COMMIT;
+ float
+---
+ 2.0
+(1 row)
+
+ text
+---
+ world
+(1 row)
+
-- compound with empty statements and spurious leading spacing
\;\; SELECT 3 + 3 \;\;\; SELECT ' ' || ' !' \;\; SELECT 1 + 4 \;;
+ ?column?
+--
+6
+(1 row)
+
+ ?column?
+--
+ !
+(1 row)
+
?column?
--
5
@@ -61,6 +81,11 @@ COMMIT;
SELECT 1 + 1 + 1 AS "add" \gset
SELECT :add + 1 + 1 AS "add" \;
SELECT :add + 1 + 1 AS "add" \gset
+ add
+-
+ 5
+(1 row)
+
-- set operator
SELECT 1 AS i UNION SELECT 2 ORDER BY i;
i
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 1ab200a4ad..0a22850912 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -127,18 +127,11 @@ echo '\x \\ SELECT * FROM foo;' | psql
commands included in the string to divide it into multiple
transactions. (See
for more details about how the server handles multi-query strings.)
- Also, psql only prints the
- result of the last SQL command in the string.
- This is different from the behavior when the same string is read from
- a file or fed to psql's standard input,
- because then psql sends
- each SQL command separately.
- Because of this behavior, putting more than one SQL command in a
- single -c string often has unexpected results.
- It's better to use repeated -c commands or feed
- multiple commands to psql's standard input,
+ If having several commands executed in one transaction is not desired,
+ use repeated -c commands or feed multiple commands to
+ psql's standard input,
either using echo as illustrated above, or
via a shell here-document, for example:
@@ -3570,10 +3563,6 @@ select 1\; select 2\; select 3;
commands included in the string to divide it into multiple
transactions. (See
for more details about how the server handles multi-query strings.)
-psql prints only the last query result
-it receives for each request; in this example, although all
-three SELECTs are indeed executed, psql
-only prints the 3.
@@ -4160,6 +4149,18 @@ bar
+SHOW_ALL_RESULTS
+
+
+When this variable is set to off, only the last
+result of a combined query (\;) is shown instead of
+all of them. The default is on. The off behavior
+is for compatibility with older versions of psql.
+
+
+
+
+
SHOW_CONTEXT
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 3503605a7d..47eabcbb8e 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -34,6 +34,8 @@ static bool DescribeQuery(const char *query, double *elapsed_msec);
static bool ExecQueryUsingCursor(const char *query, double *elapsed_msec);
static bool command_no_begin(const char *query);
static bool is_select_command(const char *query);
+static int SendQueryAndProcessResults(const char *query, double *pelapsed_msec,
+ bool is_watch, const printQueryOpt *opt, FILE *printQueryFout, bool *tx_ended);
/*
@@ -354,7 +356,7 @@ CheckConnection(void)
* Returns true for valid result, false for error state.
*/
static bool
-AcceptResult(const PGresult *result)
+AcceptResult(const PGresult *result, bool show_error)
{
bool OK;
@@ -385,7 +387,7 @@ AcceptResult(const PGresult *result)
break;
}
- if (!OK)
+ if (!OK && show_error)
{
const char *error = PQerrorMessage(pset.db);
@@ -473,6 +475,18 @@ ClearOrSaveResult(PGresult *result)
}
}
+/*
+ * Consume all results
+ */
+static void
+ClearOrSaveAllResults()
+{
+ PGresult *result;
+
+ while ((result = PQgetResult(pset.db)) != NULL)
+ ClearOrSaveResult(result);
+}
+
/*
* Print microtiming output. Always print raw milliseconds; if the interval
@@ -573,7 +587,7 @@ PSQLexec(const char *query)
ResetCancelConn();
- if (!AcceptResult(res))
+ if (!AcceptResult(res, true))
{
ClearOrSaveResult(res);
res = NULL;
@@ -596,11 +610,8 @@ int
PSQLexecWatch(const char *query,
Re: psql - add SHOW_ALL_RESULTS option
Hi,
On 2022-01-08 19:32:36 +0100, Fabien COELHO wrote:
> Attached v13 where the crash test is moved to tap.
The reason this test constantly fails on cfbot windows is a use-after-free
bug.
I figured that out in the context of another thread, so the debugging is
there:
https://postgr.es/m/20220113054123.ib4khtafgq34lv4z%40alap3.anarazel.de
> Ah, I see the bug. It's a use-after-free introduced in the patch:
>
> SendQueryAndProcessResults(const char *query, double *pelapsed_msec,
> bool is_watch, const printQueryOpt *opt, FILE *printQueryFout, bool
> *tx_ended)
>
>
> ...
> /* first result */
> result = PQgetResult(pset.db);
>
>
> while (result != NULL)
>
>
> ...
> if (!AcceptResult(result, false))
> {
> ...
> ClearOrSaveResult(result);
> success = false;
>
>
> /* and switch to next result */
> result_status = PQresultStatus(result);
> if (result_status == PGRES_COPY_BOTH ||
> result_status == PGRES_COPY_OUT ||
> result_status == PGRES_COPY_IN)
>
>
> So we called ClearOrSaveResult() with did a PQclear(), and then we go and call
> PQresultStatus().
Greetings,
Andres Freund
Re: psql - add SHOW_ALL_RESULTS option
Hello Peter,
quite weird and confusing. Maybe this type of test should be done in
the TAP framework instead.
Attached v13 where the crash test is moved to tap.
--
Fabien.diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out
index e0abe34bb6..8f7f93172a 100644
--- a/contrib/pg_stat_statements/expected/pg_stat_statements.out
+++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out
@@ -50,8 +50,28 @@ BEGIN \;
SELECT 2.0 AS "float" \;
SELECT 'world' AS "text" \;
COMMIT;
+ float
+---
+ 2.0
+(1 row)
+
+ text
+---
+ world
+(1 row)
+
-- compound with empty statements and spurious leading spacing
\;\; SELECT 3 + 3 \;\;\; SELECT ' ' || ' !' \;\; SELECT 1 + 4 \;;
+ ?column?
+--
+6
+(1 row)
+
+ ?column?
+--
+ !
+(1 row)
+
?column?
--
5
@@ -61,6 +81,11 @@ COMMIT;
SELECT 1 + 1 + 1 AS "add" \gset
SELECT :add + 1 + 1 AS "add" \;
SELECT :add + 1 + 1 AS "add" \gset
+ add
+-
+ 5
+(1 row)
+
-- set operator
SELECT 1 AS i UNION SELECT 2 ORDER BY i;
i
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 1ab200a4ad..0a22850912 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -127,18 +127,11 @@ echo '\x \\ SELECT * FROM foo;' | psql
commands included in the string to divide it into multiple
transactions. (See
for more details about how the server handles multi-query strings.)
- Also, psql only prints the
- result of the last SQL command in the string.
- This is different from the behavior when the same string is read from
- a file or fed to psql's standard input,
- because then psql sends
- each SQL command separately.
- Because of this behavior, putting more than one SQL command in a
- single -c string often has unexpected results.
- It's better to use repeated -c commands or feed
- multiple commands to psql's standard input,
+ If having several commands executed in one transaction is not desired,
+ use repeated -c commands or feed multiple commands to
+ psql's standard input,
either using echo as illustrated above, or
via a shell here-document, for example:
@@ -3570,10 +3563,6 @@ select 1\; select 2\; select 3;
commands included in the string to divide it into multiple
transactions. (See
for more details about how the server handles multi-query strings.)
-psql prints only the last query result
-it receives for each request; in this example, although all
-three SELECTs are indeed executed, psql
-only prints the 3.
@@ -4160,6 +4149,18 @@ bar
+SHOW_ALL_RESULTS
+
+
+When this variable is set to off, only the last
+result of a combined query (\;) is shown instead of
+all of them. The default is on. The off behavior
+is for compatibility with older versions of psql.
+
+
+
+
+
SHOW_CONTEXT
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index f210ccbde8..b8e8c2b245 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -33,6 +33,8 @@ static bool DescribeQuery(const char *query, double *elapsed_msec);
static bool ExecQueryUsingCursor(const char *query, double *elapsed_msec);
static bool command_no_begin(const char *query);
static bool is_select_command(const char *query);
+static int SendQueryAndProcessResults(const char *query, double *pelapsed_msec,
+ bool is_watch, const printQueryOpt *opt, FILE *printQueryFout, bool *tx_ended);
/*
@@ -353,7 +355,7 @@ CheckConnection(void)
* Returns true for valid result, false for error state.
*/
static bool
-AcceptResult(const PGresult *result)
+AcceptResult(const PGresult *result, bool show_error)
{
bool OK;
@@ -384,7 +386,7 @@ AcceptResult(const PGresult *result)
break;
}
- if (!OK)
+ if (!OK && show_error)
{
const char *error = PQerrorMessage(pset.db);
@@ -472,6 +474,18 @@ ClearOrSaveResult(PGresult *result)
}
}
+/*
+ * Consume all results
+ */
+static void
+ClearOrSaveAllResults()
+{
+ PGresult *result;
+
+ while ((result = PQgetResult(pset.db)) != NULL)
+ ClearOrSaveResult(result);
+}
+
/*
* Print microtiming output. Always print raw milliseconds; if the interval
@@ -572,7 +586,7 @@ PSQLexec(const char *query)
ResetCancelConn();
- if (!AcceptResult(res))
+ if (!AcceptResult(res, true))
{
ClearOrSaveResult(res);
res = NULL;
@@ -595,11 +609,8 @@ int
PSQLexecWatch(const char *query, const printQueryOpt *opt, FILE *printQueryFout)
{
bool timing = pset.timing;
- PGresult *res;
double elapsed_msec = 0;
- instr_time before;
- instr_time after;
- FILE *fout;
+ int res;
if (!pset.db)
{
@@ -608,77
Re: psql - add SHOW_ALL_RESULTS option
Hello Peter, With this "voluntary crash", the regression test output is now psql ... ok (test process exited with exit code 2) 281 ms Normally, I'd expect this during development if there was a crash somewhere, but showing this during a normal run now, and moreover still saying "ok", Well, from a testing perspective, the crash is voluntary and it is indeed ok:-) is quite weird and confusing. Maybe this type of test should be done in the TAP framework instead. It could. Another simpler option: add a "psql_voluntary_crash.sql" with just that test instead of modifying the "psql.sql" test script? That would keep the test exit code information, but the name of the script would make things clearer? Also, if non zero status do not look so ok, should they be noted as bad? -- Fabien.
Re: psql - add SHOW_ALL_RESULTS option
On 23.12.21 12:40, Fabien COELHO wrote: This is also a feature/bug of libpq which happens to be hidden by PQexec: when one command crashes PQgetResult actually returns *2* results. First one with the FATAL message, second one when libpq figures out that the connection was lost with the second message appended to the first. PQexec just happen to silently ignore the first result. I added a manual reset of the error message when first shown so that it is not shown twice. It is unclear to me whether the reset should be somewhere in libpq instead. I added a voluntary crash at the end of the psql test. With this "voluntary crash", the regression test output is now psql ... ok (test process exited with exit code 2) 281 ms Normally, I'd expect this during development if there was a crash somewhere, but showing this during a normal run now, and moreover still saying "ok", is quite weird and confusing. Maybe this type of test should be done in the TAP framework instead.
Re: psql - add SHOW_ALL_RESULTS option
[...] I agree that these two behaviors in libpq are dubious, especially the second one. I want to spend some time analyzing this more and see if fixes in libpq might be appropriate. Ok. My analysis is that fixing libpq behavior is not in the scope of a psql patch, and that if I was to do that it was sure delay the patch even further. Also these issues/features are corner cases that provably very few people bumped into. -- Fabien.
Re: psql - add SHOW_ALL_RESULTS option
On 23.12.21 12:40, Fabien COELHO wrote: In [0], it was reported that certain replication commands result in infinite loops because of faulty error handling. This still happens. I wrote a test for it, attached here. (I threw in a few more basic tests, just to have some more coverage that was lacking, and to have a file to put the new test in.) Hmmm… For some unclear reason on errors on a PGRES_COPY_* state PQgetResult keeps on returning an empty result. PQexec manually ignores it, so I did the same with a comment, but for me the real bug is somehow in PQgetResult behavior… In [1], it was reported that server crashes produce duplicate error messages. This also still happens. I didn't write a test for it, maybe you have an idea. (Obviously, we could check whether the error message is literally there twice in the output, but that doesn't seem very general.) But it's easy to test manually: just have psql connect, shut down the server, then run a query. This is also a feature/bug of libpq which happens to be hidden by PQexec: when one command crashes PQgetResult actually returns *2* results. First one with the FATAL message, second one when libpq figures out that the connection was lost with the second message appended to the first. PQexec just happen to silently ignore the first result. I added a manual reset of the error message when first shown so that it is not shown twice. It is unclear to me whether the reset should be somewhere in libpq instead. I added a voluntary crash at the end of the psql test. I agree that these two behaviors in libpq are dubious, especially the second one. I want to spend some time analyzing this more and see if fixes in libpq might be appropriate.
Re: psql - add SHOW_ALL_RESULTS option
Hello Peter, I finally took some time to look at this. Attached v11 is a rebase. This patch still has a few of the problems reported earlier this year. In [0], it was reported that certain replication commands result in infinite loops because of faulty error handling. This still happens. I wrote a test for it, attached here. (I threw in a few more basic tests, just to have some more coverage that was lacking, and to have a file to put the new test in.) Hmmm… For some unclear reason on errors on a PGRES_COPY_* state PQgetResult keeps on returning an empty result. PQexec manually ignores it, so I did the same with a comment, but for me the real bug is somehow in PQgetResult behavior… In [1], it was reported that server crashes produce duplicate error messages. This also still happens. I didn't write a test for it, maybe you have an idea. (Obviously, we could check whether the error message is literally there twice in the output, but that doesn't seem very general.) But it's easy to test manually: just have psql connect, shut down the server, then run a query. This is also a feature/bug of libpq which happens to be hidden by PQexec: when one command crashes PQgetResult actually returns *2* results. First one with the FATAL message, second one when libpq figures out that the connection was lost with the second message appended to the first. PQexec just happen to silently ignore the first result. I added a manual reset of the error message when first shown so that it is not shown twice. It is unclear to me whether the reset should be somewhere in libpq instead. I added a voluntary crash at the end of the psql test. Attached v12 somehow fixes these issues in "psql" code rather than in libpq. -- Fabien.diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out index e0abe34bb6..8f7f93172a 100644 --- a/contrib/pg_stat_statements/expected/pg_stat_statements.out +++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out @@ -50,8 +50,28 @@ BEGIN \; SELECT 2.0 AS "float" \; SELECT 'world' AS "text" \; COMMIT; + float +--- + 2.0 +(1 row) + + text +--- + world +(1 row) + -- compound with empty statements and spurious leading spacing \;\; SELECT 3 + 3 \;\;\; SELECT ' ' || ' !' \;\; SELECT 1 + 4 \;; + ?column? +-- +6 +(1 row) + + ?column? +-- + ! +(1 row) + ?column? -- 5 @@ -61,6 +81,11 @@ COMMIT; SELECT 1 + 1 + 1 AS "add" \gset SELECT :add + 1 + 1 AS "add" \; SELECT :add + 1 + 1 AS "add" \gset + add +- + 5 +(1 row) + -- set operator SELECT 1 AS i UNION SELECT 2 ORDER BY i; i diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index ae38d3dcc3..1d411ae124 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -127,18 +127,11 @@ echo '\x \\ SELECT * FROM foo;' | psql commands included in the string to divide it into multiple transactions. (See for more details about how the server handles multi-query strings.) - Also, psql only prints the - result of the last SQL command in the string. - This is different from the behavior when the same string is read from - a file or fed to psql's standard input, - because then psql sends - each SQL command separately. - Because of this behavior, putting more than one SQL command in a - single -c string often has unexpected results. - It's better to use repeated -c commands or feed - multiple commands to psql's standard input, + If having several commands executed in one transaction is not desired, + use repeated -c commands or feed multiple commands to + psql's standard input, either using echo as illustrated above, or via a shell here-document, for example: @@ -3564,10 +3557,6 @@ select 1\; select 2\; select 3; commands included in the string to divide it into multiple transactions. (See for more details about how the server handles multi-query strings.) -psql prints only the last query result -it receives for each request; in this example, although all -three SELECTs are indeed executed, psql -only prints the 3. @@ -4154,6 +4143,18 @@ bar +SHOW_ALL_RESULTS + + +When this variable is set to off, only the last +result of a combined query (\;) is shown instead of +all of them. The default is on. The off behavior +is for compatibility with older versions of psql. + + + + + SHOW_CONTEXT diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c index ec975c3e2a..e06699878b 100644 --- a/src/bin/psql/common.c +++ b/src/bin/psql/common.c @@ -33,6 +33,8 @@ static bool
Re: psql - add SHOW_ALL_RESULTS option
Hello Daniel, This patch still has a few of the problems reported earlier this year. The patch fails to apply and the thread seems to have taken a nap. I'm not napping:-) I just do not have enough time available this month. I intend to work on the patch in the next CF (January). AFAICR there is one necessary rebase and one bug to fix. -- Fabien.
Re: psql - add SHOW_ALL_RESULTS option
> On 8 Oct 2021, at 14:15, Peter Eisentraut > wrote: > > On 02.10.21 16:31, Fabien COELHO wrote: >>> Attached v9 integrates your tests and makes them work. >> Attached v11 is a rebase. > > This patch still has a few of the problems reported earlier this year. The patch fails to apply and the thread seems to have taken a nap. You mentioned on the "dynamic result sets support in extended query protocol" thread [0] that you were going to work on this as a pre-requisite for that patch. Is that still the plan so we should keep this in the Commitfest? -- Daniel Gustafsson https://vmware.com/ [0] https://www.postgresql.org/message-id/6f038f18-0f2b-5271-a56f-1770577f246c%40enterprisedb.com
Re: psql - add SHOW_ALL_RESULTS option
On 02.10.21 16:31, Fabien COELHO wrote:
Attached v9 integrates your tests and makes them work.
Attached v11 is a rebase.
This patch still has a few of the problems reported earlier this year.
In [0], it was reported that certain replication commands result in
infinite loops because of faulty error handling. This still happens. I
wrote a test for it, attached here. (I threw in a few more basic tests,
just to have some more coverage that was lacking, and to have a file to
put the new test in.)
In [1], it was reported that server crashes produce duplicate error
messages. This also still happens. I didn't write a test for it, maybe
you have an idea. (Obviously, we could check whether the error message
is literally there twice in the output, but that doesn't seem very
general.) But it's easy to test manually: just have psql connect, shut
down the server, then run a query.
Additionally, I looked into the Coverity issue reported in [2]. That
one is fixed, but I figured it would be good to be able to check your
patches with a static analyzer in a similar way. I don't have the
ability to run Coverity locally, so I looked at scan-build and fixed a
few minor warnings, also attached as a patch. Your current patch
appears to be okay in that regard.
[0]:
https://www.postgresql.org/message-id/69c0b369-570c-4524-8ee4-bccacecb6...@amazon.com
[1]: https://www.postgresql.org/message-id/2902362.1618244...@sss.pgh.pa.us
[2]: https://www.postgresql.org/message-id/2680034.1618157...@sss.pgh.pa.us
>From e8dbe137737f94a2eaff86dc1676f9df39c60d00 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Thu, 7 Oct 2021 22:12:40 +0200
Subject: [PATCH] psql: More tests
---
src/bin/psql/t/001_basic.pl | 42 +
1 file changed, 42 insertions(+)
create mode 100644 src/bin/psql/t/001_basic.pl
diff --git a/src/bin/psql/t/001_basic.pl b/src/bin/psql/t/001_basic.pl
new file mode 100644
index 00..cd899e851e
--- /dev/null
+++ b/src/bin/psql/t/001_basic.pl
@@ -0,0 +1,42 @@
+
+# Copyright (c) 2021, PostgreSQL Global Development Group
+
+use strict;
+use warnings;
+
+use PostgresNode;
+use TestLib;
+use Test::More tests => 25;
+
+program_help_ok('psql');
+program_version_ok('psql');
+program_options_handling_ok('psql');
+
+my ($stdout, $stderr);
+my $result;
+
+# test --help=foo, analogous to program_help_ok()
+foreach my $arg (qw(commands variables))
+{
+ $result = IPC::Run::run [ 'psql', "--help=$arg" ], '>', \$stdout, '2>',
\$stderr;
+ ok($result, "psql --help=$arg exit code 0");
+ isnt($stdout, '', "psql --help=$arg goes to stdout");
+ is($stderr, '', "psql --help=$arg nothing to stderr");
+}
+
+my $node = PostgresNode->new('main');
+$node->init;
+$node->append_conf(
+ 'postgresql.conf', q{
+wal_level = 'logical'
+max_replication_slots = 4
+max_wal_senders = 4
+});
+$node->start;
+
+$node->command_like([ 'psql', '-c', '\copyright' ], qr/Copyright/,
'\copyright');
+$node->command_like([ 'psql', '-c', '\help' ], qr/ALTER/, '\help without
arguments');
+$node->command_like([ 'psql', '-c', '\help SELECT' ], qr/SELECT/, '\help');
+
+$node->command_fails_like([ 'psql', 'replication=database', '-c',
'START_REPLICATION 123/456' ],
+ qr/^unexpected PQresultStatus: 8$/, 'handling of unexpected
PQresultStatus');
--
2.33.0
>From 96634e3dc5f775b73a4142e9a5d83190bd9aecbb Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Fri, 8 Oct 2021 13:30:15 +0200
Subject: [PATCH] psql: Fix scan-build warnings
---
src/bin/psql/common.c | 32 ++--
src/bin/psql/copy.c | 1 -
src/bin/psql/describe.c | 1 -
3 files changed, 18 insertions(+), 16 deletions(-)
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 5640786678..1b224bf9e4 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -594,6 +594,7 @@ PSQLexec(const char *query)
int
PSQLexecWatch(const char *query, const printQueryOpt *opt, FILE
*printQueryFout)
{
+ booltiming = pset.timing;
PGresult *res;
double elapsed_msec = 0;
instr_time before;
@@ -608,7 +609,7 @@ PSQLexecWatch(const char *query, const printQueryOpt *opt,
FILE *printQueryFout)
SetCancelConn(pset.db);
- if (pset.timing)
+ if (timing)
INSTR_TIME_SET_CURRENT(before);
res = PQexec(pset.db, query);
@@ -621,7 +622,7 @@ PSQLexecWatch(const char *query, const printQueryOpt *opt,
FILE *printQueryFout)
return 0;
}
- if (pset.timing)
+ if (timing)
{
INSTR_TIME_SET_CURRENT(after);
INSTR_TIME_SUBTRACT(after, before);
@@ -674,7 +675,7 @@ PSQLexecWatch(const char *query, const printQueryOpt *opt,
FILE *printQueryFout)
fflush(fout);
/* Possible microtiming output */
- if (pset.timing)
+ if (timing)
PrintTiming(elapsed_msec);
return
Re: psql - add SHOW_ALL_RESULTS option
Hello Peter,
Attached v9 integrates your tests and makes them work.
Attached v11 is a rebase.
--
Fabien.diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out
index b52d187722..0cf4a37a5f 100644
--- a/contrib/pg_stat_statements/expected/pg_stat_statements.out
+++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out
@@ -50,8 +50,28 @@ BEGIN \;
SELECT 2.0 AS "float" \;
SELECT 'world' AS "text" \;
COMMIT;
+ float
+---
+ 2.0
+(1 row)
+
+ text
+---
+ world
+(1 row)
+
-- compound with empty statements and spurious leading spacing
\;\; SELECT 3 + 3 \;\;\; SELECT ' ' || ' !' \;\; SELECT 1 + 4 \;;
+ ?column?
+--
+6
+(1 row)
+
+ ?column?
+--
+ !
+(1 row)
+
?column?
--
5
@@ -61,6 +81,11 @@ COMMIT;
SELECT 1 + 1 + 1 AS "add" \gset
SELECT :add + 1 + 1 AS "add" \;
SELECT :add + 1 + 1 AS "add" \gset
+ add
+-
+ 5
+(1 row)
+
-- set operator
SELECT 1 AS i UNION SELECT 2 ORDER BY i;
i
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 14e0a4dbe3..6866a06f2d 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -127,18 +127,11 @@ echo '\x \\ SELECT * FROM foo;' | psql
commands included in the string to divide it into multiple
transactions. (See
for more details about how the server handles multi-query strings.)
- Also, psql only prints the
- result of the last SQL command in the string.
- This is different from the behavior when the same string is read from
- a file or fed to psql's standard input,
- because then psql sends
- each SQL command separately.
- Because of this behavior, putting more than one SQL command in a
- single -c string often has unexpected results.
- It's better to use repeated -c commands or feed
- multiple commands to psql's standard input,
+ If having several commands executed in one transaction is not desired,
+ use repeated -c commands or feed multiple commands to
+ psql's standard input,
either using echo as illustrated above, or
via a shell here-document, for example:
@@ -3542,10 +3535,6 @@ select 1\; select 2\; select 3;
commands included in the string to divide it into multiple
transactions. (See
for more details about how the server handles multi-query strings.)
-psql prints only the last query result
-it receives for each request; in this example, although all
-three SELECTs are indeed executed, psql
-only prints the 3.
@@ -4132,6 +4121,18 @@ bar
+SHOW_ALL_RESULTS
+
+
+When this variable is set to off, only the last
+result of a combined query (\;) is shown instead of
+all of them. The default is on. The off behavior
+is for compatibility with older versions of psql.
+
+
+
+
+
SHOW_CONTEXT
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 5640786678..481a316fed 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -33,6 +33,8 @@ static bool DescribeQuery(const char *query, double *elapsed_msec);
static bool ExecQueryUsingCursor(const char *query, double *elapsed_msec);
static bool command_no_begin(const char *query);
static bool is_select_command(const char *query);
+static int SendQueryAndProcessResults(const char *query, double *pelapsed_msec,
+ bool is_watch, const printQueryOpt *opt, FILE *printQueryFout, bool *tx_ended);
/*
@@ -353,7 +355,7 @@ CheckConnection(void)
* Returns true for valid result, false for error state.
*/
static bool
-AcceptResult(const PGresult *result)
+AcceptResult(const PGresult *result, bool show_error)
{
bool OK;
@@ -384,7 +386,7 @@ AcceptResult(const PGresult *result)
break;
}
- if (!OK)
+ if (!OK && show_error)
{
const char *error = PQerrorMessage(pset.db);
@@ -472,6 +474,18 @@ ClearOrSaveResult(PGresult *result)
}
}
+/*
+ * Consume all results
+ */
+static void
+ClearOrSaveAllResults()
+{
+ PGresult *result;
+
+ while ((result = PQgetResult(pset.db)) != NULL)
+ ClearOrSaveResult(result);
+}
+
/*
* Print microtiming output. Always print raw milliseconds; if the interval
@@ -572,7 +586,7 @@ PSQLexec(const char *query)
ResetCancelConn();
- if (!AcceptResult(res))
+ if (!AcceptResult(res, true))
{
ClearOrSaveResult(res);
res = NULL;
@@ -594,11 +608,8 @@ PSQLexec(const char *query)
int
PSQLexecWatch(const char *query, const printQueryOpt *opt, FILE *printQueryFout)
{
- PGresult *res;
double elapsed_msec = 0;
- instr_time before;
- instr_time after;
- FILE *fout;
+ int res;
if (!pset.db)
{
@@ -607,77 +618,14 @@ PSQLexecWatch(const char *query, const printQueryOpt *opt,
Re: psql - add SHOW_ALL_RESULTS option
Hallo Peter,
It turns out that your v8 patch still has the issue complained about in [0].
The issue is that after COMMIT AND CHAIN, the internal savepoint is gone, but
the patched psql still thinks it should be there and tries to release it,
which leads to errors.
Indeed. Thanks for the catch.
Attached v9 integrates your tests and makes them work.
--
Fabien.diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out
index b52d187722..0cf4a37a5f 100644
--- a/contrib/pg_stat_statements/expected/pg_stat_statements.out
+++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out
@@ -50,8 +50,28 @@ BEGIN \;
SELECT 2.0 AS "float" \;
SELECT 'world' AS "text" \;
COMMIT;
+ float
+---
+ 2.0
+(1 row)
+
+ text
+---
+ world
+(1 row)
+
-- compound with empty statements and spurious leading spacing
\;\; SELECT 3 + 3 \;\;\; SELECT ' ' || ' !' \;\; SELECT 1 + 4 \;;
+ ?column?
+--
+6
+(1 row)
+
+ ?column?
+--
+ !
+(1 row)
+
?column?
--
5
@@ -61,6 +81,11 @@ COMMIT;
SELECT 1 + 1 + 1 AS "add" \gset
SELECT :add + 1 + 1 AS "add" \;
SELECT :add + 1 + 1 AS "add" \gset
+ add
+-
+ 5
+(1 row)
+
-- set operator
SELECT 1 AS i UNION SELECT 2 ORDER BY i;
i
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index fcab5c0d51..7c5504bc74 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -127,18 +127,11 @@ echo '\x \\ SELECT * FROM foo;' | psql
commands included in the string to divide it into multiple
transactions. (See
for more details about how the server handles multi-query strings.)
- Also, psql only prints the
- result of the last SQL command in the string.
- This is different from the behavior when the same string is read from
- a file or fed to psql's standard input,
- because then psql sends
- each SQL command separately.
- Because of this behavior, putting more than one SQL command in a
- single -c string often has unexpected results.
- It's better to use repeated -c commands or feed
- multiple commands to psql's standard input,
+ If having several commands executed in one transaction is not desired,
+ use repeated -c commands or feed multiple commands to
+ psql's standard input,
either using echo as illustrated above, or
via a shell here-document, for example:
@@ -3542,10 +3535,6 @@ select 1\; select 2\; select 3;
commands included in the string to divide it into multiple
transactions. (See
for more details about how the server handles multi-query strings.)
-psql prints only the last query result
-it receives for each request; in this example, although all
-three SELECTs are indeed executed, psql
-only prints the 3.
@@ -4132,6 +4121,18 @@ bar
+SHOW_ALL_RESULTS
+
+
+When this variable is set to off, only the last
+result of a combined query (\;) is shown instead of
+all of them. The default is on. The off behavior
+is for compatibility with older versions of psql.
+
+
+
+
+
SHOW_CONTEXT
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 5640786678..481a316fed 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -33,6 +33,8 @@ static bool DescribeQuery(const char *query, double *elapsed_msec);
static bool ExecQueryUsingCursor(const char *query, double *elapsed_msec);
static bool command_no_begin(const char *query);
static bool is_select_command(const char *query);
+static int SendQueryAndProcessResults(const char *query, double *pelapsed_msec,
+ bool is_watch, const printQueryOpt *opt, FILE *printQueryFout, bool *tx_ended);
/*
@@ -353,7 +355,7 @@ CheckConnection(void)
* Returns true for valid result, false for error state.
*/
static bool
-AcceptResult(const PGresult *result)
+AcceptResult(const PGresult *result, bool show_error)
{
bool OK;
@@ -384,7 +386,7 @@ AcceptResult(const PGresult *result)
break;
}
- if (!OK)
+ if (!OK && show_error)
{
const char *error = PQerrorMessage(pset.db);
@@ -472,6 +474,18 @@ ClearOrSaveResult(PGresult *result)
}
}
+/*
+ * Consume all results
+ */
+static void
+ClearOrSaveAllResults()
+{
+ PGresult *result;
+
+ while ((result = PQgetResult(pset.db)) != NULL)
+ ClearOrSaveResult(result);
+}
+
/*
* Print microtiming output. Always print raw milliseconds; if the interval
@@ -572,7 +586,7 @@ PSQLexec(const char *query)
ResetCancelConn();
- if (!AcceptResult(res))
+ if (!AcceptResult(res, true))
{
ClearOrSaveResult(res);
res = NULL;
@@ -594,11 +608,8 @@ PSQLexec(const char *query)
int
PSQLexecWatch(const char *query, const
Re: psql - add SHOW_ALL_RESULTS option
On 22.07.21 16:58, Fabien COELHO wrote:
Ok. I noticed. The patch got significantly broken by the watch pager
commit. I also have to enhance the added tests (per Peter request).
I wrote a test to check psql query cancel support. I checked that it
fails against the patch that was reverted. Maybe this is useful.
Here is the updated version (v8? I'm not sure what the right count is),
which works for me and for "make check", including some tests added for
uncovered paths.
I was looking at adding test coverage for the issue complained about in
[0]. That message said that the autocommit logic was broken, but
actually the issue was with the ON_ERROR_ROLLBACK logic. However, it
turned out that neither feature had any test coverage, and they are
easily testable using the pg_regress setup, so I wrote tests for both
and another little thing I found nearby.
It turns out that your v8 patch still has the issue complained about in
[0]. The issue is that after COMMIT AND CHAIN, the internal savepoint
is gone, but the patched psql still thinks it should be there and tries
to release it, which leads to errors.
[0]: https://www.postgresql.org/message-id/2671235.1618154...@sss.pgh.pa.us
>From c6042975c9cb802c5e6017c5f2452cea21beac1e Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Fri, 24 Sep 2021 14:27:35 +0200
Subject: [PATCH] psql: Add various tests
Add tests for psql features
- AUTOCOMMIT
- ON_ERROR_ROLLBACK
- ECHO errors
---
src/test/regress/expected/psql.out | 99 ++
src/test/regress/sql/psql.sql | 63 +++
2 files changed, 162 insertions(+)
diff --git a/src/test/regress/expected/psql.out
b/src/test/regress/expected/psql.out
index 1b2f6bc418..930ce8597a 100644
--- a/src/test/regress/expected/psql.out
+++ b/src/test/regress/expected/psql.out
@@ -5179,3 +5179,102 @@ List of access methods
pg_catalog | && | anyarray | anyarray | boolean | overlaps
(1 row)
+-- AUTOCOMMIT
+CREATE TABLE ac_test (a int);
+\set AUTOCOMMIT off
+INSERT INTO ac_test VALUES (1);
+COMMIT;
+SELECT * FROM ac_test;
+ a
+---
+ 1
+(1 row)
+
+COMMIT;
+INSERT INTO ac_test VALUES (2);
+ROLLBACK;
+SELECT * FROM ac_test;
+ a
+---
+ 1
+(1 row)
+
+COMMIT;
+BEGIN;
+INSERT INTO ac_test VALUES (3);
+COMMIT;
+SELECT * FROM ac_test;
+ a
+---
+ 1
+ 3
+(2 rows)
+
+COMMIT;
+BEGIN;
+INSERT INTO ac_test VALUES (4);
+ROLLBACK;
+SELECT * FROM ac_test;
+ a
+---
+ 1
+ 3
+(2 rows)
+
+COMMIT;
+\set AUTOCOMMIT on
+DROP TABLE ac_test;
+SELECT * FROM ac_test; -- should be gone now
+ERROR: relation "ac_test" does not exist
+LINE 1: SELECT * FROM ac_test;
+ ^
+-- ON_ERROR_ROLLBACK
+\set ON_ERROR_ROLLBACK on
+CREATE TABLE oer_test (a int);
+BEGIN;
+INSERT INTO oer_test VALUES (1);
+INSERT INTO oer_test VALUES ('foo');
+ERROR: invalid input syntax for type integer: "foo"
+LINE 1: INSERT INTO oer_test VALUES ('foo');
+ ^
+INSERT INTO oer_test VALUES (3);
+COMMIT;
+SELECT * FROM oer_test;
+ a
+---
+ 1
+ 3
+(2 rows)
+
+BEGIN;
+INSERT INTO oer_test VALUES (4);
+ROLLBACK;
+SELECT * FROM oer_test;
+ a
+---
+ 1
+ 3
+(2 rows)
+
+BEGIN;
+INSERT INTO oer_test VALUES (5);
+COMMIT AND CHAIN;
+INSERT INTO oer_test VALUES (6);
+COMMIT;
+SELECT * FROM oer_test;
+ a
+---
+ 1
+ 3
+ 5
+ 6
+(4 rows)
+
+DROP TABLE oer_test;
+\set ON_ERROR_ROLLBACK off
+-- ECHO errors
+\set ECHO errors
+ERROR: relation "notexists" does not exist
+LINE 1: SELECT * FROM notexists;
+ ^
+STATEMENT: SELECT * FROM notexists;
diff --git a/src/test/regress/sql/psql.sql b/src/test/regress/sql/psql.sql
index 68121d171c..e9d504baf2 100644
--- a/src/test/regress/sql/psql.sql
+++ b/src/test/regress/sql/psql.sql
@@ -1241,3 +1241,66 @@ CREATE MATERIALIZED VIEW mat_view_heap_psql USING
heap_psql AS SELECT f1 from tb
\dfa bit* small*
\do - pg_catalog.int4
\do && anyarray *
+
+-- AUTOCOMMIT
+
+CREATE TABLE ac_test (a int);
+\set AUTOCOMMIT off
+
+INSERT INTO ac_test VALUES (1);
+COMMIT;
+SELECT * FROM ac_test;
+COMMIT;
+
+INSERT INTO ac_test VALUES (2);
+ROLLBACK;
+SELECT * FROM ac_test;
+COMMIT;
+
+BEGIN;
+INSERT INTO ac_test VALUES (3);
+COMMIT;
+SELECT * FROM ac_test;
+COMMIT;
+
+BEGIN;
+INSERT INTO ac_test VALUES (4);
+ROLLBACK;
+SELECT * FROM ac_test;
+COMMIT;
+
+\set AUTOCOMMIT on
+DROP TABLE ac_test;
+SELECT * FROM ac_test; -- should be gone now
+
+-- ON_ERROR_ROLLBACK
+
+\set ON_ERROR_ROLLBACK on
+CREATE TABLE oer_test (a int);
+
+BEGIN;
+INSERT INTO oer_test VALUES (1);
+INSERT INTO oer_test VALUES ('foo');
+INSERT INTO oer_test VALUES (3);
+COMMIT;
+SELECT * FROM oer_test;
+
+BEGIN;
+INSERT INTO oer_test VALUES (4);
+ROLLBACK;
+SELECT * FROM oer_test;
+
+BEGIN;
+INSERT INTO oer_test VALUES (5);
+COMMIT AND CHAIN;
+INSERT INTO oer_test VALUES (6);
+COMMIT;
+SELECT * FROM oer_test;
+
+DROP TABLE oer_test;
+\set ON_ERROR_ROLLBACK off
+
+-- ECHO errors
+\set ECHO errors
+SELECT * FROM notexists;
+\set ECHO none
--
2.33.0
Re: psql - add SHOW_ALL_RESULTS option
On 22.07.21 16:58, Fabien COELHO wrote: Here is the updated version (v8? I'm not sure what the right count is), which works for me and for "make check", including some tests added for uncovered paths. I included your tap test (thanks again!) with some more comments and cleanup. The tap test had a merge conflict, so I fixed that and committed it separately. I was wondering about its portability, so it's good to sort that out separately from your main patch. There are already a few failures on the build farm right now, so let's see where this is heading.
Re: psql - add SHOW_ALL_RESULTS option
pá 23. 7. 2021 v 9:41 odesílatel Fabien COELHO napsal: > > >>> I tested manually for the pager feature, which mostly work, althoug > >>> "pspg --stream" does not seem to expect two tables, or maybe there is > >>> a way to switch between these that I have not found. > >> > >> pspg doesn't support this feature. > > Sure. Note that it is not a feature yet:-) > > ISTM that having some configurable pager-targetted marker would greatly > help parsing on the pager side, so this might be the way to go, if this > finally becomes a feature. > yes, It can help me lot of, and pspg can be less sensitive (or immune) against synchronization errors. Pavel > -- > Fabien. >
Re: psql - add SHOW_ALL_RESULTS option
I tested manually for the pager feature, which mostly work, althoug "pspg --stream" does not seem to expect two tables, or maybe there is a way to switch between these that I have not found. pspg doesn't support this feature. Sure. Note that it is not a feature yet:-) ISTM that having some configurable pager-targetted marker would greatly help parsing on the pager side, so this might be the way to go, if this finally becomes a feature. -- Fabien.
Re: psql - add SHOW_ALL_RESULTS option
čt 22. 7. 2021 v 17:23 odesílatel Pavel Stehule napsal: > > > čt 22. 7. 2021 v 16:58 odesílatel Fabien COELHO > napsal: > >> >> >> Ok. I noticed. The patch got significantly broken by the watch pager >> >> commit. I also have to enhance the added tests (per Peter request). >> > >> > I wrote a test to check psql query cancel support. I checked that it >> fails >> > against the patch that was reverted. Maybe this is useful. >> >> Here is the updated version (v8? I'm not sure what the right count is), >> which works for me and for "make check", including some tests added for >> uncovered paths. >> >> I included your tap test (thanks again!) with some more comments and >> cleanup. >> >> I tested manually for the pager feature, which mostly work, althoug >> "pspg --stream" does not seem to expect two tables, or maybe there is a >> way to switch between these that I have not found. >> > > pspg doesn't support this feature. Theoretically it can be implementable > (I am able to hold two datasets now), but without any help with > synchronization I don't want to implement any more complex parsing. On the > pspg side I am not able to detect what is the first result in the batch, > what is the last result (without some hard heuristics - probably I can read > some information from timestamps). And if you need two or more results in > one terminal, then mode without pager is better. > but the timestamps are localized, and again I have not enough information on the pspg side for correct parsing. So until psql will use some tags that allow more simple detection of start and end batch or relation, this feature will not be supported by pspg :-/. There are some invisible ascii codes that can be used for this purpose. > > >> -- >> Fabien. > >
Re: psql - add SHOW_ALL_RESULTS option
čt 22. 7. 2021 v 16:58 odesílatel Fabien COELHO napsal: > > >> Ok. I noticed. The patch got significantly broken by the watch pager > >> commit. I also have to enhance the added tests (per Peter request). > > > > I wrote a test to check psql query cancel support. I checked that it > fails > > against the patch that was reverted. Maybe this is useful. > > Here is the updated version (v8? I'm not sure what the right count is), > which works for me and for "make check", including some tests added for > uncovered paths. > > I included your tap test (thanks again!) with some more comments and > cleanup. > > I tested manually for the pager feature, which mostly work, althoug > "pspg --stream" does not seem to expect two tables, or maybe there is a > way to switch between these that I have not found. > pspg doesn't support this feature. Theoretically it can be implementable (I am able to hold two datasets now), but without any help with synchronization I don't want to implement any more complex parsing. On the pspg side I am not able to detect what is the first result in the batch, what is the last result (without some hard heuristics - probably I can read some information from timestamps). And if you need two or more results in one terminal, then mode without pager is better. > -- > Fabien.
Re: psql - add SHOW_ALL_RESULTS option
čt 22. 7. 2021 v 16:49 odesílatel Fabien COELHO napsal: > > Hello, > > > Minimally for PSQL_WATCH_PAGER, the pager should exit after some time, > but > > before it has to repeat data reading. Elsewhere the psql will hang. > > Sure. The "pager.pl" script I sent exits after reading a few lines. > > > can be solution to use special mode for psql, when psql will do write to > > logfile and redirect to file instead using any (simplified) pager? > > I do not want a special psql mode, I just would like "make check" to tell > me if I broke the PSQL_WATCH_PAGER feature after reworking the > multi-results patch. > > > Theoretically, there is nothing special on usage of pager, and just you > can > > test redirecting to file. > > I do not follow. For what I seen the watch pager feature is somehow a > little different, and I'd like to be sure I'm not breaking anything. > > For your information, pspg does not seem to like being fed two results > >sh> PSQL_WATCH_PAGER="pspg --stream" >psql> SELECT NOW() \; SELECT RANDOM() \watch 1 > > The first table is shown, the second seems ignored. > pspg cannot show multitable results, so it is not surprising. And I don't think about supporting this. Unfortunately I am not able to detect this situation and show some warnings, just because psql doesn't send enough data for it. Can be nice if psql sends some invisible characters, that allows synchronization. But there is nothing. I just detect the timestamp line and empty lines. > -- > Fabien. >
Re: psql - add SHOW_ALL_RESULTS option
Ok. I noticed. The patch got significantly broken by the watch pager
commit. I also have to enhance the added tests (per Peter request).
I wrote a test to check psql query cancel support. I checked that it fails
against the patch that was reverted. Maybe this is useful.
Here is the updated version (v8? I'm not sure what the right count is),
which works for me and for "make check", including some tests added for
uncovered paths.
I included your tap test (thanks again!) with some more comments and
cleanup.
I tested manually for the pager feature, which mostly work, althoug
"pspg --stream" does not seem to expect two tables, or maybe there is a
way to switch between these that I have not found.
--
Fabien.diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out
index 40b5109b55..ccce72fb85 100644
--- a/contrib/pg_stat_statements/expected/pg_stat_statements.out
+++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out
@@ -50,8 +50,28 @@ BEGIN \;
SELECT 2.0 AS "float" \;
SELECT 'world' AS "text" \;
COMMIT;
+ float
+---
+ 2.0
+(1 row)
+
+ text
+---
+ world
+(1 row)
+
-- compound with empty statements and spurious leading spacing
\;\; SELECT 3 + 3 \;\;\; SELECT ' ' || ' !' \;\; SELECT 1 + 4 \;;
+ ?column?
+--
+6
+(1 row)
+
+ ?column?
+--
+ !
+(1 row)
+
?column?
--
5
@@ -61,6 +81,11 @@ COMMIT;
SELECT 1 + 1 + 1 AS "add" \gset
SELECT :add + 1 + 1 AS "add" \;
SELECT :add + 1 + 1 AS "add" \gset
+ add
+-
+ 5
+(1 row)
+
-- set operator
SELECT 1 AS i UNION SELECT 2 ORDER BY i;
i
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index fcab5c0d51..7c5504bc74 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -127,18 +127,11 @@ echo '\x \\ SELECT * FROM foo;' | psql
commands included in the string to divide it into multiple
transactions. (See
for more details about how the server handles multi-query strings.)
- Also, psql only prints the
- result of the last SQL command in the string.
- This is different from the behavior when the same string is read from
- a file or fed to psql's standard input,
- because then psql sends
- each SQL command separately.
- Because of this behavior, putting more than one SQL command in a
- single -c string often has unexpected results.
- It's better to use repeated -c commands or feed
- multiple commands to psql's standard input,
+ If having several commands executed in one transaction is not desired,
+ use repeated -c commands or feed multiple commands to
+ psql's standard input,
either using echo as illustrated above, or
via a shell here-document, for example:
@@ -3542,10 +3535,6 @@ select 1\; select 2\; select 3;
commands included in the string to divide it into multiple
transactions. (See
for more details about how the server handles multi-query strings.)
-psql prints only the last query result
-it receives for each request; in this example, although all
-three SELECTs are indeed executed, psql
-only prints the 3.
@@ -4132,6 +4121,18 @@ bar
+SHOW_ALL_RESULTS
+
+
+When this variable is set to off, only the last
+result of a combined query (\;) is shown instead of
+all of them. The default is on. The off behavior
+is for compatibility with older versions of psql.
+
+
+
+
+
SHOW_CONTEXT
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 5640786678..564a4816de 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -33,6 +33,8 @@ static bool DescribeQuery(const char *query, double *elapsed_msec);
static bool ExecQueryUsingCursor(const char *query, double *elapsed_msec);
static bool command_no_begin(const char *query);
static bool is_select_command(const char *query);
+static int SendQueryAndProcessResults(const char *query, double *pelapsed_msec,
+ bool is_watch, const printQueryOpt *opt, FILE *printQueryFout);
/*
@@ -353,7 +355,7 @@ CheckConnection(void)
* Returns true for valid result, false for error state.
*/
static bool
-AcceptResult(const PGresult *result)
+AcceptResult(const PGresult *result, bool show_error)
{
bool OK;
@@ -384,7 +386,7 @@ AcceptResult(const PGresult *result)
break;
}
- if (!OK)
+ if (!OK && show_error)
{
const char *error = PQerrorMessage(pset.db);
@@ -472,6 +474,18 @@ ClearOrSaveResult(PGresult *result)
}
}
+/*
+ * Consume all results
+ */
+static void
+ClearOrSaveAllResults()
+{
+ PGresult *result;
+
+ while ((result = PQgetResult(pset.db)) != NULL)
+ ClearOrSaveResult(result);
+}
+
/*
* Print
Re: psql - add SHOW_ALL_RESULTS option
Hello, Minimally for PSQL_WATCH_PAGER, the pager should exit after some time, but before it has to repeat data reading. Elsewhere the psql will hang. Sure. The "pager.pl" script I sent exits after reading a few lines. can be solution to use special mode for psql, when psql will do write to logfile and redirect to file instead using any (simplified) pager? I do not want a special psql mode, I just would like "make check" to tell me if I broke the PSQL_WATCH_PAGER feature after reworking the multi-results patch. Theoretically, there is nothing special on usage of pager, and just you can test redirecting to file. I do not follow. For what I seen the watch pager feature is somehow a little different, and I'd like to be sure I'm not breaking anything. For your information, pspg does not seem to like being fed two results sh> PSQL_WATCH_PAGER="pspg --stream" psql> SELECT NOW() \; SELECT RANDOM() \watch 1 The first table is shown, the second seems ignored. -- Fabien.
Re: psql - add SHOW_ALL_RESULTS option
čt 22. 7. 2021 v 11:00 odesílatel Fabien COELHO napsal: > > Hello Pavel, > > >> The newly added PSQL_WATCH_PAGER feature which broke the patch does not > >> seem to be tested anywhere, this is tiring:-( > > > > Do you have any idea how this can be tested? > > The TAP patch sent by Peter on this thread is a very good start. > > > It requires some pager that doesn't use blocking reading, and you need > > to do remote control of this pager. So it requires a really especially > > written pager just for this purpose. It is solvable, but I am not sure > > if it is adequate to this patch. > > Not really: The point would not be to test the pager itself (that's for > the people who develop the pager, not for psql), but just to test that the > pager is actually started or not started by psql depending on conditions > (eg pset pager…) and that it does *something* when started. See for > instance the simplistic pager.pl script attached, the output of which > could be tested. Note that PSQL_PAGER is not tested at all either. > Basically "psql" is not tested, which is a pain when developing a non > trivial patch. > Minimally for PSQL_WATCH_PAGER, the pager should exit after some time, but before it has to repeat data reading. Elsewhere the psql will hang. can be solution to use special mode for psql, when psql will do write to logfile and redirect to file instead using any (simplified) pager? Theoretically, there is nothing special on usage of pager, and just you can test redirecting to file. That is not tested too. In this mode, you can send sigint to psql - and it can be emulation of sigint to pager in PSQL_WATCH_PAGER mode, > -- > Fabien.
Re: psql - add SHOW_ALL_RESULTS option
Hello Pavel, The newly added PSQL_WATCH_PAGER feature which broke the patch does not seem to be tested anywhere, this is tiring:-( Do you have any idea how this can be tested? The TAP patch sent by Peter on this thread is a very good start. It requires some pager that doesn't use blocking reading, and you need to do remote control of this pager. So it requires a really especially written pager just for this purpose. It is solvable, but I am not sure if it is adequate to this patch. Not really: The point would not be to test the pager itself (that's for the people who develop the pager, not for psql), but just to test that the pager is actually started or not started by psql depending on conditions (eg pset pager…) and that it does *something* when started. See for instance the simplistic pager.pl script attached, the output of which could be tested. Note that PSQL_PAGER is not tested at all either. Basically "psql" is not tested, which is a pain when developing a non trivial patch. -- Fabien.#! /usr/bin/perl -wn print "$.\t$_"; exit(0) if $. == 30;
Re: psql - add SHOW_ALL_RESULTS option
Hi čt 22. 7. 2021 v 7:52 odesílatel Fabien COELHO napsal: > > >>> The patch does not apply on Head anymore, could you rebase and post a > >>> patch. I'm changing the status to "Waiting for Author". > >> > >> Ok. I noticed. The patch got significantly broken by the watch pager > >> commit. I also have to enhance the added tests (per Peter request). > > > > I wrote a test to check psql query cancel support. I checked that it > fails > > against the patch that was reverted. Maybe this is useful. > > Thank you! The patch update is in progress… > > The newly added PSQL_WATCH_PAGER feature which broke the patch does not > seem to be tested anywhere, this is tiring:-( > Do you have any idea how this can be tested? It requires some pager that doesn't use blocking reading, and you need to do remote control of this pager. So it requires a really especially written pager just for this purpose. It is solvable, but I am not sure if it is adequate to this patch. Regards Pavel > -- > Fabien.
Re: psql - add SHOW_ALL_RESULTS option
The patch does not apply on Head anymore, could you rebase and post a patch. I'm changing the status to "Waiting for Author". Ok. I noticed. The patch got significantly broken by the watch pager commit. I also have to enhance the added tests (per Peter request). I wrote a test to check psql query cancel support. I checked that it fails against the patch that was reverted. Maybe this is useful. Thank you! The patch update is in progress… The newly added PSQL_WATCH_PAGER feature which broke the patch does not seem to be tested anywhere, this is tiring:-( -- Fabien.
Re: psql - add SHOW_ALL_RESULTS option
On 15.07.21 17:46, Fabien COELHO wrote:
The patch does not apply on Head anymore, could you rebase and post a
patch. I'm changing the status to "Waiting for Author".
Ok. I noticed. The patch got significantly broken by the watch pager
commit. I also have to enhance the added tests (per Peter request).
I wrote a test to check psql query cancel support. I checked that it
fails against the patch that was reverted. Maybe this is useful.
From 6ff6f8b0246cea08b7e329a3e5f49cec3f83a5bc Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Wed, 21 Jul 2021 21:46:11 +0200
Subject: [PATCH] psql: Add test for query canceling
---
src/bin/psql/t/020_cancel.pl | 32
1 file changed, 32 insertions(+)
create mode 100644 src/bin/psql/t/020_cancel.pl
diff --git a/src/bin/psql/t/020_cancel.pl b/src/bin/psql/t/020_cancel.pl
new file mode 100644
index 00..0d56b47ff3
--- /dev/null
+++ b/src/bin/psql/t/020_cancel.pl
@@ -0,0 +1,32 @@
+use strict;
+use warnings;
+
+use PostgresNode;
+use TestLib;
+use Test::More tests => 2;
+
+my $tempdir = TestLib::tempdir;
+
+my $node = get_new_node('main');
+$node->init;
+$node->start;
+
+# test query canceling by sending SIGINT to a running psql
+SKIP: {
+ skip "cancel test requires a Unix shell", 2 if $windows_os;
+
+ local %ENV = $node->_get_env();
+
+ local $SIG{ALRM} = sub {
+ my $psql_pid = TestLib::slurp_file("$tempdir/psql.pid");
+ kill 'INT', $psql_pid;
+ };
+ alarm 1;
+
+ my $stdin = "\\! echo \$PPID >$tempdir/psql.pid\nselect pg_sleep(5);";
+ my ($stdout, $stderr);
+ my $result = IPC::Run::run(['psql', '-v', 'ON_ERROR_STOP=1'], '<',
\$stdin, '>', \$stdout, '2>', \$stderr);
+
+ ok(!$result, 'query failed');
+ like($stderr, qr/canceling statement due to user request/, 'query was
canceled');
+}
--
2.32.0
Re: psql - add SHOW_ALL_RESULTS option
The patch does not apply on Head anymore, could you rebase and post a patch. I'm changing the status to "Waiting for Author". Ok. I noticed. The patch got significantly broken by the watch pager commit. I also have to enhance the added tests (per Peter request). -- Fabien.
Re: psql - add SHOW_ALL_RESULTS option
On Sat, Jun 12, 2021 at 3:11 PM Fabien COELHO wrote: > > > Hello Peter, > > >> My overly naive trust in non regression test to catch any issues has been > >> largely proven wrong. Three key features do not have a single tests. Sigh. > >> > >> I'll have some time to look at it over next week-end, but not before. > > > > I have reverted the patch and moved the commit fest entry to CF 2021-07. > > Attached a v7 which fixes known issues. The patch does not apply on Head anymore, could you rebase and post a patch. I'm changing the status to "Waiting for Author". Regards, Vignesh
Re: psql - add SHOW_ALL_RESULTS option
On 12.06.21 11:41, Fabien COELHO wrote: The patch includes basic AUTOCOMMIT and ON_ERROR_ROLLBACK tests, which did not exist before, at all. I looked at these tests first. The tests are good, they increase coverage. But they don't actually test the issue that was broken by the previous patch, namely the situation where autocommit is off and the user manually messes with the savepoints. I applied the tests against the previous patch and there was no failure. So the tests are useful, but they don't really help this patch. Would you like to enhance the tests a bit to cover this case? I think we could move forward with these tests then.
Re: psql - add SHOW_ALL_RESULTS option
Hello Peter,
My overly naive trust in non regression test to catch any issues has been
largely proven wrong. Three key features do not have a single tests. Sigh.
I'll have some time to look at it over next week-end, but not before.
I have reverted the patch and moved the commit fest entry to CF 2021-07.
Attached a v7 which fixes known issues.
I've tried to simplify the code and added a few comments. I've moved query
cancellation reset in one place in SendQuery. I've switched to an array of
buffers for notices, as suggested by Tom.
The patch includes basic AUTOCOMMIT and ON_ERROR_ROLLBACK tests, which did
not exist before, at all. I tried cancelling queries manually, but did not
develop a test for this, mostly because last time I submitted a TAP test
about psql to raise its coverage it was rejected.
As usual, what is not tested does not work…
--
Fabien.diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out
index 40b5109b55..ccce72fb85 100644
--- a/contrib/pg_stat_statements/expected/pg_stat_statements.out
+++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out
@@ -50,8 +50,28 @@ BEGIN \;
SELECT 2.0 AS "float" \;
SELECT 'world' AS "text" \;
COMMIT;
+ float
+---
+ 2.0
+(1 row)
+
+ text
+---
+ world
+(1 row)
+
-- compound with empty statements and spurious leading spacing
\;\; SELECT 3 + 3 \;\;\; SELECT ' ' || ' !' \;\; SELECT 1 + 4 \;;
+ ?column?
+--
+6
+(1 row)
+
+ ?column?
+--
+ !
+(1 row)
+
?column?
--
5
@@ -61,6 +81,11 @@ COMMIT;
SELECT 1 + 1 + 1 AS "add" \gset
SELECT :add + 1 + 1 AS "add" \;
SELECT :add + 1 + 1 AS "add" \gset
+ add
+-
+ 5
+(1 row)
+
-- set operator
SELECT 1 AS i UNION SELECT 2 ORDER BY i;
i
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index a8dfc41e40..1dbfb6a52a 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -127,18 +127,11 @@ echo '\x \\ SELECT * FROM foo;' | psql
commands included in the string to divide it into multiple
transactions. (See
for more details about how the server handles multi-query strings.)
- Also, psql only prints the
- result of the last SQL command in the string.
- This is different from the behavior when the same string is read from
- a file or fed to psql's standard input,
- because then psql sends
- each SQL command separately.
- Because of this behavior, putting more than one SQL command in a
- single -c string often has unexpected results.
- It's better to use repeated -c commands or feed
- multiple commands to psql's standard input,
+ If having several commands executed in one transaction is not desired,
+ use repeated -c commands or feed multiple commands to
+ psql's standard input,
either using echo as illustrated above, or
via a shell here-document, for example:
@@ -3532,10 +3525,6 @@ select 1\; select 2\; select 3;
commands included in the string to divide it into multiple
transactions. (See
for more details about how the server handles multi-query strings.)
-psql prints only the last query result
-it receives for each request; in this example, although all
-three SELECTs are indeed executed, psql
-only prints the 3.
@@ -4122,6 +4111,18 @@ bar
+SHOW_ALL_RESULTS
+
+
+When this variable is set to off, only the last
+result of a combined query (\;) is shown instead of
+all of them. The default is on. The off behavior
+is for compatibility with older versions of psql.
+
+
+
+
+
SHOW_CONTEXT
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 9a00499510..54a097a493 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -33,6 +33,7 @@ static bool DescribeQuery(const char *query, double *elapsed_msec);
static bool ExecQueryUsingCursor(const char *query, double *elapsed_msec);
static bool command_no_begin(const char *query);
static bool is_select_command(const char *query);
+static int SendQueryAndProcessResults(const char *query, double *pelapsed_msec, bool is_watch);
/*
@@ -353,7 +354,7 @@ CheckConnection(void)
* Returns true for valid result, false for error state.
*/
static bool
-AcceptResult(const PGresult *result)
+AcceptResult(const PGresult *result, bool show_error)
{
bool OK;
@@ -384,7 +385,7 @@ AcceptResult(const PGresult *result)
break;
}
- if (!OK)
+ if (!OK && show_error)
{
const char *error = PQerrorMessage(pset.db);
@@ -472,6 +473,18 @@ ClearOrSaveResult(PGresult *result)
}
}
+/*
+ * Consume all results
+ */
+static void
+ClearOrSaveAllResults()
+{
+ PGresult *result;
+
Re: psql - add SHOW_ALL_RESULTS option
On 15.04.21 13:51, Fabien COELHO wrote: That's a lot IMO, so my vote would be to discard this feature for now and revisit it properly in the 15 dev cycle, so as resources are redirected into more urgent issues (13 open items as of the moment of writing this email). I don't wish to tell people which open issues they ought to work on ... but this patch seems like it could be quite a large can of worms, and I'm not detecting very much urgency about getting it fixed. If it's not to be reverted then some significant effort needs to be put into it soon. My overly naive trust in non regression test to catch any issues has been largely proven wrong. Three key features do not have a single tests. Sigh. I'll have some time to look at it over next week-end, but not before. I have reverted the patch and moved the commit fest entry to CF 2021-07.
Re: psql - add SHOW_ALL_RESULTS option
Hello Tom, That's a lot IMO, so my vote would be to discard this feature for now and revisit it properly in the 15 dev cycle, so as resources are redirected into more urgent issues (13 open items as of the moment of writing this email). I don't wish to tell people which open issues they ought to work on ... but this patch seems like it could be quite a large can of worms, and I'm not detecting very much urgency about getting it fixed. If it's not to be reverted then some significant effort needs to be put into it soon. My overly naive trust in non regression test to catch any issues has been largely proven wrong. Three key features do not have a single tests. Sigh. I'll have some time to look at it over next week-end, but not before. -- Fabien.
Re: psql - add SHOW_ALL_RESULTS option
Michael Paquier writes: > On Mon, Apr 12, 2021 at 03:33:01PM -0400, Alvaro Herrera wrote: >> I, for one, would prefer to see the feature repaired in this cycle. > If it is possible to get that fixed, I would not mind waiting a bit > more but it would be nice to see some actual proposals. There are > already three identified bugs in psql introduced by this commit, > including the query cancellation. > That's a lot IMO, so my vote would be to discard this feature for now > and revisit it properly in the 15 dev cycle, so as resources are > redirected into more urgent issues (13 open items as of the moment of > writing this email). I don't wish to tell people which open issues they ought to work on ... but this patch seems like it could be quite a large can of worms, and I'm not detecting very much urgency about getting it fixed. If it's not to be reverted then some significant effort needs to be put into it soon. regards, tom lane
Re: psql - add SHOW_ALL_RESULTS option
On Mon, Apr 12, 2021 at 03:33:01PM -0400, Alvaro Herrera wrote: > Please note that there's no "for now" about it -- if the patch is > reverted, the only way to get it back is to wait for PG15. That's > undesirable. A better approach is to collect all those bugs and get > them fixed. There's plenty of time to do that. > > I, for one, would prefer to see the feature repaired in this cycle. If it is possible to get that fixed, I would not mind waiting a bit more but it would be nice to see some actual proposals. There are already three identified bugs in psql introduced by this commit, including the query cancellation. That's a lot IMO, so my vote would be to discard this feature for now and revisit it properly in the 15 dev cycle, so as resources are redirected into more urgent issues (13 open items as of the moment of writing this email). -- Michael signature.asc Description: PGP signature
Re: psql - add SHOW_ALL_RESULTS option
On Mon, Apr 12, 2021 at 07:08:21PM +, Bossart, Nathan wrote: > I think I've found another issue with this patch. If AcceptResult() > returns false in SendQueryAndProcessResults(), it seems to result in > an infinite loop of "unexpected PQresultStatus" messages. This can be > reproduced by trying to run "START_REPLICATION" via psql. Yes, that's another problem, and this causes an infinite loop where we would just report one error previously :/ -- Michael signature.asc Description: PGP signature
Re: psql - add SHOW_ALL_RESULTS option
On 2021-Apr-12, Bossart, Nathan wrote: > The following patch seems to resolve the issue, although I'll admit I > haven't dug into this too deeply. In any case, +1 for reverting the > patch for now. Please note that there's no "for now" about it -- if the patch is reverted, the only way to get it back is to wait for PG15. That's undesirable. A better approach is to collect all those bugs and get them fixed. There's plenty of time to do that. I, for one, would prefer to see the feature repaired in this cycle. -- Álvaro Herrera Valdivia, Chile
Re: psql - add SHOW_ALL_RESULTS option
On 4/12/21, 9:25 AM, "Tom Lane" wrote:
> Fabien COELHO writes:
>>> Between this and the known breakage of control-C, it seems clear
>>> to me that this patch was nowhere near ready for prime time.
>>> I think shoving it in on the last day before feature freeze was
>>> ill-advised, and it ought to be reverted. We can try again later.
>
>> The patch has been asleep for quite a while, and was resurrected, possibly
>> too late in the process. ISTM that fixing it for 14 is manageable,
>> but this is not my call.
>
> I just observed an additional issue that I assume was introduced by this
> patch, which is that psql's response to a server crash has gotten
> repetitive:
>
> regression=# CREATE VIEW v1(c1) AS (SELECT ('4' COLLATE "C")::INT FROM
> generate_series(1, 10));
> server closed the connection unexpectedly
> This probably means the server terminated abnormally
> before or while processing the request.
> The connection to the server was lost. Attempting reset: Failed.
> The connection to the server was lost. Attempting reset: Failed.
> !?>
>
> I've never seen that before, and it's not because I don't see
> server crashes regularly.
I think I've found another issue with this patch. If AcceptResult()
returns false in SendQueryAndProcessResults(), it seems to result in
an infinite loop of "unexpected PQresultStatus" messages. This can be
reproduced by trying to run "START_REPLICATION" via psql.
The following patch seems to resolve the issue, although I'll admit I
haven't dug into this too deeply. In any case, +1 for reverting the
patch for now.
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 028a357991..abafd41763 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -1176,7 +1176,7 @@ SendQueryAndProcessResults(const char *query, double
*pelapsed_msec, bool is_wat
/* and switch to next result */
result = PQgetResult(pset.db);
-continue;
+break;
}
/* must handle COPY before changing the current result */
Nathan
Re: psql - add SHOW_ALL_RESULTS option
Fabien COELHO writes:
>> Between this and the known breakage of control-C, it seems clear
>> to me that this patch was nowhere near ready for prime time.
>> I think shoving it in on the last day before feature freeze was
>> ill-advised, and it ought to be reverted. We can try again later.
> The patch has been asleep for quite a while, and was resurrected, possibly
> too late in the process. ISTM that fixing it for 14 is manageable,
> but this is not my call.
I just observed an additional issue that I assume was introduced by this
patch, which is that psql's response to a server crash has gotten
repetitive:
regression=# CREATE VIEW v1(c1) AS (SELECT ('4' COLLATE "C")::INT FROM
generate_series(1, 10));
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
The connection to the server was lost. Attempting reset: Failed.
!?>
I've never seen that before, and it's not because I don't see
server crashes regularly.
regards, tom lane
Re: psql - add SHOW_ALL_RESULTS option
Hello Tom, It's right: this is dead code because all paths through the if-nest starting at line 1373 now leave results = NULL. Hence, this patch has broken the autocommit logic; Do you mean yet another feature without a single non-regression test? :-( I tend to rely on non regression tests to catch bugs in complex multi-purpose hard-to-maintain functions when the code is modified. I have submitted a patch to improve psql coverage to about 90%, but given the lack of enthousiasm, I simply dropped it. Not sure I was right not to insist. it's no longer possible to tell whether we should do anything with our savepoint. Between this and the known breakage of control-C, it seems clear to me that this patch was nowhere near ready for prime time. I think shoving it in on the last day before feature freeze was ill-advised, and it ought to be reverted. We can try again later. The patch has been asleep for quite a while, and was resurrected, possibly too late in the process. ISTM that fixing it for 14 is manageable, but this is not my call. -- Fabien.
Re: psql - add SHOW_ALL_RESULTS option
On Sun, Apr 11, 2021 at 11:14:07AM -0400, Tom Lane wrote: > It's right: this is dead code because all paths through the if-nest > starting at line 1373 now leave results = NULL. Hence, this patch > has broken the autocommit logic; it's no longer possible to tell > whether we should do anything with our savepoint. Ugh, that's a good catch from Coverity here. > Between this and the known breakage of control-C, it seems clear > to me that this patch was nowhere near ready for prime time. > I think shoving it in on the last day before feature freeze was > ill-advised, and it ought to be reverted. We can try again later. Yes, I agree that a revert would be more adapted at this stage. Peter? -- Michael signature.asc Description: PGP signature
Re: psql - add SHOW_ALL_RESULTS option
Michael Paquier writes: > On Fri, Apr 09, 2021 at 08:52:20AM +0200, Fabien COELHO wrote: >> There is not a single test of "ctrl-c" which would have caught this trivial >> and irritating regression. ISTM that a TAP test is doable. Should one be >> added? > If you can design something reliable, I would welcome that. +1, there's a lot of moving parts there. I think avoiding any timing issues wouldn't be hard; the query-to-be-interrupted could be "select pg_sleep(1000)" or so. What's less clear is whether we can trigger the control-C response reliably across platforms. regards, tom lane
Re: psql - add SHOW_ALL_RESULTS option
On Fri, Apr 09, 2021 at 08:52:20AM +0200, Fabien COELHO wrote: > There is not a single test of "ctrl-c" which would have caught this trivial > and irritating regression. ISTM that a TAP test is doable. Should one be > added? If you can design something reliable, I would welcome that. -- Michael signature.asc Description: PGP signature
Re: psql - add SHOW_ALL_RESULTS option
... btw, Coverity also doesn't like this fragment of the patch:
/srv/coverity/git/pgsql-git/postgresql/src/bin/psql/common.c: 1084 in
ShowNoticeMessage()
1078 static void
1079 ShowNoticeMessage(t_notice_messages *notes)
1080 {
1081PQExpBufferData *current = notes->in_flip ? >flip :
>flop;
1082if (current->data != NULL && *current->data != '\0')
1083pg_log_info("%s", current->data);
>>> CID 1476041: Null pointer dereferences (FORWARD_NULL)
>>> Passing "current" to "resetPQExpBuffer", which dereferences null
>>> "current->data".
1084resetPQExpBuffer(current);
1085 }
1086
1087 /*
1088 * SendQueryAndProcessResults: utility function for use by SendQuery()
1089 * and PSQLexecWatch().
Its point here is that either the test of "current->data != NULL" is
useless, or resetPQExpBuffer needs such a test too. I'm inclined
to guess the former.
(Just as a matter of style, I don't care for the flip/flop terminology
here, not least because it's not clear why exactly two buffers suffice
and will suffice forevermore. I'd be inclined to use an array of
two buffers with an index variable.)
regards, tom lane
Re: psql - add SHOW_ALL_RESULTS option
Coverity has pointed out another problem with this patch: /srv/coverity/git/pgsql-git/postgresql/src/bin/psql/common.c: 1425 in SendQuery() 1419/* 1420 * Do nothing if they are messing with savepoints themselves: 1421 * If the user did COMMIT AND CHAIN, RELEASE or ROLLBACK, our 1422 * savepoint is gone. If they issued a SAVEPOINT, releasing 1423 * ours would remove theirs. 1424 */ >>> CID 1476042: Control flow issues (DEADCODE) >>> Execution cannot reach the expression "strcmp(PQcmdStatus(results), >>> "COMMIT") == 0" inside this statement: "if (results && (strcmp(PQcm...". 1425if (results && 1426(strcmp(PQcmdStatus(results), "COMMIT") == 0 || 1427 strcmp(PQcmdStatus(results), "SAVEPOINT") == 0 || 1428 strcmp(PQcmdStatus(results), "RELEASE") == 0 || 1429 strcmp(PQcmdStatus(results), "ROLLBACK") == 0)) 1430svptcmd = NULL; It's right: this is dead code because all paths through the if-nest starting at line 1373 now leave results = NULL. Hence, this patch has broken the autocommit logic; it's no longer possible to tell whether we should do anything with our savepoint. Between this and the known breakage of control-C, it seems clear to me that this patch was nowhere near ready for prime time. I think shoving it in on the last day before feature freeze was ill-advised, and it ought to be reverted. We can try again later. regards, tom lane
Re: psql - add SHOW_ALL_RESULTS option
Yep, it looks much better. I found it strange that the later did a reset but was not doing the set. Attached v2 does as you suggest. Close enough. I was thinking about this position of the attached, which is more consistent with the rest. Given the structural complexity of the function, the end of the file seemed like a good place to have an all-path-guaranteed reset. I find it a little bit strange to have the Set at the upper level and the Reset in many… but not all branches, though. For instance the on_error_rollback_savepoint/svptcmd branch includes a reset long after many other conditional resets, I cannot guess whether the initial set is still active or has been long wiped out and this query is just not cancellable. Also, ISTM that in the worst case a cancellation request is sent to a server which is idle, in which case it will be ignored, so the code should be in no hurry to clean it, at least not at the price of code clarity. Anyway, the place you suggest seems ok. -- Fabien.
Re: psql - add SHOW_ALL_RESULTS option
On 2021-Apr-08, Fabien COELHO wrote: > It is definitely a open item. I'm not sure where you want to add it… > possibly the "Pg 14 Open Items" wiki page? I tried but I do not have enough > privileges, if you can do it please proceed. I added an entry in the next CF > in the bugfix section. User "calvin" has privs of wiki editor. If that's not your Wiki username, please state what it is. -- Álvaro Herrera Valdivia, Chile "Cuando mañana llegue pelearemos segun lo que mañana exija" (Mowgli)
Re: psql - add SHOW_ALL_RESULTS option
On Fri, Apr 09, 2021 at 08:47:07AM +0200, Fabien COELHO wrote:
> Yep, it looks much better. I found it strange that the later did a reset but
> was not doing the set.
>
> Attached v2 does as you suggest.
Close enough. I was thinking about this position of the attached,
which is more consistent with the rest.
--
Michael
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 028a357991..f289f05843 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -1117,7 +1117,6 @@ SendQueryAndProcessResults(const char *query, double *pelapsed_msec, bool is_wat
INSTR_TIME_SET_CURRENT(before);
success = PQsendQuery(pset.db, query);
- ResetCancelConn();
if (!success)
{
@@ -1382,6 +1381,7 @@ SendQuery(const char *query)
{
/* Default fetch-it-all-and-print mode */
int res = SendQueryAndProcessResults(query, _msec, false);
+ ResetCancelConn();
OK = (res >= 0);
results = NULL;
}
signature.asc
Description: PGP signature
Re: psql - add SHOW_ALL_RESULTS option
Attached v2 does as you suggest. There is not a single test of "ctrl-c" which would have caught this trivial and irritating regression. ISTM that a TAP test is doable. Should one be added? -- Fabien.
Re: psql - add SHOW_ALL_RESULTS option
Bonjour Michaël,
I was running a long query this morning and wondered why the
cancellation was suddenly broken. So I am not alone, and here you are
with already a solution :)
So, studying through 3a51306, this stuff has changed the query
execution from a sync PQexec() to an async PQsendQuery().
Yes, because we want to handle all results whereas PQexec jumps to the
last one.
And the proposed fix changes back to the behavior where the cancellation
reset happens after getting a result, as there is no need to cancel
anything.
Yep. ISTM that was what happens internally in PQexec.
No strong objections from here if the consensus is to make
SendQueryAndProcessResults() handle the cancel reset properly though I
am not sure if this is the cleanest way to do things,
I was wondering as well, I did a quick fix because it can be irritating
and put off looking at it more precisely over the week-end.
but let's make at least the whole business consistent in the code for
all those code paths.
There are quite a few of them, some which reset the stuff and some which
do not depending on various conditions, some with early exits, all of
which required brain cells and a little time to investigate…
For example, PSQLexecWatch() does an extra ResetCancelConn() that would
be useless once we are done with SendQueryAndProcessResults(). Also, I
can see that SendQueryAndProcessResults() would not issue a cancel reset
if the query fails, for \watch when cancel is pressed, and for \watch
with COPY.
So, my opinion here would be to keep ResetCancelConn() within
PSQLexecWatch(), just add an extra one in SendQuery() to make all the
three code paths printing results consistent, and leave
SendQueryAndProcessResults() out of the cancellation logic.
Yep, it looks much better. I found it strange that the later did a reset
but was not doing the set.
Attached v2 does as you suggest.
That's strange, I don't think you need special permission there. It's
working for me so I added an item with a link to the patch!
As long as you have a community account, you should have the
possibility to edit the page.
After login as "calvin", I have "Want to edit, but don't see an edit
button when logged in? Click here.".
So if you feel that any change is required, please feel free to do so,
of course.
--
Fabien.diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 028a357991..5355086fe2 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -1117,7 +1117,6 @@ SendQueryAndProcessResults(const char *query, double *pelapsed_msec, bool is_wat
INSTR_TIME_SET_CURRENT(before);
success = PQsendQuery(pset.db, query);
- ResetCancelConn();
if (!success)
{
@@ -1486,6 +1485,8 @@ SendQuery(const char *query)
sendquery_cleanup:
+ ResetCancelConn();
+
/* reset \g's output-to-filename trigger */
if (pset.gfname)
{
Re: psql - add SHOW_ALL_RESULTS option
On Fri, Apr 9, 2021 at 6:36 AM Michael Paquier wrote: > > On Fri, Apr 09, 2021 at 01:11:35AM +0800, Julien Rouhaud wrote: > > On Thu, Apr 08, 2021 at 07:04:01PM +0200, Fabien COELHO wrote: > >> It is definitely a open item. I'm not sure where you want to add it… > >> possibly the "Pg 14 Open Items" wiki page? > > > > Correct. > > I was running a long query this morning and wondered why the > cancellation was suddenly broken. So I am not alone, and here you are > with already a solution :) > > So, studying through 3a51306, this stuff has changed the query > execution from a sync PQexec() to an async PQsendQuery(). And the > proposed fix changes back to the behavior where the cancellation > reset happens after getting a result, as there is no need to cancel > anything. > > No strong objections from here if the consensus is to make > SendQueryAndProcessResults() handle the cancel reset properly though I > am not sure if this is the cleanest way to do things, but let's make > at least the whole business consistent in the code for all those code > paths. For example, PSQLexecWatch() does an extra ResetCancelConn() > that would be useless once we are done with > SendQueryAndProcessResults(). Also, I can see that > SendQueryAndProcessResults() would not issue a cancel reset if the > query fails, for \watch when cancel is pressed, and for \watch with > COPY. So, my opinion here would be to keep ResetCancelConn() within > PSQLexecWatch(), just add an extra one in SendQuery() to make all the > three code paths printing results consistent, and leave > SendQueryAndProcessResults() out of the cancellation logic. Hi, I'm also facing the query cancellation issue, I have to kill the backend everytime to cancel a query, it's becoming difficult. With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
Re: psql - add SHOW_ALL_RESULTS option
On Fri, Apr 09, 2021 at 01:11:35AM +0800, Julien Rouhaud wrote: > On Thu, Apr 08, 2021 at 07:04:01PM +0200, Fabien COELHO wrote: >> It is definitely a open item. I'm not sure where you want to add it… >> possibly the "Pg 14 Open Items" wiki page? > > Correct. I was running a long query this morning and wondered why the cancellation was suddenly broken. So I am not alone, and here you are with already a solution :) So, studying through 3a51306, this stuff has changed the query execution from a sync PQexec() to an async PQsendQuery(). And the proposed fix changes back to the behavior where the cancellation reset happens after getting a result, as there is no need to cancel anything. No strong objections from here if the consensus is to make SendQueryAndProcessResults() handle the cancel reset properly though I am not sure if this is the cleanest way to do things, but let's make at least the whole business consistent in the code for all those code paths. For example, PSQLexecWatch() does an extra ResetCancelConn() that would be useless once we are done with SendQueryAndProcessResults(). Also, I can see that SendQueryAndProcessResults() would not issue a cancel reset if the query fails, for \watch when cancel is pressed, and for \watch with COPY. So, my opinion here would be to keep ResetCancelConn() within PSQLexecWatch(), just add an extra one in SendQuery() to make all the three code paths printing results consistent, and leave SendQueryAndProcessResults() out of the cancellation logic. >> I tried but I do not have enough >> privileges, if you can do it please proceed. I added an entry in the next CF >> in the bugfix section. > > That's strange, I don't think you need special permission there. It's > working for me so I added an item with a link to the patch! As long as you have a community account, you should have the possibility to edit the page. So if you feel that any change is required, please feel free to do so, of course. -- Michael signature.asc Description: PGP signature
Re: psql - add SHOW_ALL_RESULTS option
Bonjour Fabien, On Thu, Apr 08, 2021 at 07:04:01PM +0200, Fabien COELHO wrote: > > > > Thanks for the patch Fabien. I've hit this issue multiple time and this is > > indeed unwelcome. Should we add it as an open item? > > It is definitely a open item. I'm not sure where you want to add it… > possibly the "Pg 14 Open Items" wiki page? Correct. > I tried but I do not have enough > privileges, if you can do it please proceed. I added an entry in the next CF > in the bugfix section. That's strange, I don't think you need special permission there. It's working for me so I added an item with a link to the patch!
Re: psql - add SHOW_ALL_RESULTS option
Bonjour Julien, Attached a patch which attempts to fix this by moving the cancellation cancelling request after processing results. Thank you for your fixing. I tested and the problem has been solved after applying your patch. Thanks for the patch Fabien. I've hit this issue multiple time and this is indeed unwelcome. Should we add it as an open item? It is definitely a open item. I'm not sure where you want to add it… possibly the "Pg 14 Open Items" wiki page? I tried but I do not have enough privileges, if you can do it please proceed. I added an entry in the next CF in the bugfix section. -- Fabien.
Re: psql - add SHOW_ALL_RESULTS option
On Thu, Apr 08, 2021 at 01:32:12AM +, shiy.f...@fujitsu.com wrote: > > Attached a patch which attempts to fix this by moving the cancellation > > cancelling request after processing results. > > Thank you for your fixing. I tested and the problem has been solved after > applying your patch. Thanks for the patch Fabien. I've hit this issue multiple time and this is indeed unwelcome. Should we add it as an open item?
RE: psql - add SHOW_ALL_RESULTS option
> Attached a patch which attempts to fix this by moving the cancellation > cancelling request after processing results. Thank you for your fixing. I tested and the problem has been solved after applying your patch. Regards, Shi yu
RE: psql - add SHOW_ALL_RESULTS option
Hello,
I met a problem after commit 3a51306722.
While executing a SQL statement with psql, I can't interrupt it by pressing
ctrl+c.
For example:
postgres=# insert into test select generate_series(1,1000);
^C^CINSERT 0 1000
Press ctrl+c before finishing INSERT, and psql still continuing to INSERT.
I can confirm this unexpected change of behavior on this commit. This is
indeed e bug.
Is it the result expected?
Obviously not.
And I think maybe it is better to allow users to interrupt by pressing
ctrl+c.
Obviously yes.
The problem is that the cancellation stuff is cancelled too early after
sending an asynchronous request.
Attached a patch which attempts to fix this by moving the cancellation
cancelling request after processing results.
--
Fabien.diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 028a357991..0482e57d45 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -1117,7 +1117,6 @@ SendQueryAndProcessResults(const char *query, double *pelapsed_msec, bool is_wat
INSTR_TIME_SET_CURRENT(before);
success = PQsendQuery(pset.db, query);
- ResetCancelConn();
if (!success)
{
@@ -1257,6 +1256,9 @@ SendQueryAndProcessResults(const char *query, double *pelapsed_msec, bool is_wat
if (!CheckConnection())
return -1;
+ /* all results are processed, nothing to cancel anymore */
+ ResetCancelConn();
+
return success ? 1 : -1;
}
RE: psql - add SHOW_ALL_RESULTS option
Hi I met a problem after commit 3a51306722. While executing a SQL statement with psql, I can't interrupt it by pressing ctrl+c. For example: postgres=# insert into test select generate_series(1,1000); ^C^CINSERT 0 1000 Press ctrl+c before finishing INSERT, and psql still continuing to INSERT. Is it the result expected? And I think maybe it is better to allow users to interrupt by pressing ctrl+c. Regards, Shi yu
Re: psql - add SHOW_ALL_RESULTS option
Hello Peter, My 0.02€: I tested this updated version and do not have any comment on this version. From my point of view it could be committed. I would not bother to separate the test style ajustments. Committed. The last thing I fixed was the diff in the copy2.out regression test. The order of the notices with respect to the error messages was wrong. I fixed that by switching back to the regular notice processor during COPY handling. Btw., not sure if that was mentioned before, but a cool use of this is to \watch multiple queries at once, like select current_date \; select current_time \watch Indeed, that was one of the things I tested on the patch. I'm wondering whether the documentation should point this out explicitely. Anyway Thanks for the push! -- Fabien.
Re: psql - add SHOW_ALL_RESULTS option
On 14.03.21 10:54, Fabien COELHO wrote: Hello Peter, This reply was two months ago, and nothing has happened, so I have marked the patch as RwF. Given the ongoing work on returning multiple result sets from stored procedures[0], I went to dust off this patch. Based on the feedback, I put back the titular SHOW_ALL_RESULTS option, but set the default to on. I fixed the test failure in 013_crash_restart.pl. I also trimmed back the test changes a bit so that the resulting test output changes are visible better. (We could make those stylistic changes separately in another patch.) I'll put this back into the commitfest for another look. Thanks a lot for the fixes and pushing it forward! My 0.02€: I tested this updated version and do not have any comment on this version. From my point of view it could be committed. I would not bother to separate the test style ajustments. Committed. The last thing I fixed was the diff in the copy2.out regression test. The order of the notices with respect to the error messages was wrong. I fixed that by switching back to the regular notice processor during COPY handling. Btw., not sure if that was mentioned before, but a cool use of this is to \watch multiple queries at once, like select current_date \; select current_time \watch
Re: psql - add SHOW_ALL_RESULTS option
Hello Peter, This reply was two months ago, and nothing has happened, so I have marked the patch as RwF. Given the ongoing work on returning multiple result sets from stored procedures[0], I went to dust off this patch. Based on the feedback, I put back the titular SHOW_ALL_RESULTS option, but set the default to on. I fixed the test failure in 013_crash_restart.pl. I also trimmed back the test changes a bit so that the resulting test output changes are visible better. (We could make those stylistic changes separately in another patch.) I'll put this back into the commitfest for another look. Thanks a lot for the fixes and pushing it forward! My 0.02€: I tested this updated version and do not have any comment on this version. From my point of view it could be committed. I would not bother to separate the test style ajustments. -- Fabien.
Re: psql - add SHOW_ALL_RESULTS option
On 30.09.20 08:21, Michael Paquier wrote: On Mon, Jul 20, 2020 at 07:48:42AM +0200, Fabien COELHO wrote: Yes, indeed. I'm planning to investigate, hopefully this week. This reply was two months ago, and nothing has happened, so I have marked the patch as RwF. Given the ongoing work on returning multiple result sets from stored procedures[0], I went to dust off this patch. Based on the feedback, I put back the titular SHOW_ALL_RESULTS option, but set the default to on. I fixed the test failure in 013_crash_restart.pl. I also trimmed back the test changes a bit so that the resulting test output changes are visible better. (We could make those stylistic changes separately in another patch.) I'll put this back into the commitfest for another look. [0]: https://www.postgresql.org/message-id/flat/6e747f98-835f-2e05-cde5-86ee444a7...@2ndquadrant.com >From 22ac191084db1b6ab155202a09bc1a6fedde794f Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Sat, 27 Feb 2021 11:50:50 +0100 Subject: [PATCH v10] psql: Show all query results by default Author: Author: Fabien COELHO Discussion: https://www.postgresql.org/message-id/flat/alpine.DEB.2.21.1904132231510.8961@lancre --- .../expected/pg_stat_statements.out | 25 + doc/src/sgml/ref/psql-ref.sgml| 29 +- src/bin/psql/common.c | 639 ++ src/bin/psql/help.c | 2 + src/bin/psql/settings.h | 1 + src/bin/psql/startup.c| 10 + src/bin/psql/tab-complete.c | 2 +- src/test/regress/expected/copy2.out | 2 +- src/test/regress/expected/copyselect.out | 14 +- src/test/regress/expected/psql.out| 94 +++ src/test/regress/expected/transactions.out| 58 +- src/test/regress/sql/copyselect.sql | 4 +- src/test/regress/sql/psql.sql | 38 ++ src/test/regress/sql/transactions.sql | 2 +- 14 files changed, 609 insertions(+), 311 deletions(-) diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out index 16158525ca..4ffb7e0076 100644 --- a/contrib/pg_stat_statements/expected/pg_stat_statements.out +++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out @@ -50,8 +50,28 @@ BEGIN \; SELECT 2.0 AS "float" \; SELECT 'world' AS "text" \; COMMIT; + float +--- + 2.0 +(1 row) + + text +--- + world +(1 row) + -- compound with empty statements and spurious leading spacing \;\; SELECT 3 + 3 \;\;\; SELECT ' ' || ' !' \;\; SELECT 1 + 4 \;; + ?column? +-- +6 +(1 row) + + ?column? +-- + ! +(1 row) + ?column? -- 5 @@ -61,6 +81,11 @@ COMMIT; SELECT 1 + 1 + 1 AS "add" \gset SELECT :add + 1 + 1 AS "add" \; SELECT :add + 1 + 1 AS "add" \gset + add +- + 5 +(1 row) + -- set operator SELECT 1 AS i UNION SELECT 2 ORDER BY i; i diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index 13c1edfa4d..d14651adea 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -127,18 +127,11 @@ Options commands included in the string to divide it into multiple transactions. (See for more details about how the server handles multi-query strings.) - Also, psql only prints the - result of the last SQL command in the string. - This is different from the behavior when the same string is read from - a file or fed to psql's standard input, - because then psql sends - each SQL command separately. - Because of this behavior, putting more than one SQL command in a - single -c string often has unexpected results. - It's better to use repeated -c commands or feed - multiple commands to psql's standard input, + If having several commands executed in one transaction is not desired, + use repeated -c commands or feed multiple commands to + psql's standard input, either using echo as illustrated above, or via a shell here-document, for example: @@ -3523,10 +3516,6 @@ Meta-Commands commands included in the string to divide it into multiple transactions. (See for more details about how the server handles multi-query strings.) -psql prints only the last query result -it receives for each request; in this example, although all -three SELECTs are indeed executed, psql -only prints the 3. @@ -4102,6 +4091,18 @@ Variables +SHOW_ALL_RESULTS + + +When this variable is set to off, only the last +result of a combined (\;) query are shown instead +of all of them. Default is on. The off behavior +is for compatibility with older versions of psql. + + +
Re: psql - add SHOW_ALL_RESULTS option
On Mon, Jul 20, 2020 at 07:48:42AM +0200, Fabien COELHO wrote: > Yes, indeed. I'm planning to investigate, hopefully this week. This reply was two months ago, and nothing has happened, so I have marked the patch as RwF. -- Michael signature.asc Description: PGP signature
Re: psql - add SHOW_ALL_RESULTS option
In the meantime, here is a v9 which also fixes the behavior when using \watch, so that now one can issue several \;-separated queries and have their progress shown. I just needed that a few days ago and was disappointed but unsurprised that it did not work. This seems to break the 013_crash_restart.pl test. Yes, indeed. I'm planning to investigate, hopefully this week. -- Fabien.
Re: psql - add SHOW_ALL_RESULTS option
On Sun, Jun 7, 2020 at 2:36 AM Fabien COELHO wrote: > In the meantime, here is a v9 which also fixes the behavior when using > \watch, so that now one can issue several \;-separated queries and have > their progress shown. I just needed that a few days ago and was > disappointed but unsurprised that it did not work. Hi Fabien, This seems to break the 013_crash_restart.pl test.
Re: psql - add SHOW_ALL_RESULTS option
Hello, This patch was marked as ready for committer, but clearly there's an ongoin discussion about what should be the default behavoir, if this breaks existing apps etc. So I've marked it as "needs review" and moved it to the next CF. The issue is that root (aka Tom) seems to be against the feature, and would like the keep it as current. Although my opinion is that the previous behavior is close to insane, I'm ready to resurect the guc to control the behavior so that it would be possible, or even the default. Right now I'm waiting for a "I will not veto it on principle" from Tom (I'm okay with a reject based on bad implementation) before spending more time on it: Although my time is given for free, it is not a good reason to send it down the drain if there is a reject coming whatever I do. Tom, would you consider the feature acceptable with a guc to control it? Tom, I would appreciate if you could answer this question. For me, the current behavior is both stupid and irritating: why would pg decide to drop the result of a query that I carefully typed?. It makes the multi-query feature basically useless from psql, so I did not resurrect the guc, but I will if it would remove a veto. In the meantime, here is a v9 which also fixes the behavior when using \watch, so that now one can issue several \;-separated queries and have their progress shown. I just needed that a few days ago and was disappointed but unsurprised that it did not work. Watch does not seem to be tested anywhere, I kept it that way. Sigh. -- Fabien.diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out index f615f8c2bf..2f739445ff 100644 --- a/contrib/pg_stat_statements/expected/pg_stat_statements.out +++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out @@ -49,17 +49,42 @@ BEGIN \; SELECT 2.0 AS "float" \; SELECT 'world' AS "text" \; COMMIT; + float +--- + 2.0 +(1 row) + + text +--- + world +(1 row) + -- compound with empty statements and spurious leading spacing -\;\; SELECT 3 + 3 \;\;\; SELECT ' ' || ' !' \;\; SELECT 1 + 4 \;; - ?column? --- -5 +\;\; SELECT 3 + 3 AS "+" \;\;\; SELECT ' ' || ' !' AS "||" \;\; SELECT 1 + 4 AS "+" \;; + + +--- + 6 +(1 row) + + || +- + ! +(1 row) + + + +--- + 5 (1 row) -- non ;-terminated statements SELECT 1 + 1 + 1 AS "add" \gset SELECT :add + 1 + 1 AS "add" \; SELECT :add + 1 + 1 AS "add" \gset + add +- + 5 +(1 row) + -- set operator SELECT 1 AS i UNION SELECT 2 ORDER BY i; i @@ -102,12 +127,12 @@ SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C"; SELECT $1 +| 4 |4 +| | AS "text" | | - SELECT $1 + $2 | 2 |2 SELECT $1 + $2 + $3 AS "add" | 3 |3 + SELECT $1 + $2 AS "+"| 2 |2 SELECT $1 AS "float" | 1 |1 SELECT $1 AS "int" | 2 |2 SELECT $1 AS i UNION SELECT $2 ORDER BY i| 1 |2 - SELECT $1 || $2 | 1 |1 + SELECT $1 || $2 AS "||" | 1 |1 SELECT pg_stat_statements_reset()| 1 |1 SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C" | 0 |0 WITH t(f) AS ( +| 1 |2 diff --git a/contrib/pg_stat_statements/sql/pg_stat_statements.sql b/contrib/pg_stat_statements/sql/pg_stat_statements.sql index 75c10554a8..835497b286 100644 --- a/contrib/pg_stat_statements/sql/pg_stat_statements.sql +++ b/contrib/pg_stat_statements/sql/pg_stat_statements.sql @@ -27,7 +27,7 @@ SELECT 'world' AS "text" \; COMMIT; -- compound with empty statements and spurious leading spacing -\;\; SELECT 3 + 3 \;\;\; SELECT ' ' || ' !' \;\; SELECT 1 + 4 \;; +\;\; SELECT 3 + 3 AS "+" \;\;\; SELECT ' ' || ' !' AS "||" \;\; SELECT 1 + 4 AS "+" \;; -- non ;-terminated statements SELECT 1 + 1 + 1 AS "add" \gset diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index 62fee5a7dd..f02a49d781 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -127,18 +127,11 @@ echo '\x \\ SELECT * FROM foo;' | psql commands included in the string to divide it into multiple transactions. (See
Re: psql - add SHOW_ALL_RESULTS option
Hello Tomas, This patch was marked as ready for committer, but clearly there's an ongoin discussion about what should be the default behavoir, if this breaks existing apps etc. So I've marked it as "needs review" and moved it to the next CF. The issue is that root (aka Tom) seems to be against the feature, and would like the keep it as current. Although my opinion is that the previous behavior is close to insane, I'm ready to resurect the guc to control the behavior so that it would be possible, or even the default. Right now I'm waiting for a "I will not veto it on principle" from Tom (I'm okay with a reject based on bad implementation) before spending more time on it: Although my time is given for free, it is not a good reason to send it down the drain if there is a reject coming whatever I do. Tom, would you consider the feature acceptable with a guc to control it? -- Fabien.
Re: psql - add SHOW_ALL_RESULTS option
This patch was marked as ready for committer, but clearly there's an ongoin discussion about what should be the default behavoir, if this breaks existing apps etc. So I've marked it as "needs review" and moved it to the next CF. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: psql - add SHOW_ALL_RESULTS option
Tom Lane wrote: > I'm not really holding my breath for that to happen, considering > it would involve fundamental breakage of the wire protocol. > (For example, extended query protocol assumes that Describe > Portal only needs to describe one result set. There might be > more issues, but that one's bad enough.) I'm not sure that CALL can be used at all with the extended protocol today (just like multiple queries per statements, except that for these, I'm sure). My interpretation is that the extended protocol deliberately lefts out the possibility of multiple result sets because it doesn't fit with how it's designed and if you want to have this, you can just use the old protocol's Query message. There is no need to break anything or invent anything but on the contrary to use the older way. Considering these 3 ways to use libpq to send queries: 1. using old protocol with PQexec: only one resultset. 2. using old protocol with PQsendQuery+looping on PQgetResult: same as #1 except multiple result sets can be processed 3. using extended protocol: not for multiple result sets, not for copy, possibly not for other things, but can use bind parameters, binary format, pipelining,... The current patch is about using #2 instead of #1. They have been patches about doing bits of #3 in some cases (binary output, maybe parameters too?) and none got eventually in. ISTM that the current situation is that psql is stuck at #1 since forever so it's not fully using the capabilities of the protocol, both old and new. Best regards, -- Daniel Vérité PostgreSQL-powered mailer: http://www.manitou-mail.org Twitter: @DanielVerite
Re: psql - add SHOW_ALL_RESULTS option
"Daniel Verite" writes: > Yes. For instance if the stored procedures support gets improved to > produce several result sets, how is psql going to benefit from it > while sticking to the old way (PGresult *r = PQexec(query)) > of executing queries that discards N-1 out of N result sets? I'm not really holding my breath for that to happen, considering it would involve fundamental breakage of the wire protocol. (For example, extended query protocol assumes that Describe Portal only needs to describe one result set. There might be more issues, but that one's bad enough.) When and if we break all the things that would break, it'd be time enough for incompatible changes in psql's behavior. regards, tom lane
Re: psql - add SHOW_ALL_RESULTS option
Alvaro Herrera wrote: > if this patch enables other psql features, it might be a good step > forward. Yes. For instance if the stored procedures support gets improved to produce several result sets, how is psql going to benefit from it while sticking to the old way (PGresult *r = PQexec(query)) of executing queries that discards N-1 out of N result sets? Best regards, -- Daniel Vérité PostgreSQL-powered mailer: http://www.manitou-mail.org Twitter: @DanielVerite
Re: psql - add SHOW_ALL_RESULTS option
My own vote would be to use the initial patch (after applying any unrelated changes per later review), ie. add the feature with its disable button. I can do that, but not if there is a veto from Tom on the feature. I wish definite negative opinions by senior committers would be expressed earlier, so that people do not spend time rewiewing dead code and developing even deader code. -- Fabien.
Re: psql - add SHOW_ALL_RESULTS option
Hello Tom, This is one of the patches already marked as RFC (since September by Alvaro). Anyone interested in actually pushing it, so that it does not fall through to yet another commitfest? TBH, I think we'd be better off to reject it. This makes a nontrivial change in a very long-standing psql behavior, with AFAICS no way to get back the old semantics. (The thread title is completely misleading about that; there's no "option" in the patch as it stands.) The thread title was not misleading, the initial version of the patch did offer an option. Then I was said "the current behavior is stupid (which I agree), let us change it to the sane behavior without option", then I'm told the contrary. Sigh. I still have the patch with the option, though. Sure, in a green field this behavior would likely be more sensible ... but that has to be weighed against the fact that it's behaved the way it does for a long time, and any existing scripts that are affected by that behavior have presumably deliberately chosen to use it. I cannot imagine many people actually relying on the current insane behavior. I can't imagine that changing this will make very many people happier. It seems much more likely that people who are affected will be unhappy. The compatibility issue could be resolved by putting in the option that I suppose was there at the beginning. Indeed. But then we'd have to have a debate about which behavior would be default, The patch was keeping current behavior as the default because people do not like a change whatever. and there would still be the question of who would find this to be an improvement. If you're chaining together commands with \; then it's likely that you are happy with the way it behaves today. Certainly there's been no drumbeat of bug reports about it. Why would there be bug report if this is a feature? :-) The behavior has been irritating me for a long time. It is plain stupid to be able to send queries but not see their results. -- Fabien.
Re: psql - add SHOW_ALL_RESULTS option
On 2020-Jan-16, Tom Lane wrote: > The compatibility issue could be resolved by putting in the option > that I suppose was there at the beginning. But then we'd have to > have a debate about which behavior would be default, and there would > still be the question of who would find this to be an improvement. > If you're chaining together commands with \; then it's likely that > you are happy with the way it behaves today. Certainly there's been > no drumbeat of bug reports about it. The patch originally submitted did indeed have the option (defaulting to "off", that is, the original behavior), and it was removed at request of reviewers Daniel Vérité, Peter Eisentraut and Kyotaro Horiguchi. My own opinion is that any scripts that rely heavily on the current behavior are stepping on shaky ground anyway. I'm not saying we should break them on every chance we get -- just that keeping them unharmed is not necessarily a priority, and that if this patch enables other psql features, it might be a good step forward. My own vote would be to use the initial patch (after applying any unrelated changes per later review), ie. add the feature with its disable button. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
