public inbox for [email protected]
help / color / mirror / Atom feedFrom: Fabien COELHO <[email protected]>
To: Michael Paquier <[email protected]>
Cc: Julien Rouhaud <[email protected]>
Cc: [email protected] <[email protected]>
Cc: [email protected] <[email protected]>
Subject: Re: psql - add SHOW_ALL_RESULTS option
Date: Fri, 9 Apr 2021 08:47:07 +0200 (CEST)
Message-ID: <alpine.DEB.2.22.394.2104090828330.3253600@pseudo> (raw)
In-Reply-To: <YG+okFpc3JaKN/[email protected]>
References: <[email protected]>
<alpine.DEB.2.22.394.2103141046590.164573@pseudo>
<[email protected]>
<alpine.DEB.2.22.394.2104062323150.2555247@pseudo>
<OSZPR01MB631017521EE6887ADC9492E8FD759@OSZPR01MB6310.jpnprd01.prod.outlook.com>
<alpine.DEB.2.22.394.2104071516470.2724014@pseudo>
<OSZPR01MB6310325023E8B4109BB2AB90FD749@OSZPR01MB6310.jpnprd01.prod.outlook.com>
<20210408163050.fca7dwb37zszbsmm@nol>
<alpine.DEB.2.22.394.2104081858500.3192110@pseudo>
<20210408171135.ekhdf5nvqss554q3@nol>
<YG+okFpc3JaKN/[email protected]>
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.
Attachments:
[text/x-diff] fix-cancel-2.patch (563B, ../alpine.DEB.2.22.394.2104090828330.3253600@pseudo/2-fix-cancel-2.patch)
download | inline diff:
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 028a357991..5355086fe2 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -1117,7 +1117,6 @@ SendQueryAndProcessResults(const char *query, double *pelapsed_msec, bool is_wat
INSTR_TIME_SET_CURRENT(before);
success = PQsendQuery(pset.db, query);
- ResetCancelConn();
if (!success)
{
@@ -1486,6 +1485,8 @@ SendQuery(const char *query)
sendquery_cleanup:
+ ResetCancelConn();
+
/* reset \g's output-to-filename trigger */
if (pset.gfname)
{
view thread (104+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: psql - add SHOW_ALL_RESULTS option
In-Reply-To: <alpine.DEB.2.22.394.2104090828330.3253600@pseudo>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox