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

Should use the same connection in using query cache #29623

Merged

Conversation

kamipo
Copy link
Member

@kamipo kamipo commented Jun 29, 2017

test_cache_is_available_when_using_a_not_connected_connection is
always failed if running only the test since #29609.

% ARCONN=mysql2 be ruby -w -Itest test/cases/query_cache_test.rb -n test_cache_is_available_when_using_a_not_connected_connection
Using mysql2
Run options: -n test_cache_is_available_when_using_a_not_connected_connection --seed 15043

F

Finished in 0.070519s, 14.1806 runs/s, 28.3612 assertions/s.

  1) Failure:
QueryCacheTest#test_cache_is_available_when_using_a_not_connected_connection [test/cases/query_cache_test.rb:336]:
2 instead of 1 queries were executed.
Queries:
SELECT  `tasks`.* FROM `tasks` WHERE `tasks`.`id` = ? LIMIT ?
SET NAMES utf8 COLLATE utf8_unicode_ci,  @@SESSION.sql_mode = CONCAT(CONCAT(@@sql_mode, ',STRICT_ALL_TABLES'), ',NO_AUTO_VALUE_ON_ZERO'),  @@SESSION.sql_auto_is_null = 0, @@SESSION.wait_timeout = 2147483.
Expected: 1
  Actual: 2

1 runs, 2 assertions, 1 failures, 0 errors, 0 skips

This failure is due to LogSubscriber will use not connected
ActiveRecord::Base.connection even if Task.connection is connected.
I fixed to always pass type_casted_binds to log subscriber to avoid
the issue.

`test_cache_is_available_when_using_a_not_connected_connection` is
always failed if running only the test since rails#29609.

```
% ARCONN=mysql2 be ruby -w -Itest test/cases/query_cache_test.rb -n test_cache_is_available_when_using_a_not_connected_connection
Using mysql2
Run options: -n test_cache_is_available_when_using_a_not_connected_connection --seed 15043

F

Finished in 0.070519s, 14.1806 runs/s, 28.3612 assertions/s.

  1) Failure:
QueryCacheTest#test_cache_is_available_when_using_a_not_connected_connection [test/cases/query_cache_test.rb:336]:
2 instead of 1 queries were executed.
Queries:
SELECT  `tasks`.* FROM `tasks` WHERE `tasks`.`id` = ? LIMIT ?
SET NAMES utf8 COLLATE utf8_unicode_ci,  @@SESSION.sql_mode = CONCAT(CONCAT(@@sql_mode, ',STRICT_ALL_TABLES'), ',NO_AUTO_VALUE_ON_ZERO'),  @@SESSION.sql_auto_is_null = 0, @@SESSION.wait_timeout = 2147483.
Expected: 1
  Actual: 2

1 runs, 2 assertions, 1 failures, 0 errors, 0 skips
```

This failure is due to `LogSubscriber` will use not connected
`ActiveRecord::Base.connection` even if `Task.connection` is connected.
I fixed to always pass `type_casted_binds` to log subscriber to avoid
the issue.
@rafaelfranca rafaelfranca merged commit 9c2ad53 into rails:master Jun 29, 2017
@kamipo kamipo deleted the should_use_same_connection_in_query_cache branch June 29, 2017 16:07
@joshpencheon
Copy link
Contributor

joshpencheon commented Oct 27, 2017

@rafaelfranca - is there any scope for backporting some of this to 5.1 / 5.0?

The problematic use of ActiveRecord::Base.connection was introduced in #28526, which was backported (#28495).

Our application is affected, and has been unable to log SQL runtimes since moving from v5.0.2 to v5.0.3+

kamipo pushed a commit that referenced this pull request Oct 29, 2017
…uery_cache

Should use the same connection in using query cache
kamipo pushed a commit that referenced this pull request Oct 29, 2017
…uery_cache

Should use the same connection in using query cache
@kamipo
Copy link
Member Author

kamipo commented Oct 29, 2017

Backported in 997922d and 9c446b0.

troter added a commit to troter/activerecord-cause that referenced this pull request Mar 9, 2018
Arity of ActiveRecord::LogSubscriber#type_casted_binds was changed at
Rails-5.1.5.

- rails/rails#29623
troter added a commit to troter/activerecord-cause that referenced this pull request Mar 9, 2018
Arity of ActiveRecord::LogSubscriber#type_casted_binds was changed at
Rails-5.1.5.

- rails/rails#29623
troter added a commit to troter/activerecord-cause that referenced this pull request Mar 9, 2018
Arity of ActiveRecord::LogSubscriber#type_casted_binds was changed at
Rails-5.1.5.

- rails/rails#29623
kares added a commit to jruby/activerecord-jdbc-adapter that referenced this pull request Apr 4, 2018
so that we do not have PS param values nil-ed e.g. `['created_at', nil]`

with JDBC we do not need the type-casted pieces as we're not using them
... having a proc which is used when debug logging is gets them right
the only draw-back is it will only work on recent AR 5.0/5.1 (>= 5.0.6)
(support for a loggic proc was introduced at rails/rails#29623)
kamipo added a commit that referenced this pull request Mar 18, 2019
Internal usage for the method as public has removed at #29623.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants