Skip to content

Resolve root cause of PG Subtransaction related performance degradations

Problem to solve

Investigation has determined (thank you, @cat!) that any instance of subtransactions in our codebase can lead to performance impact across the Postgres cluster.

Given that this is the case, we're going to have to not only address models that appear in the dashboard but also remove all instances of subtransaction generating code from our codebase.

Dashboards

Request

Code search resolution

Identify lines of code in our codebase that use requires_new: true to generate subtransactions and determine an appropriate alternative that does not. An example could be to identify using grep -R 'requires_new: true' (thanks @sgoldstein) and then implement an upsert in place of a subtransaction.

Here's the active record documentation on why we're ending up with subtransactions: API docs about ActiveRecord Nested Transactions

grep -REin 'requires_new:\s*true' .
./app/models/application_setting.rb:625:    transaction(requires_new: true) do
./app/models/internal_id.rb:219:      subject.transaction(requires_new: true) do
./app/models/internal_id.rb:328:      subject.transaction(requires_new: true) do
./app/models/shard.rb:21:    transaction(requires_new: true) do
./app/models/application_record.rb:33:    transaction(requires_new: true) do
./app/models/application_record.rb:57:      transaction(requires_new: true) do
./app/models/application_record.rb:82:    transaction(requires_new: true) { all.create(*args, &block) }
./app/services/projects/move_project_members_service.rb:12:      Project.transaction(requires_new: true) do
./app/services/projects/move_forks_service.rb:8:      Project.transaction(requires_new: true) do
./app/services/projects/move_users_star_projects_service.rb:12:      Project.transaction(requires_new: true) do
./app/services/projects/move_notification_settings_service.rb:8:      Project.transaction(requires_new: true) do
./app/services/projects/move_project_authorizations_service.rb:12:      Project.transaction(requires_new: true) do
./app/services/projects/move_project_group_links_service.rb:12:      Project.transaction(requires_new: true) do
./app/services/projects/move_lfs_objects_projects_service.rb:8:      Project.transaction(requires_new: true) do
./app/services/projects/move_deploy_keys_projects_service.rb:8:      Project.transaction(requires_new: true) do
./app/services/users/migrate_to_ghost_user_service.rb:42:      user.transaction(requires_new: true) do
./ee/app/services/vulnerabilities/create_service.rb:19:      Vulnerabilities::Finding.transaction(requires_new: true) do
./ee/spec/services/vulnerabilities/create_service_spec.rb:69:      expect(Vulnerabilities::Finding).to have_received(:transaction).with(requires_new: true).once
./ee/lib/ee/gitlab/background_migration/migrate_approver_to_approval_rules.rb:68:            transaction(requires_new: true) do
./spec/models/application_record_spec.rb:132:        Project.transaction(requires_new: true) do
./spec/models/application_record_spec.rb:148:      Project.transaction(requires_new: true) do
./spec/models/application_record_spec.rb:162:        Project.transaction(requires_new: true) do
./spec/models/concerns/atomic_internal_id_spec.rb:169:      ActiveRecord::Base.transaction(requires_new: true) do
./spec/support/shared_examples/models/atomic_internal_id_shared_examples.rb:144:        ActiveRecord::Base.transaction(requires_new: true) do
./spec/lib/gitlab/database/transaction/observer_spec.rb:24:        ActiveRecord::Base.transaction(requires_new: true) do
./spec/support_specs/database/prevent_cross_database_modification_spec.rb:24:        Project.transaction(requires_new: true) do
./spec/support_specs/database/prevent_cross_database_modification_spec.rb:25:          Project.transaction(requires_new: true) do
./spec/support_specs/database/prevent_cross_database_modification_spec.rb:93:            Project.transaction(requires_new: true) do
./spec/support_specs/database/prevent_cross_database_modification_spec.rb:95:              Project.transaction(requires_new: true) do
./lib/gitlab/database/with_lock_retries.rb:125:        ActiveRecord::Base.transaction(requires_new: true) do
./lib/gitlab/background_migration/backfill_design_internal_ids.rb:76:          subject.transaction(requires_new: true) do
./lib/gitlab/background_migration/backfill_project_repositories.rb:24:          Shard.transaction(requires_new: true) do
./db/post_migrate/20201106134950_deduplicate_epic_iids.rb:88:      subject.transaction(requires_new: true) do

https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/models/application_setting.rb#L625

Model resolution

Please review the linked issues and issues labelled with subtransactions to determine whether your team owns or would be best placed to remove subtransactions from the particular model.

Considerations

Consideration should be given to the potential for bugs to be introduced due to the absence of a previously used subtransaction.
In some cases, alternatives will need to be found to existing logic that will safely account for the state of data without using subtransactions.
The presence of any subtransaction poses a risk to our PG cluster performance.
Mitigation work is considered infradev severity1 priority1.

History

With the new instrumentation available from #337843 (closed), we would want to identify the cause of subtransactions related database degradation and identify improvement opportunities.

Table of incidents

See table in comment

Scope

This is the list of models from the dashboard.

Edited by Sam Goldstein