Feature/31 readcommitedlock #39
Conversation
improve MsSQL transaction tests (come scenario might pass)
| - The fix for [#487](https://github.com/NEventStore/NEventStore/issues/487) changed how the `Commits` table is created for MySql 8.x: | ||
| to update an existing database in order to run on 8.x you need to manually update the `Commits` table schema and change the constraint of the `CommitId` column | ||
| from: `CommitId binary(16) NOT NULL CHECK (CommitId != 0)` to: `CommitId binary(16) NOT NULL CHECK (CommitId <> 0x00)`. | ||
| - MsSqlDialects now have an `useAzureSql` parameter, if set to `true` the statement `WITH (READCOMMITTEDLOCK)` will be added to any `FROM Commits` query. |
There was a problem hiding this comment.
I think this could even be made a default. It does no harm outside of Azure SQL. In fact, it will also make NEventStore more robust with local SQL Server, too, when someone enabled the READ COMMITTED SNAPSHOT option on the database.
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for READCOMMITTEDLOCK in MS SQL Server to improve compatibility with Azure SQL and READ COMMITTED SNAPSHOT isolation. The changes include version upgrades from .NET Framework 4.6.1 to 4.6.2, package updates, and fixes for MySQL 8.x constraint issues.
- Enhanced MS SQL dialect with optional READCOMMITTEDLOCK support for Azure SQL compatibility
- Updated target frameworks and package versions across all projects
- Fixed MySQL constraint issue for version 8.x compatibility
Reviewed Changes
Copilot reviewed 27 out of 28 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| MsSqlDialect.cs | Added constructor parameter and logic to inject READCOMMITTEDLOCK hints into SQL queries |
| Multiple .csproj files | Updated target frameworks from net461 to net462 and upgraded package versions |
| MySqlStatements.resx | Fixed constraint syntax for MySQL 8.x compatibility |
| Multiple test files | Updated conditional compilation directives and test method naming |
| Docker compose files | Updated MySQL image version from 5.7 to 8.0 |
Files not reviewed (1)
- src/NEventStore.Persistence.Sql/SqlDialects/MySqlStatements.Designer.cs: Language not supported
|
|
||
| string value = MsSqlStatements.PagedQueryFormat.FormatWith(select, orderBy, from); | ||
| return value; | ||
| return MsSqlStatements.PagedQueryFormat.FormatWith(select, orderBy, from); |
There was a problem hiding this comment.
[nitpick] The removal of the intermediate 'value' variable makes the code more concise, but consider keeping the variable for better debugging experience when troubleshooting SQL query issues.
| return MsSqlStatements.PagedQueryFormat.FormatWith(select, orderBy, from); | |
| var value = MsSqlStatements.PagedQueryFormat.FormatWith(select, orderBy, from); | |
| return value; |
MsSql: add support for READCOMMITTEDLOCK
still need to improve tests cause some of them involving multiple transaction will succeed and unsupported scenarios might become supported when using READCOMMITTEDLOCK.
but we need to refactor again tests initialization heavily.
we also need to cherry-pick commits becase this branch started from a release.