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.

Sign up for GitHub

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

Jump to bottom

Correctly cache create_table_info #22547

Merged
merged 1 commit into from
Dec 30, 2015
Merged

Correctly cache create_table_info #22547

merged 1 commit into from
Dec 30, 2015
+6 −2

Conversation

kamipo
Copy link
Member

@kamipo kamipo commented Dec 10, 2015

No description provided.

@rails-bot
Copy link

r? @chancancode

(@rails-bot has picked a reviewer for you, use r? to override)

@kamipo
Copy link
Member Author

kamipo commented Dec 11, 2015

Follow up to #21664.

@yui-knk
Copy link
Contributor

yui-knk commented Dec 16, 2015

Looks good to me :)
I think it is better to split create_table_info_cache to private method like:

def create_table_info_cache
  @ create_table_info_cache ||= {}
end

this hides how create_table_info works.

And "Correctly cache create_table_info" is suitable to commit message I feel 👍

@kamipo kamipo changed the title Fix to @create_table_info_cache ||= {} Correctly cache create_table_info Dec 16, 2015
@kamipo
Copy link
Member Author

kamipo commented Dec 16, 2015

@yui-knk Thank you for your comment. I added create_table_info_cache and fixed commit message. Thanks!

@kamipo
Copy link
Member Author

kamipo commented Dec 21, 2015

The number of calling create_table_info was increased to 2 times by #17569. This means that rake db:schema:dump to be slower than Rails 4.2. We need this fix.

@rafaelfranca What do you think about it?

@rafaelfranca rafaelfranca added this to the 5.0.0 milestone Dec 30, 2015
rafaelfranca
rafaelfranca reviewed Dec 30, 2015
View reviewed changes
activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
@@ -1032,9 +1033,12 @@ def extract_foreign_key_action(structure, name, action) # :nodoc:
end
end

def create_table_info_cache # :nodoc:
@create_table_info_cache ||= {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should not this be a Concurrent::Map to avoid thread safety problems?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, it is only used in the schema dump, so we don't need to worry about thread-safety.

rafaelfranca added a commit that referenced this pull request Dec 30, 2015
@rafaelfranca
Merge pull request #22547 from kamipo/fix_create_table_info_cache
d3ec854 
Correctly cache create_table_info
@rafaelfranca rafaelfranca merged commit d3ec854 into rails:master Dec 30, 2015
@kamipo kamipo deleted the fix_create_table_info_cache branch December 30, 2015 01:41
@eileencodes eileencodes mentioned this pull request Jul 1, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@rafaelfranca rafaelfranca

Labels
activerecord
Projects
None yet
Milestone
5.0.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@kamipo @rails-bot @yui-knk @rafaelfranca @chancancode