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

Performance issues using tables with no primary key #3812

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
Contributor

pimeys commented Nov 30, 2011

Operations like create for a model which table has no primary keys has horrible performance. ActiveRecord is doing queries like SHOW INDEX FROM 'table_name' WHERE Key_name = 'PRIMARY' 20-30 times per action. It will really affect performance with lots of requests per minute.

Now here's a quick fix to the problem, that sets the @primary_key variable only once, even when the database returns no primary keys.

Contributor

josevalim commented Nov 30, 2011

Can we please have a test? Otherwise we will continue have regressions like this. :)

Contributor

pimeys commented Nov 30, 2011

Sure thing, I just have to dig the rails source a bit deeper to find a new place to test this (and figure out what tools to use for e.g. testing).

Contributor

henrikhodne commented Nov 30, 2011

@pimeys I think you would put it in activerecord/test/cases/attribute_methods_test.rb, since that's where the code is. The test would be somewhat similar to the code you used when finding this issue. The test should not pass without this patch being applied, and should pass after.

Contributor

pimeys commented Nov 30, 2011

Got it, writing the test.

Contributor

pimeys commented Nov 30, 2011

Now there's a test that proves the bug.

Member

arunagw commented Nov 30, 2011

There is a one more thing you can do.. you can squash commits into one commit.. :-)

Contributor

pimeys commented Nov 30, 2011

Is it ok when I've already pushed? I have to force push and how it affects this pull request?

Member

arunagw commented Nov 30, 2011

Yeah. it's ok. you can force push in your branch. here it will effect automatically.

Contributor

pimeys commented Nov 30, 2011

Ok, done.

Member

jonleighton commented Nov 30, 2011

Thanks, I have merged this in 4e38082.

FWIW, I had to fix up the test in 1c783c6 otherwise it would permanently cache the primary key as nil on Minimalistic, which would then affect other tests later on in the test run.

Member

jonleighton commented Nov 30, 2011

Also merged to 3-1-stable so it will go in the next stable release.

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