Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v1.2.2 does not support rails 3.2 explain feature #159

Closed
atambo opened this issue Jan 30, 2012 · 26 comments
Closed

v1.2.2 does not support rails 3.2 explain feature #159

atambo opened this issue Jan 30, 2012 · 26 comments

Comments

@atambo
Copy link
Member

atambo commented Jan 30, 2012

When trying to use the new rails 3.2 explain feature (http://weblog.rubyonrails.org/2011/12/6/what-s-new-in-edge-rails-explain) I get the following error:

undefined method `explain' for #<ActiveRecord::ConnectionAdapters::SQLite3Adapter:0x1139ba37>
activerecord (3.2.1) lib/active_record/explain.rb:65:in `exec_explain'
org/jruby/RubyKernel.java:1804:in `tap'
activerecord (3.2.1) lib/active_record/explain.rb:59:in `exec_explain'
org/jruby/RubyArray.java:2318:in `collect'
activerecord (3.2.1) lib/active_record/explain.rb:58:in `exec_explain'
activerecord (3.2.1) lib/active_record/relation.rb:146:in `explain'
app/controllers/home_controller.rb:48:in `index'
@nicksieger
Copy link
Member

Thanks, I didn't know about this yet. Patches appreciated from anyone for adding this feature.

@boxofrad
Copy link

boxofrad commented Feb 3, 2012

Hope someone adds this soon!

For now i've just monkey patched over it in an initializer, so I can keep working:

module ActiveRecord::ConnectionAdapters
  class JdbcAdapter < AbstractAdapter
    def explain(*args)
      # Do nothing :'(
    end
  end
end

@donv
Copy link
Member

donv commented Mar 4, 2012

How about this:

module ActiveRecord::ConnectionAdapters
  class JdbcAdapter < AbstractAdapter
    def explain(query, *binds)
      ActiveRecord::Base.connection.execute("EXPLAIN #{query}", 'EXPLAIN', binds)
    end
  end
end

@boxofrad
Copy link

boxofrad commented Mar 5, 2012

@donv that might work as an interim solution for some RDBMSes but unfortunately not for me as i'm on SQL Server

:P

@kimptoc
Copy link
Contributor

kimptoc commented Apr 3, 2012

@boxofrad thanks - that let me progress, now things run and my couple of tests pass :)

@arunagw
Copy link
Member

arunagw commented Jul 15, 2012

anyone got this working guys ?

@codeodor
Copy link
Contributor

I've had it on my "to look into list" for a while to see if I could contribute something, but I just haven't made the time to go through it long enough to understand what's going on (with the hope that it could call back into Rails). It seems to me ar-jdbc-adapter ought to be able to reuse the SQL generated from the AR adapters for each of the DBs, but pass it through the jdbc interface instead.

cturner added a commit to cturner/activerecord-jdbc-adapter that referenced this issue Jul 17, 2012
lifted this code from the mysql2 gem and modified it to work
in the JDBC gem.  I've done testing under JRuby 1.6.7.2 with
MySQL 5.1 and 5.5 with no issues.

Github issue URL:
jruby#159
@cturner
Copy link
Contributor

cturner commented Jul 17, 2012

Created pull request (#206) to patch this. I've tested under JRuby 1.6.7.2 with MySQL 5.1 and 5.5 with no issues.

I took the code that implements explain support in the mysql2 gem and slightly modified it to work here. I'm not happy with the block copying of the PrettyPrint class, but I couldn't figure out a way to leverage the code without doing that.

@cturner
Copy link
Contributor

cturner commented Jul 17, 2012

If this approach is acceptable to the maintainers I will be glad to implement explain support for SQLite and PostgreSQL as well.

@pschambacher
Copy link
Contributor

I would also be interested in a solution to this for SQL Server, I just got hit by the issue :)

@vysakh0
Copy link

vysakh0 commented Jan 3, 2013

undefined method `explain' for #ActiveRecord::ConnectionAdapters::PostgreSQLAdapter:0x521e980f Is there a fix for postgres as well??

@elight
Copy link

elight commented Jan 4, 2013

@vysakh0: Just encountered the same problem myself.

@elight
Copy link

elight commented Jan 4, 2013

So apparently @cturner's patch works just fine when copypasta'd right into the PostgreSQL JDBC driver as well...

Any objections to extracting his fix into a mixin and sharing it between the MySQL and PostgreSQL drivers?

@ajuckel
Copy link
Contributor

ajuckel commented Jan 4, 2013

A pull request which does so would certainly be welcome.
On Jan 4, 2013 2:38 PM, "Evan David Light" notifications@github.com wrote:

So apparently @cturner https://github.com/cturner's patch works just
fine with copypasta'd right into the PostgreSQL JDBC driver as well...

Any objections to extracting his fix into a mixin and sharing it between
the MySQL and PostgreSQL drivers?


Reply to this email directly or view it on GitHubhttps://github.com//issues/159#issuecomment-11898739.

@elight
Copy link

elight commented Jan 4, 2013

Noted. Thanks!

On Friday, January 4, 2013 at 3:41 PM, Anthony Juckel wrote:

A pull request which does so would certainly be welcome.
On Jan 4, 2013 2:38 PM, "Evan David Light" <notifications@github.com (mailto:notifications@github.com)> wrote:

So apparently @cturner https://github.com/cturner's patch works just
fine with copypasta'd right into the PostgreSQL JDBC driver as well...

Any objections to extracting his fix into a mixin and sharing it between
the MySQL and PostgreSQL drivers?


Reply to this email directly or view it on GitHubhttps://github.com//issues/159#issuecomment-11898739.


Reply to this email directly or view it on GitHub (#159 (comment)).

@cturner
Copy link
Contributor

cturner commented Jan 5, 2013

Good catch sir. I wasn't aware that Postgres had the same syntax and output. Are you going to convert this into a mixin or would you like me to work on it?

On Jan 4, 2013, at 15:41, Evan David Light notifications@github.com wrote:

Noted. Thanks!

On Friday, January 4, 2013 at 3:41 PM, Anthony Juckel wrote:

A pull request which does so would certainly be welcome.
On Jan 4, 2013 2:38 PM, "Evan David Light" <notifications@github.com (mailto:notifications@github.com)> wrote:

So apparently @cturner https://github.com/cturner's patch works just
fine with copypasta'd right into the PostgreSQL JDBC driver as well...

Any objections to extracting his fix into a mixin and sharing it between
the MySQL and PostgreSQL drivers?


Reply to this email directly or view it on GitHubhttps://github.com//issues/159#issuecomment-11898739.


Reply to this email directly or view it on GitHub (#159 (comment)).


Reply to this email directly or view it on GitHub.

@elight
Copy link

elight commented Jan 5, 2013

I started. Struggling with the LOAD_PATH when requiring the mixin.

On Friday, January 4, 2013, Chris Turner wrote:

Good catch sir. I wasn't aware that Postgres had the same syntax and
output. Are you going to convert this into a mixin or would you like me to
work on it?

On Jan 4, 2013, at 15:41, Evan David Light <notifications@github.com<javascript:_e({}, 'cvml', 'notifications@github.com');>>
wrote:

Noted. Thanks!

On Friday, January 4, 2013 at 3:41 PM, Anthony Juckel wrote:

A pull request which does so would certainly be welcome.
On Jan 4, 2013 2:38 PM, "Evan David Light" <notifications@github.com<javascript:_e({}, 'cvml', 'notifications@github.com');>(mailto:
notifications@github.com <javascript:_e({}, 'cvml',
'notifications@github.com');>)> wrote:

So apparently @cturner https://github.com/cturner's patch works
just
fine with copypasta'd right into the PostgreSQL JDBC driver as
well...

Any objections to extracting his fix into a mixin and sharing it
between
the MySQL and PostgreSQL drivers?


Reply to this email directly or view it on GitHub<
https://github.com/jruby/activerecord-jdbc-adapter/issues/159#issuecomment-11898739>.


Reply to this email directly or view it on GitHub (
#159 (comment)).


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHubhttps://github.com//issues/159#issuecomment-11905788.

Evan Light
http://evan.tiggerpalace.com
@elight

@ajuckel
Copy link
Contributor

ajuckel commented Jan 5, 2013

Feel free to just throw the mixin into a shared file (like the jdbc adapter
from which they're derived). We can refactor it later if required.

@elight
Copy link

elight commented Jan 5, 2013

Deal.

On Friday, January 4, 2013, Anthony Juckel wrote:

Feel free to just throw the mixin into a shared file (like the jdbc
adapter
from which they're derived). We can refactor it later if required.
On Jan 4, 2013 6:11 PM, "Evan David Light" <notifications@github.com<javascript:_e({}, 'cvml', 'notifications@github.com');>>
wrote:

I started. Struggling with the LOAD_PATH when requiring the mixin.

On Friday, January 4, 2013, Chris Turner wrote:

Good catch sir. I wasn't aware that Postgres had the same syntax and
output. Are you going to convert this into a mixin or would you like me
to
work on it?

On Jan 4, 2013, at 15:41, Evan David Light <notifications@github.com<javascript:_e({}, 'cvml', 'notifications@github.com');><javascript:_e({},

'cvml', 'notifications@github.com <javascript:_e({}, 'cvml',
'notifications@github.com');>');>>

wrote:

Noted. Thanks!

On Friday, January 4, 2013 at 3:41 PM, Anthony Juckel wrote:

A pull request which does so would certainly be welcome.
On Jan 4, 2013 2:38 PM, "Evan David Light" <notifications@github.com<javascript:_e({}, 'cvml', 'notifications@github.com');><javascript:_e({},

'cvml', 'notifications@github.com <javascript:_e({}, 'cvml',
'notifications@github.com');>');>(mailto:

notifications@github.com <javascript:_e({}, 'cvml',
'notifications@github.com');> <javascript:_e({}, 'cvml',
'notifications@github.com <javascript:_e({}, 'cvml',
'notifications@github.com');>');>)> wrote:

So apparently @cturner https://github.com/cturner's patch works
just
fine with copypasta'd right into the PostgreSQL JDBC driver as
well...

Any objections to extracting his fix into a mixin and sharing it
between
the MySQL and PostgreSQL drivers?


Reply to this email directly or view it on GitHub<

https://github.com/jruby/activerecord-jdbc-adapter/issues/159#issuecomment-11898739>.


Reply to this email directly or view it on GitHub (

#159 (comment)).


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHub<

https://github.com/jruby/activerecord-jdbc-adapter/issues/159#issuecomment-11905788>.

Evan Light
http://evan.tiggerpalace.com
@elight


Reply to this email directly or view it on
GitHub<
https://github.com/jruby/activerecord-jdbc-adapter/issues/159#issuecomment-11905870>.


Reply to this email directly or view it on GitHubhttps://github.com//issues/159#issuecomment-11906830.

Evan Light
http://evan.tiggerpalace.com
@elight

@kares
Copy link
Member

kares commented Jan 16, 2013

on master explain for mysql/postgres and sqlite3

@kares kares closed this as completed Jan 16, 2013
@superchris
Copy link

Just hit this trying to a Model.all using the derby ar jdbc adapter. Is there a more generic fix?

@kares
Copy link
Member

kares commented Jan 19, 2013

@superchris unfortunately not as some DBs such as Derby or DB2 do not support simple "user-friendly" EXPLAIN ...
it's a bit harder to implement since you do need to query the explain data yourself ... PRs are very welcome

@superchris
Copy link

Sure, but it seems like there should be some way to stub this out with a no-op for databases that don't support this feature (or don't support it in an easily usable way)

@elight
Copy link

elight commented Jan 20, 2013

Disabling in in the app configuration isn't an option for you?

On Saturday, January 19, 2013 at 4:17 PM, Chris Nelson wrote:

Sure, but it seems like there should be some way to stub this out with a no-op for databases that don't support this feature (or don't support it in an easily usable way)


Reply to this email directly or view it on GitHub (#159 (comment)).

@amazon123
Copy link

hit the issue again on jruby1.7.2, rails 3.2.11, activerecord-jdbc-adapter 1.2.8 on MSSQL (sqlserver)

@codeodor
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests