-
Notifications
You must be signed in to change notification settings - Fork 309
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Optimize remove_columns to use a single SQL statement when supported #2182
Optimize remove_columns to use a single SQL statement when supported #2182
Conversation
Follow rails/rails#42075. This PR supports single SQL statement syntax of Oracle for `remove_columns` and fixes the CI build error. ```console % bundle exec rake (snip) Failures: 1) OracleEnhancedAdapter schema definition alter columns with column cache should remove column when using change_table Failure/Error: expect(TestPost.columns_hash["title"]).to be_nil expected: nil got: #<ActiveRecord::ConnectionAdapters::OracleEnhanced::Column:0x0000000003f99680 @name="title", @sql_typ...on=nil, @scale=nil>, @null=false, @default=nil, @default_function=nil, @collation=nil, @comment=nil> # /home/vagrant/.rvm/gems/ruby-3.0.0/gems/rspec-support-3.10.2/lib/rspec/support.rb:102:in `block in <module:Support>' # /home/vagrant/.rvm/gems/ruby-3.0.0/gems/rspec-support-3.10.2/lib/rspec/support.rb:111:in `notify_failure' # /home/vagrant/.rvm/gems/ruby-3.0.0/gems/rspec-expectations-3.10.1/lib/rspec/expectations/fail_with.rb:35:in `fail_with' # /home/vagrant/.rvm/gems/ruby-3.0.0/gems/rspec-expectations-3.10.1/lib/rspec/expectations/handler.rb:40:in `handle_failure' # /home/vagrant/.rvm/gems/ruby-3.0.0/gems/rspec-expectations-3.10.1/lib/rspec/expectations/handler.rb:56:in `block in handle_matcher' # /home/vagrant/.rvm/gems/ruby-3.0.0/gems/rspec-expectations-3.10.1/lib/rspec/expectations/handler.rb:27:in `with_matcher' # /home/vagrant/.rvm/gems/ruby-3.0.0/gems/rspec-expectations-3.10.1/lib/rspec/expectations/handler.rb:48:in `handle_matcher' # /home/vagrant/.rvm/gems/ruby-3.0.0/gems/rspec-expectations-3.10.1/lib/rspec/expectations/expectation_target.rb:65:in `to' # ./spec/active_record/connection_adapters/oracle_enhanced/schema_statements_spec.rb:931:in `block (3 levels) in <top (required)>' (snip) 2) OracleEnhancedAdapter schema definition alter columns with column cache should remove multiple columns when using change_table Failure/Error: with_retry { @connection.exec(sql, *bindvars, &block) } ActiveRecord::StatementInvalid: OCIError: ORA-00933: SQL command not properly ended # stmt.c:267:in oci8lib_300.so # /home/vagrant/.rvm/gems/ruby-3.0.0/bundler/gems/ruby-oci8-d7426d763dd0/lib/oci8/cursor.rb:137:in `exec' # /home/vagrant/.rvm/gems/ruby-3.0.0/bundler/gems/ruby-oci8-d7426d763dd0/lib/oci8/oci8.rb:271:in `exec_internal' # /home/vagrant/.rvm/gems/ruby-3.0.0/bundler/gems/ruby-oci8-d7426d763dd0/lib/oci8/oci8.rb:262:in `exec' # ./lib/active_record/connection_adapters/oracle_enhanced/oci_connection.rb:445:in `block in exec' # ./lib/active_record/connection_adapters/oracle_enhanced/oci_connection.rb:433:in `with_retry' # ./lib/active_record/connection_adapters/oracle_enhanced/oci_connection.rb:445:in `exec' # ./lib/active_record/connection_adapters/oracle_enhanced/oci_connection.rb:97:in `exec' # ./lib/active_record/connection_adapters/oracle_enhanced/database_statements.rb:13:in `block in execute' # /home/vagrant/.rvm/gems/ruby-3.0.0/bundler/gems/rails-c12ef7c9eb9e/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:725:in `block (2 levels) in log' # /home/vagrant/.rvm/gems/ruby-3.0.0/bundler/gems/rails-c12ef7c9eb9e/activesupport/lib/active_support/concurrency/load_interlock_aware_monitor.rb:26:in `block (2 levels) in synchronize' # /home/vagrant/.rvm/gems/ruby-3.0.0/bundler/gems/rails-c12ef7c9eb9e/activesupport/lib/active_support/concurrency/load_interlock_aware_monitor.rb:25:in `handle_interrupt' # /home/vagrant/.rvm/gems/ruby-3.0.0/bundler/gems/rails-c12ef7c9eb9e/activesupport/lib/active_support/concurrency/load_interlock_aware_monitor.rb:25:in `block in synchronize' # /home/vagrant/.rvm/gems/ruby-3.0.0/bundler/gems/rails-c12ef7c9eb9e/activesupport/lib/active_support/concurrency/load_interlock_aware_monitor.rb:21:in `handle_interrupt' # /home/vagrant/.rvm/gems/ruby-3.0.0/bundler/gems/rails-c12ef7c9eb9e/activesupport/lib/active_support/concurrency/load_interlock_aware_monitor.rb:21:in `synchronize' # /home/vagrant/.rvm/gems/ruby-3.0.0/bundler/gems/rails-c12ef7c9eb9e/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:724:in `block in log' # /home/vagrant/.rvm/gems/ruby-3.0.0/bundler/gems/rails-c12ef7c9eb9e/activesupport/lib/active_support/notifications/instrumenter.rb:44:in `instrument' # /home/vagrant/.rvm/gems/ruby-3.0.0/bundler/gems/rails-c12ef7c9eb9e/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:715:in `log' # ./lib/active_record/connection_adapters/oracle_enhanced/dbms_output.rb:35:in `log' # ./lib/active_record/connection_adapters/oracle_enhanced/database_statements.rb:13:in `execute' (snip) # ------------------ # --- Caused by: --- # OCIError: # ORA-00933: SQL command not properly ended # stmt.c:267:in oci8lib_300.so Finished in 3 minutes 0.9 seconds (files took 0.54912 seconds to load) 391 examples, 2 failures, 5 pending Failed examples: rspec ./spec/active_record/connection_adapters/oracle_enhanced/schema_statements_spec.rb:924 # OracleEnhancedAdapter schema definition alter columns with column cache should remove column when using change_table rspec ./spec/active_record/connection_adapters/oracle_enhanced/schema_statements_spec.rb:934 # OracleEnhancedAdapter schema definition alter columns with column cache should remove multiple columns when using change_table ``` https://github.com/rsim/oracle-enhanced/runs/2482234335
be02499
to
1662c14
Compare
def remove_columns(table_name, *column_names, type: nil, **options) # :nodoc: | ||
quoted_column_names = column_names.map { |column_name| quote_column_name(column_name) }.join(", ") | ||
|
||
execute "ALTER TABLE #{quote_table_name(table_name)} DROP (#{quoted_column_names}) CASCADE CONSTRAINTS" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about removing CASCADE CONSTRAINTS
here? How about executing remove_foreign_key
explicitly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've specified the same CASCADE CONSTRAINS
that remove_column
method uses.
def remove_column(table_name, column_name, type = nil, options = {}) #:nodoc:
execute "ALTER TABLE #{quote_table_name(table_name)} DROP COLUMN #{quote_column_name(column_name)} CASCADE CONSTRAINTS"
ensure
clear_table_columns_cache(table_name)
end
TBH, I still don't understand benefits and risks of replacing it with remove_foreign_key
method.
The aim of this PR is to make remove_columns
executable with a single SQL statement. Is it possible to separate it as another issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, right. Let's make this behavior consistent.
Thank you! |
Follow rails/rails#42075.
This PR supports single SQL statement syntax of Oracle for
remove_columns
and fixes the CI build error.https://github.com/rsim/oracle-enhanced/runs/2482234335