Problem/Motivation

When using a Merge/upsert the primary key cannot be updated in sql server

Steps to reproduce

Drupal\KernelTests\Core\Database\UpsertTest::testUpsertWithKeywords
282Drupal\Core\Database\IntegrityConstraintViolationException: SQLSTATE[42000]: [Microsoft][ODBC Driver 17 for SQL Server][SQL Server]Cannot update identity column 'id'.: MERGE [test33275901select] AS tgt USING(VALUES (:db_upsert_placeholder_0, :db_upsert_placeholder_1), (:db_upsert_placeholder_2, :db_upsert_placeholder_3)) AS src ([id], [update]) ON tgt.[id]=src.[id] WHEN MATCHED THEN UPDATE SET [id]=src.[id], [update]=src.[update] WHEN NOT MATCHED THEN INSERT ([id], [update]) VALUES (src.[id], src.[update]);; Array
283(
284)
285/opt/drupal/modules/sqlsrv/src/Driver/Database/sqlsrv/ExceptionHandler.php:47
286/opt/drupal/modules/sqlsrv/src/Driver/Database/sqlsrv/Upsert.php:76
287/opt/drupal/core/tests/Drupal/KernelTests/Core/Database/UpsertTest.php:82
288/opt/drupal/vendor/phpunit/phpunit/src/Framework/TestResult.php:703
289Caused by
290PDOException: SQLSTATE[42000]: [Microsoft][ODBC Driver 17 for SQL Server][SQL Server]Cannot update identity column 'id'.
291/opt/drupal/core/lib/Drupal/Core/Database/StatementWrapper.php:145
292/opt/drupal/modules/sqlsrv/src/Driver/Database/sqlsrv/Upsert.php:72
293/opt/drupal/core/tests/Drupal/KernelTests/Core/Database/UpsertTest.php:82
294/opt/drupal/vendor/phpunit/phpunit/src/Framework/TestResult.php:703

Proposed resolution

Remove [id]=src.[id] from the update part of the statement.

Remaining tasks

Core support extends to 8.9, 9.2 and 9.3, so This fix will be applied to 3.1.x, 4.2.x and 4.3.x

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#14 3245242-14.patch1.64 KBdaffie
#3 3245242-3.patch3.04 KBdaffie

Issue fork sqlsrv-3245242

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Beakerboy created an issue. See original summary.

Beakerboy’s picture

Beakerboy
Primary language English
Location Kentucky
CreditAttribution: Beakerboy commented

This test goes back to 9.0, but should probably be considered a bug even in 8.x.

daffie’s picture

daffie CreditAttribution: daffie commented
FileSize
3245242-3.patch3.04 KB
  1. I removed the following piece of code, because it is not usefull and it does not return the number of changed records in the database. 
    -    if (count($this->insertValues) === 1) {
-      $insert_fields = array_merge($this->defaultFields, $this->insertFields);
-      $update_fields = array_combine($insert_fields, array_shift($this->insertValues));
-      $condition = $update_fields[$this->key];
-      $merge = $this->connection->merge($this->table, $this->queryOptions)
-        ->fields($update_fields)
-        ->key($this->key, $condition);
-      $merge->execute();
-      return NULL;
-    }
  2. The code change with the identity_insert stuff fixes the problem as described in the issue summary.
  3. Added the new private method, because that code is used in 2 locations. The "do not repeat yourself" motto.

On my local machine the UpsertTest is now passing.

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Active » Needs review
Beakerboy’s picture

Beakerboy
Primary language English
Location Kentucky
CreditAttribution: Beakerboy commented

I'm surprised that this works. I thought that the "SET IDENTITY_INSERT {' . $this->table . '} ON;" had to be in the same statement as the actual query. This is why it is prepended to the query string in the Insert class. Is there any concern that the driver will close the socket connection between these calls and the database will be confused if identity_insert is still on, or if some other "malicious actor" could force an identity insert when they shouldn't be able to by timing their activity?

Secondly, SQL SERVER allows only 250 inserts in a batch, so if a user wants to insert or upsert more than that, it has to be broken up into a smaller number. The code as I checked it in sums up the number from all the individual queries and returns the total. I'm worried that using $affected_rows = $stmt->rowCount(); will just return the number from the last query. I'm guessing the core tests do not perform an upsert with many-many changes. If that's the case, we should add one to our test suite to ensure the count is correct when multiple batches are excecuted.

daffie’s picture

daffie CreditAttribution: daffie commented

Is there any concern that the driver will close the socket connection between these calls and the database will be confused if identity_insert is still on, or if some other "malicious actor" could force an identity insert when they shouldn't be able to by timing their activity?

We can wrap the 3 statement together in a transaction, when it fails which is catched by an exception, we do a rollback. Just like we do in the PostgreSQL version of the Upsert class. See: https://git.drupalcode.org/project/drupal/-/blob/HEAD/core/lib/Drupal/Co....

Beakerboy’s picture

Beakerboy
Primary language English
Location Kentucky
CreditAttribution: Beakerboy commented
Title: A merge statement cannot update a primary key » An Upsert statement cannot update a primary key
Beakerboy’s picture

Beakerboy
Primary language English
Location Kentucky
CreditAttribution: Beakerboy commented
Status: Needs review » Needs work

After transitioning the code to match the provided patch, I am seeing a new error:

3) Drupal\Tests\aggregator\Kernel\Migrate\MigrateAggregatorStubTest::testItemStub

Drupal\Core\Entity\EntityStorageException: SQLSTATE[25000]: [Microsoft][ODBC Driver 17 for SQL Server][SQL Server]Cannot roll back savepoint_1. No transaction or savepoint of that name was found.: ROLLBACK TRANSACTION savepoint_1; Array
(
)

Beakerboy’s picture

Beakerboy
Primary language English
Location Kentucky
CreditAttribution: Beakerboy commented
Status: Needs work » Needs review

I thinks it’s unrelated…Need to look at it some more.

Beakerboy’s picture

Beakerboy
Primary language English
Location Kentucky
CreditAttribution: Beakerboy commented

When an upsert has to be split up Into multiple batches, so you think the “set identitiy_insert On” and then “off” should just happen once, or with each merge statement? Should the transaction cover the entire Upsert, or each batch?

Beakerboy’s picture

Beakerboy
Primary language English
Location Kentucky
CreditAttribution: Beakerboy commented

I ran the current 4.3.x branch against MigrateAggregatorStubTest five times and it had two errors (out of 10 assertions). One was as above and one:

Drupal\Tests\aggregator\Kernel\Migrate\MigrateAggregatorStubTest::testFeedStub
170Drupal\Core\Entity\EntityStorageException: SQLSTATE[42000]: [Microsoft][ODBC Driver 17 for SQL Server][SQL Server]Operation failed. The index entry of length 1820 bytes for the index 'aggregator_feed_field__url_idx' exceeds the maximum length of 1700 bytes for nonclustered indexes.: INSERT INTO [test96925307aggregator_feed] ([uuid], [langcode], [title], [url], [refresh], [checked], [queued], [link], [description], [image], [hash], [etag], [modified]) OUTPUT (Inserted.fid) VALUES
171(:db_insert0, :db_insert1, :db_insert2, :db_insert3, :db_insert4, :db_insert5, :db_insert6, :db_insert7, :db_insert8, :db_insert9, :db_insert10, :db_insert11, :db_insert12)

When I ran the 3245242-a-merge-statement branch five times (your code), the Test class failed as indicated above twice.

I should look more closely at this test.

Beakerboy’s picture

Beakerboy
Primary language English
Location Kentucky
CreditAttribution: Beakerboy commented

Fixing #3131438: Excessive index length failures in MigrateAggregatorStubTest took care of the MigrateAggregatorStubTest errors

  • Beakerboy committed 626fbad on 4.3.x 
    Issue #3245242 by Beakerboy, daffie: An Upsert statement cannot update a...
daffie’s picture

daffie CreditAttribution: daffie commented
Version: 4.3.x-dev » 4.2.x-dev
FileSize
3245242-14.patch1.64 KB

The fix for the branch 4.2. With this fix it passes the test Drupal\KernelTests\Core\Database\UpsertTest.

Beakerboy’s picture

Beakerboy
Primary language English
Location Kentucky
CreditAttribution: Beakerboy commented
Issue summary: View changes

  • Beakerboy committed 263c7c5 on 3.1.x 
    Issue #3245242 by Beakerboy, daffie: An Upsert statement cannot update a...
Beakerboy’s picture

Beakerboy
Primary language English
Location Kentucky
CreditAttribution: Beakerboy commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.