Merge branch 'expired-ci-artifacts' into 'master'

Change SQL for expired artifacts to use new ci_job_artifacts.expire_at

See merge request gitlab-org/gitlab-ce!16578

5 files changed, 65 insertions, 6 deletions

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 20534b8eed0..7312e704932 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -41,12 +41,41 @@ module Ci
 
     scope :unstarted, ->() { where(runner_id: nil) }
     scope :ignore_failures, ->() { where(allow_failure: false) }
+
+    # This convoluted mess is because we need to handle two cases of
+    # artifact files during the migration. And a simple OR clause
+    # makes it impossible to optimize.
+
+    # Instead we want to use UNION ALL and do two carefully
+    # constructed disjoint queries. But Rails cannot handle UNION or
+    # UNION ALL queries so we do the query in a subquery and wrap it
+    # in an otherwise redundant WHERE IN query (IN is fine for
+    # non-null columns).
+
+    # This should all be ripped out when the migration is finished and
+    # replaced with just the new storage to avoid the extra work.
+
     scope :with_artifacts, ->() do
-      where('(artifacts_file IS NOT NULL AND artifacts_file <> ?) OR EXISTS (?)',
-        '', Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.job_id'))
+      old = Ci::Build.select(:id).where(%q[artifacts_file <> ''])
+      new = Ci::Build.select(:id).where(%q[(artifacts_file IS NULL OR artifacts_file = '') AND EXISTS (?)],
+                                        Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.job_id'))
+      where('ci_builds.id IN (? UNION ALL ?)', old, new)
     end
-    scope :with_artifacts_not_expired, ->() { with_artifacts.where('artifacts_expire_at IS NULL OR artifacts_expire_at > ?', Time.now) }
-    scope :with_expired_artifacts, ->() { with_artifacts.where('artifacts_expire_at < ?', Time.now) }
+
+    scope :with_artifacts_not_expired, ->() do
+      old = Ci::Build.select(:id).where(%q[artifacts_file <> '' AND (artifacts_expire_at IS NULL OR artifacts_expire_at > ?)], Time.now)
+      new = Ci::Build.select(:id).where(%q[(artifacts_file IS NULL OR artifacts_file = '') AND EXISTS (?)],
+                                        Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.job_id AND (expire_at IS NULL OR expire_at > ?)', Time.now))
+      where('ci_builds.id IN (? UNION ALL ?)', old, new)
+    end
+
+    scope :with_expired_artifacts, ->() do
+      old = Ci::Build.select(:id).where(%q[artifacts_file <> '' AND artifacts_expire_at < ?], Time.now)
+      new = Ci::Build.select(:id).where(%q[(artifacts_file IS NULL OR artifacts_file = '') AND EXISTS (?)],
+                                        Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.job_id AND expire_at < ?', Time.now))
+      where('ci_builds.id IN (? UNION ALL ?)', old, new)
+    end
+
     scope :last_month, ->() { where('created_at > ?', Date.today - 1.month) }
     scope :manual_actions, ->() { where(when: :manual, status: COMPLETED_STATUSES + [:manual]) }
     scope :ref_protected, -> { where(protected: true) }
diff --git a/changelogs/unreleased/expired-ci-artifacts.yml b/changelogs/unreleased/expired-ci-artifacts.yml
new file mode 100644
index 00000000000..2fcbdb02f84
--- /dev/null
+++ b/changelogs/unreleased/expired-ci-artifacts.yml
@@ -0,0 +1,5 @@
+---
+title: Change SQL for expired artifacts to use new ci_job_artifacts.expire_at
+merge_request: 16578
+author:
+type: performance
diff --git a/db/migrate/20180119160751_optimize_ci_job_artifacts.rb b/db/migrate/20180119160751_optimize_ci_job_artifacts.rb
new file mode 100644
index 00000000000..9b4340ed7b7
--- /dev/null
+++ b/db/migrate/20180119160751_optimize_ci_job_artifacts.rb
@@ -0,0 +1,23 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class OptimizeCiJobArtifacts < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    # job_id is just here to be a covering index for index only scans
+    # since we'll almost always be joining against ci_builds on job_id
+    add_concurrent_index(:ci_job_artifacts, [:expire_at, :job_id])
+    add_concurrent_index(:ci_builds, [:artifacts_expire_at], where: "artifacts_file <> ''")
+  end
+
+  def down
+    remove_concurrent_index(:ci_job_artifacts, [:expire_at, :job_id])
+    remove_concurrent_index(:ci_builds, [:artifacts_expire_at], where: "artifacts_file <> ''")
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index d07a4c31618..57b3f4c9988 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -293,6 +293,7 @@ ActiveRecord::Schema.define(version: 20180206200543) do
     t.integer "failure_reason"
   end
 
+  add_index "ci_builds", ["artifacts_expire_at"], name: "index_ci_builds_on_artifacts_expire_at", where: "(artifacts_file <> ''::text)", using: :btree
   add_index "ci_builds", ["auto_canceled_by_id"], name: "index_ci_builds_on_auto_canceled_by_id", using: :btree
   add_index "ci_builds", ["commit_id", "stage_idx", "created_at"], name: "index_ci_builds_on_commit_id_and_stage_idx_and_created_at", using: :btree
   add_index "ci_builds", ["commit_id", "status", "type"], name: "index_ci_builds_on_commit_id_and_status_and_type", using: :btree
@@ -333,6 +334,7 @@ ActiveRecord::Schema.define(version: 20180206200543) do
     t.string "file"
   end
 
+  add_index "ci_job_artifacts", ["expire_at", "job_id"], name: "index_ci_job_artifacts_on_expire_at_and_job_id", using: :btree
   add_index "ci_job_artifacts", ["job_id", "file_type"], name: "index_ci_job_artifacts_on_job_id_and_file_type", unique: true, using: :btree
   add_index "ci_job_artifacts", ["project_id"], name: "index_ci_job_artifacts_on_project_id", using: :btree
 
diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb
index 6ba599cdf83..f6ba3a581ca 100644
--- a/spec/factories/ci/builds.rb
+++ b/spec/factories/ci/builds.rb
@@ -180,8 +180,8 @@ FactoryBot.define do
 
     trait :artifacts do
       after(:create) do |build|
-        create(:ci_job_artifact, :archive, job: build)
-        create(:ci_job_artifact, :metadata, job: build)
+        create(:ci_job_artifact, :archive, job: build, expire_at: build.artifacts_expire_at)
+        create(:ci_job_artifact, :metadata, job: build, expire_at: build.artifacts_expire_at)
         build.reload
       end
     end