ActiveRecord::Connection::SQLiteAdapter#copy_table_indexes incorrectly renames indexes #3489

Closed
delwaterman opened this Issue Nov 1, 2011 · 6 comments

7 participants

@delwaterman

While working on some migration issues, I discovered an issue with how the SQLite Adapater is handling the #change_column in migrations. In the code I was working on here is the schema of the table before the migration. The index name is the important detail:

pcp070211pcs:profile delwateo$ sqlite3 db/development.sqlite3 
SQLite version 3.6.23.1
Enter ".help" for instructions
Enter SQL statements terminated with a ";"
sqlite> .schema resources
CREATE TABLE "resources" ("id" INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL, "name" varchar(255) NOT NULL,   "url" varchar(255) NOT NULL, "content_id" varchar(255) NOT NULL, "created_at" datetime, "updated_at" datetime, "image_url" varchar(255), "section_id" integer, "deleted" boolean DEFAULT 'f');
CREATE INDEX "resources_by_sid_and_cid" ON "resources" ("section_id", "content_id", "deleted");

Then I run the following offending migration:

class AddCategorySerializedDataToResource < ActiveRecord::Migration
  def self.up
    add_column :resources, :serialized_data, :text
    add_column :resources, :category, :string
    Resource.all.each {|r| r.update_attribute(:category, 'article') }
    change_column :resources, :category, :string, :null => false, :default => 'article'
  end

  def self.down
    remove_column :resources, :category
    remove_column :resources, :serialized_data
  end
end

checking the schema after the migration I get:

pcp070211pcs:profile delwateo$ sqlite3 db/development.sqlite3 
SQLite version 3.6.23.1
Enter ".help" for instructions
Enter SQL statements terminated with a ";"
sqlite> .schema resources
CREATE TABLE "resources" ("id" INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL, "name" varchar(255) NOT NULL, "url" varchar(255) NOT NULL, "content_id" varchar(255) NOT NULL, "created_at" datetime, "updated_at" datetime, "image_url" varchar(255), "section_id" integer, "deleted" boolean DEFAULT 'f', "serialized_data" text, "category" varchar(255) DEFAULT 'article' NOT NULL);
CREATE INDEX "altered_resources_by_sid_and_cid" ON "resources" ("section_id", "content_id", "deleted");

The index name has been incorrectly changed from 'resources_by_sid_and_cid' to 'altered_resources_by_sid_and_cid'. Which breaks future migrations that modify that index. Looking through the log/development.log file I see the following:

[1m[35mSQL (0.7ms)[0m  CREATE TEMPORARY TABLE "altered_resources" ("id" INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL, "name" varchar(255) NOT NULL, "url" varchar(255) NOT NULL, "content_id" varchar(255) NOT NULL, "created_at" datetime, "updated_at" datetime, "image_url" varchar(255), "section_id" integer, "deleted" boolean DEFAULT 'f', "serialized_data" text, "category" varchar(255)) 
[1m[36mSQL (0.1ms)[0m  [1mPRAGMA index_list("resources")[0m
[1m[35mSQL (0.1ms)[0m  PRAGMA index_info('resources_by_sid_and_cid')
[1m[36mSQL (0.0ms)[0m  [1mPRAGMA index_list("altered_resources")[0m
[1m[35mSQL (0.1ms)[0m  CREATE INDEX "temp_altered_resources_by_sid_and_cid" ON "altered_resources" ("section_id", "content_id", "deleted")
[1m[36mSQL (0.6ms)[0m  [1mDROP TABLE "resources"[0m
[1m[35mSQL (0.4ms)[0m  CREATE TABLE "resources" ("id" INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL, "name" varchar(255) NOT NULL, "url" varchar(255) NOT NULL, "content_id" varchar(255) NOT NULL, "created_at" datetime, "updated_at" datetime, "image_url" varchar(255), "section_id" integer, "deleted" boolean DEFAULT 'f', "serialized_data" text, "category" varchar(255) DEFAULT 'article' NOT NULL) 
[1m[36mSQL (0.1ms)[0m  [1mPRAGMA index_list("altered_resources")[0m
[1m[35mSQL (0.1ms)[0m  PRAGMA index_info('temp_altered_resources_by_sid_and_cid')
[1m[36mSQL (0.1ms)[0m  [1mPRAGMA index_list("resources")[0m
[1m[35mSQL (0.3ms)[0m  CREATE INDEX "altered_resources_by_sid_and_cid" ON "resources" ("section_id", "content_id", "deleted")
[1m[36mSQL (0.5ms)[0m  [1mDROP TABLE "altered_resources"[0m

After doing some hunting around I found the offending code in ActiveRecord::Connection::SQLiteAdapter. When a migration changes the default value of a column in SQLite DB, the migration uses #alter_table, which moves the current table to a temporary table and then moves it back to a normal table with the updated definition. So #alter_table calls #move_table twice which calls #copy_indexes each time its called. Here is the offending function:

# file activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb line:339
        def copy_table_indexes(from, to, rename = {}) #:nodoc:
          indexes(from).each do |index|
            name = index.name
            if to == "altered_#{from}"
              name = "temp_#{name}"
            elsif from == "altered_#{to}"
              name = name[5..-1]
            end

            to_column_names = columns(to).map { |c| c.name }
            columns = index.columns.map {|c| rename[c] || c }.select do |column|
              to_column_names.include?(column)
            end

            unless columns.empty?
              # index name can't be the same
              opts = { :name => name.gsub(/_(#{from})_/, "_#{to}_") }
              opts[:unique] = true if index.unique
              add_index(to, columns, opts)
            end
          end
        end

The effect of the first time this is called when from='resources' and to='altered_resources' is that the index 'resources_by_sid_and_cid' is renamed twice within the function. First the line

name = "temp_#{name}"

renames the index from 'resources_by_sid_and_cid' to 'temp_resources_by_sid_and_cid'. The second time by the line

opts = { :name => name.gsub(/_(#{from})_/, "_#{to}_") }

renames the index to 'temp_altered_resources_by_sid_to_cid'. This is expected behaviour.

But the second time this function is called (when moving the temp table back to the new definition) the index is not correctly renamed. The index name starts as "temp_altered_resources_by_sid_and_cid". The line

name = name[5..-1]

changes the name to "altered_resources_by_sid_to_cid". But the next line fails to rename the index. The line

opts = { :name => name.gsub(/_(#{from})_/, "_#{to}_") }

is trying to match "_altered_resources_" when it should be trying to match "altered_resources_". So the index name remains "altered_resources_by_sid_and_cid"/

But the second rename seems superfluous to me as well. Why is a second rename required? You have already renamed the index "temp_#{index_name}" to guarantee a unique name. My recommendation would be to just change the offending line to

opts = { :name => name}
@erikpukinskis

This obviously needs to be fixed, but in the meantime if you want to clean up your indicies, this rake task will do it for you. You can follow it with rake db:schema:dump to get your schema updated.

# lib/tasks/db.rake
namespace :db do
  desc "Drop 'altered_' prefix from any indexes that had it added by SqliteAdapter"
  task :fix_altered_indexes => :environment do
    ActiveRecord::Base.connection.tables.each do |table|
      indexes = ActiveRecord::Base.connection.indexes(table).map &:name
      indexes.each do |index|
        if index.starts_with? "altered_"
          ActiveRecord::Migration.send :rename_index, table, index, index[8, index.length]
        end
      end
    end
  end
end
@isaacsanders

Is this still an issue?

@delwaterman

I haven't tested it in a while since I already have the hot patch in place

@jrreed

+1 - I'm seeing this issue - rails-3.2.8

@steveklabnik
Ruby on Rails member

Thanks for confirming, @jrreed.

@senny
Ruby on Rails member

I'll look into this

@senny senny was assigned Dec 15, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment