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.
--
Michael