Re: psql - add SHOW_ALL_RESULTS option
Fabien COELHO <coelho@cri.ensmp.fr>
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>
Date: 2021-04-09T06:47:07Z
Lists: pgsql-hackers
Attachments
- fix-cancel-2.patch (text/x-diff) patch
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.
Commits
-
psql: Show all query results by default
- 7844c9918a43 15.0 landed
- 3a5130672296 14.0 landed
-
psql: Refactor ProcessResult()
- 5519d5affdfa 15.0 landed
-
psql: Refactor SendQuery()
- 8910a25fef3d 15.0 landed
-
psql: Add test for psql behavior on server crash
- 8cd7627c7b19 15.0 landed
-
psql: Additional tests
- 9028cce426ba 15.0 landed
-
Improve some psql test code
- fbee60f6a4ff 15.0 landed
-
psql: Fix some scan-build warnings
- 390edeeb570c 15.0 landed
-
psql: Add test for handling of replication commands
- 67c069848a99 15.0 landed
-
psql: More tests
- c0280bc3edeb 15.0 landed
-
psql: Add various tests
- 14d755b00037 15.0 landed
-
psql: Add test for query canceling
- 5b3f471ff23a 15.0 landed
-
Revert "psql: Show all query results by default"
- fae65629cec8 14.0 landed
-
Extend a test case a little
- b3a9e9897ec7 14.0 landed