search

UnwashedMeme / clsql

Several branches containing different patches, see the wiki for branch descriptions
Other
20 stars 12 forks source link

MySQL CLIENT_MULTI_STATEMENTS support #4

Closed deadtrickster closed 10 years ago

deadtrickster commented 10 years ago

Reusing pooled connection after calling stored procedure generates the following: ` Database connection #<CLSQL-MYSQL:MYSQL-DATABASE ......... OPEN {1010193913}> had an error while acquiring from the pool:

<CLSQL-SYS:SQL-DATABASE-DATA-ERROR {10103116F3}>

. In fact MySQL responds with Error 2014 / Commands out of sync; you can't run this command now toshow errors limit 1` which is executed while acquiring connection from pool.

As a reference for quick fix I've used example from here: http://dev.mysql.com/doc/refman/5.1/en/c-api-multiple-queries.html. Unfortunately I messed up tabs in mysql-sql.lisp, and there is no tests -> no pull request. Link to my commit: https://github.com/Publitechs/clsql/commit/4f278d8fd5eed99e37cb11995a74ec3ec0113f62 Link to changes in mysql-sql.lisp https://github.com/Publitechs/clsql/blob/4f278d8fd5eed99e37cb11995a74ec3ec0113f62/db-mysql/mysql-sql.lisp#L250-297

bobbysmith007 commented 10 years ago

Thanks for taking the time to work this up. The mysql backend is probably the one most in need of attention and also the one I tend to need the least, so I appreciate you taking the time to track this down.

Reading the commit, it seems that while the comment is implying this is mostly about connection pooling (which I thought was working on the mysql backend), it seems to be more about allowing multiple result sets.

Multiple result sets is great, but I do want to get this committed with correct comments and tests documenting the failure case. I am currently trying to get a minimal reproduction case, then I will attempt to merge in your changes.

Additionally it sounds more like we need to be doing something like mysql_reset_connection() (mysql 5.7) when we release a connection to the pool, rather than relying on multiple result sets to accomplish this. The show errors limit 1 is never intended to return anything to userland, its simply used to verify that the connection we got from the pool is actually still valid (previously we would occasionally return invalid connections to the user).

bobbysmith007 commented 10 years ago

ps you seem to have accidentally committed an emacs #backupfile#

deadtrickster commented 10 years ago

I created branch for this https://github.com/Publitechs/clsql/tree/mysql_client_multi_statement

I hope I've included everything relevant, but looks like my indentation style in conflict with original. Also I'm thinking about tests... do you have any suggestions?

deadtrickster commented 10 years ago

As for mysql_reset_connection looks like here we need to know server version before calling this. Because now for example I'm working with 5.1.x.

bobbysmith007 commented 10 years ago

FYI: I got a working reproduction test of this bug in place.

https://github.com/UnwashedMeme/clsql/tree/issue-4-mysql-connection-pooling-bugs