Merge branch 'improve-project-external-issue-trackers' into 'master'

Greatly improve external_issue_tracker performance

See 3d41133d48f6522b8755bb91b804864e8e520871 for all the details. As an aside, this commit contains a set of migrations that will introduce downtime as they add a column with a default value which in turn locks the entire table (at least on PostgreSQL).

See merge request !2498

8 files changed, 92 insertions, 33 deletions

diff --git a/app/models/project.rb b/app/models/project.rb
index 7e131151513..cb668e0c2f7 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -468,12 +468,9 @@ class Project < ActiveRecord::Base
     !external_issue_tracker
   end
 
-  def external_issues_trackers
-    services.select(&:issue_tracker?).reject(&:default?)
-  end
-
   def external_issue_tracker
-    @external_issues_tracker ||= external_issues_trackers.find(&:activated?)
+    @external_issue_tracker ||=
+      services.issue_trackers.active.without_defaults.first
   end
 
   def can_have_issues_tracker_id?
diff --git a/app/models/project_services/ci_service.rb b/app/models/project_services/ci_service.rb
index c3f70d1f972..e10b5529b42 100644
--- a/app/models/project_services/ci_service.rb
+++ b/app/models/project_services/ci_service.rb
@@ -23,14 +23,12 @@
 # List methods you need to implement to get your CI service
 # working with GitLab Merge Requests
 class CiService < Service
-  def category
-    :ci
-  end
-  
+  default_value_for :category, 'ci'
+
   def valid_token?(token)
     self.respond_to?(:token) && self.token.present? && self.token == token
   end
-  
+
   def supported_events
     %w(push)
   end
diff --git a/app/models/project_services/gitlab_issue_tracker_service.rb b/app/models/project_services/gitlab_issue_tracker_service.rb
index 7aa04309f54..05436cd0f79 100644
--- a/app/models/project_services/gitlab_issue_tracker_service.rb
+++ b/app/models/project_services/gitlab_issue_tracker_service.rb
@@ -24,9 +24,7 @@ class GitlabIssueTrackerService < IssueTrackerService
 
   prop_accessor :title, :description, :project_url, :issues_url, :new_issue_url
 
-  def default?
-    true
-  end
+  default_value_for :default, true
 
   def to_param
     'gitlab'
diff --git a/app/models/project_services/issue_tracker_service.rb b/app/models/project_services/issue_tracker_service.rb
index ed201979d39..25045224ce5 100644
--- a/app/models/project_services/issue_tracker_service.rb
+++ b/app/models/project_services/issue_tracker_service.rb
@@ -23,12 +23,10 @@ class IssueTrackerService < Service
 
   validates :project_url, :issues_url, :new_issue_url, presence: true, if: :activated?
 
-  def category
-    :issue_tracker
-  end
+  default_value_for :category, 'issue_tracker'
 
   def default?
-    false
+    default
   end
 
   def issue_url(iid)
diff --git a/app/models/service.rb b/app/models/service.rb
index 24f4bf7646e..721273250ea 100644
--- a/app/models/service.rb
+++ b/app/models/service.rb
@@ -43,6 +43,9 @@ class Service < ActiveRecord::Base
   validates :project_id, presence: true, unless: Proc.new { |service| service.template? }
 
   scope :visible, -> { where.not(type: ['GitlabIssueTrackerService', 'GitlabCiService']) }
+  scope :issue_trackers, -> { where(category: 'issue_tracker') }
+  scope :active, -> { where(active: true) }
+  scope :without_defaults, -> { where(default: false) }
 
   scope :push_hooks, -> { where(push_events: true, active: true) }
   scope :tag_push_hooks, -> { where(tag_push_events: true, active: true) }
@@ -51,6 +54,8 @@ class Service < ActiveRecord::Base
   scope :note_hooks, -> { where(note_events: true, active: true) }
   scope :build_hooks, -> { where(build_events: true, active: true) }
 
+  default_value_for :category, 'common'
+
   def activated?
     active
   end
@@ -60,7 +65,7 @@ class Service < ActiveRecord::Base
   end
 
   def category
-    :common
+    read_attribute(:category).to_sym
   end
 
   def initialize_properties
@@ -153,7 +158,7 @@ class Service < ActiveRecord::Base
 
   # Returns a hash of the properties that have been assigned a new value since last save,
   # indicating their original values (attr => original value).
-  # ActiveRecord does not provide a mechanism to track changes in serialized keys, 
+  # ActiveRecord does not provide a mechanism to track changes in serialized keys,
   # so we need a specific implementation for service properties.
   # This allows to track changes to properties set with the accessor methods,
   # but not direct manipulation of properties hash.
@@ -164,7 +169,7 @@ class Service < ActiveRecord::Base
   def reset_updated_properties
     @updated_properties = nil
   end
-  
+
   def async_execute(data)
     return unless supported_events.include?(data[:object_kind])
 
diff --git a/db/migrate/20160119111158_add_services_category.rb b/db/migrate/20160119111158_add_services_category.rb
new file mode 100644
index 00000000000..a9110a8418b
--- /dev/null
+++ b/db/migrate/20160119111158_add_services_category.rb
@@ -0,0 +1,39 @@
+class AddServicesCategory < ActiveRecord::Migration
+  def up
+    add_column :services, :category, :string, default: 'common', null: false
+
+    category = quote_column_name('category')
+    type     = quote_column_name('type')
+
+    execute <<-EOF
+UPDATE services
+SET #{category} = 'issue_tracker'
+WHERE #{type} IN (
+  'CustomIssueTrackerService',
+  'GitlabIssueTrackerService',
+  'IssueTrackerService',
+  'JiraService',
+  'RedmineService'
+);
+EOF
+
+    execute <<-EOF
+UPDATE services
+SET #{category} = 'ci'
+WHERE #{type} IN (
+  'BambooService',
+  'BuildkiteService',
+  'CiService',
+  'DroneCiService',
+  'GitlabCiService',
+  'TeamcityService'
+);
+    EOF
+
+    add_index :services, :category
+  end
+
+  def down
+    remove_column :services, :category
+  end
+end
diff --git a/db/migrate/20160119112418_add_services_default.rb b/db/migrate/20160119112418_add_services_default.rb
new file mode 100644
index 00000000000..69a42d7b873
--- /dev/null
+++ b/db/migrate/20160119112418_add_services_default.rb
@@ -0,0 +1,20 @@
+class AddServicesDefault < ActiveRecord::Migration
+  def up
+    add_column :services, :default, :boolean, default: false
+
+    default = quote_column_name('default')
+    type    = quote_column_name('type')
+
+    execute <<-EOF
+UPDATE services
+SET #{default} = true
+WHERE #{type} = 'GitlabIssueTrackerService'
+EOF
+
+    add_index :services, :default
+  end
+
+  def down
+    remove_column :services, :default
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 9045135dd9a..a08181b910f 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: 20160113111034) do
+ActiveRecord::Schema.define(version: 20160119112418) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -725,20 +725,24 @@ ActiveRecord::Schema.define(version: 20160113111034) do
     t.string   "type"
     t.string   "title"
     t.integer  "project_id"
-    t.datetime "created_at"
-    t.datetime "updated_at"
-    t.boolean  "active",                default: false, null: false
+    t.datetime "created_at",                                           null: false
+    t.datetime "updated_at",                                           null: false
+    t.boolean  "active",                                               null: false
     t.text     "properties"
-    t.boolean  "template",              default: false
-    t.boolean  "push_events",           default: true
-    t.boolean  "issues_events",         default: true
-    t.boolean  "merge_requests_events", default: true
-    t.boolean  "tag_push_events",       default: true
-    t.boolean  "note_events",           default: true,  null: false
-    t.boolean  "build_events",          default: false, null: false
-  end
-
+    t.boolean  "template",                          default: false
+    t.boolean  "push_events",                       default: true
+    t.boolean  "issues_events",                     default: true
+    t.boolean  "merge_requests_events",             default: true
+    t.boolean  "tag_push_events",                   default: true
+    t.boolean  "note_events",                       default: true,     null: false
+    t.boolean  "build_events",                      default: false,    null: false
+    t.string   "category",                          default: "common", null: false
+    t.boolean  "default",                           default: false
+  end
+
+  add_index "services", ["category"], name: "index_services_on_category", using: :btree
   add_index "services", ["created_at", "id"], name: "index_services_on_created_at_and_id", using: :btree
+  add_index "services", ["default"], name: "index_services_on_default", using: :btree
   add_index "services", ["project_id"], name: "index_services_on_project_id", using: :btree
   add_index "services", ["template"], name: "index_services_on_template", using: :btree