Bug: Identity Values not set correctly with Bulk Insert from Sql Server due to Incorrect Sorting of data.

[cajuncoding]

Bug Description

There is a critical issue with the existing implementation of Bulk Insert for Sql Server whereby the Identity values are not correctly set on the models passed into RepoDb. In the existing implementation the models are passed in and RepoDb mutates the models to set their Identity values that are generated by Sql Server and returned via the Output clause of the Merge queries. This has been specifically observed in SqlAzure.

However, the processing order of SqlBulkCopy API and/or the Output clause is not guaranteed and the order of items returned may be different than the order of the original entities specified, which results in mismatched Identity column values being set on the entities specified.

And, mutating the list to result in a different sort order than that which is specified would be really bad for consuming client code and pose many other problems. So the intuitive process and result would be for the original entity enumerable set to remain in the exact order specified (as it does now), but with 100% guarantee that the Idenity values will be correctly set on all entities.

This can be due to multiple possible issues such as:

• SqlBulkCopy.WriteToServer() method may affect the order where the order of data being written is not guaranteed, and there is still no support for the ORDER() hint. In my testing this has resulted in the data being written to the temp Sql Server table in inverted order.
  • This appears to often invert the order, but ultimately cannot be relied upon, and data needs to be sortable by a known factor that matches the entity ordinal sort order as specified.
• Sql Merge query Output clause may not write the results in the exact same order of the source data due to Sql Server Merge query optimizations and indexes on the source/target tables.
  • This can be quite complex to determine if it will impact due to many underlying reasons, but ultimately cannot be relied upon without an explicit Order By clause.

This is most easily observed when there are various Indexes set on the target tables for which Bulk Insert is writing data into.

Note: Even though a clustered is being added to the temp table after data is inserted, but before the merge query is executed, this clustered index provides a sorted index based on qualifiers, and may not match the order enumerable list of entities specified to the original BulkMerge() method.

Recommended Solution:
Regardless of the potential cause of different ordering, the solution to guarantee the order of data from Sql Server is to always specify and Order By clause of the results being retrieved. An example of retrieving the output data in the correct order so that is always 100% of the time matches the original enumerable entity list order is here in the SqlBulkHelpers library:
https://github.com/cajuncoding/SqlBulkHelpers/blob/7770338ad94aa933b57637662be5c2949b898c5a/NetStandard.SqlBulkHelpers/SqlBulkHelpers/QueryProcessing/SqlBulkHelpersMergeQueryBuilder.cs#L91

The easiest way to guarantee that Identity values are ordered correctly is to provide a unique key in the Temp Table that is the ordinal position (INT) of each enumerable entity added into the temp table record at execution time. So each item in the enumerable list would be added into the the temp table with an additional column that denotes it's ordinal position from the enumerable list.

This is done here in a library that's dedicated to Sql Server bulk inserting for identities -- and adding the ordinal position as the data is converted into a DataTable (slightly less efficient, but provides expected behavior) -- also sample from the SqlBulkHelpers library:

https://github.com/cajuncoding/SqlBulkHelpers/blob/7770338ad94aa933b57637662be5c2949b898c5a/NetStandard.SqlBulkHelpers/SqlBulkHelpers/QueryProcessing/SqlBulkHelpersObjectMapper.cs#L67

Schema and Model:

Please share to us the schema of the table (not actual) that could help us replicate the issue if necessary.

/****** Object:  Table [dbo].[SqlBulkHelpersTestElements]    Script Date: 1/3/2021 2:28:17 PM ******/
SET ANSI_NULLS ON
GO

SET QUOTED_IDENTIFIER ON
GO

CREATE TABLE [dbo].[SqlBulkTestEntities](
	[Id] [int] IDENTITY(1,1) NOT NULL,
	[Key] [nvarchar](max) NULL,
	[Value] [nvarchar](max) NULL,
        CONSTRAINT [PK_SqlBulkTestEntities] PRIMARY KEY CLUSTERED ( [Id] ASC ) 
                WITH (STATISTICS_NORECOMPUTE = OFF, IGNORE_DUP_KEY = OFF) ON [PRIMARY]
) ON [PRIMARY] TEXTIMAGE_ON [PRIMARY]
GO

CREATE NONCLUSTERED INDEX IDX_Id_Includes_Value
	ON [dbo].[SqlBulkTestEntities] ([Id])
	INCLUDE (Value);
GO

And also the model that corresponds the schema.

    [Map("SqlBulkTestEntities")]
    public class SqlBulkTestEntity
    {
        public int Id { get; set; }
        public string Key { get; set; }
        public string Value { get; set; }

        public override string ToString()
        {
            return $"Id=[{Id}], Key=[{Key}]";
        }
    }

Library Version:
RepoDb v1.12.4 and RepoDb.SqlServer v1.1.1

But likely affects the later RepoDb v1.12.5 also based on code review....

[mikependon]

Thank you for reporting this. Historically we had analyzed such possibilities (exactly same possibilities) to both the batch and bulk operations, but we had not prove it happens. We had pre-assumed that it will never happened, but unfortunately, it does on your end (for the first time).

We will issue a prioritize fix for this and also to the Batch operations (InsertAll and MergeAll).

Just FYI, Entity Framework Core does the same batch operation, but I personally investigated it already and they are already doing the suggestions you mentioned above in relation to Ordering.

[cajuncoding]

@mikependon yes, I had similarly made the same presumption in my SqlBulkHelpers library which was written specifically for Bulk Insert with elegant support for returning Identity values from Sql Server -- before RepoDb was available :-).

And my team pretty quickly identified the ordering problem in testing it's use in real world projects.... so I had to enhance the library to then support ordinal ordering and enforcing 'Order By' when retrieving the results of the Merge.

This was all before RepoDb was published :-), and we have since started using RepoDb on recent new development -- instead of Dapper -- with great hopes to have a unified library that also supported Bulk Insert/Updates, in addition to other great functionality such as supporting dynamic qualifiers to be specified, expression parsing, built in Model Mapping, etc. -- which I had not implemented.

Unfortunately my team encountered similar (not identical) ordering issues with RepoDb, and I'm just now getting around to reporting the issue. . . therefore to keep the team going I implemented support for qualifiers to be specified as well as a couple other enhancements to SqlBulkHelpers so our project can move forward.

But, it will be great if we can get RepoDb reliability fixed for this issue . . . I'm also concerned for anyone else that may not realize they have a risk of corrupted Identity value data if they aren't aware of this. . . . So some public notification on the Website, chat, etc. may be useful.

[mikependon]

@cajuncoding - I had introduce the initial fix for this, but is only for IEnumerable<T>. The technique is quite similar to what you had introduced and also with EF, see below the logic.

Consider the following if the isReturnIdentity is TRUE:

• Let us say, the User is going to bulk-insert the 100 rows
• RepoDB creates a pseudo-temp table (temp/physical) with column __RepoDb_OrderColumn
• RepoDB convers the data entities to DataEntityDataReader with a extra column named __RepoDb_OrderColumn
• When the WriteToServer() is called, the DataEntityDataReader is identifying the mentioned column __RepoDb_OrderColumn ordering and value.
• During the call to the GetOrdinal(), the typeof(DataEntity).GetProperties().Count + 1 is returned, faking the column index position
• During the actual call to the GetValue() method, the POSITION INDEX of the entity from the enumerable is returned. IMPORTANT
• The values are bulk-inserted into the database with an extra column named __RepoDb_OrderColumn where the values are the INDEX POSITION of the entity from the list.
• The library is using INSERT INTO instead of MERGE INTO. This can be modified of-course if necessary. In this SQL, what the library is doing is below:

INSERT INTO OrigTable (...)
OUTPUT INSERTED.ID
SELECT ... FROM PseudoTempTable
ORDER BY __RepoDb_OrderColumn ASC;

Notice, the __RepoDb_OrderColumn is not a part of the resultset back to the client, therefore, we do not have an extra logic to validate the INDEX POSITION, however, you also notice that we had called the ORDER BY keyword using such column that guarantees insertion process.

I had one question, in case you already investigated it. As I cannot return the __RepoDb_OrderColumn column using INSERT INTO, but I can return it via MERGE INTO.

Is it still necesarry to return to the client to add more extra validation on the position index?

Note: The logic change is not difficult, however, it would affect the performance a little if we enforce the MERGE INTO in the BulkInsert. Such keyword is only used in BulkMerge 😸 . Your thoughts!

[mikependon]

Linking the fix of #697

[mikependon]

@cajuncoding - ping?

[cajuncoding]

@mikependon yeah this is conceptually the same process, and sounds good.

TL;DR: I think this is great!

Its not clear in your post if you are using Output...Into to output the identity values back into the pseudo temp table? I assume you are though based on the final retrieval query which must implement the ORDER BY __RepoDb_OrderColumn ASC;.

I figured you would be able to implement this, as you have along with keeping the efficiency gained with the use of DataEntityDataReader, nice!

In my opinion, as long as the user can rely on documented behavior that all entities remain in the same order along with correct identity values being set then that's all they can/should be concerned with... :-)

I can't think of any valid uses for the client to need the position returned...so I agree, returning the ordinal does not seem like it would add any value. I'm not sure there would be performance impact between Insert vs Merge, but I have read that there are some odd edge case issues (bugs) with Merge across Sql Server versions so leaving it as an Insert is likely the most reliable way to go 😄

[mikependon]

@cajuncoding - thanks for the comment and cheers!

Its not clear in your post if you are using Output...Into to output the identity values back into the pseudo temp table?

When we do push the data from the temp pseudo table to actual table, the newly generated ID is committed into that table. The answer to this is NO, I only used OUTPUT INSERTED.<IdentityId> to return the newly generated identities as query result. In the client, RepoDB is using the ExecuteReader to get the instance of DbDataReader and iterate the result, then, set back the value of the identities into the entities. You can see it here and here.

[mikependon]

My interest about returning the __RepoDb_OrderColumn is when I am extracting the data from here. The ordinal 0 is always the OUTPUT INSERTED.<IdentityId> and the ordinal 1 is always order column. Then, I will pass the value of the ordinal 1 when retrieving the index from the list in the line 57 before the actual call to the compiled expression.

But as you said, as per your investigation, this is not needed anymore 😄, many thanks to this clarity.