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

This reverts merge request !16578

app/models/ci/build.rb

@@ -41,41 +41,12 @@ 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
-      old = Ci::Build.select(:id).where(%q[artifacts_file <> ''])
+      where('(artifacts_file IS NOT NULL AND artifacts_file <> ?) OR EXISTS (?)',
-      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'))
-                                        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_artifacts_not_expired, ->() do
+    scope :with_expired_artifacts, ->() { with_artifacts.where('artifacts_expire_at < ?', Time.now) }
-      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) }

changelogs/unreleased/expired-ci-artifacts.yml

@@ -1,5 +0,0 @@
----
-title: Change SQL for expired artifacts to use new ci_job_artifacts.expire_at
-merge_request: 16578
-author:
-type: performance

db/migrate/20180119160751_optimize_ci_job_artifacts.rb

@@ -1,23 +0,0 @@
-# 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

db/schema.rb

@@ -293,7 +293,6 @@ ActiveRecord::Schema.define(version: 20180208183958) 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
@@ -334,7 +333,6 @@ ActiveRecord::Schema.define(version: 20180208183958) 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
 

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, expire_at: build.artifacts_expire_at)
+        create(:ci_job_artifact, :archive, job: build)
-        create(:ci_job_artifact, :metadata, job: build, expire_at: build.artifacts_expire_at)
+        create(:ci_job_artifact, :metadata, job: build)
         build.reload
       end
     end