BigW Consortium Gitlab

Remove the 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.
parent 2dec1772
...@@ -130,7 +130,7 @@ class Project < ActiveRecord::Base ...@@ -130,7 +130,7 @@ class Project < ActiveRecord::Base
has_many :hooks, dependent: :destroy, class_name: 'ProjectHook' has_many :hooks, dependent: :destroy, class_name: 'ProjectHook'
has_many :protected_branches, dependent: :destroy 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 :authorized_users, through: :project_authorizations, source: :user, class_name: 'User'
has_many :project_members, -> { where(requested_at: nil) }, dependent: :destroy, as: :source has_many :project_members, -> { where(requested_at: nil) }, dependent: :destroy, as: :source
alias_method :members, :project_members alias_method :members, :project_members
......
...@@ -73,7 +73,7 @@ class User < ActiveRecord::Base ...@@ -73,7 +73,7 @@ class User < ActiveRecord::Base
has_many :created_projects, foreign_key: :creator_id, class_name: 'Project' has_many :created_projects, foreign_key: :creator_id, class_name: 'Project'
has_many :users_star_projects, dependent: :destroy has_many :users_star_projects, dependent: :destroy
has_many :starred_projects, through: :users_star_projects, source: :project 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 :authorized_projects, through: :project_authorizations, source: :project
has_many :snippets, dependent: :destroy, foreign_key: :author_id has_many :snippets, dependent: :destroy, foreign_key: :author_id
...@@ -444,7 +444,7 @@ class User < ActiveRecord::Base ...@@ -444,7 +444,7 @@ class User < ActiveRecord::Base
end end
def remove_project_authorizations(project_ids) def remove_project_authorizations(project_ids)
project_authorizations.where(id: project_ids).delete_all project_authorizations.where(project_id: project_ids).delete_all
end end
def set_authorized_projects_column def set_authorized_projects_column
......
...@@ -35,7 +35,7 @@ module Users ...@@ -35,7 +35,7 @@ module Users
# rows not in the new list or with a different access level should be # rows not in the new list or with a different access level should be
# removed. # removed.
if !fresh[project_id] || fresh[project_id] != row.access_level if !fresh[project_id] || fresh[project_id] != row.access_level
array << row.id array << row.project_id
end end
end end
...@@ -100,7 +100,7 @@ module Users ...@@ -100,7 +100,7 @@ module Users
end end
def current_authorizations def current_authorizations
user.project_authorizations.select(:id, :project_id, :access_level) user.project_authorizations.select(:project_id, :access_level)
end end
def fresh_authorizations def fresh_authorizations
......
---
title: Remove the project_authorizations.id column
merge_request:
author:
# 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
...@@ -11,7 +11,7 @@ ...@@ -11,7 +11,7 @@
# #
# It's strongly recommended that you check this file into your version control system. # 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 # These are extensions that must be enabled in order to support this database
enable_extension "plpgsql" enable_extension "plpgsql"
...@@ -861,7 +861,7 @@ ActiveRecord::Schema.define(version: 20161227192806) do ...@@ -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", ["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 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 "user_id"
t.integer "project_id" t.integer "project_id"
t.integer "access_level" t.integer "access_level"
......
...@@ -20,7 +20,7 @@ describe Users::RefreshAuthorizedProjectsService do ...@@ -20,7 +20,7 @@ describe Users::RefreshAuthorizedProjectsService do
to_remove = create_authorization(project2, user) to_remove = create_authorization(project2, user)
expect(service).to receive(:update_with_lease). 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 service.execute
end end
...@@ -29,7 +29,7 @@ describe Users::RefreshAuthorizedProjectsService do ...@@ -29,7 +29,7 @@ describe Users::RefreshAuthorizedProjectsService do
to_remove = create_authorization(project, user, Gitlab::Access::DEVELOPER) to_remove = create_authorization(project, user, Gitlab::Access::DEVELOPER)
expect(service).to receive(:update_with_lease). 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 service.execute
end end
...@@ -90,7 +90,7 @@ describe Users::RefreshAuthorizedProjectsService do ...@@ -90,7 +90,7 @@ describe Users::RefreshAuthorizedProjectsService do
it 'removes authorizations that should be removed' do it 'removes authorizations that should be removed' do
authorization = create_authorization(project, user) authorization = create_authorization(project, user)
service.update_authorizations([authorization.id]) service.update_authorizations([authorization.project_id])
expect(user.project_authorizations).to be_empty expect(user.project_authorizations).to be_empty
end end
...@@ -147,7 +147,12 @@ describe Users::RefreshAuthorizedProjectsService do ...@@ -147,7 +147,12 @@ describe Users::RefreshAuthorizedProjectsService do
end end
it 'sets the values to the project authorization rows' do 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
end end
...@@ -167,10 +172,6 @@ describe Users::RefreshAuthorizedProjectsService do ...@@ -167,10 +172,6 @@ describe Users::RefreshAuthorizedProjectsService do
expect(service.current_authorizations.length).to eq(1) expect(service.current_authorizations.length).to eq(1)
end 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 it 'includes the project ID for every row' do
expect(row.project_id).to eq(project.id) expect(row.project_id).to eq(project.id)
end end
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment