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
- subtraction rate by models https://thanos.gitlab.net/graph?g0.expr=sum%20by%20(model)%20(rate(gitlab_active_record_subtransactions_total%7Benv%3D%22gprd%22%7D%5B1m%5D))&g0.tab=0&g0.stacked=0&g0.range_input=1d&g0.max_source_resolution=0s&g0.deduplicate=1&g0.partial_response=0&g0.store_matches=%5B%5D
- subtransaction counts by model https://thanos.gitlab.net/graph?g0.expr=sum%20by%20(model)%20(count_over_time(gitlab_active_record_subtransactions_total%7Benv%3D%22gprd%22%7D%5B1m%5D))&g0.tab=0&g0.stacked=0&g0.range_input=2h&g0.max_source_resolution=0s&g0.deduplicate=1&g0.partial_response=0&g0.store_matches=%5B%5D
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.