Adds support for concurrent indexing in PostgreSQL adapter #9923

Merged
merged 2 commits into from Mar 26, 2013

Projects

None yet

6 participants

Contributor

Can be called via

add_index(:people, :last_name, :concurrently => true)
@jeremy jeremy and 2 others commented on an outdated diff Mar 25, 2013
...d/connection_adapters/postgresql/schema_statements.rb
@@ -410,9 +410,18 @@ def rename_column(table_name, column_name, new_column_name)
end
def add_index(table_name, column_name, options = {}) #:nodoc:
- if options.is_a?(Hash) && options[:using]
+ if options.is_a?(Hash) && (options[:using] || options[:concurrently])
+ concurrently = options.delete :concurrently # We remove the concurrently key so that it will not interfere with add_index_options call
jeremy
jeremy Mar 25, 2013 Owner

Prefer to avoid mutating arguments, since someone may pass in their own DEFAULT_OPTIONS, for example, which would change it for the next add_index call.

danmcclain
danmcclain Mar 25, 2013 Contributor

@jeremy: I was trying to avoid adding :concurrently to add_index_options in the AbstractAdapter, seeing that it will only apply to PostgreSQL. Should I add it?

rafaelfranca
rafaelfranca Mar 25, 2013 Owner

No, but you can remove it from the options when calling add_index_options using except

@jeremy jeremy commented on an outdated diff Mar 25, 2013
activerecord/CHANGELOG.md
@@ -1,5 +1,12 @@
## Rails 4.0.0 (unreleased) ##
+* Add suport for concurrent indexing in PostgreSQL adapter via the
+ `:concurrently => true`
+
+ add_index(:people, :last_name, :concurrently => true)
jeremy
jeremy Mar 25, 2013 Owner

Prefer 1.9 hash syntax for new code and docs: concurrently: true

@jeremy jeremy and 1 other commented on an outdated diff Mar 25, 2013
...d/connection_adapters/postgresql/schema_statements.rb
index_name, index_type, index_columns, index_options = add_index_options(table_name, column_name, options)
- execute "CREATE #{index_type} INDEX #{quote_column_name(index_name)} ON #{quote_table_name(table_name)} USING #{options[:using]} (#{index_columns})#{index_options}"
+ if options[:using]
+ if concurrently
+ execute "CREATE #{index_type} INDEX CONCURRENTLY #{quote_column_name(index_name)} ON #{quote_table_name(table_name)} USING #{options[:using]} (#{index_columns})#{index_options}"
+ else
+ execute "CREATE #{index_type} INDEX #{quote_column_name(index_name)} ON #{quote_table_name(table_name)} USING #{options[:using]} (#{index_columns})#{index_options}"
+ end
+ else
+ execute "CREATE #{index_type} INDEX CONCURRENTLY #{quote_column_name(index_name)} ON #{quote_table_name(table_name)} (#{index_columns})#{index_options}"
+ end
jeremy
jeremy Mar 25, 2013 Owner

Between USING and CONCURRENTLY, this is getting wordy.

How about moving these upstream as a generic :algorithm (which mysql also supports) and :using (ditto mysql)

rafaelfranca
rafaelfranca Mar 25, 2013 Owner

Seems a good idea

Owner
jeremy commented Mar 25, 2013

👍

MySQL has a similar directive for INPLACE vs COPY vs DEFAULT index creation.

Contributor

@jeremy So have :algorithm as the argument instead of :concurrently and move the code to this up to the AbstractAdapters add_index_options and add_index?

add_index(:people, :last_name, algorithm: :concurrently)

Correct?

Owner
jeremy commented Mar 25, 2013

👍

Contributor

@jeremy Let me know about the latest iteration. If all is good, I can squash the commits

Owner

It seems good to me. Needs a rebase thought.

Owner
jeremy commented Mar 25, 2013

Nicely done!

Contributor
stouset commented Mar 25, 2013

👍

Contributor

I'll rebase this within the next hour or so

@stouset stouset commented on the diff Mar 25, 2013
...cord/test/cases/adapters/mysql2/active_schema_test.rb
@@ -21,30 +21,33 @@ def test_add_index
ActiveRecord::ConnectionAdapters::Mysql2Adapter.send(:define_method, :index_name_exists?) do |*|
false
end
- expected = "CREATE INDEX `index_people_on_last_name` ON `people` (`last_name`)"
+ expected = "CREATE INDEX `index_people_on_last_name` ON `people` (`last_name`) "
stouset
stouset Mar 25, 2013 Contributor

Is the extra space really necessary?

rafaelfranca
rafaelfranca Mar 25, 2013 Owner

Actually no but I don't think it is a problem.

stouset
stouset Mar 25, 2013 Contributor

Having to change a bunch of tests just to include an arbitrary extra space seems... less than optimal. Pass that one line through String#squish! (or build the SQL in a way that doesn't inject an extra space when not necessary), and the problem goes away.

stouset
stouset Mar 25, 2013 Contributor

I guess there's already an extra space between "CREATE INDEX" anyway.

jeremy
jeremy Mar 25, 2013 Owner

No worries re. spacing 😁

@danmcclain danmcclain Adds support for concurrent indexing in PostgreSQL adapter
Adds support for algorithm option in MySQL indexes
Moves USING and algorithm options upstream

The syntax is still specific to the Adapter, so the actual executed string happens
in the corresponding adapter
e199dc1
Contributor

The tests in question already have extra strings, so I went with the example that was there.

I rebased against master. I can pull out the extra spaces if necessary

@jeremy jeremy commented on an outdated diff Mar 25, 2013
...ord/connection_adapters/abstract/schema_statements.rb
index_type = options[:unique] ? "UNIQUE" : ""
index_name = options[:name].to_s if options.key?(:name)
max_index_length = options.fetch(:internal, false) ? index_name_length : allowed_index_name_length
+ algorithm = index_algorithms[options[:algorithm]]
jeremy
jeremy Mar 25, 2013 Owner

Make a typo in the :algorithm option, or pass one that's unsupported, and there's no error to flag the mistake. Consider:

index_algorithms.fetch(options[:algorithm])

Plus a test to demonstrate what happens when you pass an unknown :algorithm

Contributor

Let me know if you want me to squash this PR

@rafaelfranca rafaelfranca commented on an outdated diff Mar 26, 2013
...ord/connection_adapters/abstract/schema_statements.rb
index_type = options[:unique] ? "UNIQUE" : ""
index_name = options[:name].to_s if options.key?(:name)
max_index_length = options.fetch(:internal, false) ? index_name_length : allowed_index_name_length
+ if index_algorithms.key?(options[:algorithm])
+ algorithm = index_algorithms[options[:algorithm]]
+ elsif options[:algorithm].present?
+ key_strings = index_algorithms.keys.map{ |k| Symbol === k ? ":#{k}" : k }
rafaelfranca
rafaelfranca Mar 26, 2013 Owner

we can change the block with a inspect call.

key_strings = index_algorithms.keys.map(&:inspect)
Contributor

@rafaelfranca: Cleaned up that call, I had a ternary operator there, and pulled that out in favor of inspect

@rafaelfranca rafaelfranca merged commit 2d33796 into rails:master Mar 26, 2013
Owner

Thank you

@danmcclain danmcclain deleted the danmcclain:psql-concurrent-indexes branch Mar 26, 2013
Contributor
le0pard commented Mar 30, 2013

Hello, guys. This is greate feature, I like "algorithm: :concurrently". But right now this feature doesn't work for PostgreSQL (I didn't check MySQL). First of all we should look at PostgreSQL documentation:

http://www.postgresql.org/docs/9.2/static/sql-createindex.html#SQL-CREATEINDEX-CONCURRENTLY

Regular index builds permit other regular index builds on the same table to occur in parallel, but only one concurrent index build can occur on a table at a time. In both cases, no other types of schema modification on the table are allowed meanwhile. Another difference is that a regular CREATE INDEX command can be performed within a transaction block, but CREATE INDEX CONCURRENTLY cannot.

And of course each migration for PostgreSQL works in transaction block (it's perfect, because on fail it will rollback all changes).

I worked under this feature also and when I saw this pull request, I didn't found in changes, how this solved.

So right now for such migration:

class CreateUsers < ActiveRecord::Migration
  def change
    create_table :users do |t|
      t.string :email
      t.timestamps
    end
    add_index :users, :email, algorithm: :concurrently
  end
end

we have such problem:

$ rake db:migrate

==  CreateUsers: migrating ====================================================
-- create_table(:users)
   -> 0.0274s
-- add_index(:users, :email, {:algorithm=>:concurrently})
rake aborted!
An error has occurred, this and all later migrations canceled:

PG::Error: ERROR:  CREATE INDEX CONCURRENTLY cannot run inside a transaction block
: CREATE  INDEX CONCURRENTLY "index_users_on_email" ON "users"  ("email")/Users/leo/.rvm/gems/ruby-2.0.0-p0/bundler/gems/rails-3b0b573ac3ec/activerecord/lib/active_record/connection_adapters/postgresql/database_statements.rb:128:in `exec'
/Users/leo/.rvm/gems/ruby-2.0.0-p0/bundler/gems/rails-3b0b573ac3ec/activerecord/lib/active_record/connection_adapters/postgresql/database_statements.rb:128:in `block in execute'
/Users/leo/.rvm/gems/ruby-2.0.0-p0/bundler/gems/rails-3b0b573ac3ec/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:425:in `block in log'
/Users/leo/.rvm/gems/ruby-2.0.0-p0/bundler/gems/rails-3b0b573ac3ec/activesupport/lib/active_support/notifications/instrumenter.rb:20:in `instrument'
/Users/leo/.rvm/gems/ruby-2.0.0-p0/bundler/gems/rails-3b0b573ac3ec/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:420:in `log'
/Users/leo/.rvm/gems/ruby-2.0.0-p0/bundler/gems/rails-3b0b573ac3ec/activerecord/lib/active_record/connection_adapters/postgresql/database_statements.rb:127:in `execute'
/Users/leo/.rvm/gems/ruby-2.0.0-p0/bundler/gems/rails-3b0b573ac3ec/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb:416:in `add_index'
/Users/leo/.rvm/gems/ruby-2.0.0-p0/bundler/gems/rails-...
...
Tasks: TOP => db:migrate
(See full trace by running task with --trace)

To create such indexes, I think in code should exists some queue, which will collect all such indexes in migration and execute this indexes after success execution of transaction block.

P.S. rails tested version:

GIT
  remote: git://github.com/rails/rails.git
  revision: 3b0b573ac3ecbec5b1fa021abbe86b267bbefb8c
  specs:
    actionmailer (4.0.0.beta1)
      actionpack (= 4.0.0.beta1)
      mail (~> 2.5.3)
    actionpack (4.0.0.beta1)
      activesupport (= 4.0.0.beta1)
      builder (~> 3.1.0)
      erubis (~> 2.7.0)
      rack (~> 1.5.2)
      rack-test (~> 0.6.2)
    activemodel (4.0.0.beta1)
      activesupport (= 4.0.0.beta1)
      builder (~> 3.1.0)
    activerecord (4.0.0.beta1)
      activemodel (= 4.0.0.beta1)
      activerecord-deprecated_finders (~> 0.0.3)
      activesupport (= 4.0.0.beta1)
      arel (~> 4.0.0.beta2)
    activesupport (4.0.0.beta1)
      i18n (~> 0.6, >= 0.6.4)
      minitest (~> 4.2)
      multi_json (~> 1.3)
      thread_safe (~> 0.1)
      tzinfo (~> 0.3.37)
    rails (4.0.0.beta1)
      actionmailer (= 4.0.0.beta1)
      actionpack (= 4.0.0.beta1)
      activerecord (= 4.0.0.beta1)
      activesupport (= 4.0.0.beta1)
      bundler (>= 1.3.0, < 2.0)
      railties (= 4.0.0.beta1)
      sprockets-rails (~> 2.0.0.rc3)
    railties (4.0.0.beta1)
      actionpack (= 4.0.0.beta1)
      activesupport (= 4.0.0.beta1)
      rake (>= 0.8.7)
      thor (>= 0.18.1, < 2.0)

Thanks for all.

@le0pard 4ce9843 introduced disable_ddl_transaction! method for migrations, which you can use to turn off the wrapping transaction for the specific migrations you need. That should solve your issue I think, by disabling the transaction in this migration. Hope that helps!

Contributor
le0pard commented Mar 30, 2013

Yep, I understand your point @carlosantoniodasilva, but I didn't see any notice, what "algorithm: :concurrently" must(!!!) running with "disable_ddl_transaction!". Also, in this case we have "add_index" method which always will work correctly in migration except with this option "algorithm: :concurrently". I think we should fix "add_index" for this option or create separate method "add_index_concurrently" with notice, what user should run this without ddl transaction.

Contributor
le0pard commented Mar 30, 2013

@carlosantoniodasilva , also can we have such problem:

class AddToFirstField < ActiveRecord::Migration
  self.disable_ddl_transaction!
  def change
    add_index :users, :first_field, algorithm: :concurrently
    add_column :users, invalid, :integersss
  end
end

Start migration first time:

==  AddToFirstField: migrating ================================================
-- add_index(:users, :first_field, {:algorithm=>:concurrently})
   -> 0.0066s
-- invalid()
rake aborted!
An error has occurred, all later migrations canceled:

undefined local variable or method `invalid' for #<AddToFirstField:0x007fedc2aec1a0>/Users/leo/.rvm/gems/ruby-2.0.0-p0/bundler/gems/rails-3b0b573ac3ec/activerecord/lib/active_record/migration.rb:621:in `block in method_missing'

Start migration second time:

==  AddToFirstField: migrating ================================================
-- add_index(:users, :first_field, {:algorithm=>:concurrently})
rake aborted!
An error has occurred, all later migrations canceled:

Index name 'index_users_on_first_field' on table 'users' already exists/Users/leo/.rvm/gems/ruby-2.0.0-p0/bundler/gems/rails-3b0b573ac3ec/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb:806:in `add_index_options'

And this case:

class AddToFirstField < ActiveRecord::Migration
  self.disable_ddl_transaction!
  def change
    add_index :users, :second_field, algorithm: :concurrently
    add_index :users, :third_field, algorithm: :concurrently
  end
end

Start migration first time:

==  AddToFirstField: migrating ================================================
-- add_index(:users, :second_field, {:algorithm=>:concurrently})
   -> 0.0075s
-- add_index(:users, :third_field, {:algorithm=>:concurrently})
rake aborted!
An error has occurred, all later migrations canceled:

PG::Error: ERROR:  column "third_field" does not exist
: CREATE  INDEX CONCURRENTLY "index_users_on_third_field" ON "users"  ("third_field")/Users/leo/.rvm/gems/ruby-2.0.0-p0/bundler/gems/rails-3b0b573ac3ec/activerecord/lib/active_record/connection_adapters/postgresql/database_statements.rb:128:in `exec'

Start migration second time:

==  AddToFirstField: migrating ================================================
-- add_index(:users, :second_field, {:algorithm=>:concurrently})
rake aborted!
An error has occurred, all later migrations canceled:

Index name 'index_users_on_second_field' on table 'users' already exists/Users/leo/.rvm/gems/ruby-2.0.0-p0/bundler/gems/rails-3b0b573ac3ec/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb:806:in `add_index_options'

Conclusion: we should create separate migration for each concurent index with "disable_ddl_transaction" and without any aditional migration commands (add_index, add_column, etc.):

class AddToFirstField < ActiveRecord::Migration
  self.disable_ddl_transaction!
  def change
    add_index :users, :first_field, algorithm: :concurrently
  end
end

class AddToSecondField < ActiveRecord::Migration
  self.disable_ddl_transaction!
  def change
    add_index :users, :second_field, algorithm: :concurrently
  end
end

This isn't Rails way (I hope).

Owner

This is a database limitation and we (Rails) are providing an API to make this possible. So, yes, if you need to add an index concurrently you should know you can't do inside a transaction and with additional migration commands.

You should know your database and its limitation. We can't provide everything.

Contributor
le0pard commented Mar 30, 2013

Thanks, @rafaelfranca. Ok, but is it good idea fix this limitation by concurent index queue and execute such indexes after success ddl transaction? Like it done in pg_power. I just thinking add this fix in Rails in such way.

Owner

No. I don't think is worth to add this complexity to Rails code base right now due a database constraint.

Owner

If this feature become popular and we have to support more feature like this one so we can think to add this to Rails

Contributor
le0pard commented Mar 30, 2013

Thanks, @rafaelfranca, this is reasonable. In conclusion, I think we should write about this limitation in readme about this feature and what "disable_ddl_transaction" help to use it.

Owner

Totally agree. We can put this in the CHANGELOG entry and also in the RDOC documentation. Want to open a pull request?

Contributor
le0pard commented Mar 30, 2013

My English is bad, I just can write good code. You can do this (or maybe someone can finish this). Thanks anyway for all.

Owner

Ok. I'll do later

Owner

BTW, Thank you so much ❤️

+1 for adding a notice somewhere about running it without a transaction 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment