|From:||Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>|
|To:||Michael Paquier <michael(at)paquier(dot)xyz>|
|Cc:||Julien Rouhaud <rjuju123(at)gmail(dot)com>, "shiy(dot)fnst(at)fujitsu(dot)com" <shiy(dot)fnst(at)fujitsu(dot)com>, "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org>|
|Subject:||Re: psql - add SHOW_ALL_RESULTS option|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
> 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
> 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
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.
|Next Message||Fabien COELHO||2021-04-09 06:52:20||Re: psql - add SHOW_ALL_RESULTS option|
|Previous Message||Robins Tharakan||2021-04-09 06:44:54||Re: buildfarm instance bichir stuck|