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

Already on GitHub? Sign in to your account

Don't EXPLAIN the unexplainable in postgresql #7544

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
5 participants
Contributor

dlee commented Sep 6, 2012

Only run EXPLAIN for statements that are supported. Really fixes Issue #5430.

Contributor

kennyj commented Sep 6, 2012

It seems that this is related to #6458 .

Contributor

dlee commented Sep 6, 2012

@rafaelfranca Ah, didn't even see that. What do you think about making it a whitelist instead of a blacklist? @kennyj adds a whitelist approach here: kennyj/rails@4a25742

Owner

rafaelfranca commented Sep 6, 2012

@dlee I prefer the whitelist too. Could you guys work on this one and ping me when finished?

Contributor

kennyj commented Sep 11, 2012

@dlee my approach is similar to your approach.
But I think supports of other adapter (mysql, sqlite...) is important.
And I also think queries (Thread.current[:available_queries_for_explain]) shouldn't be long. Thus I fixed it in the explain_subscriber.rb.
wdyt?

Contributor

dlee commented Sep 11, 2012

@kennyj Yeah, I think your whitelist approach is better. Would it make sense to take out the IGNORED_PAYLOADS and just use EXPLAINED_SQLS?

Contributor

kennyj commented Sep 12, 2012

I think we should use both IGNORED_PAYLOADS and EXPLAINED_SQLS. For example {CACHE, "select * from users where users.id = 1"}, this should be ignored.

Owner

rafaelfranca commented Sep 12, 2012

👍 Thank you to take care of this one guys. Ping me when done

Contributor

dlee commented Sep 12, 2012

@kennyj Should we have a ACCEPTED_PAYLOADS instead of IGNORED_PAYLOADS? Or is the list of accepted payloads too hard to maintain?

Contributor

kennyj commented Sep 15, 2012

@dlee Sorry for keeping you waiting for this reply.

IMO, we should use IGNORED_PAYLOADS, because payloads usage is inconsistency.

for example, https://github.com/rails/rails/blob/master/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb#L406

I think too hard to maintain too.

Contributor

kennyj commented Sep 15, 2012

If you agree with me, I submit PR about 4a25742 .

Owner

fxn commented Sep 15, 2012

Shouldn't that PRAGMA query be marked as SCHEMA?

Contributor

kennyj commented Sep 15, 2012

@fxn I agree with you. I'll send a PR that against it.

BTW, I investigated name's varietions when executing rake.
https://gist.github.com/3726395

Owner

rafaelfranca commented Sep 15, 2012

@kennyj I think the PR is fine. Please send it

Contributor

kennyj commented Sep 16, 2012

@rafaelfranca I sent the PR.

@rafaelfranca rafaelfranca added a commit that referenced this pull request Sep 17, 2012

@rafaelfranca @sikachu rafaelfranca + sikachu Backport explain fixes.
* Mark as SCHEMA some schema database queries. #7648
* Don't explain queries except normal CRUD sql. #7657

Closes #6458
Closes #7544
b9329a2

I don't see this in the current 3.2.x code. Any chance this will get pushed out to the active_record gem soon?

Owner

rafaelfranca commented Dec 14, 2012

It was released on 3.2.9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment