Move Regional Records computation to Auxiliary table (#10122)

* Add helper method for temporary table swapping

* Add Regional Records lookup table

* Add regional records lookup to CAD computation

* Use lookup table in CRR script

* Add note about CRR depending on CAD runs

* Specify what CAD actually does

* Add sanitizer for new lookup table in dev dump

* Add result ID to regional_records_lookup

* Add common helper method for lookup table indexing

* Update CRR warning box text

* Don't run CRR on the read replica (now that we have lookup tables)

* Add an option to recompute CRR index on form submit

* Switch back aux computation order in final import step

app/controllers/admin_controller.rb

@@ -323,15 +323,20 @@ def do_generate_public_export
   end
 
   def check_regional_records
+    refresh_index_param = params[:refresh_index] || nil
+
     @check_records_request = CheckRegionalRecordsForm.new(
       competition_id: params[:competition_id] || nil,
       event_id: params[:event_id] || nil,
+      refresh_index: ActiveRecord::Type::Boolean.new.cast(refresh_index_param) || false,
     )
+
+    @cad_timestamp = ComputeAuxiliaryData.successful_start_date&.to_fs || 'never'
   end
 
   def override_regional_records
     action_params = params.require(:check_regional_records_form)
-                          .permit(:competition_id, :event_id)
+                          .permit(:competition_id, :event_id, :refresh_index)
 
     @check_records_request = CheckRegionalRecordsForm.new(action_params)
     @check_results = @check_records_request.run_check

app/models/check_regional_records_form.rb

@@ -3,15 +3,17 @@
 class CheckRegionalRecordsForm
   include ActiveModel::Model
 
-  attr_accessor :competition_id, :event_id
+  attr_accessor :competition_id, :event_id, :refresh_index
 
   def check_event_id
     self.event_id == 'all' ? nil : self.event_id
   end
 
   def run_check
-    ActiveRecord::Base.connected_to(role: :read_replica) do
-      CheckRegionalRecords.check_records(self.check_event_id, self.competition_id)
+    if self.refresh_index && self.competition_id.present?
+      CheckRegionalRecords.add_to_lookup_table(self.competition_id)
     end
+
+    CheckRegionalRecords.check_records(self.check_event_id, self.competition_id)
   end
 end

app/views/admin/_import_results_steps.html.erb

@@ -38,7 +38,7 @@
     <% else %>
       <p>All inbox data has been imported. You should consider the following steps next:</p>
       <ul>
-        <li><%= link_to "Check record markers", admin_override_regional_records_path(check_regional_records_form: { competition_id: @competition.id, event_id: 'all' }), target: '_blank' %></li>
+        <li><%= link_to "Check record markers", admin_override_regional_records_path(check_regional_records_form: { competition_id: @competition.id, event_id: 'all', refresh_index: true }), target: '_blank' %></li>
         <li><%= link_to "Compute auxiliary data", admin_do_compute_auxiliary_data_path, target: '_blank' %></li>
       </ul>
       <p>

app/views/admin/check_regional_records.html.erb

@@ -29,11 +29,20 @@
       huge).
     </p>
 
+    <%= alert :warning do %>
+      This script relies on auxiliary tables which are computed as part of <%= link_to "running CAD", admin_compute_auxiliary_data_path, target: :_blank %>.
+      <br/>
+      <b>The detected records will not automatically be up-to-date with the <code>Results</code> table by default.</b>
+      <br/>
+      Instead, they are up to date with CAD (last successful run: <%= @cad_timestamp %>)
+    <% end %>
+
     <hr/>
 
     <%= simple_form_for @check_records_request, url: admin_override_regional_records_path, method: :get do |f| %>
       <%= f.input :competition_id, as: :competition_id, only_one: true, label: "Competition ID", hint: "Leave blank to check for all competitions" %>
       <%= f.input :event_id, as: :events_picker, allowed_events: Event.all, only_one: true, include_all: true, label: "Event", hint: "First symbol means 'all events'" %>
+      <%= f.input :refresh_index, as: :boolean, label: "Refresh index for selected competition(s)", hint: "This will attempt to compute the lookup on-the-fly only if a specific competition has been selected above. It can lead to timeouts, please rely on CAD when that happens." %>
 
       <%= f.button :submit, value: "Run check", class: "btn-primary" %>
     <% end %>

app/views/admin/compute_auxiliary_data.html.erb

@@ -7,7 +7,7 @@
         <h4>This script:</h4>
         <ul class="list-group">
           <li class="list-group-item">Computes some auxiliary tables in the database (ConciseSingleResults, ConciseAverageResults, RanksSingle and RanksAverage).</li>
-          <li class="list-group-item">Clears some cache files (living on the PHP side)</li>
+          <li class="list-group-item">Computes the lookup for assigning record markers</li>
           <li class="list-group-item">Deletes cached results (so they can be cached again)</li>
         </ul>
       </div>

db/migrate/20241022093010_create_regional_records_lookup_table.rb

@@ -0,0 +1,21 @@
+# frozen_string_literal: true
+
+class CreateRegionalRecordsLookupTable < ActiveRecord::Migration[7.2]
+  def change
+    create_table :regional_records_lookup do |t|
+      t.references :Results, foreign_key: true, null: false, type: :int
+      t.string "countryId", null: false
+      t.string "eventId", null: false
+      t.date "competitionEndDate", null: false
+      t.integer "best", default: 0, null: false
+      t.integer "average", default: 0, null: false
+
+      t.index [:eventId, :countryId, :best, :competitionEndDate]
+      t.index [:eventId, :countryId, :average, :competitionEndDate]
+    end
+
+    # Small hack because Rails doesn't support custom `t.references` names
+    #   but our Results tables have their own nomenclature
+    rename_column :regional_records_lookup, :Results_id, :resultId
+  end
+end

db/schema.rb

@@ -960,6 +960,18 @@
     t.index ["name"], name: "index_regional_organizations_on_name"
   end
 
+  create_table "regional_records_lookup", charset: "utf8mb4", collation: "utf8mb4_unicode_ci", force: :cascade do |t|
+    t.integer "resultId", null: false
+    t.string "countryId", null: false
+    t.string "eventId", null: false
+    t.date "competitionEndDate", null: false
+    t.integer "best", default: 0, null: false
+    t.integer "average", default: 0, null: false
+    t.index ["eventId", "countryId", "average", "competitionEndDate"], name: "idx_on_eventId_countryId_average_competitionEndDate_b424c59953"
+    t.index ["eventId", "countryId", "best", "competitionEndDate"], name: "idx_on_eventId_countryId_best_competitionEndDate_4e01b1ae38"
+    t.index ["resultId"], name: "index_regional_records_lookup_on_resultId"
+  end
+
   create_table "registration_competition_events", id: :integer, charset: "utf8mb4", collation: "utf8mb4_unicode_ci", force: :cascade do |t|
     t.integer "registration_id"
     t.integer "competition_event_id"
@@ -1338,6 +1350,7 @@
   add_foreign_key "oauth_openid_requests", "oauth_access_grants", column: "access_grant_id", on_delete: :cascade
   add_foreign_key "payment_intents", "users", column: "initiated_by_id"
   add_foreign_key "paypal_records", "paypal_records", column: "parent_record_id"
+  add_foreign_key "regional_records_lookup", "Results", column: "resultId"
   add_foreign_key "registration_history_changes", "registration_history_entries"
   add_foreign_key "sanity_check_exclusions", "sanity_checks"
   add_foreign_key "sanity_checks", "sanity_check_categories"

lib/auxiliary_data_computation.rb

@@ -4,6 +4,7 @@ module AuxiliaryDataComputation
   def self.compute_everything
     self.compute_concise_results
     self.compute_rank_tables
+    self.insert_regional_records_lookup
   end
 
   ## Build 'concise results' tables.
@@ -12,10 +13,8 @@ def self.compute_concise_results
       %w(best ConciseSingleResults),
       %w(average ConciseAverageResults),
     ].each do |field, table_name|
-      temp_table_name = "#{table_name}_temp"
-      ActiveRecord::Base.connection.execute("CREATE TABLE #{temp_table_name} LIKE #{table_name}")
-      ActiveRecord::Base.connection.execute("ALTER TABLE #{temp_table_name} AUTO_INCREMENT = 1")
-      ActiveRecord::Base.connection.execute <<-SQL
+      DbHelper.with_temp_table(table_name) do |temp_table_name|
+        ActiveRecord::Base.connection.execute <<-SQL
           INSERT INTO #{temp_table_name} (id, #{field}, valueAndId, personId, eventId, countryId, continentId, year, month, day)
           SELECT
             result.id,
@@ -39,15 +38,8 @@ def self.compute_concise_results
             JOIN Competitions competition ON competition.id = competitionId
             JOIN Countries country ON country.id = result.countryId
             JOIN Events event ON event.id = eventId
-      SQL
-
-      # Atomically swap the tables
-      ActiveRecord::Base.connection.execute("RENAME TABLE #{table_name} TO #{table_name}_old, #{temp_table_name} TO #{table_name}")
-
-      # Drop the old table
-      ActiveRecord::Base.connection.execute("DROP TABLE #{table_name}_old")
-    ensure
-      ActiveRecord::Base.connection.execute("DROP TABLE IF EXISTS #{temp_table_name}")
+        SQL
+      end
     end
   end
 
@@ -57,70 +49,66 @@ def self.compute_rank_tables
       %w(best RanksSingle ConciseSingleResults),
       %w(average RanksAverage ConciseAverageResults),
     ].each do |field, table_name, concise_table_name|
-      temp_table_name = "#{table_name}_temp"
-      ActiveRecord::Base.connection.execute("CREATE TABLE #{temp_table_name} LIKE #{table_name}")
-      ActiveRecord::Base.connection.execute("ALTER TABLE #{temp_table_name} AUTO_INCREMENT = 1")
-
-      current_country_by_wca_id = Person.current.pluck(:wca_id, :countryId).to_h
-      # Get all personal records (note: people that changed their country appear once for each country).
-      personal_records_with_event = ActiveRecord::Base.connection.execute <<-SQL
+      DbHelper.with_temp_table(table_name) do |temp_table_name|
+        current_country_by_wca_id = Person.current.pluck(:wca_id, :countryId).to_h
+        # Get all personal records (note: people that changed their country appear once for each country).
+        personal_records_with_event = ActiveRecord::Base.connection.execute <<-SQL
           SELECT eventId, personId, countryId, continentId, min(#{field}) value
           FROM #{concise_table_name}
           GROUP BY personId, countryId, continentId, eventId
           ORDER BY eventId, value
-      SQL
-      personal_records_with_event.group_by(&:first).each do |event_id, personal_records|
-        personal_rank = Hash.new { |h, k| h[k] = {} }
-        ranked = Hash.new { |h, k| h[k] = {} }
-        counter = Hash.new(0)
-        current_rank = Hash.new(0)
-        previous_value = {}
-        personal_records.each do |_, person_id, country_id, continent_id, value|
-          # Update the region states (unless we have ranked this person already,
-          # e.g. 2008SEAR01 twice in North America and World because of his two countries).
-          ["World", continent_id, country_id].each do |region|
-            next if ranked[region][person_id]
-            counter[region] += 1
-            # As we ordered by value it can either be greater or tie the previous one.
-            current_rank[region] = counter[region] if previous_value[region].nil? || value > previous_value[region]
-            previous_value[region] = value
-            ranked[region][person_id] = true
-          end
-          cached_country = Country.c_find(current_country_by_wca_id[person_id])
-          # The only known case where this happens if current_country_by_wca_id[person_id] is nil,
-          # in other words, the personId from the Concise*Results table is not found in the Persons table.
-          # In the past, this has occurred when temporary results have been inserted for newcomers.
-          next if cached_country.nil?
-          # Set the person's data (first time the current location is matched).
-          personal_rank[person_id][:best] ||= value
-          personal_rank[person_id][:world_rank] ||= current_rank["World"]
-          if continent_id == cached_country.continentId
-            personal_rank[person_id][:continent_rank] ||= current_rank[continent_id]
+        SQL
+        personal_records_with_event.group_by(&:first).each do |event_id, personal_records|
+          personal_rank = Hash.new { |h, k| h[k] = {} }
+          ranked = Hash.new { |h, k| h[k] = {} }
+          counter = Hash.new(0)
+          current_rank = Hash.new(0)
+          previous_value = {}
+          personal_records.each do |_, person_id, country_id, continent_id, value|
+            # Update the region states (unless we have ranked this person already,
+            # e.g. 2008SEAR01 twice in North America and World because of his two countries).
+            ["World", continent_id, country_id].each do |region|
+              next if ranked[region][person_id]
+              counter[region] += 1
+              # As we ordered by value it can either be greater or tie the previous one.
+              current_rank[region] = counter[region] if previous_value[region].nil? || value > previous_value[region]
+              previous_value[region] = value
+              ranked[region][person_id] = true
+            end
+            cached_country = Country.c_find(current_country_by_wca_id[person_id])
+            # The only known case where this happens if current_country_by_wca_id[person_id] is nil,
+            # in other words, the personId from the Concise*Results table is not found in the Persons table.
+            # In the past, this has occurred when temporary results have been inserted for newcomers.
+            next if cached_country.nil?
+            # Set the person's data (first time the current location is matched).
+            personal_rank[person_id][:best] ||= value
+            personal_rank[person_id][:world_rank] ||= current_rank["World"]
+            if continent_id == cached_country.continentId
+              personal_rank[person_id][:continent_rank] ||= current_rank[continent_id]
+            end
+            if country_id == cached_country.id
+              personal_rank[person_id][:country_rank] ||= current_rank[country_id]
+            end
           end
-          if country_id == cached_country.id
-            personal_rank[person_id][:country_rank] ||= current_rank[country_id]
+          values = personal_rank.map do |person_id, rank_data|
+            # NOTE: continent_rank and country_rank may be not present because of a country change, in such case we default to 0.
+            "('#{person_id}', '#{event_id}', #{rank_data[:best]}, #{rank_data[:world_rank]}, #{rank_data[:continent_rank] || 0}, #{rank_data[:country_rank] || 0})"
           end
-        end
-        values = personal_rank.map do |person_id, rank_data|
-          # NOTE: continent_rank and country_rank may be not present because of a country change, in such case we default to 0.
-          "('#{person_id}', '#{event_id}', #{rank_data[:best]}, #{rank_data[:world_rank]}, #{rank_data[:continent_rank] || 0}, #{rank_data[:country_rank] || 0})"
-        end
-        # Insert 500 rows at once to avoid running into too long query.
-        values.each_slice(500) do |values_subset|
-          ActiveRecord::Base.connection.execute <<-SQL
+          # Insert 500 rows at once to avoid running into too long query.
+          values.each_slice(500) do |values_subset|
+            ActiveRecord::Base.connection.execute <<-SQL
               INSERT INTO #{temp_table_name} (personId, eventId, best, worldRank, continentRank, countryRank) VALUES
               #{values_subset.join(",\n")}
-          SQL
+            SQL
+          end
         end
       end
+    end
+  end
 
-      # Atomically swap the tables
-      ActiveRecord::Base.connection.execute("RENAME TABLE #{table_name} TO #{table_name}_old, #{temp_table_name} TO #{table_name}")
-
-      # Drop the old table
-      ActiveRecord::Base.connection.execute("DROP TABLE #{table_name}_old")
-    ensure
-      ActiveRecord::Base.connection.execute("DROP TABLE IF EXISTS #{temp_table_name}")
+  def self.insert_regional_records_lookup
+    DbHelper.with_temp_table("regional_records_lookup") do |temp_table_name|
+      CheckRegionalRecords.add_to_lookup_table(table_name: temp_table_name)
     end
   end
 end

lib/check_regional_records.rb

@@ -2,6 +2,24 @@
 
 module CheckRegionalRecords
   REGION_WORLD = '__World'
+  LOOKUP_TABLE_NAME = 'regional_records_lookup'
+
+  def self.add_to_lookup_table(competition_id = nil, table_name: LOOKUP_TABLE_NAME)
+    ActiveRecord::Base.connection.execute <<-SQL
+      INSERT INTO #{table_name}
+      (resultId, countryId, eventId, competitionEndDate, best, average)
+      SELECT Results.id, Results.countryId, Results.eventId, Competitions.end_date, Results.best, Results.average
+      FROM Results
+      INNER JOIN Competitions ON Results.competitionId = Competitions.id
+      #{competition_id.present? ? "WHERE Results.competition_id = #{competition_id}" : ''}
+      ON DUPLICATE KEY UPDATE
+        countryId = Results.countryId,
+        eventId = Results.eventId,
+        competitionEndDate = Competitions.end_date,
+        best = Results.best,
+        average = Results.average
+    SQL
+  end
 
   def self.compute_record_marker(regional_records, result, wca_value)
     computed_marker = nil
@@ -120,28 +138,14 @@ def self.check_records(event_id = nil, competition_id = nil)
 
       if competition_id.present?
         model_comp = Competition.find(competition_id)
-
-        non_zero_results = Result.select(:eventId, :competitionId, :countryId, value_column.to_sym)
-                                 .where.not(value_column => ..0)
-
-        if event_id.present?
-          non_zero_results = non_zero_results.where(eventId: event_id)
-        end
-
-        earlier_competitions = Competition.select(:id)
-                                          .where(end_date: ...model_comp.start_date)
-
-        previous_min_results = Result.select("r.eventId, r.countryId, MIN(r.#{value_column}) AS `value`")
-                                     .from("(#{non_zero_results.to_sql}) AS r")
-                                     .joins("INNER JOIN (#{earlier_competitions.to_sql}) c ON c.id = r.competitionId")
-                                     .group("r.eventId, r.countryId")
-
-        unless event_id.present?
-          competition_events = CompetitionEvent.select(:event_id)
-                                               .where(competition_id: model_comp.id)
-
-          previous_min_results = previous_min_results.joins("INNER JOIN (#{competition_events.to_sql}) ce ON ce.event_id = r.eventId")
-        end
+        event_filter = event_id || model_comp.event_ids
+
+        previous_min_results = Result.select("eventId, countryId, MIN(#{value_column}) AS `value`")
+                                     .from("#{LOOKUP_TABLE_NAME} AS Results")
+                                     .where.not(value_column => ..0)
+                                     .where(competitionEndDate: ...model_comp.start_date)
+                                     .where(eventId: event_filter)
+                                     .group(:eventId, :countryId)
 
         previous_min_results.each do |r|
           event_records = base_records[r.event_id] || {}

lib/database_dumper.rb

@@ -555,6 +555,7 @@ def self.actions_to_column_sanitizers(columns_by_action)
         },
       ),
     }.freeze,
+    "regional_records_lookup" => :skip_all_rows,
     "registration_competition_events" => {
       column_sanitizers: actions_to_column_sanitizers(
         copy: %w(

lib/db_helper.rb

@@ -10,4 +10,25 @@ def self.execute_sql(sql)
     raw_connection.set_server_option(Mysql2::Client::OPTION_MULTI_STATEMENTS_OFF)
     raw_connection.abandon_results!
   end
+
+  def self.with_temp_table(table_name)
+    temp_table_name = "#{table_name}_temp"
+    old_table_name = "#{table_name}_old"
+
+    ActiveRecord::Base.connection.execute("CREATE TABLE #{temp_table_name} LIKE #{table_name}")
+    ActiveRecord::Base.connection.execute("ALTER TABLE #{temp_table_name} AUTO_INCREMENT = 1")
+
+    ActiveRecord::Base.transaction do
+      yield temp_table_name
+    end
+
+    # Atomically swap the tables
+    ActiveRecord::Base.connection.execute("RENAME TABLE #{table_name} TO #{old_table_name}, #{temp_table_name} TO #{table_name}")
+
+    # Drop the old table
+    ActiveRecord::Base.connection.execute("DROP TABLE #{old_table_name}")
+  ensure
+    ActiveRecord::Base.connection.execute("DROP TABLE IF EXISTS #{temp_table_name}")
+    ActiveRecord::Base.connection.execute("DROP TABLE IF EXISTS #{old_table_name}")
+  end
 end