Skip to content

Disconnect connections used for acquiring advisory lock in migration. - #40101

Merged
eileencodes merged 1 commit into
rails:masterfrom
tgxworld:fix_not_disconnecting_pool
Sep 4, 2020
Merged

Disconnect connections used for acquiring advisory lock in migration.#40101
eileencodes merged 1 commit into
rails:masterfrom
tgxworld:fix_not_disconnecting_pool

Conversation

@tgxworld

@tgxworld tgxworld commented Aug 25, 2020

Copy link
Copy Markdown
Contributor

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

@tgxworld

Copy link
Copy Markdown
Contributor Author

@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
tgxworld force-pushed the fix_not_disconnecting_pool branch from e721789 to ae330af Compare August 25, 2020 07:43
@tgxworld

tgxworld commented Sep 4, 2020

Copy link
Copy Markdown
Contributor Author

@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
eileencodes merged commit 3d42877 into rails:master Sep 4, 2020
@tgxworld
tgxworld deleted the fix_not_disconnecting_pool branch September 4, 2020 13:28
@tgxworld

tgxworld commented Sep 5, 2020

Copy link
Copy Markdown
Contributor Author

@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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

New Advisory Lock that opens a second connection causes bug in Postgres app

2 participants