Rework indexes on redirect_routes to be more effective and enforce a case insensitive unique pathfix-redirect-routes-schema

4 files changed, 76 insertions, 3 deletions

diff --git a/changelogs/unreleased/fix-redirect-routes-schema.yml b/changelogs/unreleased/fix-redirect-routes-schema.yml
new file mode 100644
index 00000000000..ea2b916307a
--- /dev/null
+++ b/changelogs/unreleased/fix-redirect-routes-schema.yml
@@ -0,0 +1,5 @@
+---
+title: rework indexes on redirect_routes
+merge_request:
+author:
+type: performance
diff --git a/db/migrate/20180113220114_rework_redirect_routes_indexes.rb b/db/migrate/20180113220114_rework_redirect_routes_indexes.rb
new file mode 100644
index 00000000000..ab9971be074
--- /dev/null
+++ b/db/migrate/20180113220114_rework_redirect_routes_indexes.rb
@@ -0,0 +1,68 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class ReworkRedirectRoutesIndexes < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  INDEX_NAME_UNIQUE = "index_redirect_routes_on_path_unique_text_pattern_ops"
+
+  INDEX_NAME_PERM = "index_redirect_routes_on_path_text_pattern_ops_where_permanent"
+  INDEX_NAME_TEMP = "index_redirect_routes_on_path_text_pattern_ops_where_temporary"
+
+  OLD_INDEX_NAME_PATH_TPOPS = "index_redirect_routes_on_path_text_pattern_ops"
+  OLD_INDEX_NAME_PATH_LOWER = "index_on_redirect_routes_lower_path"
+
+  def up
+    disable_statement_timeout
+
+    # this is a plain btree on a single boolean column. It'll never be
+    # selective enough to be valuable. This class is called by
+    # setup_postgresql.rake so it needs to be able to handle this
+    # index not existing.
+    if index_exists?(:redirect_routes, :permanent)
+      remove_concurrent_index(:redirect_routes, :permanent)
+    end
+
+    # If we're on MySQL then the existing index on path is ok. But on
+    # Postgres we need to clean things up:
+    return unless Gitlab::Database.postgresql?
+
+    if_not_exists = Gitlab::Database.version.to_f >= 9.5 ? "IF NOT EXISTS" : ""
+
+    # Unique index on lower(path) across both types of redirect_routes:
+    execute("CREATE UNIQUE INDEX CONCURRENTLY #{if_not_exists} #{INDEX_NAME_UNIQUE} ON redirect_routes (lower(path) varchar_pattern_ops);")
+
+    # Make two indexes on path -- one for permanent and one for temporary routes:
+    execute("CREATE INDEX CONCURRENTLY #{if_not_exists} #{INDEX_NAME_PERM} ON redirect_routes (lower(path) varchar_pattern_ops) where (permanent);")
+    execute("CREATE INDEX CONCURRENTLY #{if_not_exists} #{INDEX_NAME_TEMP} ON redirect_routes (lower(path) varchar_pattern_ops) where (not permanent or permanent is null) ;")
+
+    # Remove the old indexes:
+
+    # This one needed to be on lower(path) but wasn't so it's replaced with the two above
+    execute "DROP INDEX CONCURRENTLY IF EXISTS #{OLD_INDEX_NAME_PATH_TPOPS};"
+
+    # This one isn't needed because we only ever do = and LIKE on this
+    # column so the varchar_pattern_ops index is sufficient
+    execute "DROP INDEX CONCURRENTLY IF EXISTS #{OLD_INDEX_NAME_PATH_LOWER};"
+  end
+
+  def down
+    disable_statement_timeout
+
+    add_concurrent_index(:redirect_routes, :permanent)
+
+    return unless Gitlab::Database.postgresql?
+
+    execute("CREATE INDEX CONCURRENTLY #{OLD_INDEX_NAME_PATH_TPOPS} ON redirect_routes (path varchar_pattern_ops);")
+    execute("CREATE INDEX CONCURRENTLY #{OLD_INDEX_NAME_PATH_LOWER} ON redirect_routes (LOWER(path));")
+
+    execute("DROP INDEX CONCURRENTLY IF EXISTS #{INDEX_NAME_UNIQUE};")
+    execute("DROP INDEX CONCURRENTLY IF EXISTS #{INDEX_NAME_PERM};")
+    execute("DROP INDEX CONCURRENTLY IF EXISTS #{INDEX_NAME_TEMP};")
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 8a6db61250b..5c9b950cd6f 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20180105212544) do
+ActiveRecord::Schema.define(version: 20180113220114) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -1536,8 +1536,6 @@ ActiveRecord::Schema.define(version: 20180105212544) do
   end
 
   add_index "redirect_routes", ["path"], name: "index_redirect_routes_on_path", unique: true, using: :btree
-  add_index "redirect_routes", ["path"], name: "index_redirect_routes_on_path_text_pattern_ops", using: :btree, opclasses: {"path"=>"varchar_pattern_ops"}
-  add_index "redirect_routes", ["permanent"], name: "index_redirect_routes_on_permanent", using: :btree
   add_index "redirect_routes", ["source_type", "source_id"], name: "index_redirect_routes_on_source_type_and_source_id", using: :btree
 
   create_table "releases", force: :cascade do |t|
diff --git a/lib/tasks/migrate/setup_postgresql.rake b/lib/tasks/migrate/setup_postgresql.rake
index c9e3eed82f2..c996537cfbe 100644
--- a/lib/tasks/migrate/setup_postgresql.rake
+++ b/lib/tasks/migrate/setup_postgresql.rake
@@ -7,6 +7,7 @@ require Rails.root.join('db/migrate/20170317203554_index_routes_path_for_like')
 require Rails.root.join('db/migrate/20170724214302_add_lower_path_index_to_redirect_routes')
 require Rails.root.join('db/migrate/20170503185032_index_redirect_routes_path_for_like')
 require Rails.root.join('db/migrate/20171220191323_add_index_on_namespaces_lower_name.rb')
+require Rails.root.join('db/migrate/20180113220114_rework_redirect_routes_indexes.rb')
 
 desc 'GitLab | Sets up PostgreSQL'
 task setup_postgresql: :environment do
@@ -17,4 +18,5 @@ task setup_postgresql: :environment do
   AddLowerPathIndexToRedirectRoutes.new.up
   IndexRedirectRoutesPathForLike.new.up
   AddIndexOnNamespacesLowerName.new.up
+  ReworkRedirectRoutesIndexes.new.up
 end