MySQL adapter update to enable use of stored procedures

I totally might be missing something, but in general it would be nice to see something a bit more DB agnostic. Perhaps something in the AbstractAdapter, maybe in DatabaseStatements at the class level. It could raise a not implemented behavior by default and/or it could just lay the foundation with a partial implementation. This is what I have in the SQL Server adapter now.

def execute_procedure(proc_name, *variables)
  vars = variables.map{ |v| quote(v) }.join(', ')
  sql = "EXEC #{proc_name} #{vars}".strip
  select(sql,'Execute Procedure',true).inject([]) do |results,row|
    results << row.with_indifferent_access
  end
end

This patch fixes a configuration bug with the MySQL adapter. Adding an abstraction for executing procedures in general is interesting but that's not my intent here.

Glad someone finally fixed this. Calling stored procedures from your app is pretty basic functionality to have been missing from the rails/mysql stack.

I sure could have used this in my last project.

Why does this need a configuration option? What breaks if we always enable CLIENT_MULTI_RESULTS?

Basically, fear. Who knows what effect this flag has on the native driver? The adapter hasn't changed in a long time and I didn't want apps to get broken unexpectedly.

Maybe it would make sense to make this the default in 3.0, but leave it opt-in for 2.x?

No fear needed: the mysql driver handles multiple results.

First report and patch at http://dev.rubyonrails.org/ticket/5260

Could you change the patch to always enable the flag and update the test to check that the query returns the expected results, rather than just doesn't raise an exception?

Could you change the patch to always enable the flag and update the test to check that the query returns the expected results, rather than just doesn't raise an exception?

Will do!

What branch/tag/commit should I format the new patch against? This one was against the v2.3.4 tag.

Could you change the patch to always enable the flag and update the test to check that the query returns the expected results, rather than just doesn't raise an exception?

On further reflection, as mentioned in the older patch, this flag is only compatible with MySQL versions > 4.1.1. 3 years ago that was an unacceptable constraint. Is that still true?

If the driver can do a version check then its a moot point but I don't know if that's possible (yet).

Can add the flag if Mysql.client_version >= 40101

First report and patch at http://dev.rubyonrails.org/ticket/5260 Could you change the patch to always enable the flag and update the test to check that the query returns the expected results, rather than just doesn't raise an exception?

I really like the original patch and I've changed my solution to match. This new patch allows you to specify flags for the MySQL driver (including CLIENT_MULTI_RESULTS, among others). Now anyone who needs the flag, or any future flags, can have them.

It's pretty easy to use - add a "flags" key to your database.yml:

development:
  adapter: mysql
...
  flags: CLIENT_MULTI_RESULTS
...

As can be seen, multiple formats are supported. The attached patch replaces my previous one. New unit tests have been added to make sure flags are interpreted correctly. This patch is against the v2.3.4 tag.

What do you think?

Could you ... update the test to check that the query returns the expected results ...

Forgot to mention I made this change also.

I think database YAML files should try to stay as agnostic as possible. It could be argued that the flags key would not be used in other adapters, inside core or not, and even if so the value collisions would still be minimal. That said, it could be bike shedding and a matter of taste, but I think the database YAML files should try not to allow specific keys from a core point of view.

If this is a limitation of MySQL that you have to pass these down in some way, then I guess there is no way to avoid it. But if this was something I was doing on the SQL Server adapter, I would reflect internally as much as I could to the DB server for what I needed. Along those lines, I really liked Jeremy's comments and suggestions.

If this is a limitation of MySQL that you have to pass these down in some way, then I guess there is no way to avoid it.

That is pretty much it. I can't think of a another way to get adapter-specific flags to the
adapter. Is there another way in which Rails allows the database connection to be configured?

Well there seems to be a few ways. One is like reflecting on the DB at hand. I like Jeremy's suggestion for doing something like this "Mysql.client_version >= 40101" if that "flag" is version specific to the underlying DB. IE, if the version is high enough and the flag is on all the time and causes no issues... then just assume that people are allowed to execute procedures.

On the flip side, AbstractAdapter and its kin, have methods that set basic defaults like supports_ddl_transaction? etc that each subclass can change. I'm not saying this is the way to go by extending abstract with something like supports_stored_procedures? cause what your ticket is more centric on is the underlying what to do even if said method returned yes... IE... your back to the reflecting example above.

I hope this is helpful.

This looks like a non-issue to me. Database YML files are already not database-agnostic. For example the DB2 adapter understands a wealth of keys:

development:
  adapter:     ibm_db
  username:    db2inst1
  password:
  database:    <%= app_name[0,4] %>_dev
  #schema:      db2inst1
  #host:        localhost
  #port:        50000
  #account:     my_account
  #app_user:    my_app_user
  #application: my_application
  #workstation: my_workstation

Agreed. We even have :mode and :dsn in the SQL Server adapter. I just think additions to the core should be at a higher standard.

Personally, I do not like SPs. But if I did choose to use one in MySQL, it would be nice not to add a flag to a config file to do so. I really liked Jeremy's suggestions on how to just set that flag on all the time, especially if all tests pass and reflect on the DB if there are issues with that flag internally in the adapter if the MySQL version of the DB does not support it.

BTW, when I say flag, I am referring to something internal in the mysql_adapter implementation, not an external conf file.

Configurable flags are cool but are a separate concern.

In this case, we just want a default flag. This needs a very tiny adapter change:

default_flags = Mysql.client_version > 40101 ? [Mysql::CLIENT_MULTI_RESULTS] : []
ConnectionAdapters::MysqlAdapter.new(mysql, logger, [host, username, password, database, port, socket, default_flags], config)

Could you split the patch in two and open a new ticket for the second half?

I really liked Jeremy's suggestions on how to just set that flag on all the time ...

The problem with Jeremy's suggestion is that the server version must also be checked. I'd rather not put that cost on everyone using MySQL from Rails. Another choice is to set the flag could just be set for everyone, with no checks, and Rails can be incompatible with MySQL < 4.1.1.

However, I think a better choice is to let the developer decided. With this patch, there is no change to existing behavior, and no additional maintenance/testing work is introduced by the patch. If a developer wants the benefits of this patch, they can choose to use the right flags. If they don't want it, they never even need to know its there.

Like Mike and you have said, there are already a wealth of db-specific flags in the config file. You could argue it's standard practice. Its the first place a Rails developer goes to update their connection information and thus seems a natural place to expose this change.

I've received good advice from Jeremy before on a few of my tickets. I would suggest following his lead and let him assess the penalties and help drive it. For instance, it seemed to me that his recommendation would not cost anything since it would be done once on connection initialization.

Could you split the patch in two and open a new ticket for the second half?

I've created ticket #3204 Enable use of MySQL stored procedures with a patch for this request.

I really like the original patch and I've changed my solution to match. This new patch allows you to specify flags for the MySQL driver (including CLIENT_MULTI_RESULTS, among others).

This patch as submitted comments out some unit tests, accidentally. The attached diff should be correct.

Apologies for the churn on this. Here is the same patch as above, plus the CLIENT_MULTI_RESULTS flag is enabled by default.

Again, patched against the v2.3.4 tag.

It looks like CALLing a MySQL stored procedure from Rails kills the current connection:

$ script/console
Loading development environment (Rails 2.3.5)
>> ActiveRecord::Base.connection.active?
=> true
>> User.all.size
=> 2
>> ActiveRecord::Base.connection.active?
=> true
>> ActiveRecord::Base.connection.execute("CALL proc01")
=> #<Mysql::Result:0x103429c90>
>> ActiveRecord::Base.connection.execute("CALL proc01")
ActiveRecord::StatementInvalid: Mysql::Error: Commands out of sync; 
you can't run this command now: CALL proc01
from /Library/Ruby/Gems/1.8/gems/activerecord-2.3.5/
lib/active_record/connection_adapters/abstract_adapter.rb:219:in log'
from /Library/Ruby/Gems/1.8/gems/activerecord-2.3.5/
lib/active_record/connection_adapters/mysql_adapter.rb:323:inexecute'
from (irb):5
>> ActiveRecord::Base.connection.active?
=> false
>> ActiveRecord::Base.connection.reconnect!
=> nil
>> ActiveRecord::Base.connection.execute("CALL proc01")
=> #<Mysql::Result:0x1034102e0>
>> ActiveRecord::Base.connection.active?
=> false
>> ActiveRecord::Base.connection.reconnect!
=> nil
>> ActiveRecord::Base.connection.execute("CALL proc01")
=> #<Mysql::Result:0x1033fc8a8>
>> ActiveRecord::Base.connection.reconnect!
=> nil
>> ActiveRecord::Base.connection.execute("CALL proc01")
=> #<Mysql::Result:0x1033f0b98>

The problem experienced by Horace Ho results from the database connection being left in an unclean state by 'MysqlAdapter.select'. 'MysqlAdapter.select_rows' has the same problem.

:CLIENT_MULTI_RESULTS is necessary for calling stored procedures but it expects the response to be treated as potentially containing multiple result sets, of course. MysqlAdapter.select and MysqlAdapter.select_rows retrieve the one result set they are expecting but do not request further result sets. MySQL is expecting such a request (even when, as in these cases, there are no more results sets). If a request for another result set is not the next call made through the connection then MySQL deactivates the connection. Such an erroneous call could be made by 'ActiveRecord::Base.connection.active?' :-))

To clean up the connection so that it is ready for subsequent calls, all that is needed is to call 'more_results' and, if it returns true, call 'next_result'. The updated code for 'MysqlAdapter.select' and 'MysqlAdapter.select_rows' is:

  @connection.query_with_result = true
  result = execute(sql, name)
  rows = []
  result.each_hash { |row| rows << row }
  result.free
  connection.more_results && @connection.next_result    # invoking stored procedures with CLIENT_MULTI_RESULTS requires this to tidy up else connection will be dropped
  rows

The additional sixth line is benign to all calling code (all tests pass).

In the attached patch was implemented against Rails 3.0.0.0RC. I have created one test for 'MysqlAdapter.select' and one test for 'MysqlAdapter.select_rows' and added the two lines of code to MysqlAdapter.

(from [90176a6f15a4fddbafb1ab6b445bcc9e6a41e44a]) Bug Fix -- clean up connection after stored procedure [#3151 MySQL adapter update to enable use of stored procedures state:resolved] http://github.com/rails/rails/commit/90176a6f15a4fddbafb1ab6b445bcc...

(from [7ce1539934b67e536446f7323c7848fdd6ba68e3]) Bug Fix -- clean up connection after stored procedure [#3151 MySQL adapter update to enable use of stored procedures state:resolved] http://github.com/rails/rails/commit/7ce1539934b67e536446f7323c7848...

(from [c017041e68c3b7ca4677db0ab420cb7d8bccb72f]) Bug Fix -- clean up connection after stored procedure [#3151 MySQL adapter update to enable use of stored procedures state:resolved] http://github.com/rails/rails/commit/c017041e68c3b7ca4677db0ab420cb...

(from [d35a67bba3b9b108aff953c6d0221fbcc58c65a2]) Bug Fix -- clean up connection after stored procedure [#3151 MySQL adapter update to enable use of stored procedures state:resolved] for 2-3-stable http://github.com/rails/rails/commit/d35a67bba3b9b108aff953c6d0221f...

(from [7e79889d1cd7d9f1b924c3a8d67d2fd70db3fb64]) Bug Fix -- clean up connection after stored procedure [#3151 MySQL adapter update to enable use of stored procedures state:resolved] for 2-3-stable http://github.com/rails/rails/commit/7e79889d1cd7d9f1b924c3a8d67d2f...

There are still problems with this in 2.3.10 and 3.0.3 (I've tried both)

(1) If the stored procedure doesn't return any result set at all, you get the following exception:

/var/lib/gems/1.8/gems/activerecord-3.0.3/lib/active_record/connection_adapters/mysql_adapter.rb:621:in `select': undefined method `each_hash' for nil:NilClass (NoMethodError)
    from /var/lib/gems/1.8/gems/activerecord-3.0.3/lib/active_record/connection_adapters/abstract/database_statements.rb:7:in `select_all'
    from /var/lib/gems/1.8/gems/activerecord-3.0.3/lib/active_record/connection_adapters/abstract/query_cache.rb:56:in `select_all'

[with 2.3.10 the exception is: undefined method 'all_hashes' for nil:NilClass]

Here's how to replicate it:

mysql> drop procedure if exists dummytest;
Query OK, 0 rows affected, 1 warning (0.00 sec)
mysql> delimiter $$
mysql> create procedure dummytest()
    -> begin
    -> end $$
Query OK, 0 rows affected (0.00 sec)
mysql> delimiter ;
mysql>

And the corresponding Ruby code:

require 'rubygems'
require 'active_record'
ActiveRecord::Base.establish_connection(
  :adapter => "mysql",
  :host => "X.X.X.X",
  :user => "XXXX",
  :password => "XXXX",
  :database => "XXXX"
)
p ActiveRecord::Base.connection.select_all("call dummytest")   # raises exception

(2) If the stored procedure does return a result set, but you call it with execute instead of select_all, then the second call raises an out-of-sync exception:

/var/lib/gems/1.8/gems/activerecord-3.0.3/lib/active_record/connection_adapters/abstract_adapter.rb:202:in `log': Mysql::Error: Commands out of sync; you can't run this command now: call simpletest (ActiveRecord::StatementInvalid)
    from /var/lib/gems/1.8/gems/activerecord-3.0.3/lib/active_record/connection_adapters/mysql_adapter.rb:289:in `execute'

To replicate:

mysql> delimiter $$
mysql> create procedure simpletest()
    -> begin
    -> select 1 from dual;
    -> end $$
Query OK, 0 rows affected (0.00 sec)
mysql> delimiter ;

require 'rubygems'
require 'active_record'
ActiveRecord::Base.establish_connection(
  #... as above
)
p ActiveRecord::Base.connection.execute("call simpletest")   # OK
p ActiveRecord::Base.connection.execute("call simpletest")   # Out of sync exception

Now: if you're careful, you can call SP's which don't return any result set with #execute, and those which do return a result set with #select_all, and it all works. Unfortunately, I have some SP's which may or may not return a result set, depending on the arguments passed :-(

Here's a suggested fix so that select_all works in both cases.

--- activerecord-3.0.3/lib/active_record/connection_adapters/mysql_adapter.rb.orig   2011-01-20 15:56:33.666915998 +0000
+++ activerecord-3.0.3/lib/active_record/connection_adapters/mysql_adapter.rb   2011-01-20 15:59:08.236915999 +0000
@@ -617,9 +617,11 @@
         def select(sql, name = nil)
           @connection.query_with_result = true
           result = execute(sql, name)
-          rows = []
-          result.each_hash { |row| rows << row }
-          result.free
+          if result
+            rows = []
+            result.each_hash { |row| rows << row }
+            result.free
+          end
           @connection.more_results && @connection.next_result    # invoking stored procedures with CLIENT_MULTI_RESULTS requires this to tidy up else connection will be dropped
           rows
         end

Until this is fixed, a reasonable workaround in application code is to rescue the NoMethodError, because the stored procedure has successfully run before it's raised.

def execute(proc_name, *args)
  conn = ActiveRecord::Base.connection
  vars = args.map{ |v| conn.quote(v) }.join(',')
  conn.select_all("call #{proc_name}(#{vars})") 
rescue NoMethodError  # bug in select_all if SP returns no result set
end

p execute("dummytest")
p execute("dummytest")
p execute("simpletest")
p execute("simpletest")