psql - add SHOW_ALL_RESULTS option

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

psql - add SHOW_ALL_RESULTS option

Fabien COELHO-3

Hello devs,

The attached patch implements a new SHOW_ALL_RESULTS option for psql,
which shows all results of a combined query (\;) instead of only the last
one.

This solves a frustration that intermediate results were hidden from view
for no good reason that I could think of.

For that, call PQsendQuery instead of (mostly not documented) PQexec, and
rework how results are processed afterwards.

Timing is moved to ProcessQueryResults to keep the last result handling
out of the measured time. I think it would not be a big deal to include
it, but this is the previous behavior.

In passing, refactor a little and add comments. Make function names about
results plural or singular consistently with the fact the it processes one
or several results. Change "PrintQueryResult" to "HandleQueryResult"
because it was not always printing something. Also add a HandleCopyResult
function, which makes the patch a little bigger by moving things around
but clarifies the code.

Code in "common.c" is actually a little shorter than the previous version.
From my point of view the code is clearer than before because there is
only one loop over results, not an implicit one within PQexec and another
one afterwards to handle copy.

Add a few tests for the new feature.

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.

--
Fabien.

psql-show-all-results-1.patch (25K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

RE: psql - add SHOW_ALL_RESULTS option

Iwata, Aya
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



Reply | Threaded
Open this post in threaded view
|

RE: psql - add SHOW_ALL_RESULTS option

Fabien COELHO-3

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.

psql-show-all-results-2.patch (39K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

RE: psql - add SHOW_ALL_RESULTS option

Daniel Verite
        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


Reply | Threaded
Open this post in threaded view
|

RE: psql - add SHOW_ALL_RESULTS option

Fabien COELHO-3

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.


Reply | Threaded
Open this post in threaded view
|

RE: psql - add SHOW_ALL_RESULTS option

Fabien COELHO-3
In reply to this post by Daniel Verite

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.

psql-show-all-results-3.patch (39K) Download Attachment