mssql improvements, code clean up and fix issue 428 #431
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alexeyzimarev
changed the title
alexeyzimarev
requested changes
Sep 27, 2025
nmummau
force-pushed
the
feature/my-change
branch
2 times, most recently
from
2ace9de to
bad1390
Compare
Changelist by BitoThis pull request implements the following key changes.
|
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #29d3df
Actionable Suggestions - 3
-
src/SqlServer/src/Eventuous.SqlServer/Scripts/1_Schema.sql - 2
- Breaking schema change · Line 28-30
- Breaking NOT NULL constraint · Line 30-30
-
src/SqlServer/src/Eventuous.SqlServer/Scripts/6_ReadStreamForwards.sql - 1
- Wrong message ordering · Line 40-40
Review Details
-
Files reviewed - 8 · Commit Range:
67cf323..bad1390- src/SqlServer/src/Eventuous.SqlServer/Scripts/1_Schema.sql
- src/SqlServer/src/Eventuous.SqlServer/Scripts/2_AppendEvents.sql
- src/SqlServer/src/Eventuous.SqlServer/Scripts/3_CheckStream.sql
- src/SqlServer/src/Eventuous.SqlServer/Scripts/4_ReadAllForwards.sql
- src/SqlServer/src/Eventuous.SqlServer/Scripts/5_ReadStreamBackwards.sql
- src/SqlServer/src/Eventuous.SqlServer/Scripts/6_ReadStreamForwards.sql
- src/SqlServer/src/Eventuous.SqlServer/Scripts/7_ReadStreamSub.sql
- src/SqlServer/src/Eventuous.SqlServer/Scripts/8_TruncateStream.sql
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at [email protected].
Documentation & Help
AI Code Review powered by
src/SqlServer/src/Eventuous.SqlServer/Scripts/6_ReadStreamForwards.sql
Outdated
Show resolved
Hide resolved
Test Results 32 files + 16 32 suites +16 1h 0m 32s ⏱️ + 30m 38s Results for commit aec84c8. ± Comparison against base commit f4444af. This pull request removes 1 and adds 2 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
nmummau
force-pushed
the
feature/my-change
branch
5 times, most recently
from
9f4b09e to
83e0f4f
Compare
nmummau
force-pushed
the
feature/my-change
branch
from
72826c2 to
49dddfe
Compare
- SET NOCOUNT ON - Wrap keywords in brackets ([Version], [Messages]) - formatting: each column on it's own line. Best practice for maintainability of procedures - proper casing of keywords (INT, AND) - use alias to DELETE statement - semicolons
nmummau
force-pushed
the
feature/my-change
branch
from
49dddfe to
69ec7ac
Compare
|
/review |
There was a problem hiding this comment.
Code Review Agent Run #f05148
Actionable Suggestions - 4
-
src/SqlServer/src/Eventuous.SqlServer/Scripts/1_Schema.sql - 3
- Missing DEFAULT for Created · Line 30-30
- Missing DEFAULT for Created · Line 30-30
- Type compatibility issue · Line 58-59
-
src/SqlServer/src/Eventuous.SqlServer/Scripts/7_ReadStreamSub.sql - 1
- SQL syntax error · Line 11-11
Additional Suggestions - 1
-
src/SqlServer/src/Eventuous.SqlServer/Scripts/5_ReadStreamBackwards.sql - 1
-
Inconsistent table naming · Line 37-37The Messages table reference uses square brackets (__schema__.[Messages]) which creates inconsistency with the schema placeholder pattern used throughout the codebase. The __schema__ placeholder appears without brackets in __schema__.Streams and should follow the same pattern. This affects downstream consumers like `SqlServerStore.GetReadBackwardsCommand` which expects consistent naming conventions.
Code suggestion
@@ -37,1 +37,1 @@ - FROM __schema__.[Messages] + FROM __schema__.Messages
-
Review Details
-
Files reviewed - 9 · Commit Range:
10f371d..69ec7ac- src/Postgres/src/Eventuous.Postgresql/Scripts/6_ReadStreamForwards.sql
- src/SqlServer/src/Eventuous.SqlServer/Scripts/1_Schema.sql
- src/SqlServer/src/Eventuous.SqlServer/Scripts/2_AppendEvents.sql
- src/SqlServer/src/Eventuous.SqlServer/Scripts/3_CheckStream.sql
- src/SqlServer/src/Eventuous.SqlServer/Scripts/4_ReadAllForwards.sql
- src/SqlServer/src/Eventuous.SqlServer/Scripts/5_ReadStreamBackwards.sql
- src/SqlServer/src/Eventuous.SqlServer/Scripts/6_ReadStreamForwards.sql
- src/SqlServer/src/Eventuous.SqlServer/Scripts/7_ReadStreamSub.sql
- src/SqlServer/src/Eventuous.SqlServer/Scripts/8_TruncateStream.sql
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at [email protected].
Documentation & Help
AI Code Review powered by
src/SqlServer/src/Eventuous.SqlServer/Scripts/7_ReadStreamSub.sql
Outdated
Show resolved
Hide resolved
- SET NOCOUNT ON - Wrap keywords in brackets ([Messages]) - formatting: each column on it's own line. Best practice for maintainability of procedures - proper casing of keywords (INT, BIGINT) - remove 'Messages.' from query since it was redundant - semicolons
- SET NOCOUNT ON - Wrap keywords in brackets ([Version], [Messages]) - formatting: each column on it's own line. Best practice for maintainability of procedures - proper casing of keywords (INT) - remove 'Messages.' from query since it was redundant - semicolons
- SET NOCOUNT ON - Wrap keywords in brackets ([Version], [Messages]) - formatting: each column on it's own line. Best practice for maintainability of procedures - proper casing of keywords (INT) - remove 'Messages.' from query since it was redundant - semicolons
- SET NOCOUNT ON - formatting: each column on it's own line. Best practice for maintainability of procedures - use aliases - semicolons
- SET NOCOUNT ON - formatting: each column on it's own line. Best practice for maintainability of procedures - proper casing of keywords (AND) - semicolons
- SET NOCOUNT ON - remove unused declared variables - specify all parameters when calling [check_stream] procedure - formatting: each column on it's own line. Best practice for maintainability of procedures - proper casing of keywords (IF, ISNULL, NVARCHAR) - use alias to UPDATE statement - remove 'AS' keyword - semicolons
- consistent casing of [Version] column - explicitly specify NULLable columns
SQL Server schema change so that Messages.Created is NOT NULL. This is safe and accurate to do since append_events will always set this value if a NULL is provided to the procedure.
- Messages TABLE
- JsonMetaData is NOT NULL
- json columns are checked to ensure they contain json data
- StreamMessage TYPE
- json_metadata is NOT NULL
…hat in the scripts
…move redundencies in the CHECK definition
nmummau
force-pushed
the
feature/my-change
branch
4 times, most recently
from
7184881 to
9426516
Compare
At first when writing this change I assumed I would be out to fix issue Eventuous#429. Turns out that is not an issue as the client wraps this procedure call in a transaction. Thus, no transction is needed within the procedure itself. Here are some improvements, nonetheless. - Changed @stream_name parameter to NVARCHAR(850) for consistency - Introduced @Inserted table to capture inserted GlobalPosition values atomically - Normalized optimistic concurrency errors (2627, 2601) to client-detectable messages - Thrown error messages still StartsWith 'WrongExpectedVersion' but now also includes the full SQL error text for client tracing - Removed redundant post-commit queries and error-parsing logic
…treams and Messages - Added WITH (OPTIMIZE_FOR_SEQUENTIAL_KEY = ON) to clustered primary keys on __schema__.Streams (PK_Streams) and __schema__.Messages (PK_Events) - Improves insert throughput and reduces latch contention on identity-based keys - No changes to schema shape, constraints, or indexes beyond PK optimization This improves concurrent insert performance under high write workloads by allowing SQL Server to better handle last-page insert contention on sequential identity keys. See docs on OPTIMIZE_FOR_SEQUENTIAL_KEY: https://learn.microsoft.com/en-us/sql/t-sql/statements/alter-table-index-option-transact-sql?view=sql-server-ver17#optimize_for_sequential_key---on--off-
…ntuous Added SET XACT_ABORT ON to all SQL stored procedures invoked through GetStoredProcCommand() to ensure atomic rollback behavior when the client transaction encounters an error. This prevents partial commits and inconsistent transactional states during event append and stream operations. Related to issue Eventuous#429
nmummau
force-pushed
the
feature/my-change
branch
from
6cc3ba9 to
1599c02
Compare
|
/review |
There was a problem hiding this comment.
Code Review Agent Run #664c60
Actionable Suggestions - 5
-
src/SqlServer/src/Eventuous.SqlServer/Scripts/6_ReadStreamForwards.sql - 1
- Incorrect ordering clause · Line 41-41
-
src/SqlServer/src/Eventuous.SqlServer/Scripts/2_AppendEvents.sql - 2
- Arithmetic overflow risk · Line 56-56
- Race condition in UPDATE · Line 86-90
-
src/SqlServer/src/Eventuous.SqlServer/Scripts/1_Schema.sql - 1
- SQL Server version compatibility · Line 36-37
-
src/SqlServer/src/Eventuous.SqlServer/Scripts/5_ReadStreamBackwards.sql - 1
- Wrong boundary condition · Line 25-28
Review Details
-
Files reviewed - 9 · Commit Range:
9e2b609..1599c02- src/Postgres/src/Eventuous.Postgresql/Scripts/6_ReadStreamForwards.sql
- src/SqlServer/src/Eventuous.SqlServer/Scripts/1_Schema.sql
- src/SqlServer/src/Eventuous.SqlServer/Scripts/2_AppendEvents.sql
- src/SqlServer/src/Eventuous.SqlServer/Scripts/3_CheckStream.sql
- src/SqlServer/src/Eventuous.SqlServer/Scripts/4_ReadAllForwards.sql
- src/SqlServer/src/Eventuous.SqlServer/Scripts/5_ReadStreamBackwards.sql
- src/SqlServer/src/Eventuous.SqlServer/Scripts/6_ReadStreamForwards.sql
- src/SqlServer/src/Eventuous.SqlServer/Scripts/7_ReadStreamSub.sql
- src/SqlServer/src/Eventuous.SqlServer/Scripts/8_TruncateStream.sql
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at [email protected].
Documentation & Help
AI Code Review powered by
src/SqlServer/src/Eventuous.SqlServer/Scripts/2_AppendEvents.sql
Outdated
Show resolved
Hide resolved
src/SqlServer/src/Eventuous.SqlServer/Scripts/5_ReadStreamBackwards.sql
Outdated
Show resolved
Hide resolved
Code Review Agent Run #0f1763Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Code Review Agent Run #07cb94Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Interaction Diagram by BitosequenceDiagram
participant App as Application
participant Store as EventStore<br/>(PostgresStore/SqlServerStore)
participant Schema as Schema<br/>(MODIFIED)
participant DB as Database<br/>(Postgres/SqlServer)
participant Scripts as SQL Scripts<br/>(MODIFIED)
Note over Scripts: Enhanced ordering, constraints<br/>& concurrency control
App->>Store: ReadEvents/AppendEvents
Store->>Schema: Get SQL command
Schema->>Scripts: Execute stored procedure
Scripts->>DB: Query with improved logic
DB-->>Scripts: Results
Scripts-->>Schema: Response
Schema-->>Store: Data
Store-->>App: Events/AppendResult
Critical path: Application->EventStore->Schema (MODIFIED)->SQL Scripts (MODIFIED)->Database
|
Code Review Agent Run #aa2f1aActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
nmummau
force-pushed
the
feature/my-change
branch
from
fd68a88 to
8add56e
Compare
…g the error message
… querying the table for the record we just inserted
Replaced invalid guard `@current_version < @from_position + @count` with proper range validation `@from_position < 0 OR @from_position > @current_version`. Previous logic incorrectly returned no results when valid messages existed. New check correctly validates start position independently of page size.
nmummau
force-pushed
the
feature/my-change
branch
from
8add56e to
73c56ce
Compare
and added `@@ROWCOUNT` check to detect concurrent updates. If no rows are affected, the procedure now throws a `WrongExpectedVersion` error, aligning behavior with duplicate append handling. - Prevents silent version overwrite on race conditions - Ensures consistency with optimistic concurrency semantics - Keeps existing WrongExpectedVersion error format for client handling
|
/review |
Code Review Agent Run #a7a288Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
There was a problem hiding this comment.
Code Review Agent Run #66ce0b
Actionable Suggestions - 2
-
src/SqlServer/src/Eventuous.SqlServer/Scripts/2_AppendEvents.sql - 1
- Incorrect position for zero-message appends · Line 102-106
-
src/SqlServer/src/Eventuous.SqlServer/Scripts/1_Schema.sql - 1
- Incompatible SQL Server feature · Line 31-31
Review Details
-
Files reviewed - 9 · Commit Range:
9e2b609..aec84c8- src/Postgres/src/Eventuous.Postgresql/Scripts/6_ReadStreamForwards.sql
- src/SqlServer/src/Eventuous.SqlServer/Scripts/1_Schema.sql
- src/SqlServer/src/Eventuous.SqlServer/Scripts/2_AppendEvents.sql
- src/SqlServer/src/Eventuous.SqlServer/Scripts/3_CheckStream.sql
- src/SqlServer/src/Eventuous.SqlServer/Scripts/4_ReadAllForwards.sql
- src/SqlServer/src/Eventuous.SqlServer/Scripts/5_ReadStreamBackwards.sql
- src/SqlServer/src/Eventuous.SqlServer/Scripts/6_ReadStreamForwards.sql
- src/SqlServer/src/Eventuous.SqlServer/Scripts/7_ReadStreamSub.sql
- src/SqlServer/src/Eventuous.SqlServer/Scripts/8_TruncateStream.sql
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at [email protected].
Documentation & Help
AI Code Review powered by
alexeyzimarev
approved these changes
Oct 18, 2025
nmummau
added a commit
to nmummau/eventuous
that referenced
this pull request
Oct 22, 2025
* feat(mssql): Minor SQL changes; no functional changes
- SET NOCOUNT ON
- Wrap keywords in brackets ([Version], [Messages])
- formatting: each column on it's own line. Best practice for maintainability of procedures
- proper casing of keywords (INT, AND)
- use alias to DELETE statement
- semicolons
* feat(mssql): Minor SQL changes; no functional changes
- SET NOCOUNT ON
- Wrap keywords in brackets ([Messages])
- formatting: each column on it's own line. Best practice for maintainability of procedures
- proper casing of keywords (INT, BIGINT)
- remove 'Messages.' from query since it was redundant
- semicolons
* feat(mssql): Minor SQL changes; no functional changes
- SET NOCOUNT ON
- Wrap keywords in brackets ([Version], [Messages])
- formatting: each column on it's own line. Best practice for maintainability of procedures
- proper casing of keywords (INT)
- remove 'Messages.' from query since it was redundant
- semicolons
* feat(mssql): Minor SQL changes; no functional changes
- SET NOCOUNT ON
- Wrap keywords in brackets ([Version], [Messages])
- formatting: each column on it's own line. Best practice for maintainability of procedures
- proper casing of keywords (INT)
- remove 'Messages.' from query since it was redundant
- semicolons
* feat(mssql): Minor SQL changes; no functional changes
- SET NOCOUNT ON
- formatting: each column on it's own line. Best practice for maintainability of procedures
- use aliases
- semicolons
* feat(mssql): Minor SQL changes; no functional changes
- SET NOCOUNT ON
- formatting: each column on it's own line. Best practice for maintainability of procedures
- proper casing of keywords (AND)
- semicolons
* feat(mssql): Minor SQL changes; no functional changes
- SET NOCOUNT ON
- remove unused declared variables
- specify all parameters when calling [check_stream] procedure
- formatting: each column on it's own line. Best practice for maintainability of procedures
- proper casing of keywords (IF, ISNULL, NVARCHAR)
- use alias to UPDATE statement
- remove 'AS' keyword
- semicolons
* feat(mssql): Minor SQL updates
- consistent casing of [Version] column
- explicitly specify NULLable columns
* fix(mssql): Messages.Created NOT NULL
SQL Server schema change so that Messages.Created is NOT NULL. This is safe and accurate to do since append_events will always set this value if a NULL is provided to the procedure.
* feat(mssql): json is not null
- Messages TABLE
- JsonMetaData is NOT NULL
- json columns are checked to ensure they contain json data
- StreamMessage TYPE
- json_metadata is NOT NULL
* feat(mssql): DATETIME2 is actually a DATETIME2(7) so explicitly say that in the scripts
* feat(mssql): ORDER BY StreamPosition per bito-code-review, and and remove redundencies in the CHECK definition
* feat(postgresql): ORDER BY stream_position per bito-code-review
* refactor(mssql): remove square brackets to satisfy 'inconsistent table naming' warning that bito-code-review flagged
* feat(mssql): append_events improve
At first when writing this change I assumed I would be out to fix issue Eventuous#429. Turns out that is not an issue as the client wraps this procedure call in a transaction. Thus, no transction is needed within the procedure itself. Here are some improvements, nonetheless.
- Changed @stream_name parameter to NVARCHAR(850) for consistency
- Introduced @Inserted table to capture inserted GlobalPosition values atomically
- Normalized optimistic concurrency errors (2627, 2601) to client-detectable messages
- Thrown error messages still StartsWith 'WrongExpectedVersion' but now also includes the full SQL error text for client tracing
- Removed redundant post-commit queries and error-parsing logic
* perf(mssql): enable OPTIMIZE_FOR_SEQUENTIAL_KEY on primary keys for Streams and Messages
- Added WITH (OPTIMIZE_FOR_SEQUENTIAL_KEY = ON) to clustered primary keys on __schema__.Streams (PK_Streams) and __schema__.Messages (PK_Events)
- Improves insert throughput and reduces latch contention on identity-based keys
- No changes to schema shape, constraints, or indexes beyond PK optimization
This improves concurrent insert performance under high write workloads by allowing SQL Server to better handle last-page insert contention on sequential identity keys.
See docs on OPTIMIZE_FOR_SEQUENTIAL_KEY: https://learn.microsoft.com/en-us/sql/t-sql/statements/alter-table-index-option-transact-sql?view=sql-server-ver17#optimize_for_sequential_key---on--off-
* chore(mssql): enable XACT_ABORT for all stored procedures used by Eventuous
Added SET XACT_ABORT ON to all SQL stored procedures invoked through
GetStoredProcCommand() to ensure atomic rollback behavior when the client
transaction encounters an error. This prevents partial commits and
inconsistent transactional states during event append and stream operations.
Related to issue Eventuous#429
* fix(mssql): truncate_stream wasn't properly formatting and and sending the error message
* refactor(mssql): use SCOPE_IDENTITY() to get new StreamId rather than querying the table for the record we just inserted
* fix(mssql): correct backwards read bounds check
Replaced invalid guard `@current_version < @from_position + @count` with proper
range validation `@from_position < 0 OR @from_position > @current_version`.
Previous logic incorrectly returned no results when valid messages existed.
New check correctly validates start position independently of page size.
* fix(mssql): moved stream version update inside TRY block
and added `@@ROWCOUNT` check to detect concurrent updates.
If no rows are affected, the procedure now
throws a `WrongExpectedVersion` error, aligning behavior with duplicate
append handling.
- Prevents silent version overwrite on race conditions
- Ensures consistency with optimistic concurrency semantics
- Keeps existing WrongExpectedVersion error format for client handling
---------
Co-authored-by: Neal Mummau <[email protected]>
nmummau
added a commit
to nmummau/eventuous
that referenced
this pull request
Oct 23, 2025
* feat(mssql): Minor SQL changes; no functional changes
- SET NOCOUNT ON
- Wrap keywords in brackets ([Version], [Messages])
- formatting: each column on it's own line. Best practice for maintainability of procedures
- proper casing of keywords (INT, AND)
- use alias to DELETE statement
- semicolons
* feat(mssql): Minor SQL changes; no functional changes
- SET NOCOUNT ON
- Wrap keywords in brackets ([Messages])
- formatting: each column on it's own line. Best practice for maintainability of procedures
- proper casing of keywords (INT, BIGINT)
- remove 'Messages.' from query since it was redundant
- semicolons
* feat(mssql): Minor SQL changes; no functional changes
- SET NOCOUNT ON
- Wrap keywords in brackets ([Version], [Messages])
- formatting: each column on it's own line. Best practice for maintainability of procedures
- proper casing of keywords (INT)
- remove 'Messages.' from query since it was redundant
- semicolons
* feat(mssql): Minor SQL changes; no functional changes
- SET NOCOUNT ON
- Wrap keywords in brackets ([Version], [Messages])
- formatting: each column on it's own line. Best practice for maintainability of procedures
- proper casing of keywords (INT)
- remove 'Messages.' from query since it was redundant
- semicolons
* feat(mssql): Minor SQL changes; no functional changes
- SET NOCOUNT ON
- formatting: each column on it's own line. Best practice for maintainability of procedures
- use aliases
- semicolons
* feat(mssql): Minor SQL changes; no functional changes
- SET NOCOUNT ON
- formatting: each column on it's own line. Best practice for maintainability of procedures
- proper casing of keywords (AND)
- semicolons
* feat(mssql): Minor SQL changes; no functional changes
- SET NOCOUNT ON
- remove unused declared variables
- specify all parameters when calling [check_stream] procedure
- formatting: each column on it's own line. Best practice for maintainability of procedures
- proper casing of keywords (IF, ISNULL, NVARCHAR)
- use alias to UPDATE statement
- remove 'AS' keyword
- semicolons
* feat(mssql): Minor SQL updates
- consistent casing of [Version] column
- explicitly specify NULLable columns
* fix(mssql): Messages.Created NOT NULL
SQL Server schema change so that Messages.Created is NOT NULL. This is safe and accurate to do since append_events will always set this value if a NULL is provided to the procedure.
* feat(mssql): json is not null
- Messages TABLE
- JsonMetaData is NOT NULL
- json columns are checked to ensure they contain json data
- StreamMessage TYPE
- json_metadata is NOT NULL
* feat(mssql): DATETIME2 is actually a DATETIME2(7) so explicitly say that in the scripts
* feat(mssql): ORDER BY StreamPosition per bito-code-review, and and remove redundencies in the CHECK definition
* feat(postgresql): ORDER BY stream_position per bito-code-review
* refactor(mssql): remove square brackets to satisfy 'inconsistent table naming' warning that bito-code-review flagged
* feat(mssql): append_events improve
At first when writing this change I assumed I would be out to fix issue Eventuous#429. Turns out that is not an issue as the client wraps this procedure call in a transaction. Thus, no transction is needed within the procedure itself. Here are some improvements, nonetheless.
- Changed @stream_name parameter to NVARCHAR(850) for consistency
- Introduced @Inserted table to capture inserted GlobalPosition values atomically
- Normalized optimistic concurrency errors (2627, 2601) to client-detectable messages
- Thrown error messages still StartsWith 'WrongExpectedVersion' but now also includes the full SQL error text for client tracing
- Removed redundant post-commit queries and error-parsing logic
* perf(mssql): enable OPTIMIZE_FOR_SEQUENTIAL_KEY on primary keys for Streams and Messages
- Added WITH (OPTIMIZE_FOR_SEQUENTIAL_KEY = ON) to clustered primary keys on __schema__.Streams (PK_Streams) and __schema__.Messages (PK_Events)
- Improves insert throughput and reduces latch contention on identity-based keys
- No changes to schema shape, constraints, or indexes beyond PK optimization
This improves concurrent insert performance under high write workloads by allowing SQL Server to better handle last-page insert contention on sequential identity keys.
See docs on OPTIMIZE_FOR_SEQUENTIAL_KEY: https://learn.microsoft.com/en-us/sql/t-sql/statements/alter-table-index-option-transact-sql?view=sql-server-ver17#optimize_for_sequential_key---on--off-
* chore(mssql): enable XACT_ABORT for all stored procedures used by Eventuous
Added SET XACT_ABORT ON to all SQL stored procedures invoked through
GetStoredProcCommand() to ensure atomic rollback behavior when the client
transaction encounters an error. This prevents partial commits and
inconsistent transactional states during event append and stream operations.
Related to issue Eventuous#429
* fix(mssql): truncate_stream wasn't properly formatting and and sending the error message
* refactor(mssql): use SCOPE_IDENTITY() to get new StreamId rather than querying the table for the record we just inserted
* fix(mssql): correct backwards read bounds check
Replaced invalid guard `@current_version < @from_position + @count` with proper
range validation `@from_position < 0 OR @from_position > @current_version`.
Previous logic incorrectly returned no results when valid messages existed.
New check correctly validates start position independently of page size.
* fix(mssql): moved stream version update inside TRY block
and added `@@ROWCOUNT` check to detect concurrent updates.
If no rows are affected, the procedure now
throws a `WrongExpectedVersion` error, aligning behavior with duplicate
append handling.
- Prevents silent version overwrite on race conditions
- Ensures consistency with optimistic concurrency semantics
- Keeps existing WrongExpectedVersion error format for client handling
---------
Co-authored-by: Neal Mummau <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Each commit has isolated changes to a SQL table or procedure with detailed changes
No functional changes; these changes were a result of me doing a deep review of the procedure/tables that Eventuous creates.
This fixes #428
Other changes do include preferences, but I feel will aid to this SQL being easier to maintain for the long term.
Summary by Bito
This pull request addresses issue #428 by implementing improvements across PostgreSQL and SQL Server components. The PostgreSQL update corrects stream data ordering, while SQL Server changes refactor schema definitions and stored procedures. Updates standardize data types, constraints, and error handling to enhance transaction safety, maintainability, and performance. These modifications improve the overall reliability and consistency of the event store implementation.