X-Git-Url: https://git.arvados.org/arvados.git/blobdiff_plain/e6f3c9faab3f10d7efc7348be2ef85a6ea14766b..864c3b0afd16c77e046f0072d8517d34c5a44792:/services/api/app/models/arvados_model.rb

diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index 86e9461529..f2bae3a4b5 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -286,6 +286,7 @@ class ArvadosModel < ApplicationRecord
 
     sql_conds = nil
     user_uuids = users_list.map { |u| u.uuid }
+    all_user_uuids = []
 
     # For details on how the trashed_groups table is constructed, see
     # see db/migrate/20200501150153_permission_table.rb
@@ -319,12 +320,31 @@ class ArvadosModel < ApplicationRecord
       # A user can have can_manage access to another user, this grants
       # full access to all that user's stuff.  To implement that we
       # need to include those other users in the permission query.
-      user_uuids_subquery = USER_UUIDS_SUBQUERY_TEMPLATE % {user: ":user_uuids", perm_level: 1}
+
+      # This was previously implemented by embedding the subquery
+      # directly into the query, but it was discovered later that this
+      # causes the Postgres query planner to do silly things because
+      # the query heuristics assumed the subquery would have a lot
+      # more rows that it does, and choose a bad merge strategy.  By
+      # doing the query here and embedding the result as a constant,
+      # Postgres also knows exactly how many items there are and can
+      # choose the right query strategy.
+      #
+      # (note: you could also do this with a temporary table, but that
+      # would require all every request be wrapped in a transaction,
+      # which is not currently the case).
+
+      all_user_uuids = ActiveRecord::Base.connection.exec_query %{
+#{USER_UUIDS_SUBQUERY_TEMPLATE % {user: "'#{user_uuids.join "', '"}'", perm_level: 1}}
+},
+                                             'readable_by.user_uuids'
+
+      user_uuids_subquery = ":user_uuids"
 
       # Note: it is possible to combine the direct_check and
-      # owner_check into a single EXISTS() clause, however it turns
+      # owner_check into a single IN (SELECT) clause, however it turns
       # out query optimizer doesn't like it and forces a sequential
-      # table scan.  Constructing the query with separate EXISTS()
+      # table scan.  Constructing the query with separate IN (SELECT)
       # clauses enables it to use the index.
       #
       # see issue 13208 for details.
@@ -353,6 +373,14 @@ class ArvadosModel < ApplicationRecord
       if sql_table != "api_client_authorizations" and sql_table != "groups" then
         owner_check = "#{sql_table}.owner_uuid IN (SELECT target_uuid FROM #{PERMISSION_VIEW} "+
                       "WHERE user_uuid IN (#{user_uuids_subquery}) AND perm_level >= 1 AND traverse_owned) "
+
+        # We want to do owner_check before direct_check in the OR
+        # clause.  The order of the OR clause isn't supposed to
+        # matter, but in practice, it does -- apparently in the
+        # absence of other hints, it uses the ordering from the query.
+        # For certain types of queries (like filtering on owner_uuid),
+        # every item will match the owner_check clause, so then
+        # Postgres will optimize out the direct_check entirely.
         direct_check = " OR " + direct_check
       end
 
@@ -379,7 +407,7 @@ class ArvadosModel < ApplicationRecord
     end
 
     self.where(sql_conds,
-               user_uuids: user_uuids,
+               user_uuids: all_user_uuids.collect{|c| c["target_uuid"]},
                permission_link_classes: ['permission', 'resources'])
   end
 
@@ -827,6 +855,40 @@ class ArvadosModel < ApplicationRecord
     nil
   end
 
+  # Fill in implied zero/false values in database records that were
+  # created before #17014 made them explicit, and reset the Rails
+  # "changed" state so the record doesn't appear to have been modified
+  # after loading.
+  #
+  # Invoked by Container and ContainerRequest models as an after_find
+  # hook.
+  def fill_container_defaults_after_find
+    fill_container_defaults
+    set_attribute_was('runtime_constraints', runtime_constraints)
+    set_attribute_was('scheduling_parameters', scheduling_parameters)
+    clear_changes_information
+  end
+
+  # Fill in implied zero/false values. Invoked by ContainerRequest as
+  # a before_validation hook in order to (a) ensure every key has a
+  # value in the updated database record and (b) ensure the attribute
+  # whitelist doesn't reject a change from an explicit zero/false
+  # value in the database to an implicit zero/false value in an update
+  # request.
+  def fill_container_defaults
+    self.runtime_constraints = {
+      'API' => false,
+      'keep_cache_ram' => 0,
+      'ram' => 0,
+      'vcpus' => 0,
+    }.merge(attributes['runtime_constraints'] || {})
+    self.scheduling_parameters = {
+      'max_run_time' => 0,
+      'partitions' => [],
+      'preemptible' => false,
+    }.merge(attributes['scheduling_parameters'] || {})
+  end
+
   # ArvadosModel.find_by_uuid needs extra magic to allow it to return
   # an object in any class.
   def self.find_by_uuid uuid