Disconnect connections used for acquiring advisory lock in migration. #40101
Conversation
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
tgxworld
force-pushed
the
fix_not_disconnecting_pool
branch
2 times, most recently
from
f1e7263 to
e721789
Compare
|
@eileencodes Can you review this? Thank you in advanced. |
Not disconnecting the connections in the pool results in a session left idling in the DB. An idle session will prevent the database being dropped and affect commands like `bin/rails db:test:prepare`. While this is an edge case, it is still good practice to clean up the connections in the pool.
tgxworld
force-pushed
the
fix_not_disconnecting_pool
branch
from
e721789 to
ae330af
Compare
|
@eileencodea Trying to catch you while you’re around :) Do you mind reviewing this as well? The fix is straightforward and we might have to back port the fixes to Rails 6.0. |
|
@eileencodes Thank you for reviewing 👍 |
stevschmid
added a commit
to hakuna/kari
that referenced
this pull request
May 13, 2023
1) Faster connection retrieval to establish advisory lock: Migration of multiple tenants (schemas) has slowed down considerably in Rails 6.1+, see following comment: rails-on-services/apartment#147 (comment) Rails 6.0 -> 6.1 introduced a change in how the advisory lock to prevent concurrent migrations is established, see PR: rails/rails@45add34 The PR seems to address the issue that the previous solution using a global ActiveRecord::Base AdvisoryLockBase changes the internal ActiveRecord::Base internal state, leaking into the Rails application, messing with the sharding configuration. Reverting back to AdvisoryLockBase (as outlined in the first link) improves migration performance significantly but comes with the earlier mentioned side-effect. The issue with the new Rails 6.1 solution stems from the fact that a new connection pool is created for every migration. If we "cache" this connection pool, similar how the default ActiveRecord connection pool is cached, we can re-use is in the same thread and thus for all tenant schemas. Rails 7 offers ActiveSupport::IsolatedExecutionState (thread/fiber state), for Rails 6.1 we fall back to Thread.current. Why a new, separate connection/connection pool for this advisory lock is required in the first place, is explained in the PR introducing the original AdvisoryLockBase solution: rails/rails#38235 2) Tenant-specific lock_id for advisory lock: Improves the performance quite a bit. A side bonus is that parallel migrations of tenant should be possible (not tested). Separate note: A follow-up PR added `pool&.disconnect` to the `with_advisory_lock_connection` function: rails/rails#40101 I cannot reproduce the original issue, connections are cleaned up with this solution after the migration ends, even without the explicit disconnect
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.
Not disconnecting the connections in the pool results in a session left idling in the DB.
An idle session will prevent the database being dropped and affect
commands like
bin/rails db:test:prepare. While this is an edge case,it is still good practice to clean up the connections in the pool.
Resolves #40029