[ActiveRecord] Add a condition on the join table make multiple SHOW TABLES calls #3595

Closed
noefroidevaux opened this Issue Nov 10, 2011 · 6 comments

Projects

None yet

3 participants

Contributor

Hi,

For example, if we have a Document model with a parent_id and privacy attributes (with "belongs_to :parent, :foreign_key => 'parent_id', :class_name => 'Document'"), I try to have a query like:

Document.joins(:parent).where("parents_documents.privacy = ?", 1)

Every time I run this query, a call is made on database with "SHOW TABLES", because ActiveRecord don't cache the tables which is not in database.

In ConnectionPool (lib/active_record/connection_adapters/abstract/connection_pool.rb), currently, we have:

def table_exists?(name)
  return true if @tables.key? name

  with_connection do |conn|
    conn.tables.each { |table| @tables[table] = true }
      @tables[name] = true if !@tables.key?(name) && conn.table_exists?(name)
    end

  @tables.key? name
end

I propose to change to:

def table_exists?(name)
  return @tables[name] if @tables.key? name

  with_connection do |conn|
    conn.tables.each { |table| @tables[table] = true }
    @tables[name] = !@tables.key?(name) && conn.table_exists?(name) ? true : false
  end

  @tables[name] if @tables.key? name
end

With this update, "parents_documents" will be a key in @tables with the value false. And it don't make any more request to conn.tables or conn.table_exists? (which send the "SHOW TABLES").

What do you think? It is the right way?

Owner

I'm not sure this will work. Please try running the tests. I think the migrations depend on this behavior.

Contributor

I tried testing before opening this issue :) All pass on master.

Contributor

And I think the last

@tables[name] if @tables.key? name

can be symplified by

@tables[name]
Owner

I think you can also remove the true : false from your ternary. Boolean expressions will already return boolean values.

Can you make those changes and try testing again?

Contributor

Changed, and the tests always pass.

Pull request added here: #3609

Member

Closing this, since a PR had already been applied.

@vijaydev vijaydev closed this Nov 13, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment