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

Don't use transactions when working with unsafed records in associations #1601

Closed
wants to merge 2 commits into from
Closed

Conversation

benedikt
Copy link
Contributor

@benedikt benedikt commented Jun 9, 2011

Currently rails wraps every operation on association collections in a transaction. This results in unecessary BEGIN COMMIT statements sent to the database server when working with unsaved records. In our application these unecessary statements caused about 70% of the whole database time.

Example:

f = Firm.new
f.clients_of_firm << Client.new("name" => "New Client")
# Results in an empty transaction statement in the database:
# BEGIN
# COMMIT

The same issue applies not only to CollectionAssociation#concat but also to CollectionAssociation#replace and CollectionAssociation#delete_or_destroy. This pull request fixes the issue by only wrapping the calls when the transaction is actually needed.

… they are not needed, so the connection adapter does not send empty BEGIN COMMIT transactions blocks to the database.
@josevalim
Copy link
Contributor

@jonleighton I thought we removed the code that wraps all operations in a transaction?

@benedikt
Copy link
Contributor Author

benedikt commented Jun 9, 2011

Of course this would solve this, too... but is this really what you want? Especially when adding/replacing/removing several records at once the transaction is quite handy, isn't it?

@jonleighton
Copy link
Member

@josevalim We remove it for e.g. foo.items.build, but it looks like there are further issues...

@benedikt thanks for reporting and providing a patch! I want to work on the code a bit before merging, but I aim to have it merged by Sunday night UTC+1

@tenderlove giving you the heads up about this - I think this fix ought to go in 3.1 final as it's a perf regression. If the current RC has no "omg" issues then maybe we can create another RC with just an absolutely minimal set of important changes cherry-picked (e.g. this) and this have a shorter RC cycle for that.

@josevalim
Copy link
Contributor

tks @jonleighton

@benedikt
Copy link
Contributor Author

@jonleighton If you want me to change anything in the patch (or anything related) just tell me. :)

@jonleighton
Copy link
Member

This is merged now:

Thanks for offering to make changes. I would definitely have told you what I wanted changed if it was obvious, but I had to actually get in there and play with the code before I could work out how I wanted to change it!

Thanks for contributing.

jake3030 pushed a commit to jake3030/rails that referenced this pull request Jun 28, 2011
Signed-off-by: Joshua Peek <josh@joshpeek.com>
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

Successfully merging this pull request may close these issues.

None yet

3 participants