This project is archived and is in readonly mode.

#3151 ✓resolved
Justin Bailey

MySQL adapter update to enable use of stored procedures

Reported by Justin Bailey | September 5th, 2009 @ 05:13 PM | in 2.3.9

Rails apps running against MySQL databases cannot execute stored procedures. Passing the Mysql::CLIENT_MULTI_RESULTS flag to the native driver when the connection is first created fixes the issue. A Google search shows that this problem has been long-standing but unsolved.

This patch adds a configuration option to the MySQL adapter, :enable_call. If database.yml contains this option, the flag will be passed to the driver and the connection will be set up to allow stored procedures. For example, this database.yml uses the new option:

development:
  adapter: mysql
  database: gamesite
  username: root
  password: 
  enable_call: true
  host: localhost
  odbc-name: localhost_gamesite

The option is disabled by default, so the adapter continues to behave as before.

This patch is against the v2.3.4 tag. It adds a unit test to activerecord/test/cases/connection_test_mysql.rb which demonstrates the bug. A stored procedure has been added to the database schema for the test. It can be created by running "rake test_mysql TEST=test/cases/aaa_create_tables_test.rb". Right now, a number of tests are failing or throwing errors, but none of those are caused by my patch. "rake test_mysql' before my patch gives:

2115 tests, 6692 assertions, 3 failures, 86 errors

and afterward

2116 tests, 6694 assertions, 3 failures, 86 errors

Comments from reviewers welcome. Please let me know what I can do to get this patch committed.

Comments and changes to this ticket

  • Ken Collins

    Ken Collins September 5th, 2009 @ 07:54 PM

    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
    
  • Justin Bailey

    Justin Bailey September 6th, 2009 @ 05:33 PM

    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.

  • Joshua Siler

    Joshua Siler September 9th, 2009 @ 06:30 PM

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

  • chris finne

    chris finne September 11th, 2009 @ 09:59 AM

    I sure could have used this in my last project.

  • Jeremy Kemper

    Jeremy Kemper September 11th, 2009 @ 11:00 PM

    • State changed from “new” to “open”
    • Assigned user set to “Jeremy Kemper”

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

  • Jeremy Kemper

    Jeremy Kemper September 11th, 2009 @ 11:06 PM

    • Milestone set to 2.3.6
  • Justin Bailey

    Justin Bailey September 11th, 2009 @ 11:42 PM

    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?

  • Jeremy Kemper

    Jeremy Kemper September 12th, 2009 @ 12:16 AM

    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?

  • Justin Bailey

    Justin Bailey September 12th, 2009 @ 12:21 AM

    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!

  • Justin Bailey

    Justin Bailey September 12th, 2009 @ 12:22 AM

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

  • Justin Bailey

    Justin Bailey September 12th, 2009 @ 12:27 AM

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

  • Jeremy Kemper

    Jeremy Kemper September 12th, 2009 @ 03:50 AM

    Can add the flag if Mysql.client_version >= 40101

  • Justin Bailey

    Justin Bailey September 14th, 2009 @ 07:00 AM

    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?

  • Justin Bailey

    Justin Bailey September 14th, 2009 @ 07:01 AM

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

    Forgot to mention I made this change also.

  • Ken Collins

    Ken Collins September 14th, 2009 @ 01:36 PM

    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.

  • Justin Bailey

    Justin Bailey September 14th, 2009 @ 07:46 PM

    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?

  • Ken Collins

    Ken Collins September 14th, 2009 @ 08:05 PM

    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.

  • CancelProfileIsBroken

    CancelProfileIsBroken September 14th, 2009 @ 08:18 PM

    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
    
  • Ken Collins

    Ken Collins September 14th, 2009 @ 08:24 PM

    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.

  • Jeremy Kemper

    Jeremy Kemper September 14th, 2009 @ 09:17 PM

    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)
    
    And the test that you can CALL and get the expected results.

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

  • Justin Bailey

    Justin Bailey September 14th, 2009 @ 09:26 PM

    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.

  • Ken Collins

    Ken Collins September 14th, 2009 @ 11:42 PM

    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.

  • Justin Bailey

    Justin Bailey September 15th, 2009 @ 01:59 AM

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

    I've created ticket #3204 with a patch for this request.

  • Justin Bailey

    Justin Bailey September 15th, 2009 @ 02:41 AM

    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.

  • Justin Bailey

    Justin Bailey September 15th, 2009 @ 06:00 AM

    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.

  • Horace Ho

    Horace Ho March 23rd, 2010 @ 09:23 AM

    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>
    
    Is the above behavior related to "CLIENT_MULTI_RESULTS" flag? Thanks~
  • Rizwan Reza

    Rizwan Reza May 16th, 2010 @ 02:41 AM

    • Tag changed from stored procedure, 2.3.4, activerecord, adapters, database, database.yml, mysql, patch to stored procedure, 2.3.4, activerecord, adapters, bugmash, database, database.yml, mysql, patch
  • Jeremy Kemper

    Jeremy Kemper May 23rd, 2010 @ 05:54 PM

    • Milestone changed from 2.3.6 to 2.3.7
  • Jeremy Kemper

    Jeremy Kemper May 24th, 2010 @ 09:40 AM

    • Milestone changed from 2.3.7 to 2.3.8
  • Jeremy Kemper

    Jeremy Kemper May 25th, 2010 @ 11:45 PM

    • Milestone changed from 2.3.8 to 2.3.9
  • Jeff Lawson

    Jeff Lawson August 1st, 2010 @ 12:38 PM

    • Importance changed from “” to “”

    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.

    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, I have created one test for 'MysqlAdapter.select' and one test for 'MysqlAdapter.select_rows' and added the two lines of code to MysqlAdapter.

  • Jeff Lawson

    Jeff Lawson August 1st, 2010 @ 12:49 PM

    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.

  • Jeff Lawson

    Jeff Lawson August 1st, 2010 @ 05:59 PM

    • Tag changed from stored procedure, 2.3.4, activerecord, adapters, bugmash, database, database.yml, mysql, patch to stored procedure, 2.3.4, 3.0.0.0rc, activerecord, adapters, bugmash, database, database.yml, mysql, patch

    I am new to the formatting used here. Perhaps someone could delete 3151-32. The code sample in 3151-33 should be:

      @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 patch is fine (two commits). I apologise for any confusion.

  • Repository

    Repository August 17th, 2010 @ 09:16 PM

    • State changed from “open” to “resolved”

    (from [82a58abe05af7c1aa9f7b591661680847800ec8f]) Bug Fix -- clean up connection after stored procedure [#3151 state:resolved] http://github.com/rails/rails/commit/82a58abe05af7c1aa9f7b591661680...

  • Repository
  • Repository
  • Repository
  • Repository
  • Repository
  • Brian Candler

    Brian Candler January 20th, 2011 @ 04:03 PM

    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
    
  • Brian Candler

    Brian Candler January 20th, 2011 @ 04:34 PM

    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")
    

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Referenced by

Pages