In the testbot results from https://git.drupalcode.org/project/drupalsqlsrvci/-/jobs/8393, there are a lot of errors like:
PDOException: SQLSTATE[23000]: [Microsoft][ODBC Driver 17 for SQL Server][SQL Server]Violation of PRIMARY KEY constraint 'test67130852users_pkey'. Cannot insert duplicate key in object 'dbo.test67130852users'. The duplicate key value is (1).
They are the result of the issue: #838992: Change the uid field from integer to serial by leveraging NO_AUTO_VALUE_ON_ZERO on MySQL. The problem is that we need to insert the user with uid "0" into the users table. For PostgreSQL and SQLite this is not a problem. For MySQL it was. MySQL does not like it when you try to insert the number zero into a serial field. MySQL likes to change the value zero to the first free auto increment value. Usually that will be the number 1. When Drupal then tries to insert the admin user with uid 1, the database response with an error for duplicate key. We had to add the option NO_AUTO_VALUE_ON_ZERO to the SQL mode. We need to find out how to fix this in MS SQL Server.
From https://stackoverflow.com/questions/5427142/sql-primary-key-can-accept-0:
set Identity_insert dbo.table1 ON
insert dbo.table1 (id, myfield)
Values (0, 'test')
set Identity_insert dbo.table1 OFF
TBD
None
None
None
TBD
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | 3231206-8.patch | 1.77 KB | daffie |
Comments
Comment #2
daffie commentedComment #3
beakerboyThe driver is already capable of doing this. Migration scripts specify the key values on insertion, so I check any insertions to see if a serial field is included. If it is, the “ set Identity_insert dbo.table1 ON;” is prepended to the statement and allow_delimiter_in_query option is set to true. I wonder if I need to run one of these test and echo each query to the screen to make sure it’s happening like it should.
Comment #4
beakerboyThese errors do not happen in Drupal 9.3. Were there changes in the way 9.3 inserts user ids that would affect SQL Server in a good way?
Comment #5
daffie commentedI opened this issue some time ago and after testing the problem myself I found that the problem does not exists on SQL Server.
Comment #6
beakerboyThe thing is there IS a problem in 4.2.x. I don't know if it is a "testing only" problem, or an actual problem with the code. There are failures. The fact that the failures do not happen in 4.3.x means something changed in either 4.3.x or in Drupal 9.3, and I'm curious what that change was.
Comment #7
beakerboyMoving back to active because the module does throw an exception during the test. If the problem is in the way the test code is written, can we patch the test to allow it to do what it is supposed to do in sqlsrv?
In both 9.2 and 9.2 EntityAutocompleteElementFormTest.php manually creates a user with uid=1, and then creates another user. In 9.2, sqlsrv tries to create the new one with an id of 1, while in 9.3 it must not. Why is sqlsrv not inserting the second user with the correct value? Is it because the field in not serial and Drupal for some reason does not know the next value? is it because Insert::execute() is using the deprecated fetchColumn() method in 9.2 and $stmt->fetchField() in 9.3?
Comment #8
daffie commentedI found out why the test EntityAutocompleteElementFormTest is failing. The method Connection::nextId($existing) does not return the next value. If you set $existing to 1 the method returns the value 1 and not 2 as you should expect.
I have created a patch to fix the method, it now also passes the test Drupal\KernelTests\Core\Database\NextIdTest.
Comment #9
beakerboyThanks for this patch. It works, but I have no idea why. I think I need to run some extra tests and add some comments. To be honest, the code that is currently in the driver, which I did not write, should work given everything I know about SQL. The output section must return the value before the statement is executed or something like that.
Comment #10
beakerboyI added some extra tests and one fails with your patch.
https://git.drupalcode.org/project/sqlsrv/-/blob/4.2.x-nextId/tests/src/...
Comment #11
beakerboyI guess if we accept that sometimes values will be skipped and incremented by two instead of one than this is okay. The requirements just say that the returned value will be larger than the parameter passed in. I’ll make a few more tests but I think this is good.
Comment #13
beakerboyNope, this causes a lot more failures. There are many tests that are expecting different ID values than they are receiving. The number of errors has decreased, but the number of failures has significantly increased.
Drupal\Tests\media\Kernel\MediaEmbedFilterTest::testFilterIntegration
Comment #14
beakerboyI think I figured out the root cause on why the original algorithm was not working anymore. When Connection::prepare_statement() and Connection::query() were refactored, the check on if there was a delimiter in the query was moved. Connection::queryDirect() used to not care if there was a semicolon or not because it trusted that the query was safe. Now options['allow_delimiter_in_query'] has to be set. The driver was throwing an exception because the statement contained a semicolon, but it was caught and not rethrown. The insert was not ever actually occurring, which was why a value of 3 was returning instead of 1001.
In your code, if the insert causes a problem, the "SET IDENTITY_INSERT OFF" would not actually happen if the insert value was already in the database, and the next insert would throw an exception because the DB is expecting a primary key since IDENTITY_INSERT is still ON.
A few possible solutions. If we separate this into multiple statements, there probably needs to be a "finally" section where we set IDENTITY_INSERT to OFF, or this needs to happen in the catch section. We should probably also check the type of exception, and if it is not the exception we expect, it needs to be rethrown.
Comment #16
beakerboyTesting on 4.3.
Comment #17
beakerboyComment #18
beakerboy