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

Share the column and table name quote cache between connections #36637

Merged
merged 1 commit into from Jul 10, 2019

Conversation

@casperisfine
Copy link

commented Jul 9, 2019

For apps using multiple database connections, especially if they are shards or replicas, different connections are very likely to share parts or all their table and column names.

Because of this I believe this cache should not be stored as an instance variable of ConnectionAdapter, but on the adapter class itself. This way all instances of a same type will share that same cache.

Should it use a Concurrent::Map ?

I believe it doesn't need it because if a race condition was to happen, both thread would generate the same value. So the locking needed would cause more harm than good.

cc @rafaelfranca @Edouard-chin @kaspth

@rails-bot rails-bot bot added the activerecord label Jul 9, 2019

@quoted_column_names ||= {}
end

def self.quoted_table_names

This comment has been minimized.

Copy link
@kamipo

kamipo Jul 9, 2019

Member

Is it intended to expose these in API doc?

This comment has been minimized.

Copy link
@casperisfine

casperisfine Jul 9, 2019

Author

Good point, no it's not. I think they should be private. Not sure what the best way to do it.

I can certainly # nodoc them, but they are still accessible. I'm open to suggestions.

This comment has been minimized.

Copy link
@casperisfine

casperisfine Jul 9, 2019

Author

I guess I could use class variables (@@) instead, but then I'm afraid about them being inherited, hence shared between distinct adapter implementations, in unexpected ways.

This comment has been minimized.

Copy link
@diasjorge

diasjorge Jul 16, 2019

Contributor

Maybe you could make it a protected class method?

This comment has been minimized.

Copy link
@casperisfine

casperisfine Jul 16, 2019

Author

Nope:

class Foo
  class << self
    protected

    def cache
    end
  end

  def access_cache
    self.class.cache
  end
end

Foo.new.access_cache # protected method `cache' called for Foo:Class (NoMethodError)
@kamipo

kamipo approved these changes Jul 10, 2019

@kamipo kamipo merged commit 15748f6 into rails:master Jul 10, 2019

2 checks passed

buildkite/rails Build #62100 passed (8 minutes, 50 seconds)
Details
codeclimate All good!
Details

kamipo added a commit that referenced this pull request Jul 11, 2019

Merge pull request #36637 from Shopify/share-quote-cache
Share the column and table name quote cache between connections

yahonda added a commit to yahonda/oracle-enhanced that referenced this pull request Jul 12, 2019

Share the column and table name quote cache between connections
Refer rails/rails#36637

This pull request addresses this kind of failures:

```
$ bundle exec rspec ./spec/active_record/connection_adapters/oracle_enhanced/context_index_spec.rb:65
... snip ...
/home/yahonda/git/oracle-enhanced/lib/active_record/connection_adapters/oracle_enhanced/quoting.rb:73: warning: instance variable @quoted_table_names not initialized
FFFFFFFFFF/home/yahonda/git/oracle-enhanced/lib/active_record/connection_adapters/oracle_enhanced/quoting.rb:73: warning: instance variable @quoted_table_names not initialized

An error occurred in an `after(:context)` hook.
Failure/Error: @quoted_table_names[name] ||= [name.split(".").map { |n| quote_column_name(n) }].join(".")

NoMethodError:
  undefined method `[]' for nil:NilClass
```

yahonda added a commit to yahonda/oracle-enhanced that referenced this pull request Jul 13, 2019

Share the column and table name quote cache between connections
Refer rails/rails#36637

This pull request addresses this kind of failures:

```
$ bundle exec rspec ./spec/active_record/connection_adapters/oracle_enhanced/context_index_spec.rb:65
... snip ...
/home/yahonda/git/oracle-enhanced/lib/active_record/connection_adapters/oracle_enhanced/quoting.rb:73: warning: instance variable @quoted_table_names not initialized
FFFFFFFFFF/home/yahonda/git/oracle-enhanced/lib/active_record/connection_adapters/oracle_enhanced/quoting.rb:73: warning: instance variable @quoted_table_names not initialized

An error occurred in an `after(:context)` hook.
Failure/Error: @quoted_table_names[name] ||= [name.split(".").map { |n| quote_column_name(n) }].join(".")

NoMethodError:
  undefined method `[]' for nil:NilClass
```
@ryantownsend

This comment has been minimized.

Copy link

commented Jul 16, 2019

@casperisfine I know this PR is a week old, but you said in your description:

different connections are very likely to share parts or all their table and column names

Doesn’t this change make the assumption that all connections will share these and effectively break scenarios where databases don’t match?

I might be missing something completely obvious, so thought I’d ask.

@casperisfine

This comment has been minimized.

Copy link
Author

commented Jul 16, 2019

@ryantownsend I'm not quite sure to understand what you mean exactly.

The change make it so that the cache is now shared on the adapter class. Which mean it's only shared between connections of the exact same type. e.g. MySQL2Adapter won't share it's cache with PostgresAdapter.

Then, yes you'll only save memory if the two databases shared some column and table names. That's quite a common scenario if you make use of sharding, or simply use a read replica.

@casperisfine casperisfine deleted the Shopify:share-quote-cache branch Jul 16, 2019

@ryantownsend

This comment has been minimized.

Copy link

commented Jul 16, 2019

@casperisfine ignore me – I've had some coffee and now understand this usage better. Apologies!

@krzcho

This comment has been minimized.

Copy link

commented Jul 16, 2019

What if I do not have exactly the same shards... I would prefer initial setup or being able to switch between the two.

@casperisfine

This comment has been minimized.

Copy link
Author

commented Jul 16, 2019

What if I do not have exactly the same shards...

Then you don't gain much (I still bet your shards will share a few names such as id, updated_at, etc), but won't loose anything.

I would prefer initial setup

If by that you mean eagerloading of the cache, it's in my plans, but it's a little bit tricky.

@krzcho

This comment has been minimized.

Copy link

commented Jul 16, 2019

I guess I "gain" a cache that does not reflect database's state f.e. cache that the column exists while it actually isn't ; am I wrong here?

@casperisfine

This comment has been minimized.

Copy link
Author

commented Jul 16, 2019

That cache doesn't "reflect" any state. It simply memoize table and column quotation:

"id" => "`id`"

There is no reason whatsoever for an application to reach directly into that cache.

So really, I don't understand why you are disturbed by it.

@krzcho

This comment has been minimized.

Copy link

commented Jul 16, 2019

yes, sorry, disregard; now I see which cache it is about; I was thinking about schema cache

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.