Use ActiveRecord::Base.connected? to avoid creating unneeded connections

[Sjeanpierre]

This PR makes use of ActiveRecord::Base.connected? in the inside_transaction method to avoid the needless creation of extra connections to the primary configured DB.

[kenn]

Which database adapter? Which AR version?

As long as I read the code in 5.2.3, open_transactions takes @stack.size which is just an array. What is causing the extra query?

https://github.com/rails/rails/blob/v5.2.3/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb#L263-L265

      def open_transactions
        @stack.size
      end

In any case, inside_transaction? exists to prevent mistakes and I don't think it's a good idea to just remove the feature without assessment for trade-offs. I'd like to hear your thoughts.

[Sjeanpierre]

For the example posted in the screenshot I am making use of the console script from the repo which uses AR 3.0.0 and Sqlite adapter

In our app environment where we see this same behavior we are using AR 3.2 with the MySQL adapter.

In any case, inside_transaction? exists to prevent mistakes and I don't think it's a good idea to just remove the feature without assessment for trade-offs. I'd like to hear your thoughts.

This change does not remove any functionality from the inside_transaction method, it just does an early return if there are no existing connections established by AR. This prevents the round trip we were seeing the the DB when connections did not yet exist.

[kenn]

@open_transactions is just an instance variable on AR 3.2 too.

https://github.com/rails/rails/blob/3-2-stable/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb

So it's more likely that ActiveRecord::Base.connection is not reusing the pooled connection and causing the connection leak? I'd appreciate if you could track down what exactly is causing the extra query.

This change does not remove any functionality from the inside_transaction method, it just does an early return if there are no existing connections established by AR. This prevents the round trip we were seeing the the DB when connections did not yet exist.

inside_transaction? should run anytime when it's called, and whether connection is established or not doesn't matter.

[Sjeanpierre]

Hi Kenn, the open_transactions portion of line 28 is not the problem. It is the preceding call to connection which is causing a new DB connection to happen via the connection holder.

In our case, if a thread had not yet used a DB connection, this call would cause the thread to establish a new connection for no reason. I performed a new test of this today using ActiveRecord 5.2.3 and the MySQL2 adapter

I hope this helps

[Sjeanpierre]

Kenn, have you had a chance to read my last clarification about this?

[nicholasdower]

Filed https://github.com/kenn/standby/issues/36. Attempting a fix in https://github.com/kenn/standby/pull/35.