Check if insert_all/upsert_all can be implemented using MERGE

[wpolicarpo]

Issue

Rails 6 introduced upsert_all and the SQL Server adapter does not implement it yet.

There's no such thing as MySQL's ON DUPLICATE KEY UPDATE in SQL Server, but we could possibly use the MERGE clause to achieve the same result.

Expected behavior

upsert_all and insert_all could possibly be implemented using MERGE.

Actual behavior

upsert_all and insert_all are not implemented.

[mgrunberg]

@wpolicarpo I created PR #869 to explore this possibility. I found some issues.

[andyundso]

Hi @aidanharan

I would like to pick up the work by @mgrunberg in #869, mostly because I would like to use Solid Queue with MSSQL. There is a recent comment by @justinko regarding a caveat.

I think MERGE is still appropriate for this ... it just doesn't support duplicates in the "source". More info on that here: https://www.ibm.com/docs/en/informix-servers/14.10?topic=statement-handling-duplicate-rows

Question is if it would be okay if the activerecord-sql-adapter diverges from the other reference implementations (MySQL, SQLite and PostgreSQL) and de-duplicates the rows in the source (using the uniquely identifiable information, as suggested in the PR). I would assume most users never update the same thing multiple times within the same database query, but for those who do, it could be documented in the code that SQL server does this.

Otherwise, an official gem for the insert / upsert functionality also might be an idea to signal "hey, we can offer this functionality, but since it has a catch, it is its own thing".

[aidanharan]

@andyundso Yes, I think de-duplicating the rows in the source in the adapter is fine if it can be made to work. We can document the difference to other adapters in the README.

I'm no longer using MSSQL in work so I have limited time to give to the issue.

[andyundso]

@aidanharan question about the tests: I do not really understand what this coerce does? does this disable tests from Rails upstream? how would I best go to add tests for this new MERGE mechanism?

[aidanharan]

@andyundso You only need to coerce a test if the Rails test doesn't suit the adapter. For example, in the ActiveRecord UniquenessValidationWithIndexTest#test_partial_index test the where option needed to be slightly modified for MSSQL. So we coerce the ActiveRecord test and re-implement it as UniquenessValidationWithIndexTest#test_partial_index_coerced. Note, the naming of the test with _coerced is just to let the developer know if it was a coerced test.

ActiveRecord: https://github.com/rails/rails/blob/3626b8f52c425c2dd7a0d0aa698cff9f10d789b3/activerecord/test/cases/validations/uniqueness_validation_test.rb#L728

MSSQL:

activerecord-sqlserver-adapter/test/cases/coerced_tests.rb

Line 67 in 8739eea

coerce_tests! :test_partial_index

For insert_all/upsert_all you should not need to coerce any tests. The adapter's supports_insert_on_duplicate_skip? and supports_insert_on_duplicate_update? flags should be enabled as part of the PR so that tests such as InsertAllTest#test_insert_all_with_skip_duplicates_and_autonumber_id_given will not be skipped when run against the MSSQL adapter.

ActiveRecord: https://github.com/rails/rails/blob/3626b8f52c425c2dd7a0d0aa698cff9f10d789b3/activerecord/test/cases/insert_all_test.rb#L177

[aidanharan]

Closing as feature added in #1312