Move advisory locks to own connection handler. - #39758
Merged
eileencodes merged 1 commit intoAug 4, 2020
Merged
Conversation
Contributor
Author
|
I didn't add a test for this because the bug has to be reproduced through the use of |
eileencodes
reviewed
Jul 31, 2020
tgxworld
force-pushed
the
create_own_connection_handler_for_advisory_locks
branch
from
August 3, 2020 00:49
688cf2a to
b1dc282
Compare
Contributor
Author
|
@eileencodes I've updated the PR as per your comment. Thank you for taking the time to review this 👍 |
Member
|
Postgres and mysql tests are failing with: It looks like |
Removes the use of `ActiveRecord::AdvisoryLockBase` since it inherits from `ActiveRecord::Base` and hence share module attributes that are defined in `ActiveRecord::Base`. This is problematic because establishing connections through `ActiveRecord::AdvisoryLockBase` can end up changing state of the default connection handler of `ActiveRecord::Base` leading to unexpected behaviors in a Rails application. In the case of rails#39157, Running migrations with `rails db:migrate:primary_shard_one` was not working as the application itself defined the following ``` class ApplicationRecord < ActiveRecord::Base self.abstract_class = true connects_to shards: { default: { writing: :primary }, shard_one: { writing: :primary_shard_one } } end ``` In the database migrate rake task, the default connection was established with the database config of `primary_shard_one`. However, the default connection was altered to that of `primary` because `ActiveRecord::AdvisoryLockBase.establish_connection` ended up loading `ApplicationRecord` which calls `connects_to shards:`. Since all we really need here is just a normal database connection, we can avoid accidentally altering the default connection handler state during the migration by creating a custom connection handler used for retrieving a connection.
tgxworld
force-pushed
the
create_own_connection_handler_for_advisory_locks
branch
from
August 4, 2020 02:18
b1dc282 to
45add34
Compare
Contributor
Author
|
@eileencodes The test should be fixed now. Apologies for not checking. |
Member
|
Thanks! |
Contributor
Author
|
Thank you for reviewing too :) |
9 tasks
eileencodes
added a commit
to eileencodes/rails
that referenced
this pull request
Oct 12, 2022
In rails#38235 I moved advisory locks to their own named connections, then in rails#39758 the advisory lock was left on Base.connection but then moved it it's own connection handler. I believe with rails#45450 that this change was made obsolete and can be returned to the prior behavior without having to open an additional connection. The tests added pass and I also tested this in my local demo to ensure that this is working correctly. When I originally changed the behavior here Matthew noted that this could be surprising for some setups that expect only one connection for a running migration. I thought there was an issue related to this but I can't find it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Resolves #39157
Removes the use of
ActiveRecord::AdvisoryLockBasesince it inheritsfrom
ActiveRecord::Baseand hence share module attributes that are defined inActiveRecord::Base.This is problematic because establishing connections through
ActiveRecord::AdvisoryLockBasecan end up changing state of the defaultconnection handler of
ActiveRecord::Baseleading to unexpectedbehaviors in a Rails application.
In the case of #39157,
Running migrations with
rails db:migrate:primary_shard_onewas not working asthe application itself defined the following
In the database migrate rake task, the default connection was
established with the database config of
primary_shard_one. However,the default connection was altered to that of
primarybecauseActiveRecord::AdvisoryLockBase.establish_connectionended up loadingApplicationRecordwhich callsconnects_to shards:. Since all wereally need here is just a normal database connection, we can avoid
accidentally altering the default connection handler state during the migration
by creating a custom connection handler used for retrieving a connection.
Other Information
Alternate take on #38235