Merge branch 'bvl-remove-db-pool-ff' into 'master'

Removes the DB_USE_NEW_POOL_SIZE_LOGIC flag

See merge request gitlab-org/gitlab!38049

changelogs/unreleased/bvl-remove-db-pool-ff.yml

+---
+title: Automatically calculate the database connection pool size
+merge_request: 38049
+author:
+type: other

config/database.yml.postgresql

@@ -5,7 +5,6 @@ production:
   adapter: postgresql
   encoding: unicode
   database: gitlabhq_production
-  pool: 10
   username: git
   password: "secure password"
   host: localhost
@@ -26,7 +25,6 @@ development:
   adapter: postgresql
   encoding: unicode
   database: gitlabhq_development
-  pool: 5
   username: postgres
   password: "secure password"
   host: localhost
@@ -38,7 +36,6 @@ staging:
   adapter: postgresql
   encoding: unicode
   database: gitlabhq_staging
-  pool: 10
   username: git
   password: "secure password"
   host: localhost
@@ -50,7 +47,6 @@ test: &test
   adapter: postgresql
   encoding: unicode
   database: gitlabhq_test
-  pool: 5
   username: postgres
   password:
   host: localhost

config/database_geo.yml.postgresql

@@ -5,7 +5,6 @@ production:
   adapter: postgresql
   encoding: unicode
   database: gitlabhq_geo_production
-  pool: 10
   username: git
   password: "secure password"
   host: localhost
@@ -18,7 +17,6 @@ development:
   adapter: postgresql
   encoding: unicode
   database: gitlabhq_geo_development
-  pool: 5
   username: postgres
   password: "secure password"
   host: localhost
@@ -31,7 +29,6 @@ staging:
   adapter: postgresql
   encoding: unicode
   database: gitlabhq_geo_staging
-  pool: 10
   username: git
   password: "secure password"
   host: localhost
@@ -44,7 +41,6 @@ test: &test
   adapter: postgresql
   encoding: unicode
   database: gitlabhq_geo_test
-  pool: 5
   username: postgres
   password:
   host: localhost

config/initializers/database_config.rb

@@ -20,64 +20,34 @@ Gitlab.ee do
   end
 end
 
-# TODO get rid of feature flag https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/495
-if ENV['DB_USE_NEW_POOL_SIZE_LOGIC'] == '1'
-  # Because of the way Ruby on Rails manages database connections, it is
-  # important that we have at least as many connections as we have
-  # threads. While there is a 'pool' setting in database.yml, it is not
-  # very practical because you need to maintain it in tandem with the
-  # number of application threads. Because of this we override the number
-  # of allowed connections in the database connection pool based on the
-  # configured number of application threads.
-  #
-  # Gitlab::Runtime.max_threads is the number of "user facing" application
-  # threads the process has been configured with. We also have auxiliary
-  # threads that use database connections. Because it is not practical to
-  # keep an accurate count of the number auxiliary threads as the
-  # application evolves over time, we just add a fixed headroom to the
-  # number of user-facing threads. It is OK if this number is too large
-  # because connections are instantiated lazily.
+# Because of the way Ruby on Rails manages database connections, it is
+# important that we have at least as many connections as we have
+# threads. While there is a 'pool' setting in database.yml, it is not
+# very practical because you need to maintain it in tandem with the
+# number of application threads. Because of this we override the number
+# of allowed connections in the database connection pool based on the
+# configured number of application threads.
+#
+# Gitlab::Runtime.max_threads is the number of "user facing" application
+# threads the process has been configured with. We also have auxiliary
+# threads that use database connections. Because it is not practical to
+# keep an accurate count of the number auxiliary threads as the
+# application evolves over time, we just add a fixed headroom to the
+# number of user-facing threads. It is OK if this number is too large
+# because connections are instantiated lazily.
+
+headroom = (ENV["DB_POOL_HEADROOM"].presence || 10).to_i
+calculated_pool_size = Gitlab::Runtime.max_threads + headroom
+
+db_config = Gitlab::Database.config ||
+            Rails.application.config.database_configuration[Rails.env]
+
+db_config['pool'] = calculated_pool_size
+ActiveRecord::Base.establish_connection(db_config)
 
-  headroom = (ENV["DB_POOL_HEADROOM"].presence || 10).to_i
-  calculated_pool_size = Gitlab::Runtime.max_threads + headroom
-
-  db_config = Gitlab::Database.config ||
-      Rails.application.config.database_configuration[Rails.env]
-
-  db_config['pool'] = calculated_pool_size
-  ActiveRecord::Base.establish_connection(db_config)
-
-  Gitlab.ee do
-    if Gitlab::Runtime.sidekiq? && Gitlab::Geo.geo_database_configured?
-      Rails.configuration.geo_database['pool'] = calculated_pool_size
-      Geo::TrackingBase.establish_connection(Rails.configuration.geo_database)
-    end
-  end
-elsif Gitlab::Runtime.multi_threaded?
-  # When running on multi-threaded runtimes like Puma or Sidekiq,
-  # set the number of threads per process as the minimum DB connection pool size.
-  # This is to avoid connectivity issues as was documented here:
-  # https://github.com/rails/rails/pull/23057
-  max_threads = Gitlab::Runtime.max_threads
-  db_config = Gitlab::Database.config ||
-      Rails.application.config.database_configuration[Rails.env]
-  previous_db_pool_size = db_config['pool']
-
-  db_config['pool'] = [db_config['pool'].to_i, max_threads].max + ENV["DB_POOL_HEADROOM"].to_i
-
-  ActiveRecord::Base.establish_connection(db_config)
-
-  current_db_pool_size = ActiveRecord::Base.connection.pool.size
-
-  log_pool_size('DB', previous_db_pool_size, current_db_pool_size)
-
-  Gitlab.ee do
-    if Gitlab::Runtime.sidekiq? && Gitlab::Geo.geo_database_configured?
-      previous_geo_db_pool_size = Rails.configuration.geo_database['pool']
-      Rails.configuration.geo_database['pool'] = max_threads
-      Geo::TrackingBase.establish_connection(Rails.configuration.geo_database)
-      current_geo_db_pool_size = Geo::TrackingBase.connection_pool.size
-      log_pool_size('Geo DB', previous_geo_db_pool_size, current_geo_db_pool_size)
-    end
+Gitlab.ee do
+  if Gitlab::Runtime.sidekiq? && Gitlab::Geo.geo_database_configured?
+    Rails.configuration.geo_database['pool'] = calculated_pool_size
+    Geo::TrackingBase.establish_connection(Rails.configuration.geo_database)
   end
 end

doc/install/installation.md

@@ -567,7 +567,6 @@ sudo -u git cp config/database.yml.postgresql config/database.yml
 #     adapter: postgresql
 #     encoding: unicode
 #     database: gitlabhq_production
-#     pool: 10
 #
 sudo -u git -H editor config/database.yml
 

ee/spec/initializers/database_config_spec.rb

@@ -7,57 +7,23 @@ RSpec.describe 'Database config initializer for GitLab EE' do
     load Rails.root.join('config/initializers/database_config.rb')
   end
 
-  before do
-    stub_geo_database_config(pool_size: 1)
-  end
-
-  context "when using multi-threaded runtime" do
-    let(:max_threads) { 8 }
-
-    before do
-      allow(Gitlab::Runtime).to receive(:multi_threaded?).and_return(true)
-      allow(Gitlab::Runtime).to receive(:max_threads).and_return(max_threads)
-      allow(ActiveRecord::Base).to receive(:establish_connection)
-
-      expect(Geo::TrackingBase).to receive(:establish_connection)
-    end
-
-    context "and the runtime is Sidekiq" do
-      before do
-        allow(Gitlab::Runtime).to receive(:sidekiq?).and_return(true)
-      end
+  let(:max_threads) { 8 }
 
-      it "sets Geo DB connection pool size to the max number of worker threads" do
-        expect { subject }.to change { Rails.configuration.geo_database['pool'] }.from(1).to(max_threads)
-      end
-    end
-  end
+  before do
+    allow(Gitlab::Runtime).to receive(:max_threads).and_return(max_threads)
+    allow(ActiveRecord::Base).to receive(:establish_connection)
 
-  context "when using single-threaded runtime" do
-    it "does nothing" do
-      expect { subject }.not_to change { Rails.configuration.geo_database['pool'] }
-    end
+    expect(Geo::TrackingBase).to receive(:establish_connection)
   end
 
-  context "with new pool size logic" do
-    let(:max_threads) { 8 }
-
+  context "and the runtime is Sidekiq" do
     before do
-      stub_env('DB_USE_NEW_POOL_SIZE_LOGIC', '1')
-      allow(Gitlab::Runtime).to receive(:max_threads).and_return(max_threads)
-      allow(ActiveRecord::Base).to receive(:establish_connection)
-
-      expect(Geo::TrackingBase).to receive(:establish_connection)
+      stub_geo_database_config(pool_size: 1)
+      allow(Gitlab::Runtime).to receive(:sidekiq?).and_return(true)
     end
 
-    context "and the runtime is Sidekiq" do
-      before do
-        allow(Gitlab::Runtime).to receive(:sidekiq?).and_return(true)
-      end
-
-      it "sets Geo DB connection pool size to the max number of worker threads" do
-        expect { subject }.to change { Rails.configuration.geo_database['pool'] }.from(1).to(18)
-      end
+    it "sets Geo DB connection pool size to the max number of worker threads" do
+      expect { subject }.to change { Rails.configuration.geo_database['pool'] }.from(1).to(18)
     end
   end
 

spec/initializers/database_config_spec.rb

@@ -9,119 +9,53 @@ RSpec.describe 'Database config initializer' do
 
   before do
     allow(ActiveRecord::Base).to receive(:establish_connection)
+    allow(Gitlab::Runtime).to receive(:max_threads).and_return(max_threads)
   end
 
-  context "when using multi-threaded runtime" do
-    let(:max_threads) { 8 }
+  let(:max_threads) { 8 }
 
+  context "no existing pool size is set" do
     before do
-      allow(Gitlab::Runtime).to receive(:multi_threaded?).and_return(true)
-      allow(Gitlab::Runtime).to receive(:max_threads).and_return(max_threads)
+      stub_database_config(pool_size: nil)
     end
 
-    context "and no existing pool size is set" do
-      before do
-        stub_database_config(pool_size: nil)
-      end
-
-      it "sets it to the max number of worker threads" do
-        expect { subject }.to change { Gitlab::Database.config['pool'] }.from(nil).to(max_threads)
-      end
-    end
-
-    context "and the existing pool size is smaller than the max number of worker threads" do
-      before do
-        stub_database_config(pool_size: max_threads - 1)
-      end
-
-      it "sets it to the max number of worker threads" do
-        expect { subject }.to change { Gitlab::Database.config['pool'] }.by(1)
-      end
+    it "sets it based on the max number of worker threads" do
+      expect { subject }.to change { Gitlab::Database.config['pool'] }.from(nil).to(18)
     end
+  end
 
-    context "and the existing pool size is larger than the max number of worker threads" do
-      before do
-        stub_database_config(pool_size: max_threads + 1)
-      end
-
-      it "keeps the configured pool size" do
-        expect { subject }.not_to change { Gitlab::Database.config['pool'] }
-      end
+  context "the existing pool size is smaller than the max number of worker threads" do
+    before do
+      stub_database_config(pool_size: 1)
     end
 
-    context "when specifying headroom through an ENV variable" do
-      let(:headroom) { 10 }
-
-      before do
-        stub_database_config(pool_size: 1)
-        stub_env("DB_POOL_HEADROOM", headroom)
-      end
-
-      it "adds headroom on top of the calculated size" do
-        expect { subject }.to change { Gitlab::Database.config['pool'] }
-                                .from(1)
-                                .to(max_threads + headroom)
-      end
+    it "sets it based on the max number of worker threads" do
+      expect { subject }.to change { Gitlab::Database.config['pool'] }.from(1).to(18)
     end
   end
 
-  context "when using single-threaded runtime" do
-    it "does nothing" do
-      expect { subject }.not_to change { Gitlab::Database.config['pool'] }
-    end
-  end
-
-  context "with new pool size logic" do
-    let(:max_threads) { 8 }
-
+  context "and the existing pool size is larger than the max number of worker threads" do
     before do
-      stub_env('DB_USE_NEW_POOL_SIZE_LOGIC', '1')
-      allow(Gitlab::Runtime).to receive(:max_threads).and_return(max_threads)
-    end
-
-    context "and no existing pool size is set" do
-      before do
-        stub_database_config(pool_size: nil)
-      end
-
-      it "sets it based on the max number of worker threads" do
-        expect { subject }.to change { Gitlab::Database.config['pool'] }.from(nil).to(18)
-      end
+      stub_database_config(pool_size: 100)
     end
 
-    context "and the existing pool size is smaller than the max number of worker threads" do
-      before do
-        stub_database_config(pool_size: 1)
-      end
-
-      it "sets it based on the max number of worker threads" do
-        expect { subject }.to change { Gitlab::Database.config['pool'] }.from(1).to(18)
-      end
+    it "sets it based on the max number of worker threads" do
+      expect { subject }.to change { Gitlab::Database.config['pool'] }.from(100).to(18)
     end
+  end
 
-    context "and the existing pool size is larger than the max number of worker threads" do
-      before do
-        stub_database_config(pool_size: 100)
-      end
+  context "when specifying headroom through an ENV variable" do
+    let(:headroom) { 15 }
 
-      it "sets it based on the max number of worker threads" do
-        expect { subject }.to change { Gitlab::Database.config['pool'] }.from(100).to(18)
-      end
+    before do
+      stub_database_config(pool_size: 1)
+      stub_env("DB_POOL_HEADROOM", headroom)
     end
 
-    context "when specifying headroom through an ENV variable" do
-      let(:headroom) { 15 }
-
-      before do
-        stub_database_config(pool_size: 1)
-        stub_env("DB_POOL_HEADROOM", headroom)
-      end
-
-      it "adds headroom on top of the calculated size" do
-        expect { subject }.to change { Gitlab::Database.config['pool'] }
-                                .from(1)
-                                .to(max_threads + headroom)
-      end
+    it "adds headroom on top of the calculated size" do
+      expect { subject }.to change { Gitlab::Database.config['pool'] }
+                              .from(1)
+                              .to(max_threads + headroom)
     end
   end