Remove the project_authorizations.id columnremove-project-authorizations-id-column

This column used to be a 32 bits integer, allowing for only a maximum of
2 147 483 647 rows. Given enough users one can hit this limit pretty
quickly, as was the case for GitLab.com.

Changing this type to bigint (= 64 bits) would give us more space, but
we'd eventually hit the same limit given enough users and projects. A
much more sustainable solution is to simply drop the "id" column.

There were only 2 lines of code depending on this column being present,
and neither truly required it to be present. Instead the code now uses
the "project_id" column combined with the "user_id" column. This means
that instead of something like this:

    DELETE FROM project_authorizations
    WHERE user_id = X
    AND id = Y;

We now run the following when removing rows:

    DELETE FROM project_authorizations
    WHERE user_id = X
    AND project_id = Y;

Since both user_id and project_id are indexed this should not slow down
the DELETE query.

This commit also removes the "dependent: destroy" clause from the
"project_authorizations" relation in the User and Project models.
Keeping this prevents Rails from being able to remove data as it relies
on an "id" column being present. Since the "project_authorizations"
table has proper foreign keys set up (with cascading removals) we don't
need to depend on any Rails logic.

7 files changed, 33 insertions, 16 deletions

diff --git a/app/models/project.rb b/app/models/project.rb
index ec40def6fb1..94a6f3ba799 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -130,7 +130,7 @@ class Project < ActiveRecord::Base
   has_many :hooks,              dependent: :destroy, class_name: 'ProjectHook'
   has_many :protected_branches, dependent: :destroy
 
-  has_many :project_authorizations, dependent: :destroy
+  has_many :project_authorizations
   has_many :authorized_users, through: :project_authorizations, source: :user, class_name: 'User'
   has_many :project_members, -> { where(requested_at: nil) }, dependent: :destroy, as: :source
   alias_method :members, :project_members
diff --git a/app/models/user.rb b/app/models/user.rb
index 66a768d54bb..06dd98a3188 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -73,7 +73,7 @@ class User < ActiveRecord::Base
   has_many :created_projects,         foreign_key: :creator_id, class_name: 'Project'
   has_many :users_star_projects, dependent: :destroy
   has_many :starred_projects, through: :users_star_projects, source: :project
-  has_many :project_authorizations, dependent: :destroy
+  has_many :project_authorizations
   has_many :authorized_projects, through: :project_authorizations, source: :project
 
   has_many :snippets,                 dependent: :destroy, foreign_key: :author_id
@@ -444,7 +444,7 @@ class User < ActiveRecord::Base
   end
 
   def remove_project_authorizations(project_ids)
-    project_authorizations.where(id: project_ids).delete_all
+    project_authorizations.where(project_id: project_ids).delete_all
   end
 
   def set_authorized_projects_column
diff --git a/app/services/users/refresh_authorized_projects_service.rb b/app/services/users/refresh_authorized_projects_service.rb
index 8559908e0c3..21ec1bd9e65 100644
--- a/app/services/users/refresh_authorized_projects_service.rb
+++ b/app/services/users/refresh_authorized_projects_service.rb
@@ -35,7 +35,7 @@ module Users
         # rows not in the new list or with a different access level should be
         # removed.
         if !fresh[project_id] || fresh[project_id] != row.access_level
-          array << row.id
+          array << row.project_id
         end
       end
 
@@ -100,7 +100,7 @@ module Users
     end
 
     def current_authorizations
-      user.project_authorizations.select(:id, :project_id, :access_level)
+      user.project_authorizations.select(:project_id, :access_level)
     end
 
     def fresh_authorizations
diff --git a/changelogs/unreleased/remove-project-authorizations-id-column.yml b/changelogs/unreleased/remove-project-authorizations-id-column.yml
new file mode 100644
index 00000000000..24c86f0fb1b
--- /dev/null
+++ b/changelogs/unreleased/remove-project-authorizations-id-column.yml
@@ -0,0 +1,4 @@
+---
+title: Remove the project_authorizations.id column
+merge_request:
+author:
diff --git a/db/post_migrate/20170106172224_remove_project_authorizations_id_column.rb b/db/post_migrate/20170106172224_remove_project_authorizations_id_column.rb
new file mode 100644
index 00000000000..7c788160022
--- /dev/null
+++ b/db/post_migrate/20170106172224_remove_project_authorizations_id_column.rb
@@ -0,0 +1,12 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class RemoveProjectAuthorizationsIdColumn < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    remove_column :project_authorizations, :id, :primary_key
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 923ece86edb..379e566e9ec 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: 20161227192806) do
+ActiveRecord::Schema.define(version: 20170106172224) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -861,7 +861,7 @@ ActiveRecord::Schema.define(version: 20161227192806) do
   add_index "personal_access_tokens", ["token"], name: "index_personal_access_tokens_on_token", unique: true, using: :btree
   add_index "personal_access_tokens", ["user_id"], name: "index_personal_access_tokens_on_user_id", using: :btree
 
-  create_table "project_authorizations", force: :cascade do |t|
+  create_table "project_authorizations", id: false, force: :cascade do |t|
     t.integer "user_id"
     t.integer "project_id"
     t.integer "access_level"
@@ -1306,4 +1306,4 @@ ActiveRecord::Schema.define(version: 20161227192806) do
   add_foreign_key "subscriptions", "projects", on_delete: :cascade
   add_foreign_key "trending_projects", "projects", on_delete: :cascade
   add_foreign_key "u2f_registrations", "users"
-end
\ No newline at end of file
+end
diff --git a/spec/services/users/refresh_authorized_projects_service_spec.rb b/spec/services/users/refresh_authorized_projects_service_spec.rb
index 1f6919151de..9fbb61565e3 100644
--- a/spec/services/users/refresh_authorized_projects_service_spec.rb
+++ b/spec/services/users/refresh_authorized_projects_service_spec.rb
@@ -20,7 +20,7 @@ describe Users::RefreshAuthorizedProjectsService do
       to_remove = create_authorization(project2, user)
 
       expect(service).to receive(:update_with_lease).
-        with([to_remove.id], [[user.id, project.id, Gitlab::Access::MASTER]])
+        with([to_remove.project_id], [[user.id, project.id, Gitlab::Access::MASTER]])
 
       service.execute
     end
@@ -29,7 +29,7 @@ describe Users::RefreshAuthorizedProjectsService do
       to_remove = create_authorization(project, user, Gitlab::Access::DEVELOPER)
 
       expect(service).to receive(:update_with_lease).
-        with([to_remove.id], [[user.id, project.id, Gitlab::Access::MASTER]])
+        with([to_remove.project_id], [[user.id, project.id, Gitlab::Access::MASTER]])
 
       service.execute
     end
@@ -90,7 +90,7 @@ describe Users::RefreshAuthorizedProjectsService do
     it 'removes authorizations that should be removed' do
       authorization = create_authorization(project, user)
 
-      service.update_authorizations([authorization.id])
+      service.update_authorizations([authorization.project_id])
 
       expect(user.project_authorizations).to be_empty
     end
@@ -147,7 +147,12 @@ describe Users::RefreshAuthorizedProjectsService do
     end
 
     it 'sets the values to the project authorization rows' do
-      expect(hash.values).to eq([ProjectAuthorization.first])
+      expect(hash.values.length).to eq(1)
+
+      value = hash.values[0]
+
+      expect(value.project_id).to eq(project.id)
+      expect(value.access_level).to eq(Gitlab::Access::MASTER)
     end
   end
 
@@ -167,10 +172,6 @@ describe Users::RefreshAuthorizedProjectsService do
         expect(service.current_authorizations.length).to eq(1)
       end
 
-      it 'includes the row ID for every row' do
-        expect(row.id).to be_a_kind_of(Numeric)
-      end
-
       it 'includes the project ID for every row' do
         expect(row.project_id).to eq(project.id)
       end