Обсуждение: psql - add SHOW_ALL_RESULTS option

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.

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
itis 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:
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

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.

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

> 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(¬es);

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,
+                ©_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(¬es);
+      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

Hello Kyotaro-san,

> Thanks. I looked this more closely.

Indeed! Thanks for this detailed review.

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

Ok, added.

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

Hmmm. More or less. "COPY" commands have two results, one for taking over 
the input or output streams more or less directly at the protocol level, 
and one for the final summary, which is quite special compared to other 
commands, all that managed in "copy.c". So ISTM that the comment is 
somehow still appropriate.

The difference with the previous behavior is that other results could be 
immediately discarded but these ones, while now they are all processed.

I've kept this comment and added another one to try to make that clear.

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

I have tried to improved the comment based on your suggestion.

> +    if (!AcceptResult(result, false))
> +    {
> +      /* some error occured, record that */
> +      ShowNoticeMessage(¬es);
>
> 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?

Ok. Some form put back.

> +    success = handleCopyOut(pset.db,
> +                copystream,
> +                ©_result)
> +      && success
> +      && (copystream != NULL);
>
> success is always true at thie point so "&& success" is no longer
> useful.

Ok.

> (It is same for the COPY IN case).

Ok.

> +    /* 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?

Alas, I think not.

The issue is that I need to know whether this is the last result (eg \gset 
applies only on the last result), so I'll call PQgetResult() to get that.

However, on COPY, this is the second "final" result which says how much 
was copied. If I have not send/received the data, the count will not be 
right.

> +    if (result_status == PGRES_COPY_IN ||
> +      result_status == PGRES_COPY_OUT)
> +    {
> +      ShowNoticeMessage(¬es);
> +      HandleCopyResult(&result);
> +    }
>
> It seems that it is wrong that this ignores the return value of
> HandleCopyResult().

Yep. Fixed.

> +    /* 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.

I think that this point is desperate, because the way timing is 
implemented client-side.

Although we could try to stop timing before each result processing, it 
would not prevent the server to go on with other queries and send back 
results, psql to receive further results (next_result), so the final 
figures would be stupid anyway, just another form of stupid.

Basically the approach cannot work with combined queries: It only worked 
before because the intermediate results were coldly discarded.

Maybe the server to report its execution times for each query somehow, but 
then the communication time would not be included.

I have added a comment about why timing does not make much sense with 
combined queries.

Attached a v5.

-- 
Fabien.

This v6 is just Fabien's v5, rebased over a very minor conflict, and
pgindented.  No further changes.  I've marked this Ready for Committer.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Fri, Sep 13, 2019 at 1:01 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>
> This v6 is just Fabien's v5, rebased over a very minor conflict, and
> pgindented.  No further changes.  I've marked this Ready for Committer.
>
Should we add function header for the below function to maintain the
common standard of this file:
+
+static void
+AppendNoticeMessage(void *arg, const char *msg)
+{
+ t_notice_messages *notes = (t_notice_messages *) arg;
+
+ appendPQExpBufferStr(notes->in_flip ? ¬es->flip : ¬es->flop, msg);
+}
+
+static void
+ShowNoticeMessage(t_notice_messages *notes)
+{
+ PQExpBufferData *current = notes->in_flip ? ¬es->flip : ¬es->flop;
+
+ if (current->data != NULL && *current->data != '\0')
+ pg_log_info("%s", current->data);
+ resetPQExpBuffer(current);
+}
+
+/*
+ * SendQueryAndProcessResults: utility function for use by SendQuery() only
+ *

+static void
+ShowErrorMessage(const PGresult *result)
+{
+ const char *error = PQerrorMessage(pset.db);
+
+ if (strlen(error))
+ pg_log_info("%s", error);
+
+ CheckConnection();
+}

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

> Should we add function header for the below function to maintain the
> common standard of this file:

Yes. Attached v6 does that.

-- 
Fabien.

On Fri, Sep 20, 2019 at 1:41 PM Fabien COELHO <coelho@cri.ensmp.fr> wrote:
>
>
> > Should we add function header for the below function to maintain the
> > common standard of this file:
>
> Yes. Attached v6 does that.
>
Thanks for fixing it.

The below addition can be removed, it seems to be duplicate:
@@ -3734,6 +3734,11 @@ listTables(const char *tabtypes, const char
*pattern, bool verbose, bool showSys
  translate_columns[cols_so_far] = true;
  }

+ /*
+ * We don't bother to count cols_so_far below here, as there's no need
+ * to; this might change with future additions to the output columns.
+ */
+
  /*
  * We don't bother to count cols_so_far below here, as there's no need
  * to; this might change with future additions to the output columns.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

> The below addition can be removed, it seems to be duplicate:

Indeed. I'm unsure how this got into the patch, probably some rebase 
mix-up. Attached v7 removes the duplicates.

-- 
Fabien.

>> The below addition can be removed, it seems to be duplicate:
>
> Indeed. I'm unsure how this got into the patch, probably some rebase mix-up. 
> Attached v7 removes the duplicates.

Attached patch v8 is a rebase.


-- 
Fabien.

Hi,

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?

regards

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

Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
> 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.)  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 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.  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.

            regards, tom lane

On Thu, Jan 16, 2020 at 01:08:16PM -0500, Tom Lane wrote:
>Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
>> 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.)  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 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.  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.
>

I don't know, really, I only pinged this as a CFM who sees a patch
marked as RFC for months ...

The current behavior certainly seems strange/wrong to me - if I send
multiple queries to psql, I'd certainly expect results for all of them,
not just the last one. So the current behavior seems pretty surprising.

I'm unable to make any judgments about risks/benefits of this change. I
can't imagine anyone intentionally relying on the current behavior, so
I'd say the patch is unlikely to break anything (which is not already
broken). But I don't have any data to support this ...

Essentially, I'm just advocating to make a decision - we should either
commit or reject the patch, not just move it to the next commitfest over
and over.


regards

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

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 Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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.

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

    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

"Daniel Verite" <daniel@manitou-mail.org> 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

    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

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

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.

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.

On Sun, Jun 7, 2020 at 2:36 AM Fabien COELHO <coelho@cri.ensmp.fr> 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.

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

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

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

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.

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,10000000);
^C^CINSERT 0 10000000

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

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,10000000);
> ^C^CINSERT 0 10000000
>
> 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.

> 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

On Thu, Apr 08, 2021 at 01:32:12AM +0000, shiy.fnst@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?

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.

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!

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

On Fri, Apr 9, 2021 at 6:36 AM Michael Paquier <michael@paquier.xyz> 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

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.

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

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

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)

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

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...". 
1425                     if (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))
1430                         svptcmd = 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

... 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     {
1081         PQExpBufferData    *current = notes->in_flip ? ¬es->flip : ¬es->flop;
1082         if (current->data != NULL && *current->data != '\0')
1083             pg_log_info("%s", current->data);
>>>     CID 1476041:  Null pointer dereferences  (FORWARD_NULL)
>>>     Passing "current" to "resetPQExpBuffer", which dereferences null "current->data".
1084         resetPQExpBuffer(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

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

Michael Paquier <michael@paquier.xyz> 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

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

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.

Fabien COELHO <coelho@cri.ensmp.fr> 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

On 4/12/21, 9:25 AM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
> Fabien COELHO <coelho@cri.ensmp.fr> 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

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

On Mon, Apr 12, 2021 at 07:08:21PM +0000, 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

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

Michael Paquier <michael@paquier.xyz> 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

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.

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.

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.

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.

On Sat, Jun 12, 2021 at 3:11 PM Fabien COELHO <coelho@cri.ensmp.fr> 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

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

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.

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

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.

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.

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

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

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.

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.1618154047@sss.pgh.pa.us

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.

Hello Peter,

> Attached v9 integrates your tests and makes them work.

Attached v11 is a rebase.

-- 
Fabien.

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-BCCACECB6BEE@amazon.com

[1]: https://www.postgresql.org/message-id/2902362.1618244606@sss.pgh.pa.us

[2]: https://www.postgresql.org/message-id/2680034.1618157764@sss.pgh.pa.us

> On 8 Oct 2021, at 14:15, Peter Eisentraut <peter.eisentraut@enterprisedb.com> 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

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.

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.

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.

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

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.

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.

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.

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

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.

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.

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.

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?

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.

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?

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?

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?

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

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

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.

Peter Eisentraut <peter.eisentraut@enterprisedb.com> 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

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.

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:<stdin>:2: FATAL:  terminating connection due to 
administrator command
+psql:<stdin>: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:<stdin>:2: fatal: connection to server was lost', "expected error 
message");

The expected output (which passes) contains this line twice:

psql:<stdin>:2: FATAL:  terminating connection due to administrator command
psql:<stdin>: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.

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.

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:<stdin>:2: FATAL:  terminating connection due to administrator command
> psql:<stdin>: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.

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.

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

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.

Attached a rebase.

-- 
Fabien.

> Attached a rebase.

Again, after the SendQuery refactoring extraction.

-- 
Fabien.

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.

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

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.

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

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.

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