On 2016/04/28 13:45, Michael Paquier wrote:
> On Wed, Apr 27, 2016 at 12:16 PM, Etsuro Fujita
> <fujita.etsuro@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