-
Notifications
You must be signed in to change notification settings - Fork 310
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
Introduce column cache per connection #1744
Conversation
@koic I'd like you to review this pull request since this implementation just works, but does not look good. def columns(table_name)
table_name = table_name.to_s
if @columns_cache[table_name]
@columns_cache[table_name]
else
@columns_cache[table_name] =
column_definitions(table_name).map do |field|
new_column_from_field(table_name, field)
end
end
@columns_cache[table_name]
end Reasons:
Any feedbacks are welcomed. |
Sure thing. I will look it next week. Please wait a moment. |
end | ||
end | ||
@columns_cache[table_name] | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about using super
to transfer to AR::ConnectionAdapters::SchemaStatements#columns(table_name)
as follows?
def columns(table_name)
table_name = table_name.to_s
if @columns_cache[table_name]
@columns_cache[table_name]
else
+ @columns_cache[table_name] = super(table_name)
- @columns_cache[table_name] =
- column_definitions(table_name).map do |field|
- new_column_from_field(table_name, field)
- end
end
- @columns_cache[table_name]
end
There will be no duplication of implementation with super class.
I wrote a comment on https://github.com/rsim/oracle-enhanced/pull/1744/files#r208188648.
On the other hand. I don't have an idea to this dup codes... |
4a1ed11
to
d3a75c0
Compare
@koic Thanks for the review. Making use of |
else | ||
@columns_cache[table_name] = super(table_name) | ||
end | ||
@columns_cache[table_name] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this explicit return value line is unnecessary. The return value is determined by L163 or L165.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. Updated.
rsim#1490 removed Oracle enhanced adapter own column cache feature to use Rails `db:schema:cache:dump`. However, it caused slower performance about metadata search, such as column information. This commit restores Oracle enhanced adapter own column cache feature. It used to be class level caching, it is connection level caching now since it is much easier and cleaner than class level ones. Related to rsim#1720
d3a75c0
to
94336cf
Compare
Thanks for fixing pragmatic issue! |
#1490 removed Oracle enhanced adapter own column cache feature
to use Rails
db:schema:cache:dump
. However, it caused slower performanceabout metadata search, such as column information.
This commit restores Oracle enhanced adapter own column cache feature.
It used to be class level caching, it is connection level caching now
since it is much easier and cleaner than class level ones.
Related to #1720