search

MonetDB / pymonetdb

The Python API for MonetDB
https://www.monetdb.org/
Mozilla Public License 2.0
28 stars 20 forks source link

Multi line response handling #11

Closed kutsurak closed 8 years ago

kutsurak commented 8 years ago

Hello Gijs,

Continuing from yesterday's PR I noticed the following:

In mapi.py, when executing a command the response might consist of multiple lines. For instance if autocommit mode is on and one transaction fails because of concurrency conflicts the client gets the following response:

>>&2 1 -1
!40000!COMMIT: transaction is aborted because of concurrency conflicts, will ROLLBACK instead
<<

(I have added the >> and << sequence to indicate newlines).

In this case I believe that an exception should be raised, since the client should be notified that the transaction failed. What do you think? I am not an expert in the mapi protocol (not by a long shot), but I will try to create some tests and send a pull request early next week. Would that be ok with you?

Best regards, Panos.

gijzelaerr commented 8 years ago

yeah sure, please do so. and yes a test would be perfect so we know it works correctly.

kutsurak commented 8 years ago

Hi,

after some discussions with people in the group, we are thinking that this kind of thing will happen only when an update transaction fails in autocommit mode. Of course there can be multi-line responses, as with the '#' character, but in this case we have a header '&2', and after that an error, and I think that this is what confuses pymonetdb.

One problem is that it's kind of hard to write a test replicating this: you would need two threads performing inserts in autocommit mode, but even then you don't know exactly which transaction will fail.

I have a quick and dirty fix (kutsurak/pymonetdb@eee153e9ec23901bd31f9e8b89b9e7e665335b37) on my fork, but I wouldn't want to create a PR until I've tested a bit more. Another point is that this fix could potentially kill the performance, when you have a big result set (I would need to test for that as well). I will take a look at the JDBC driver to see how they solved this, and if their solution is applicable.

Best regards, Panos.

kutsurak commented 8 years ago

Hi,

I wanted to ask how do you feel about mocking? Since this problem happens in a non-deterministic (when a transaction fails due to ) manner I don't see an easy way to write a test without mocking the connection to monetdb. I was thinking to use the pymock package, but I wanted to ask first since I will need to add a new dependency. There is mocking support in the standard library after version 3.3 but I see that you are testing for 2.6 and 2.7 as well.

Panos.

gijzelaerr commented 8 years ago

o sure! Mocking is a great idea, making it much more like proper unittest. The test suite is a bit of a mess actually, but I never took the time to clean it up.

I would recommend using https://pypi.python.org/pypi/mock for python 2.x, since that it the packport of the python3 one. If python2.6 support is becoming a problem then at some point we should just drop that support.

kutsurak commented 8 years ago

Great! I will start ASAP.

kutsurak commented 8 years ago

Hopefully this is fixed with #12

kutsurak commented 8 years ago

Hi,

While testing a different application than the one that prompted #10 and #12, I got an exception in mapi.py with python 2.7, but not with python 3.5. It all works perfectly with pymonetdb 1.0.2, so I think I missed something in one of those PRs. I will take a more detailed look tomorrow. I am really sorry about this.

Panos.

gijzelaerr commented 8 years ago

well, nobody complained yet! Don't worry, probably we are good. I'm busy moving to South Africa at the moment so can't revert the release. But the older versions are still in pip so people can use those. Quite sure it is something simple, let me know when you isolated the problem.

kutsurak commented 8 years ago

It seems to be working without issues today... Most probably this had something to do with the other application I was working on. I will keep it in mind if in encounter problems in the future, but yes, I agree that we should be OK. Good luck with your moving :)