psql - add SHOW_ALL_RESULTS option

Hi Fabien,

I review your patch.

Add a few tests for the new feature.

+++ b/src/test/regress/expected/psql.out
@@ -4729,3 +4729,46 @@ drop schema testpart;
 set search_path to default;
 set role to default;
 drop role testrole_partitioning;
+-- 
There is space (+--' '). Please delete it. It is cause of regression test failed.

IMHO this new setting should be on by default: few people know about \; so
it would not change anything for most, and I do not see why those who use
it would not be interested by the results of all the queries they asked for.

I agree with your opinion.

I test some query combination case. And I found when warning happen, the message is printed in head of results. I think it is not clear in which query the warning occurred.
How about print warning message before the query that warning occurred?

For example,
-- devide by ';'
postgres=# BEGIN; BEGIN; SELECT 1 AS one; COMMIT; BEGIN; BEGIN; SELECT 1 AS one; COMMIT;
BEGIN
psql: WARNING: there is already a transaction in progress
BEGIN
one
-----
1
(1 row)

COMMIT
BEGIN
psql: WARNING: there is already a transaction in progress
BEGIN
one
-----
1
(1 row)

COMMIT

-- devide by '\;' and set SHOW_RESULT_ALL on
postgres=# \set SHOW_ALL_RESULTS on
postgres=# BEGIN\; BEGIN\; SELECT 1 AS one\; COMMIT\; BEGIN\; BEGIN\; SELECT 1 AS one\; COMMIT;
psql: WARNING: there is already a transaction in progress
BEGIN
BEGIN
one
-----
1
(1 row)

psql: WARNING: there is already a transaction in progress
COMMIT
BEGIN
BEGIN
one
-----
1
(1 row)

COMMIT

I will check the code soon.

Regards,
Aya Iwata

Hello Aya-san,

Thanks for this review.

There is space (+--' '). Please delete it. It is cause of regression test failed.

Indeed, unsure how I could do that. Fixed.

IMHO this new setting should be on by default: few people know about \; so
it would not change anything for most, and I do not see why those who use
it would not be interested by the results of all the queries they asked for.

I agree with your opinion.

Ok. I did not yet change the default in the attached version, though.

I test some query combination case. And I found when warning happen, the
message is printed in head of results. I think it is not clear in which
query the warning occurred.

Indeed.

How about print warning message before the query that warning occurred?

Sure. It happened to be trickier than I thought to achieve this, because
there is a callback hook to send notifications.

This attached version does:
- ensure that warnings appear just before its
- add the entry in psql's help
- redefine the function boundary so that timing is cleaner
- include somehow improved tests

--
Fabien.

Fabien COELHO wrote:

IMHO this new setting should be on by default: few people know about \; so
it would not change anything for most, and I do not see why those who use
it would not be interested by the results of all the queries they asked for.

I agree with your opinion.

Ok. I did not yet change the default in the attached version, though.

I'd go further and suggest that there shouldn't be a variable
controlling this. All results that come in should be processed, period.
It's not just about \; If the ability of CALL to produce multiple
resultsets gets implemented (it was posted as a POC during v11
development), this will be needed too.

This attached version does:
- ensure that warnings appear just before its
- add the entry in psql's help
- redefine the function boundary so that timing is cleaner
- include somehow improved tests

\errverbose seems to no longer work with the patch:

test=> select 1/0;
psql: ERROR: division by zero

test=> \errverbose
There is no previous error.

as opposed to this output with PG11:

test=> \errverbose
ERROR: 22012: division by zero
LOCATION: int4div, int.c:820

\errverbose has probably no regression tests because its output
includes these ever-changing line numbers; hence `make check`
cannot be used to find this regression.

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

Bonjour Daniel,

IMHO this new setting should be on by default: few people know about \; so
it would not change anything for most, and I do not see why those who use
it would not be interested by the results of all the queries they asked for.

I agree with your opinion.

Ok. I did not yet change the default in the attached version, though.

I'd go further and suggest that there shouldn't be a variable
controlling this. All results that come in should be processed, period.
It's not just about \; If the ability of CALL to produce multiple
resultsets gets implemented (it was posted as a POC during v11
development), this will be needed too.

I do agree, but I'm afraid that if there is no opt-out it could be seen as
a regression by some.

This attached version does:
- ensure that warnings appear just before its
- add the entry in psql's help
- redefine the function boundary so that timing is cleaner
- include somehow improved tests

\errverbose seems to no longer work with the patch:

test=> select 1/0;
psql: ERROR: division by zero

test=> \errverbose
There is no previous error.

as opposed to this output with PG11:

test=> \errverbose
ERROR: 22012: division by zero
LOCATION: int4div, int.c:820

Thanks for the catch. I'll investigate.

\errverbose has probably no regression tests because its output includes
these ever-changing line numbers; hence `make check` cannot be used to
find this regression.

What is not tested does not work:-( The TAP infrastructure for psql
included in some patch (https://commitfest.postgresql.org/23/2100/ I
guess) would help testing such slightly varying features which cannot be
tested with a hardcoded reference text.

--
Fabien.

Re-bonjour Daniel,

This attached version does:
- ensure that warnings appear just before its
- add the entry in psql's help
- redefine the function boundary so that timing is cleaner
- include somehow improved tests

\errverbose seems to no longer work with the patch:

Here is a v3 which fixes \errverbose, hopefully.

The feature is still an option which is not enabled by default.

--
Fabien.

Here is a v3 which fixes \errverbose, hopefully.

V5 is a rebase and an slightly improved documentation.

--
Fabien.

Here is a v3 which fixes \errverbose, hopefully.

V5 is a rebase and an slightly improved documentation.

It was really v4. v5 is a rebase.

--
Fabien.

On 2019-05-15 18:41, Daniel Verite wrote:

I'd go further and suggest that there shouldn't be a variable
controlling this. All results that come in should be processed, period.

I agree with that.

It's not just about \; If the ability of CALL to produce multiple
resultsets gets implemented (it was posted as a POC during v11
development), this will be needed too.

See previous patch here:
/messages/by-id/4580ff7b-d610-eaeb-e06f-4d686896b93b@2ndquadrant.com

In that patch, I discussed the specific ways in which \timing works in
psql and how that conflicts with multiple result sets. What is the
solution to that in this patch?

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hello Peter,

I'd go further and suggest that there shouldn't be a variable
controlling this. All results that come in should be processed, period.

I agree with that.

I kind of agree as well, but I was pretty sure that someone would complain
if the current behavior was changed.

Should I produce a patch where the behavior is not an option, or turn the
option on by default, or just keep it like that for the time being?

It's not just about \; If the ability of CALL to produce multiple
resultsets gets implemented (it was posted as a POC during v11
development), this will be needed too.

See previous patch here:
/messages/by-id/4580ff7b-d610-eaeb-e06f-4d686896b93b@2ndquadrant.com

In that patch, I discussed the specific ways in which \timing works in
psql and how that conflicts with multiple result sets. What is the
solution to that in this patch?

\timing was kind of a ugly feature to work around. The good intention
behind \timing is that it should reflect the time to perform the query
from the client perspective, but should not include processing the
results.

However, if a message results in several queries, they are processed as
they arrive, so that \timing reports the time to perform all queries and
the time to process all but the last result.

Although on paper we could try to get all results first, take the time,
then process them, this does not work in the general case because COPY
takes on the connection so you have to process its result before switching
to the next result.

There is also some stuff to handle notices which are basically send as
events when they occur, so that the notice shown are related to the
result under processing.

--
Fabien.

Fabien COELHO wrote:

I'd go further and suggest that there shouldn't be a variable
controlling this. All results that come in should be processed, period.

I agree with that.

I kind of agree as well, but I was pretty sure that someone would complain
if the current behavior was changed.

If queries in a compound statement must be kept silent,
they can be converted to CTEs or DO-blocks to produce the
same behavior without having to configure anything in psql.
That cost on users doesn't seem too bad, compared to introducing
a knob in psql, and presumably maintaining it forever.

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

Bonjour Daniel,

I kind of agree as well, but I was pretty sure that someone would complain
if the current behavior was changed.

If queries in a compound statement must be kept silent,
they can be converted to CTEs or DO-blocks to produce the
same behavior without having to configure anything in psql.
That cost on users doesn't seem too bad, compared to introducing
a knob in psql, and presumably maintaining it forever.

Ok.

Attached a "do it always version", which does the necessary refactoring.
There is seldom new code, it is rather moved around, some functions are
created for clarity.

--
Fabien.

Fabien COELHO wrote:

Attached a "do it always version", which does the necessary refactoring.
There is seldom new code, it is rather moved around, some functions are
created for clarity.

Thanks for the update!
FYI you forgot to remove that bit:

--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -3737,7 +3737,7 @@ psql_completion(const char *text, int start, int end)
	else if (TailMatchesCS("\\set", MatchAny))
	{
		if (TailMatchesCS("AUTOCOMMIT|ON_ERROR_STOP|QUIET|"
-						  "SINGLELINE|SINGLESTEP"))
+						 
"SINGLELINE|SINGLESTEP|SHOW_ALL_RESULTS"))

Also copydml does not seem to be exercised with combined
queries, so do we need this chunk:

--- a/src/test/regress/sql/copydml.sql
+++ b/src/test/regress/sql/copydml.sql
@@ -70,10 +70,10 @@ drop rule qqq on copydml_test;
 create function qqq_trig() returns trigger as $$
 begin
 if tg_op in ('INSERT', 'UPDATE') then
-    raise notice '% %', tg_op, new.id;
+    raise notice '% % %', tg_when, tg_op, new.id;
     return new;
 else
-    raise notice '% %', tg_op, old.id;
+    raise notice '% % %', tg_when, tg_op, old.id;
     return old;
 end if;

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

Bonsoir Daniel,

FYI you forgot to remove that bit:

+ "SINGLELINE|SINGLESTEP|SHOW_ALL_RESULTS"))

Indeed. I found another such instance in "help.c".

Also copydml does not seem to be exercised with combined
queries, so do we need this chunk:

--- a/src/test/regress/sql/copydml.sql

Yep, because I reorganized the notice code significantly, and I wanted to
be sure that the right notices are displayed in the right order, which
does not show if the trigger just says "NOTICE: UPDATE 8".

Attached a v2 for the always-show-all-results variant. Thanks for the
debug!

--
Fabien.

Hello.

At Thu, 25 Jul 2019 21:42:11 +0000 (GMT), Fabien COELHO <coelho@cri.ensmp.fr> wrote in <alpine.DEB.2.21.1907252135060.21130@lancre>

Bonsoir Daniel,

FYI you forgot to remove that bit:

+ "SINGLELINE|SINGLESTEP|SHOW_ALL_RESULTS"))

Indeed. I found another such instance in "help.c".

Also copydml does not seem to be exercised with combined
queries, so do we need this chunk:

--- a/src/test/regress/sql/copydml.sql

Yep, because I reorganized the notice code significantly, and I wanted
to be sure that the right notices are displayed in the right order,
which does not show if the trigger just says "NOTICE: UPDATE 8".

Attached a v2 for the always-show-all-results variant. Thanks for the
debug!

I have some comments on this patch.

I'm +1 for always output all results without having knobs.

Documentation (psql-ref.sgml) has another place that needs the
same amendment.

Looking the output for -t, -0, -A or something like, we might need
to introduce result-set separator.

# -eH looks broken for me but it would be another issue.

Valid setting of FETCH_COUNT disables this feature. I think it is
unwanted behavior.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hello Kyotaro-san,

Attached a v2 for the always-show-all-results variant. Thanks for the
debug!

I have some comments on this patch.

I'm +1 for always output all results without having knobs.

That makes 4 opinions expressed towards this change of behavior, and none
against.

Documentation (psql-ref.sgml) has another place that needs the
same amendment.

Indeed.

Looking the output for -t, -0, -A or something like, we might need
to introduce result-set separator.

Yep, possibly. I'm not sure this is material for this patch, though.

# -eH looks broken for me but it would be another issue.

It seems to work for me. Could you be more precise about how it is broken?

Valid setting of FETCH_COUNT disables this feature. I think it is
unwanted behavior.

Yes and no: this behavior (bug, really) is pre-existing, FETCH_COUNT does
not work with combined queries:

sh> /usr/bin/psql
psql (12beta2 ...)
fabien=# \set FETCH_COUNT 2
fabien=# SELECT 1234 \; SELECT 5432 ;
fabien=#

same thing with pg 11.4, and probably down to every version of postgres
since the feature was implemented...

I think that fixing this should be a separate bug report and patch. I'll
try to look at it.

Thanks for the feedback. Attached v3 with further documentation updates.

--
Fabien.

Fabien COELHO wrote:

sh> /usr/bin/psql
psql (12beta2 ...)
fabien=# \set FETCH_COUNT 2
fabien=# SELECT 1234 \; SELECT 5432 ;
fabien=#

same thing with pg 11.4, and probably down to every version of postgres
since the feature was implemented...

I think that fixing this should be a separate bug report and patch. I'll
try to look at it.

That reminds me that it was already discussed in [1]/messages/by-id/a0a854b6-563c-4a11-bf1c-d6c6f924004d@manitou-mail.org. I should add the
proposed fix to the next commitfest.

[1]: /messages/by-id/a0a854b6-563c-4a11-bf1c-d6c6f924004d@manitou-mail.org
/messages/by-id/a0a854b6-563c-4a11-bf1c-d6c6f924004d@manitou-mail.org

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

Thanks for the feedback. Attached v3 with further documentation updates.

Attached v4 also fixes pg_stat_statements non regression tests, per pg
patch tester travis run.

--
Fabien.

Hello, Fabien.

At Fri, 26 Jul 2019 08:19:47 +0000 (GMT), Fabien COELHO <coelho@cri.ensmp.fr> wrote in <alpine.DEB.2.21.1907260738240.13195@lancre>

Hello Kyotaro-san,

Attached a v2 for the always-show-all-results variant. Thanks for the
debug!

I have some comments on this patch.

I'm +1 for always output all results without having knobs.

That makes 4 opinions expressed towards this change of behavior, and
none against.

Documentation (psql-ref.sgml) has another place that needs the
same amendment.

Indeed.

Looking the output for -t, -0, -A or something like, we might need
to introduce result-set separator.

Yep, possibly. I'm not sure this is material for this patch, though.

I'm fine with that.

# -eH looks broken for me but it would be another issue.

It seems to work for me. Could you be more precise about how it is
broken?

It emits bare command string before html result. It's not caused
by this patch.

Valid setting of FETCH_COUNT disables this feature. I think it is
unwanted behavior.

Yes and no: this behavior (bug, really) is pre-existing, FETCH_COUNT
does not work with combined queries:

sh> /usr/bin/psql
psql (12beta2 ...)
fabien=# \set FETCH_COUNT 2
fabien=# SELECT 1234 \; SELECT 5432 ;
fabien=#

same thing with pg 11.4, and probably down to every version of
postgres
since the feature was implemented...

I think that fixing this should be a separate bug report and
patch. I'll try to look at it.

Ah, I didin't notieced that. Thanks for the explanation.

Thanks for the feedback. Attached v3 with further documentation
updates.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hello.

On 2019/07/29 6:36, Fabien COELHO wrote:>

Thanks for the feedback. Attached v3 with further documentation updates.

Attached v4 also fixes pg_stat_statements non regression tests, per pg
patch tester travis run.

Thanks. I looked this more closely.

+ * Marshal the COPY data.  Either subroutine will get the
+ * connection out of its COPY state, then call PQresultStatus()
+ * once and report any error.

This comment doesn't explain what the result value means.

+ * When our command string contained a COPY FROM STDIN or COPY TO STDOUT,
+ * the PGresult associated with these commands must be processed.  In that
+ * event, we'll marshal data for the COPY.

I think this is not needed. This phrase was needed to explain why
we need to loop over subsequent results after PQexec in the
current code, but in this patch PQsendQuery is used instead,
which doesn't suffer somewhat confusing behavior. All results are
handled without needing an unusual processing.

+ * Update result if further processing is necessary.  (Returning NULL
+ * prevents the command status from being printed, which we want in that
+ * case so that the status line doesn't get taken as part of the COPY data.)

It seems that the purpose of the returned PGresult is only
printing status of this COPY. If it is true, I'd like to see
something like the following example.

| Returns result in the case where queryFout is safe to output
| result status. That is, in the case of COPY IN, or in the case
| where COPY OUT is written to other than pset.queryFout.

+    if (!AcceptResult(result, false))
+    {
+      /* some error occured, record that */
+      ShowNoticeMessage(&notes);

The comment in the original code was:

- /*
- * Failure at this point is always a server-side failure or a
- * failure to submit the command string. Either way, we're
- * finished with this command string.
- */

The first half of the comment seems to be true for this
patch. Don't we preserve that comment?

+    success = handleCopyOut(pset.db,
+                copystream,
+                &copy_result)
+      && success
+      && (copystream != NULL);

success is always true at thie point so "&& success" is no longer
useful. (It is same for the COPY IN case).

+    /* must handle COPY before changing the current result */
+    result_status = PQresultStatus(result);
+    if (result_status == PGRES_COPY_IN ||
+      result_status == PGRES_COPY_OUT)

I didn't get "before changing the curren result" in the
comment. Isn't "handle COPY stream if any" enough?

+    if (result_status == PGRES_COPY_IN ||
+      result_status == PGRES_COPY_OUT)
+    {
+      ShowNoticeMessage(&notes);
+      HandleCopyResult(&result);
+    }

It seems that it is wrong that this ignores the return value of
HandleCopyResult().

+    /* timing measure before printing the last result */
+    if (last && pset.timing)

I'm not sure whether we reached any consensus with ths
behavior. This means the timing includes result-printing time of
other than the last one. If we don't want include printing time
at all, we can exclude it with a small amount of additional
complexity.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center