extend versions index to include number, and uniqueify it

also improve find_each_with_temp_table to support sqlite
and non-transactional

Change-Id: Ie5b9dfe3fc619fd2fcc4350612dceed1e385c5cf
Reviewed-on: https://gerrit.instructure.com/20919
Tested-by: Jenkins <jenkins@instructure.com>
Reviewed-by: Duane Johnson <duane@instructure.com>
Product-Review: Cody Cutrer <cody@instructure.com>
QA-Review: Cody Cutrer <cody@instructure.com>

config/initializers/active_record.rb

@@ -465,15 +465,18 @@ class ActiveRecord::Base
     Canvas::TempTable.new(connection, construct_finder_sql({}), options)
   end
 
-  def self.find_in_batches_with_temp_table(options = {})
-    generate_temp_table(options).execute! do |table|
-      table.find_in_batches(options) { |batch| yield batch }
+  def self.find_in_batches_with_temp_table(options = {}, &block)
+    temptable = generate_temp_table(options)
+    with_exclusive_scope do
+      temptable.execute do |table|
+        table.find_in_batches(options, &block)
+      end
     end
   end
 
-  def self.find_each_with_temp_table(options = {})
+  def self.find_each_with_temp_table(options = {}, &block)
     find_in_batches_with_temp_table(options) do |batch|
-      batch.each { |record| yield record }
+      batch.each(&block)
     end
     self
   end

db/migrate/20130524164516_add_number_to_versions_index.rb

@@ -0,0 +1,36 @@
+class AddNumberToVersionsIndex < ActiveRecord::Migration
+  tag :postdeploy
+  self.transactional = false
+
+  def self.up
+    # eliminate duplicates
+    Version.select([:versionable_id, :versionable_type, :number]).
+      group(:versionable_id, :versionable_type, :number).
+      having("COUNT(*) > 1").
+      # number DESC so that dups in earlier numbers don't alter the dups in later numbers,
+      # since we've already cached them in a temp table
+      order("number DESC").
+      find_each_with_temp_table(:transactional => false) do |row|
+        versionable_object_scope = Version.where(:versionable_id => row['versionable_id'],
+                                                 :versionable_type => row['versionable_type'])
+        dups = versionable_object_scope.where(:number => row['number']).order(:created_at, :id).all
+        # leave the first one alone
+        dups.shift
+        next if dups.empty? # ???
+        # move later versions out of the way
+        versionable_object_scope.where("number>?", row['number']).update_all("number=number+#{dups.length}")
+        dups.each_with_index do |dup, idx|
+          dup.number += idx + 1
+          dup.save!
+        end
+      end
+
+    add_index :versions, [:versionable_id, :versionable_type, :number], :unique => true, :concurrently => true, :name => "index_versions_on_versionable_object_and_number"
+    remove_index :versions, [:versionable_id, :versionable_type]
+  end
+
+  def self.down
+    add_index :versions, [:versionable_id, :versionable_type], :concurrently => true
+    remove_index :versions, :name => "index_versions_on_versionable_object_and_number"
+  end
+end

lib/canvas/temp_table.rb

@@ -5,36 +5,48 @@ module Canvas
       @sql = sql
       @name = '_' + (options.delete(:name) || 'temp_table')
       @index = 'temp_primary_key'
+      @transactional = options[:transactional]
     end
 
     def name
       @name
     end
 
-    def execute!
-      ActiveRecord::Base.transaction do
-        begin
-          @connection.execute "create temporary table #{@name} as #{@sql}"
-          case @connection.adapter_name
-            when 'PostgreSQL'
-              @connection.execute "ALTER TABLE #{@name}
-                                   ADD temp_primary_key SERIAL PRIMARY KEY"
-            when 'MySQL', 'Mysql2'
-              @connection.execute "ALTER TABLE #{@name}
-                                   ADD temp_primary_key MEDIUMINT NOT NULL PRIMARY KEY AUTO_INCREMENT"
-            else
-              raise "Temp tables not supported!"
-          end
-
-          yield self
-        ensure
-          @connection.execute "drop table #{@name}"
+    def execute(&block)
+      if @transactional
+        ActiveRecord::Base.transaction do
+          execute_frd(&block)
         end
+      else
+        execute_frd(&block)
+      end
+    end
+
+    def execute_frd
+      begin
+        @connection.execute "CREATE TEMPORARY TABLE #{@name} AS #{@sql}"
+        case @connection.adapter_name
+        when 'PostgreSQL'
+          @connection.execute "ALTER TABLE #{@name}
+                               ADD temp_primary_key SERIAL PRIMARY KEY"
+        when 'MySQL', 'Mysql2'
+          @connection.execute "ALTER TABLE #{@name}
+                               ADD temp_primary_key MEDIUMINT NOT NULL PRIMARY KEY AUTO_INCREMENT"
+        when 'SQLite'
+          # Sqlite always has an implicit primary key
+          @index = 'rowid'
+        else
+          raise "Temp tables not supported!"
+        end
+
+        yield self
+      ensure
+        @connection.execute "DROP TABLE #{@name}"
       end
     end
 
     def size
-      @connection.select_value("SELECT Count(1) FROM #{@name}").to_i
+      @connection.select_value("SELECT COUNT(1) FROM #{@name}").to_i
     end
 
     def find_each(options)

spec/lib/canvas/temp_table_spec.rb

@@ -46,7 +46,7 @@ describe "Canvas::TempTable" do
     temp_table = Canvas::TempTable.new(@scope.connection, @sql)
     table = temp_table.name
 
-    temp_table.execute! do
+    temp_table.execute do
       @scope.connection.select_all("select * from #{table}").length.should == 6
     end
     expect { @scope.connection.select_all("select * from #{table}") }.to raise_error
@@ -54,7 +54,7 @@ describe "Canvas::TempTable" do
 
   it "should give me the length of the table" do
     temp_table = Canvas::TempTable.new(@scope.connection, @sql)
-    temp_table.execute! do
+    temp_table.execute do
       temp_table.size.should == 6
     end
   end

spec/migrations/populate_submission_versions_spec.rb

@@ -41,7 +41,8 @@ describe DataFixup::PopulateSubmissionVersions do
     course_with_student
     submission = @user.submissions.build(:assignment => @course.assignments.create!)
     submission.without_versioning{ submission.save! }
-    versions = n.times.map{ Version.create(:versionable => submission, :yaml => submission.attributes.to_yaml) }
+    submission.versions.exists?.should be_false
+    n.times { |x| Version.create(:versionable => submission, :yaml => submission.attributes.to_yaml) }
     lambda{
       DataFixup::PopulateSubmissionVersions.run
     }.should change(SubmissionVersion, :count).by(n)