Skip to content

Remove subtransactions in Ci::Minutes::UpdateProjectAndNamespaceUsageService

In #338346 (comment 652623314), we identified the use of subtransactions can cause significant performance issues on PostgreSQL replicas at scale. If a long running transaction occurs, the subtransaction cache may not fit into the working set of memory, which can lead to a context switch storm when PostgreSQL needs to load subtransaction data from disk. While this arguably is a PostgreSQL bug, we should remove subtransactions entirely because they aren't performant at GitLab.com scale, nor are they strictly necessary.

From https://thanos.gitlab.net/classic/graph?g0.range_input=2h&g0.stacked=0&g0.max_source_resolution=0s&g0.expr=sum(rate(gitlab_active_record_subtransactions_total%7Benv%3D%22gprd%22%7D%5B1m%5D))%20by%20(model)&g0.tab=0 you can see ::Ci::Minutes::NamespaceMonthlyUsage and ::Ci::Minutes::ProjectMonthlyUsage generate a significant number of subtransactions:

image

Where does this happen? In https://gitlab.com/gitlab-org/gitlab/blob/dd36b5f5767ff0e04f8b27fea15219c7c1cd5748/ee/app/services/ci/minutes/update_project_and_namespace_usage_service.rb#L16-23, we essentially do:

        ApplicationRecord.transaction do
          ::Ci::Minutes::NamespaceMonthlyUsage.increase_usage(namespace_usage, consumption) if namespace_usage
          ::Ci::Minutes::ProjectMonthlyUsage.increase_usage(project_usage, consumption) if project_usage
<snip>

This creates at least two subtransactions per update:

  1. ::Ci::Minutes::NamespaceMonthlyUsage.safe_find_or_create_by from https://gitlab.com/gitlab-org/gitlab/blob/f558ca40fea462f2ec493e158feda641b691c19f/ee/app/services/ci/minutes/update_project_and_namespace_usage_service.rb#L62
  2. ::Ci::Minutes::ProjectMonthlyUsage.safe_find_or_create_by from https://gitlab.com/gitlab-org/gitlab/blob/f558ca40fea462f2ec493e158feda641b691c19f/ee/app/services/ci/minutes/update_project_and_namespace_usage_service.rb#L68

We use safe_find_or_create_by to ensure we create a new model atomically.

We could just use a PostgreSQL insert or upsert to ensure these records exist before incrementing the data.

If we wanted to rewrite safe_find_or_create_by to use insert or upsert (making sure we handle the ON CONFLICT case), we would also need to account for the fact that these methods don't make ActiveRecord callbacks. From https://stackoverflow.com/questions/39058213/postgresql-upsert-differentiate-inserted-and-updated-rows-using-system-columns-x/39204667, we can potentially solve this by returning xmax and checking whether the value is 0. If it is 0, it is newly-inserted, and we can fire the callbacks ourselves. There may be some callbacks (e.g. before_save) that may not work, however.

Edited by Stan Hu