Date: Fri, 9 Apr 2021 08:47:07 +0200 (CEST)
From: Fabien COELHO <coelho@cri.ensmp.fr>
To: Michael Paquier <michael@paquier.xyz>
cc: Julien Rouhaud <rjuju123@gmail.com>,
    "shiy.fnst@fujitsu.com" <shiy.fnst@fujitsu.com>,
    "pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>
Subject: Re: psql - add SHOW_ALL_RESULTS option
In-Reply-To: <YG+okFpc3JaKN/zB@paquier.xyz>
Message-ID: <alpine.DEB.2.22.394.2104090828330.3253600@pseudo>
References: <d5b5b051-61e4-9e45-a035-e4b291f94e3b@2ndquadrant.com>
 <alpine.DEB.2.22.394.2103141046590.164573@pseudo>
 <1ec5ef49-b5c8-4129-e680-fe42f8d0670b@2ndquadrant.com>
 <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/zB@paquier.xyz>
User-Agent: Alpine 2.22 (DEB 394 2020-01-19)
MIME-Version: 1.0
Content-Type: multipart/mixed;
 BOUNDARY="2056645051-296723125-1617950672=:3253600"
Content-ID: <alpine.DEB.2.22.394.2104090844520.3253600@pseudo>
Archived-At: 
 <https://www.postgresql.org/message-id/alpine.DEB.2.22.394.2104090828330.3253600%40pseudo>
Precedence: bulk

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--2056645051-296723125-1617950672=:3253600
Content-Type: text/plain; CHARSET=UTF-8; format=flowed
Content-Transfer-Encoding: 8BIT
Content-ID: <alpine.DEB.2.22.394.2104090844521.3253600@pseudo>


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.
--2056645051-296723125-1617950672=:3253600
Content-Type: text/x-diff; name=fix-cancel-2.patch
Content-Transfer-Encoding: BASE64
Content-ID: <alpine.DEB.2.22.394.2104090847070.3253600@pseudo>
Content-Description: 
Content-Disposition: attachment; filename=fix-cancel-2.patch

ZGlmZiAtLWdpdCBhL3NyYy9iaW4vcHNxbC9jb21tb24uYyBiL3NyYy9iaW4v
cHNxbC9jb21tb24uYw0KaW5kZXggMDI4YTM1Nzk5MS4uNTM1NTA4NmZlMiAx
MDA2NDQNCi0tLSBhL3NyYy9iaW4vcHNxbC9jb21tb24uYw0KKysrIGIvc3Jj
L2Jpbi9wc3FsL2NvbW1vbi5jDQpAQCAtMTExNyw3ICsxMTE3LDYgQEAgU2Vu
ZFF1ZXJ5QW5kUHJvY2Vzc1Jlc3VsdHMoY29uc3QgY2hhciAqcXVlcnksIGRv
dWJsZSAqcGVsYXBzZWRfbXNlYywgYm9vbCBpc193YXQNCiAJCUlOU1RSX1RJ
TUVfU0VUX0NVUlJFTlQoYmVmb3JlKTsNCiANCiAJc3VjY2VzcyA9IFBRc2Vu
ZFF1ZXJ5KHBzZXQuZGIsIHF1ZXJ5KTsNCi0JUmVzZXRDYW5jZWxDb25uKCk7
DQogDQogCWlmICghc3VjY2VzcykNCiAJew0KQEAgLTE0ODYsNiArMTQ4NSw4
IEBAIFNlbmRRdWVyeShjb25zdCBjaGFyICpxdWVyeSkNCiANCiBzZW5kcXVl
cnlfY2xlYW51cDoNCiANCisJUmVzZXRDYW5jZWxDb25uKCk7DQorDQogCS8q
IHJlc2V0IFxnJ3Mgb3V0cHV0LXRvLWZpbGVuYW1lIHRyaWdnZXIgKi8NCiAJ
aWYgKHBzZXQuZ2ZuYW1lKQ0KIAl7DQo=

--2056645051-296723125-1617950672=:3253600--