Rails Ruby on Rails → 2.3.9

Comments and changes to this ticket

• You flagged this item as spam.
  

  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
  

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

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

  I sure could have used this in my last project.

• 

  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 September 11th, 2009 @ 11:06 PM

  • Milestone set to “2.3.6”

• You flagged this item as spam.
  

  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 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?

• You flagged this item as spam.
  

  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!

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

  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 September 12th, 2009 @ 03:50 AM

  Can add the flag if Mysql.client_version >= 40101

• You flagged this item as spam.
  

  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?

  • mysql_flags.diff 10.4 KB

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

  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?

• You flagged this item as spam.
  

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

• You flagged this item as spam.
  

  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 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?

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

  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.

  • mysql_flags.diff 10.4 KB

• You flagged this item as spam.
  

  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.

  • mysql_flags.diff 8.7 KB

• You flagged this item as spam.
  

  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 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”

  [bulk edit]

• 

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

  • Milestone changed from “2.3.6” to “2.3.7”

  [bulk edit]

• 

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

  • Milestone changed from “2.3.7” to “2.3.8”

  [bulk edit]

• 

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

  • Milestone changed from “2.3.8” to “2.3.9”

  [bulk edit]

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

  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.

  • bug_fix_for_3151_clean_up_connection_after_stored_procedure.diff 4.8 KB

• You flagged this item as spam.
  

  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 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 August 17th, 2010 @ 09:16 PM

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

• 

  Repository August 17th, 2010 @ 09:16 PM

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

• 

  Repository August 17th, 2010 @ 09:16 PM

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

• 

  Repository August 18th, 2010 @ 04:34 PM

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

• 

  Repository August 18th, 2010 @ 04:34 PM

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

• You flagged this item as spam.
  

  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
  

• You flagged this item as spam.
  

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