Skip to content


Subversion checkout URL

You can clone with
Download ZIP


Backport #2251 into 3-1-stable #2257

merged 1 commit into from

4 participants

@spastorino spastorino merged commit 5bc0020 into rails:3-1-stable

This commit broke tests on Oracle as Oracle does not allow ORDER BY in subselect of UPDATE, see

As this example represents very specific case where such ordered update would be necessary I suggest that in this case manually written SQL UPDATE statement would be used (in databases which allow such syntax) without using Arel. For example, in Oracle you could just do "UPDATE orders SET id = id + 1" and it will succeed as Oracle will validate uniqueness constraint just at the end of all UPDATE statement.

Therefore I propose that Arel should not generate SQL which semantics is too specific for one database. SQL updates are set operations and we should not think of them as sequential procedural programs :)

The success of UPDATE does depend on the ORDER BY in DBs that do support it. Maybe Arel's Oracle engine should just ignore ORDER BYs for UPDATE queries? /cc @tenderlove

Therefore I propose that Arel should not generate SQL which semantics is too specific for one database. SQL updates are set operations and we should not think of them as sequential procedural programs :)

ActiveRecord is perfectly fine generating SQL that is going to fail on Oracle, for example see the next test.

I've created a pull request for Arel rails/arel#69.

Thanks for the fix :) Will verify if this fixes faling tests on Oracle.

FYI, this broke a few things in SQL Server too. Always finding @rsim is my partner in crime too :)

Well I am passing all tests again but putting this in the SQL Server adapter's included visitor.

def visit_Arel_Nodes_UpdateStatement(o)
  if o.orders.any? && o.limit.nil?
    o.limit =

Basically we are OK with orders in a sub-select/derived table if you include a top, so I just shim that in. It does feel that we are working against the grain tho. Then again, I think it is in our camp to make our visitors cope with the stuff ActiveRecord is doing for the mainstream. This is one of the reasons I have never considered working and submitting patches to Arel's mssql visitor vs building and including our own sqlserver one our adapter. Too much of our visitor is coping with what ActiveRecord does.

Anyways, thanks @thedarkone for the purposed work around.


Oh my... didn't mean to cause you guys so much trouble :(.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
14 activerecord/lib/active_record/relation.rb
@@ -216,17 +216,13 @@ def update_all(updates, conditions = nil, options = {})
if conditions || options.present?
where(conditions).apply_finder_options(options.slice(:limit, :order)).update_all(updates)
- limit = nil
- order = []
- # Apply limit and order only if they're both present
- if @limit_value.present? == @order_values.present?
- limit = arel.limit
- order = arel.orders
+ stmt = arel.compile_update(Arel.sql(@klass.send(:sanitize_sql_for_assignment, updates)))
+ if limit = arel.limit
+ stmt.take limit
- stmt = arel.compile_update(Arel.sql(@klass.send(:sanitize_sql_for_assignment, updates)))
- stmt.take limit if limit
- stmt.order(*order)
+ stmt.order(*arel.orders)
stmt.key = table[primary_key]
@klass.connection.update stmt.to_sql, 'SQL', bind_values
20 activerecord/test/cases/persistence_test.rb
@@ -29,6 +29,26 @@ def test_update_all_ignores_order_without_limit_from_association
+ def test_update_all_doesnt_ignore_order
+ assert_equal authors(:david).id + 1, authors(:mary).id # make sure there is going to be a duplicate PK error
+ test_update_with_order_succeeds = lambda do |order|
+ begin
+ Author.order(order).update_all('id = id + 1')
+ rescue ActiveRecord::ActiveRecordError
+ false
+ end
+ end
+ if'id DESC')
+ assert !'id ASC') # test that this wasn't a fluke and using an incorrect order results in an exception
+ else
+ # test that we're failing because the current Arel's engine doesn't support UPDATE ORDER BY queries is using subselects instead
+ assert_sql(/\AUPDATE .+ \(SELECT .* ORDER BY id DESC\)\Z/i) do
+'id DESC')
+ end
+ end
+ end
def test_update_all_with_order_and_limit_updates_subset_only
author = authors(:david)
assert_nothing_raised do
Something went wrong with that request. Please try again.