Commit 95d3ff34 authored by Yorick Peterse's avatar Yorick Peterse Committed by Thong Kuah

Don't release primary connections in the DB LB

In https://gitlab.com/gitlab-org/gitlab/-/merge_requests/68042 we found
that releasing primary connections can result in test suite errors. This
can happen when tests reuse a connection also used by Puma, but release
it instead of letting the Puma thread release it. Reproducing this is
difficult, but it happens often enough to be a problem.

Since Rails already releases model connections for us, we don't actually
need to release primary connections in the first place. See the
following for more information:

- https://tenderlovemaking.com/2011/10/20/connection-management-in-activerecord.html
- https://gitlab.com/gitlab-org/gitlab/-/merge_requests/68042#note_672850193

This commit changes the load balancer code to not release primary
connections, with the exception of one case where this _is_ needed.

Changelog: fixed
parent 04fa33f1
...@@ -163,8 +163,6 @@ module Gitlab ...@@ -163,8 +163,6 @@ module Gitlab
def primary_write_location def primary_write_location
load_balancer.primary_write_location load_balancer.primary_write_location
ensure
load_balancer.release_primary_connection
end end
def database_replica_location def database_replica_location
......
...@@ -16,7 +16,8 @@ module Gitlab ...@@ -16,7 +16,8 @@ module Gitlab
end end
def release_connection def release_connection
@load_balancer.release_primary_connection # no-op as releasing primary connections isn't needed.
nil
end end
def enable_query_cache! def enable_query_cache!
...@@ -51,8 +52,6 @@ module Gitlab ...@@ -51,8 +52,6 @@ module Gitlab
def primary_write_location def primary_write_location
@load_balancer.primary_write_location @load_balancer.primary_write_location
ensure
@load_balancer.release_primary_connection
end end
def database_replica_location def database_replica_location
......
...@@ -20,10 +20,8 @@ RSpec.describe Gitlab::Database::LoadBalancing::PrimaryHost do ...@@ -20,10 +20,8 @@ RSpec.describe Gitlab::Database::LoadBalancing::PrimaryHost do
end end
describe '#release_connection' do describe '#release_connection' do
it 'releases a connection from the pool' do it 'does nothing' do
expect(load_balancer).to receive(:release_primary_connection) expect(host.release_connection).to be_nil
host.release_connection
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment