psql - add SHOW_ALL_RESULTS option

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
27 messages Options
12
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
Reply | Threaded
Open this post in threaded view
|

RE: psql - add SHOW_ALL_RESULTS option

Fabien COELHO-3

> Here is a v3 which fixes \errverbose, hopefully.

V5 is a rebase and an slightly improved documentation.

--
Fabien.

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

RE: psql - add SHOW_ALL_RESULTS option

Fabien COELHO-3

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

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

Re: psql - add SHOW_ALL_RESULTS option

Peter Eisentraut-6
In reply to this post by Daniel Verite
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:
https://www.postgresql.org/message-id/flat/4580ff7b-d610-eaeb-e06f-4d686896b93b%402ndquadrant.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


Reply | Threaded
Open this post in threaded view
|

Re: psql - add SHOW_ALL_RESULTS option

Fabien COELHO-3

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:
> https://www.postgresql.org/message-id/flat/4580ff7b-d610-eaeb-e06f-4d686896b93b%402ndquadrant.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.


Reply | Threaded
Open this post in threaded view
|

Re: psql - add SHOW_ALL_RESULTS option

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


Reply | Threaded
Open this post in threaded view
|

Re: psql - add SHOW_ALL_RESULTS option

Fabien COELHO-3

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.

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

Re: psql - add SHOW_ALL_RESULTS option

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


Reply | Threaded
Open this post in threaded view
|

Re: psql - add SHOW_ALL_RESULTS option

Fabien COELHO-3

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.

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

Re: psql - add SHOW_ALL_RESULTS option

Kyotaro Horiguchi-4
Hello.

At Thu, 25 Jul 2019 21:42:11 +0000 (GMT), Fabien COELHO <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: psql - add SHOW_ALL_RESULTS option

Fabien COELHO-3

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.

psql-always-show-results-3.patch (47K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: psql - add SHOW_ALL_RESULTS option

Daniel Verite
        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]. I should add the
proposed fix to the next commitfest.


[1]
https://www.postgresql.org/message-id/flat/a0a854b6-563c-4a11-bf1c-d6c6f924004d%40manitou-mail.org


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
In reply to this post by Fabien COELHO-3

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

psql-always-show-results-4.patch (51K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: psql - add SHOW_ALL_RESULTS option

Kyotaro Horiguchi-4
In reply to this post by Fabien COELHO-3
Hello, Fabien.

At Fri, 26 Jul 2019 08:19:47 +0000 (GMT), Fabien COELHO <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: psql - add SHOW_ALL_RESULTS option

Kyotaro Horiguchi-4
In reply to this post by Fabien COELHO-3
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


12