Skip to content

Fix multiple nil identity columns for merge insert #1327

New issue

Have a question about this project? Sign up for a free account to open an issue and contact its maintainers and the community.

By clicking “Sign up for ”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on ? Sign in to your account

Merged
merged 3 commits into from
May 12, 2025

Conversation

aidanharan
Copy link
Contributor

@aidanharan aidanharan commented May 7, 2025

Handle merge inserts that contain multiple nil identity columns. Eg:

Book.upsert_all [
  { id: nil, name: "New edition 1" },
  { id: nil, name: "New edition 2" },
]

Previously the SQL generated would have been:

MERGE INTO [books] WITH (updlock, holdlock) AS target
using (SELECT *
       FROM   (SELECT [id],
                      [name],
                      [created_at],
                      [updated_at],
                      [updated_on],
                      Row_number()
                        OVER (
                          partition BY [id]
                          ORDER BY [id] DESC ) AS rn_0
               FROM   (VALUES 
                             (Ident_current(N'[books]') + Ident_incr(N'[books]'),
                      N'New edition 2',
                      CURRENT_TIMESTAMP,
                      CURRENT_TIMESTAMP,
                      CURRENT_TIMESTAMP),
                              (Ident_current(N'[books]') + Ident_incr(N'[books]'),
                      N'New edition 1',
                      CURRENT_TIMESTAMP,
                      CURRENT_TIMESTAMP,
                      CURRENT_TIMESTAMP)) AS t1 ([id], [name], [created_at],
                      [updated_at],
                      [updated_on])) AS ranked_source
       WHERE  rn_0 = 1) AS source
ON ( target.[id] = source.[id] )
WHEN matched THEN
  UPDATE SET target.[updated_at] = CASE
                                     WHEN (( source.[name] = target.[name]
                                              OR ( source.[name] IS NULL
                                                   AND target.[name] IS NULL ) )
                                          ) THEN
                                     target.[updated_at]
                                     ELSE CURRENT_TIMESTAMP
                                   END,
             target.[updated_on] = CASE
                                     WHEN (( source.[name] = target.[name]
                                              OR ( source.[name] IS NULL
                                                   AND target.[name] IS NULL ) )
                                          ) THEN
                                     target.[updated_on]
                                     ELSE CURRENT_TIMESTAMP
                                   END,
             target.[name] = source.[name]
WHEN NOT matched BY target THEN
  INSERT ([id],
          [name],
          [created_at],
          [updated_at],
          [updated_on])
  VALUES (source.[id],
          source.[name],
          source.[created_at],
          source.[updated_at],
          source.[updated_on])
output inserted.[id]; 

When this is run only the book with title "New edition 1" is created in the table. The reason is that "New edition 2" is first inserted but then "New edition 1" is updated over "New edition 2" since it matches ON ( target.[id] = source.[id] ).

The fix changes the inserted ID columns to the following so that they don't match and both records get inserted:

  • Ident_current(N'[books]') + (Ident_incr(N'[books]') * 1)
  • Ident_current(N'[books]') + (Ident_incr(N'[books]') * 2)

This fixes the following tests which were added by rails/rails#54962

  • InsertAllTest#test_insert_all_implicitly_sets_primary_keys_when_nil
  • InsertAllTest#test_upsert_all_implicitly_sets_primary_keys_when_nil

@aidanharanaidanharan requested a review from andyundso May 8, 2025 13:02
@aidanharanaidanharan marked this pull request as ready for review May 8, 2025 13:02
@aidanharanaidanharan merged commit 1c5fb1f into main May 12, 2025
12 of 14 checks passed
@aidanharanaidanharan deleted the insert-all-default-primary-key branch May 12, 2025 09:44
Sign up for free to join this conversation on . Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants