[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