On 2016/04/28 13:45, Michael Paquier wrote:
On Wed, Apr 27, 2016 at 12:16 PM, Etsuro Fujita
<fujita.ets...@lab.ntt.co.jp> wrote:
On 2016/04/26 21:45, Etsuro Fujita wrote:
While re-reviewing the fix, I noticed that since PQcancel we added to
pgfdw_xact_callback to cancel a DML pushdown query isn't followed by a
ROLLBACK, the connection to the remote server will be discarded at the
end of the while loop in that function, which will cause a FATAL error
of "connection to client lost". Probably, that was proposed by me in
the first version of the patch, but I don't think that's a good idea.
Shouldn't we execute ROLLBACK after that PQcancel?
Another thing I noticed is, ISTM that we miss the case where DML
pushdown queries are performed in subtransactions. I think cancellation
logic would also need to be added to pgfdw_subxact_callback.
Attached is a patch for that.
I have spent some time looking at that...
And yeah, losing the connection because of that is a little bit
annoying if there are ways to make things clean, and as a START
TRANSACTION is always sent for such queries it seems really better to
issue a ROLLBACK in any case. Actually, by using PQcancel there is no
way to be sure if the cancel will be effective or not. So it could be
possible that the command is still able to complete correctly, or it
could be able to cancel correctly and it would return an ERROR
earlier. In any case, doing the ROLLBACK unconditionally seems adapted
to me because we had better clean up the remote state in both cases.
Thanks for the review!
I'll add this to the next CF. I think this should be addressed in
advance of the release of 9.6, though.
Best regards,
Etsuro Fujita
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
