Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

[release/3.1] Fix driver behavior of sending Attention signal for successful Bulk Copy operation #42617

Merged
merged 1 commit into from
Jan 14, 2020

Conversation

cheenamalhotra
Copy link
Member

@cheenamalhotra cheenamalhotra commented Nov 15, 2019

Port of: dotnet/runtime#61 and dotnet/SqlClient#308

Summary

Currently when performing Bulk Copy operations with SqlClient driver, on successful completion, an Attention Signal is sent to SQL Server which is not correct behavior and should not happen.

Customer Impact

Reported by a .NET Framework customer.

Customers wouldn't notice these attention signals being sent and the transactions would only get impacted if the Abort bit is checked during transaction prepare/commit time. This case would be easily hit with HK tables when the transaction inserts a large number of rows, hence causing all the LCs to be filled up and LC flush IO to fall behind LCs being requested for log serialization at transaction prepare time.

I would vote for getting the fix in 2.1 as this would result in unsuccessful Bulk Copy transactions for customers even though no error has occurred on their end as this was incorrect behavior by design.

Also to note, it's very likely to occur if customers are using Hekaton/memory-optimized tables. Log serialization in HK is done right at the end during the Hekaton transaction “preparation” phase, prior to commit. This implies that all required log records are generated/serialized into Log Cache at transaction prepare time. As a result, if there are a large number of log records to serialize, there could be a situation where Log Cache get filled up and need to be flushed while more log records are waiting to be serialized into Cache. However, if LC flush IO cannot keep up with the rate of LCs being requested for remaining log records, SQLServerLogMgr::WaitFlushOldestLC() is called, which will essentially wait for a free Log Cache. This function when executed also checks whether the task has an Abort bit set (which is set when the server receives the Attention packet), which ends up aborting the transaction commit itself.

Regression?

No. The behavior is the original design ported from .NET Framework.

Testing

When performing tests, the transactions would always commit successfully, hence this attention signal was not noticed. A special test case will be investigated and designed using HK tables in dotnet/sqlclient test lab to cover this scenario. Since all new changes will be made in dotnet/sqlclient repository, we'll be adding new tests there in future.

Risk

Low: The fix is to not send attention signal when performing cleanups post Bulk Copy in successful scenarios.

cc: @danmosemsft @David-Engel @saurabh500

@cheenamalhotra cheenamalhotra added the Servicing-consider Issue for next servicing release review label Nov 15, 2019
@leecow
Copy link
Member

leecow commented Nov 21, 2019

Are the active customer issues related to this issue or is this an internal find? If there are customer reports, we will reconsider Tuesday. If the fix is accepted, it will be for 3.1 only (unless there is a 2.1 customer that has gone through CSS).

@cheenamalhotra
Copy link
Member Author

No active customer issues are waiting for this fix in .NET Core.

The support issue we received was from a customer using .NET Framework, we're proposing fix here to address the design flaw and safeguard any potential issues in future.

@danmoseley
Copy link
Member

Based on the above I'm going to close the 2.1 PR #42619 We can reactivate and reconsider if it meets that bar described.

@danmoseley
Copy link
Member

@ajcvickers do you have any other comments/concerns for this one.

@leecow leecow added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Nov 26, 2019
@leecow leecow added this to the 3.1.2 milestone Nov 26, 2019
@danmoseley
Copy link
Member

@Anipik please merge when ready for 3.1.2 changes.

@vivmishra
Copy link

Moved to Mar as per Tactics. Will need to be explicitly approved for Feb, if required.

@vivmishra vivmishra modified the milestones: 3.1.2, 3.1.3 Jan 9, 2020
@danmoseley danmoseley modified the milestones: 3.1.3, 3.1.2 Jan 14, 2020
@danmoseley
Copy link
Member

Moved back to 3.1.2 per tactics.

@Anipik Anipik merged commit 1ba6a19 into dotnet:release/3.1 Jan 14, 2020
@cheenamalhotra cheenamalhotra deleted the port-PR308-SqlClient-3.1 branch January 14, 2020 19:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants