Skip to content

Lock persistence journal table on write #6639

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 7, 2023

Conversation

Arkatufus
Copy link
Contributor

@Arkatufus Arkatufus commented Apr 7, 2023

Changes

Change journal table write transaction isolation from undefined to serializable.
This is done to prevent a racy condition in a very write heavy environment, coupled with very eager read on the database.
The explanation for this is here: https://learn.microsoft.com/en-us/sql/t-sql/statements/create-table-transact-sql-identity-property?view=sql-server-ver15#remarks
Basically, for an identity column to be guaranteed to be assigned sequential number during concurrent writes is to lock the table so that only one transaction can operate on the table at any given time.

Performance Comparison

Performance was measured on MS SQL Server, 5 runs were done on each variant and only the highest result is taken, this is to assume that the most optimal condition was used on each test.
Numbers are measured in total messages per second.

Spec Old New
Persist 305 307
PersistAll 1318 1355
PersistAllAsync 3130 3158
PersistAsync 1026 1052
PersistGroup10 962 1033
PersistGroup100 920 1055
PersistGroup200 986 1004
PersistGroup25 982 1009
PersistGroup50 783 777
Recovering 66507 65705
RecoveringFour 71158 69620
RecoveringTwo 55984 60593

@Aaronontheweb Aaronontheweb added this to the 1.5.3 milestone Apr 7, 2023
@Aaronontheweb
Copy link
Member

Performance results look acceptable to me - no major impact.

Copy link
Member

@Aaronontheweb Aaronontheweb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants