[ARVADOS] updated: 1.3.0-2606-ga4ade714a
Git user
git at public.arvados.org
Thu May 28 19:59:08 UTC 2020
Summary of changes:
.../api/app/controllers/database_controller.rb | 4 +-
services/api/app/models/database_seeds.rb | 4 +-
services/api/app/models/group.rb | 2 +-
services/api/app/models/link.rb | 2 +-
services/api/app/models/user.rb | 4 +-
services/api/db/structure.sql | 8 +-
...sh_permission_view.rb => update_permissions.rb} | 136 ++++++++++++---------
services/api/test/performance/permission_test.rb | 2 +-
services/api/test/test_helper.rb | 4 +-
services/api/test/unit/owner_test.rb | 15 ++-
services/api/test/unit/user_test.rb | 3 -
11 files changed, 102 insertions(+), 82 deletions(-)
rename services/api/lib/{refresh_permission_view.rb => update_permissions.rb} (52%)
via a4ade714a38980e239e9bc01244cba6b33575206 (commit)
from c36d5cb91099d79184b85388829d637f842be34e (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
commit a4ade714a38980e239e9bc01244cba6b33575206
Author: Peter Amstutz <peter.amstutz at curii.com>
Date: Thu May 28 15:35:29 2020 -0400
16007: Enable permission correctness checking (only for tests)
* Explicitly set up a transaction in update_permissions
* Rename refresh_permission_view.rb -> update_permissions.rb
* Add skip_check_permissions_against_full_refresh
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz at curii.com>
diff --git a/services/api/app/controllers/database_controller.rb b/services/api/app/controllers/database_controller.rb
index 24c6cf5a7..5c4cf7bc1 100644
--- a/services/api/app/controllers/database_controller.rb
+++ b/services/api/app/controllers/database_controller.rb
@@ -75,9 +75,9 @@ class DatabaseController < ApplicationController
raise
end
- require 'refresh_permission_view'
+ require 'update_permissions'
- refresh_permission_view
+ refresh_permissions
refresh_trashed
# Done.
diff --git a/services/api/app/models/database_seeds.rb b/services/api/app/models/database_seeds.rb
index 0fea2cf7b..39f491503 100644
--- a/services/api/app/models/database_seeds.rb
+++ b/services/api/app/models/database_seeds.rb
@@ -2,7 +2,7 @@
#
# SPDX-License-Identifier: AGPL-3.0
-require 'refresh_permission_view'
+require 'update_permissions'
class DatabaseSeeds
extend CurrentApiClient
@@ -14,7 +14,7 @@ class DatabaseSeeds
anonymous_group_read_permission
anonymous_user
empty_collection
- refresh_permission_view
+ refresh_permissions
refresh_trashed
end
end
diff --git a/services/api/app/models/group.rb b/services/api/app/models/group.rb
index 9b8a9877e..485205f1e 100644
--- a/services/api/app/models/group.rb
+++ b/services/api/app/models/group.rb
@@ -80,7 +80,7 @@ on conflict (group_uuid) do update set trash_at=EXCLUDED.trash_at;
def before_ownership_change
if owner_uuid_changed? and !self.owner_uuid_was.nil?
MaterializedPermission.where(user_uuid: owner_uuid_was, target_uuid: uuid).delete_all
- update_permissions self.owner_uuid, self.uuid, 0
+ update_permissions self.owner_uuid_was, self.uuid, 0
end
end
diff --git a/services/api/app/models/link.rb b/services/api/app/models/link.rb
index 33e18b296..da4ca6c87 100644
--- a/services/api/app/models/link.rb
+++ b/services/api/app/models/link.rb
@@ -86,7 +86,7 @@ class Link < ArvadosModel
def check_permissions
if self.link_class == 'permission'
- #check_permissions_against_full_refresh
+ check_permissions_against_full_refresh
end
end
diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb
index d48dde25a..cb7efe9cc 100644
--- a/services/api/app/models/user.rb
+++ b/services/api/app/models/user.rb
@@ -436,7 +436,9 @@ update #{PERMISSION_VIEW} set target_uuid=$1 where target_uuid = $2
if redirect_to_new_user
update_attributes!(redirect_to_user_uuid: new_user.uuid, username: nil)
end
- update_permissions self.owner_uuid, self.uuid, 3, false
+ skip_check_permissions_against_full_refresh do
+ update_permissions self.owner_uuid, self.uuid, 3
+ end
update_permissions new_user.owner_uuid, new_user.uuid, 3
end
end
diff --git a/services/api/db/structure.sql b/services/api/db/structure.sql
index 5f71554ff..c4bf90dca 100644
--- a/services/api/db/structure.sql
+++ b/services/api/db/structure.sql
@@ -46,9 +46,9 @@ CREATE FUNCTION public.compute_permission_subgraph(perm_origin_uuid character va
LANGUAGE sql STABLE
AS $$
with
-perm_from_start(perm_origin_uuid, target_uuid, val, traverse_owned) as (
- select perm_origin_uuid, target_uuid, val, traverse_owned
- from search_permission_graph(starting_uuid, starting_perm)),
+ perm_from_start(perm_origin_uuid, target_uuid, val, traverse_owned) as (
+ select perm_origin_uuid, target_uuid, val, traverse_owned
+ from search_permission_graph(starting_uuid, starting_perm)),
additional_perms(perm_origin_uuid, target_uuid, val, traverse_owned) as (
select edges.tail_uuid as perm_origin_uuid, ps.target_uuid, ps.val,
@@ -156,7 +156,7 @@ $$;
--
CREATE FUNCTION public.should_traverse_owned(starting_uuid character varying, starting_perm integer) RETURNS boolean
- LANGUAGE sql STABLE
+ LANGUAGE sql IMMUTABLE
AS $$
select starting_uuid like '_____-j7d0g-_______________' or
(starting_uuid like '_____-tpzed-_______________' and starting_perm >= 3);
diff --git a/services/api/lib/refresh_permission_view.rb b/services/api/lib/update_permissions.rb
similarity index 52%
rename from services/api/lib/refresh_permission_view.rb
rename to services/api/lib/update_permissions.rb
index 826c44c3b..b2cf6595b 100644
--- a/services/api/lib/refresh_permission_view.rb
+++ b/services/api/lib/update_permissions.rb
@@ -5,7 +5,7 @@
PERMISSION_VIEW = "materialized_permissions"
TRASHED_GROUPS = "trashed_groups"
-def refresh_permission_view
+def refresh_permissions
ActiveRecord::Base.transaction do
ActiveRecord::Base.connection.execute("LOCK TABLE #{PERMISSION_VIEW}")
ActiveRecord::Base.connection.execute("DELETE FROM #{PERMISSION_VIEW}")
@@ -26,7 +26,7 @@ def refresh_trashed
end
end
-def update_permissions perm_origin_uuid, starting_uuid, perm_level, check=false
+def update_permissions perm_origin_uuid, starting_uuid, perm_level
#
# Update a subset of the permission table affected by adding or
# removing a particular permission relationship (ownership or a
@@ -45,7 +45,7 @@ def update_permissions perm_origin_uuid, starting_uuid, perm_level, check=false
# can_manage=3
# or call with perm_level=0 to revoke permissions
#
- # check: for testing/debugging only, compare the result of the
+ # check: for testing/debugging, compare the result of the
# incremental update against a full table recompute. Throws an
# error if the contents are not identical (ie they produce different
# permission results)
@@ -68,60 +68,62 @@ def update_permissions perm_origin_uuid, starting_uuid, perm_level, check=false
# see db/migrate/20200501150153_permission_table.rb for details on
# how the permissions are computed.
- # "Conflicts with the ROW EXCLUSIVE, SHARE UPDATE EXCLUSIVE, SHARE
- # ROW EXCLUSIVE, EXCLUSIVE, and ACCESS EXCLUSIVE lock modes. This
- # mode protects a table against concurrent data changes."
- ActiveRecord::Base.connection.execute "LOCK TABLE #{PERMISSION_VIEW} in SHARE MODE"
-
- # Workaround for
- # BUG #15160: planner overestimates number of rows in join when there are more than 200 rows coming from CTE
- # https://www.postgresql.org/message-id/152395805004.19366.3107109716821067806@wrigleys.postgresql.org
- #
- # For a crucial join in the compute_permission_subgraph() query, the
- # planner mis-estimates the number of rows in a Common Table
- # Expression (CTE, this is a subquery in a WITH clause) and as a
- # result it chooses the wrong join order. The join starts with the
- # permissions table because it mistakenly thinks
- # count(materalized_permissions) < count(new computed permissions)
- # when actually it is the other way around.
- #
- # Because of the incorrect join order, it choose the wrong join
- # strategy (merge join, which works best when two tables are roughly
- # the same size). As a workaround, we can tell it not to use that
- # join strategy, this causes it to pick hash join instead, which
- # turns out to be a bit better. However, because the join order is
- # still wrong, we don't get the full benefit of the index.
- #
- # This is very unfortunate because it makes the query performance
- # dependent on the size of the materalized_permissions table, when
- # the goal of this design was to make permission updates scale-free
- # and only depend on the number of permissions affected and not the
- # total table size. In several hours of researching I wasn't able
- # to find a way to force the correct join order, so I'm calling it
- # here and I have to move on.
- #
- # This is apparently addressed in Postgres 12, but I developed &
- # tested this on Postgres 9.6, so in the future we should reevaluate
- # the performance & query plan on Postgres 12.
- #
- # https://git.furworks.de/opensourcemirror/postgresql/commit/a314c34079cf06d05265623dd7c056f8fa9d577f
- #
- # Disable merge join for just this query (also local for this transaction), then reenable it.
- ActiveRecord::Base.connection.exec_query "SET LOCAL enable_mergejoin to false;"
+ ActiveRecord::Base.transaction do
- temptable_perms = "temp_perms_#{rand(2**64).to_s(10)}"
- ActiveRecord::Base.connection.exec_query %{
+ # "Conflicts with the ROW EXCLUSIVE, SHARE UPDATE EXCLUSIVE, SHARE
+ # ROW EXCLUSIVE, EXCLUSIVE, and ACCESS EXCLUSIVE lock modes. This
+ # mode protects a table against concurrent data changes."
+ ActiveRecord::Base.connection.execute "LOCK TABLE #{PERMISSION_VIEW} in SHARE MODE"
+
+ # Workaround for
+ # BUG #15160: planner overestimates number of rows in join when there are more than 200 rows coming from CTE
+ # https://www.postgresql.org/message-id/152395805004.19366.3107109716821067806@wrigleys.postgresql.org
+ #
+ # For a crucial join in the compute_permission_subgraph() query, the
+ # planner mis-estimates the number of rows in a Common Table
+ # Expression (CTE, this is a subquery in a WITH clause) and as a
+ # result it chooses the wrong join order. The join starts with the
+ # permissions table because it mistakenly thinks
+ # count(materalized_permissions) < count(new computed permissions)
+ # when actually it is the other way around.
+ #
+ # Because of the incorrect join order, it choose the wrong join
+ # strategy (merge join, which works best when two tables are roughly
+ # the same size). As a workaround, we can tell it not to use that
+ # join strategy, this causes it to pick hash join instead, which
+ # turns out to be a bit better. However, because the join order is
+ # still wrong, we don't get the full benefit of the index.
+ #
+ # This is very unfortunate because it makes the query performance
+ # dependent on the size of the materalized_permissions table, when
+ # the goal of this design was to make permission updates scale-free
+ # and only depend on the number of permissions affected and not the
+ # total table size. In several hours of researching I wasn't able
+ # to find a way to force the correct join order, so I'm calling it
+ # here and I have to move on.
+ #
+ # This is apparently addressed in Postgres 12, but I developed &
+ # tested this on Postgres 9.6, so in the future we should reevaluate
+ # the performance & query plan on Postgres 12.
+ #
+ # https://git.furworks.de/opensourcemirror/postgresql/commit/a314c34079cf06d05265623dd7c056f8fa9d577f
+ #
+ # Disable merge join for just this query (also local for this transaction), then reenable it.
+ ActiveRecord::Base.connection.exec_query "SET LOCAL enable_mergejoin to false;"
+
+ temptable_perms = "temp_perms_#{rand(2**64).to_s(10)}"
+ ActiveRecord::Base.connection.exec_query %{
create temporary table #{temptable_perms} on commit drop
as select * from compute_permission_subgraph($1, $2, $3)
},
- 'update_permissions.select',
- [[nil, perm_origin_uuid],
- [nil, starting_uuid],
- [nil, perm_level]]
+ 'update_permissions.select',
+ [[nil, perm_origin_uuid],
+ [nil, starting_uuid],
+ [nil, perm_level]]
- ActiveRecord::Base.connection.exec_query "SET LOCAL enable_mergejoin to true;"
+ ActiveRecord::Base.connection.exec_query "SET LOCAL enable_mergejoin to true;"
- ActiveRecord::Base.connection.exec_delete %{
+ ActiveRecord::Base.connection.exec_delete %{
delete from #{PERMISSION_VIEW} where
target_uuid in (select target_uuid from #{temptable_perms}) and
not exists (select 1 from #{temptable_perms}
@@ -129,27 +131,29 @@ delete from #{PERMISSION_VIEW} where
user_uuid=#{PERMISSION_VIEW}.user_uuid and
val>0)
},
- "update_permissions.delete"
+ "update_permissions.delete"
- ActiveRecord::Base.connection.exec_query %{
+ ActiveRecord::Base.connection.exec_query %{
insert into #{PERMISSION_VIEW} (user_uuid, target_uuid, perm_level, traverse_owned)
select user_uuid, target_uuid, val as perm_level, traverse_owned from #{temptable_perms} where val>0
on conflict (user_uuid, target_uuid) do update set perm_level=EXCLUDED.perm_level, traverse_owned=EXCLUDED.traverse_owned;
},
- "update_permissions.insert"
+ "update_permissions.insert"
- if check and perm_level>0
- check_permissions_against_full_refresh
+ if perm_level>0
+ check_permissions_against_full_refresh
+ end
end
end
def check_permissions_against_full_refresh
- #
- # For debugging, this checks contents of the
- # incrementally-updated 'materialized_permission' against a
- # from-scratch permission refresh.
- #
+ # No-op except when running tests
+ return unless Rails.env == 'test' and !Thread.current[:no_check_permissions_against_full_refresh]
+
+ # For checking correctness of the incremental permission updates.
+ # Check contents of the current 'materialized_permission' table
+ # against a from-scratch permission refresh.
q1 = ActiveRecord::Base.connection.exec_query %{
select user_uuid, target_uuid, perm_level, traverse_owned from #{PERMISSION_VIEW}
@@ -182,3 +186,13 @@ order by users.uuid, target_uuid
end
end
end
+
+def skip_check_permissions_against_full_refresh
+ check_perm_was = Thread.current[:no_check_permissions_against_full_refresh]
+ Thread.current[:no_check_permissions_against_full_refresh] = true
+ begin
+ yield
+ ensure
+ Thread.current[:no_check_permissions_against_full_refresh] = check_perm_was
+ end
+end
diff --git a/services/api/test/performance/permission_test.rb b/services/api/test/performance/permission_test.rb
index 0fea3b135..d0e6413b1 100644
--- a/services/api/test/performance/permission_test.rb
+++ b/services/api/test/performance/permission_test.rb
@@ -40,7 +40,7 @@ class PermissionPerfTest < ActionDispatch::IntegrationTest
end
end
end
- refresh_permission_view
+ refresh_permissions
end
end)
end
diff --git a/services/api/test/test_helper.rb b/services/api/test/test_helper.rb
index 12f729e33..c99a57aaf 100644
--- a/services/api/test/test_helper.rb
+++ b/services/api/test/test_helper.rb
@@ -2,7 +2,7 @@
#
# SPDX-License-Identifier: AGPL-3.0
-require 'refresh_permission_view'
+require 'update_permissions'
ENV["RAILS_ENV"] = "test"
unless ENV["NO_COVERAGE_TEST"]
@@ -209,5 +209,5 @@ class ActionDispatch::IntegrationTest
end
# Ensure permissions are computed from the test fixtures.
-refresh_permission_view
+refresh_permissions
refresh_trashed
diff --git a/services/api/test/unit/owner_test.rb b/services/api/test/unit/owner_test.rb
index 2bdc198a3..ca02e2db5 100644
--- a/services/api/test/unit/owner_test.rb
+++ b/services/api/test/unit/owner_test.rb
@@ -87,9 +87,11 @@ class OwnerTest < ActiveSupport::TestCase
assert_equal(true, Specimen.where(owner_uuid: o.uuid).any?,
"need something to be owned by #{o.uuid} for this test")
- assert_raises(ActiveRecord::DeleteRestrictionError,
- "should not delete #{ofixt} that owns objects") do
- o.destroy
+ skip_check_permissions_against_full_refresh do
+ assert_raises(ActiveRecord::DeleteRestrictionError,
+ "should not delete #{ofixt} that owns objects") do
+ o.destroy
+ end
end
end
@@ -108,9 +110,14 @@ class OwnerTest < ActiveSupport::TestCase
assert User.where(uuid: o.uuid).any?, "new User should really be in DB"
assert_equal(true, o.update_attributes(owner_uuid: o.uuid),
"setting owner to self should work")
- assert(o.destroy, "should delete User that owns self")
+
+ skip_check_permissions_against_full_refresh do
+ assert(o.destroy, "should delete User that owns self")
+ end
+
assert_equal(false, User.where(uuid: o.uuid).any?,
"#{o.uuid} should not be in DB after deleting")
+ check_permissions_against_full_refresh
end
test "change uuid of User that owns self" do
diff --git a/services/api/test/unit/user_test.rb b/services/api/test/unit/user_test.rb
index 5d25361ed..596cd415f 100644
--- a/services/api/test/unit/user_test.rb
+++ b/services/api/test/unit/user_test.rb
@@ -168,9 +168,6 @@ class UserTest < ActiveSupport::TestCase
act_as_system_user do
users(:admin).update_attributes!(is_admin: false)
end
- # need to manually call clear_permissions because we used 'delete' instead of 'destory'
- # we use delete here because 'destroy' will throw an error
- #users(:admin).clear_permissions
@all_users = User.where("uuid not like '%-000000000000000'").where(:is_admin => true)
assert_equal 0, @all_users.count, "No admin users should exist (except for the system user)"
end
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list