Add | Feature: SqlBatch api #1825
Conversation
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlBatch.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/ref/Microsoft.Data.SqlClient.Batch.cs
Outdated
Show resolved
Hide resolved
|
Something is wrong with the CI builds. Even if the netfx ref build can't find IAsyncEnumerable it should still work for net6.0 builds because with net6.0 it's in-box. But all builds are failing. That implies that the net6.0 builds aren't targetting net6.0 somehow. Local clean+build+build-tests works. I also don't understand how the net462 src project is locating IAsyncEnumerable and Task. They're used in the code and the netfx src build works, but ref build doesn't and yet there is no reference to the location that VS says task is being imported from in src. If it's a transitive dependency I can't see the parent. |
Wraith2
force-pushed
the
api-batching-net6
branch
from
6676e23
to
78d46b7
Compare
|
@JRahnama I'm going to pick on you since you were involved in setting up the net6 pipelines. and notice that it's from the you can see that it's doing a netstandard 2.0 build of the ref assembly. It's doing this on every leg. How do i replicate this build configuration locally? the build docs only list netfx and netcore targets. how do i target netstandard2.0? |
|
@Wraith2 I will provide full info on how to build and run the tests as the pipeline does, but I will do that on Monday if you don't mind. I had a very busy day setting up Kerberos testing machine on azure and it just took all my time. |
|
Ok, have a good weekend. |
|
@Wraith2 sorry for the delay: here is the process:
This step builds the driver and publishes the artifacts. This is to make sure everything works fine, and builds are done with no issues.
The next steps when it comes to testing with Sql server 2019 and windows:
For Unix:
There are different type of test for Package. Let me know if you need them as well. |
|
The command: That isn't the error that I'm seeing in the CI but it may have a similar cause. Do you have any idea how to resolve the problems? I don't see a way to get the builds containing the correct items and also work with the combinations of references used. |
|
@Wraith2 are you able to build locally? I am able to do so. |
|
If you can address the conflict, I will spend tomorrow morning to fix the pipeline issue, if there is any. |
Wraith2
force-pushed
the
api-batching-net6
branch
from
78d46b7
to
7e38ed0
Compare
|
No, I can't build locally anymore. The dependencies aren't right somewhere. I've added what I need to the Versions.props file but I'm getting a conflict with two other versions of the same assembly and I can't see either of them being referenced. Code changes I'll fight with for hours but msbuild is something else entirely. I'd be very grateful if you could see if you can untangle whatever dependency mess is going on here. |
this is weird. I just bult your branch successfully. On CLI and on VS 2022. msbuild version:
can you confirm you have same MSBuild? |
Same Try it in visual studio. Also notice that none of the CI legs can build it now. Just running basic build steps work but when you get to running tests (not compiling) you'll find it doesn't work because it's using a netstandard 2.1 ref which can't work on net6 because both contain SqlBatch implementations. |
Wraith2
force-pushed
the
api-batching-net6
branch
from
7e38ed0
to
b67e2c6
Compare
|
Rebased and pushed, lets see what the CI thinks. |
DavoudEshtehari
requested review from
David-Engel,
JRahnama,
DavoudEshtehari and
Kaur-Parminder
|
@David-Engel could I get some eyes on this please? As far as I know the only problem is that the CI is trying to use a ns2.1 ref assembly inappropriately which causes a type to be multiply defined. |
|
/azp run CI-SqlClient |
|
Pull request contains merge conflicts. |
|
Given that netconf and the release of net8 and the likely release of the accompanying ef core version are next week it feels like this isn't going to get merged and released in time. Disappointed is too small and simple a word to convey how I feel about this. |
|
Just as an FYI, EF Core 8.0 is a long-term support release, and therefore can only take a dependency on a SqlClient LTS release as well - AFAIK as I know SqlClient 5.2 won't be one (since 5.1 was LTS). So EF 8.0 wouldn't be able to take a dependency on SqlClient 5.2 (unless I'm getting the SqlClient LTS/STS wrong here). Beyond that, EF itself currently doesn't yet make use of the DbBatch - that's very much in the plan, but is a non-trivial refactor (follow dotnet/efcore#18990). As more ADO.NET providers implement the API, it would become more valuable to do this in EF - so I'm definitely looking forward to seeing this PR merged in SqlClient! |
|
@Wraith2 We definitely appreciate all the work you have done on this. |
David-Engel
dismissed
cheenamalhotra’s stale review
Requested changes have been addressed.
Ditto on the appreciation. Merged now. Was just waiting for that second reviewer. Team has been under extra pressure that has pushed several non-critical things out. The good news is that we have a couple of new people onboarding that I hope will improve things after they've ramped up. |
|
Thanks @Wraith2! Super happy to see this, looking forward to playing around with it once the release is out! |
|
Woohoo on the merge! Any ETA on binaries? I'm literally writing Dapper's batch support right now - would love to be able to smoke test this |
|
@mgravell 5.2 preview4 should be in November... |
Actually, it is planned for sometime in first week of December if everything goes as planned. |
|
@JRahnama This year hopefully 😅 |
|
When is 5.2 RTM supposed to be happening? How many more previews are they likely to be? |
|
@Wraith2 I saw January 2024 mentioned somewhere |
Preview 4 should be last preview and GA version is somewhere in the new year as @ErikEJ mentioned. |
|
@Wraith2 FYI |
|
FYI, some numbers from Dapper (after adding some local hackery re #2222): | Method | Categories | Count | IsOpen | Mean | Error | StdDev | Ratio | RatioSD | Gen0 | Gen1 | Allocated | Alloc Ratio |
|------------------------------ |---------------- |------ |------- |-----------:|-----------:|----------:|------:|--------:|---------:|---------:|----------:|------------:|
| DapperAsync | Async,SqlClient | 1000 | True | 120.845 ms | 23.8903 ms | 1.3095 ms | 1.00 | 0.01 | - | - | 1609750 B | 1.41 |
| DapperAotAsync_BatchNone | Async,SqlClient | 1000 | True | 116.517 ms | 30.0250 ms | 1.6458 ms | 0.96 | 0.01 | - | - | 1145102 B | 1.00 |
| DapperAotAsync_BatchAll | Async,SqlClient | 1000 | True | 73.106 ms | 5.4160 ms | 0.2969 ms | 0.60 | 0.01 | - | - | 1221267 B | 1.07 |
| DapperAotAsync_Batch32 | Async,SqlClient | 1000 | True | 73.775 ms | 17.7965 ms | 0.9755 ms | 0.61 | 0.02 | - | - | 1250546 B | 1.09 |
| DapperAot_ManualAsync | Async,SqlClient | 1000 | True | 72.290 ms | 9.7130 ms | 0.5324 ms | 0.60 | 0.01 | - | - | 1218281 B | 1.06 |
| DapperAot_ManualPreparedAsync | Async,SqlClient | 1000 | True | 72.019 ms | 30.2447 ms | 1.6578 ms | 0.60 | 0.02 | - | - | 1220233 B | 1.07 |
| ManualAsync | Async,SqlClient | 1000 | True | 120.889 ms | 38.3381 ms | 2.1014 ms | 1.00 | 0.00 | - | - | 1144504 B | 1.00 |
| EntityFrameworkAsync | Async,SqlClient | 1000 | True | 6.981 ms | 1.8516 ms | 0.1015 ms | 0.06 | 0.00 | 320.3125 | 234.3750 | 5386977 B | 4.71 |
| SqlBulkCopyFastMemberAsync | Async,SqlClient | 1000 | True | 2.856 ms | 0.1298 ms | 0.0071 ms | 0.02 | 0.00 | 31.2500 | 31.2500 | 557015 B | 0.49 |
| | | | | | | | | | | | | |
| Dapper | Sync,SqlClient | 1000 | True | 126.079 ms | 28.5030 ms | 1.5623 ms | 1.01 | 0.01 | - | - | 465309 B | 1,817.61 |
| DapperAot_BatchNone | Sync,SqlClient | 1000 | True | 118.006 ms | 30.8140 ms | 1.6890 ms | 0.94 | 0.03 | - | - | 709 B | 2.77 |
| DapperAot_BatchAll | Sync,SqlClient | 1000 | True | 70.783 ms | 28.0035 ms | 1.5350 ms | 0.57 | 0.02 | - | - | 1191080 B | 4,652.66 |
| DapperAot_Batch32 | Sync,SqlClient | 1000 | True | 75.743 ms | 7.9881 ms | 0.4379 ms | 0.61 | 0.01 | - | - | 858882 B | 3,355.01 |
| DapperAotManual | Sync,SqlClient | 1000 | True | 74.230 ms | 22.6914 ms | 1.2438 ms | 0.59 | 0.02 | - | - | 1191080 B | 4,652.66 |
| DapperAot_PreparedManual | Sync,SqlClient | 1000 | True | 75.121 ms | 17.0236 ms | 0.9331 ms | 0.60 | 0.02 | - | - | 1191098 B | 4,652.73 |
| Manual | Sync,SqlClient | 1000 | True | 125.183 ms | 35.4165 ms | 1.9413 ms | 1.00 | 0.00 | - | - | 256 B | 1.00 |
| EntityFramework | Sync,SqlClient | 1000 | True | 6.970 ms | 3.3219 ms | 0.1821 ms | 0.06 | 0.00 | 312.5000 | 187.5000 | 5325728 B | 20,803.62 |
| SqlBulkCopyFastMember | Sync,SqlClient | 1000 | True | 1.863 ms | 0.1745 ms | 0.0096 ms | 0.01 | 0.00 | - | - | 13672 B | 53.41 |
| SqlBulkCopyDapper | Sync,SqlClient | 1000 | True | 1.839 ms | 0.4902 ms | 0.0269 ms | 0.01 | 0.00 | - | - | 7942 B | 31.02 |what I'm looking at here is the
analysis:
great job, @Wraith2 ! |
|
top allocs:
the parameters are not mine; they're internal (BatchState.Execute is after the commands have all been populated this method is basically
ditto the string builders:
the strings ... some of these seem to be from uint parsing? maybe that can be optimized pretty easily
basically, assume that everything of note is "external call" (i.e. SqlClient) underneath |
|
Parameters are very heavy and are always going to be a big source of allocations for anything in this api. Even if the user doesn't provide any the internal method of sending rpc uses several of them each time. It's something I've looked into many times to see if I can reduce the size of usage but made little difference on. If you can upload the benchmark projects somewhere once you're dong I'll do a perf focussed pass over them and see what I can reduce. |
|
Just throwing out a seed for thought, parameters as a collection can be thought of as a solution where user can pass an array of parameters together with statement. Something that ODBC driver supports (ref. Array of Parameters), but SqlClient doesn't yet. While you're focusing on perf improvements, this can be looked further into as well :) |
Implements the core of the SqlBatch api re: #19
Implements batching and some tests to cover the basic functionality provided.
Does not implement Always Encrypted for batches. AE prepares statements by roundtripping to the server to get encryption information so there is no way to send them in a single statement unless the entire batch were prepared. I think this scenario needs some discussion about how it should be implemented with knowledge of how batching will work because an implementation is attempted.
We could really do with more tests. Anyone who is interested in this feature please feel free to work up any test scenarios you can and try them to see what the sharp edges may be and if any modifications are required.
[Edit]
There is no documentation added. If someone is going to suffer through producing documentation for these new apis they should at least be getting paid for it so I'll leave that to the MS team.