-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Add a perf-importent Notification index #16590
Conversation
public async Task<int> UpdateFrom1Async() | ||
{ | ||
await SchemaBuilder.AlterIndexTableAsync<NotificationIndex>(table => table | ||
.CreateIndex("IDX_NotificationIndex_UserId", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the index used somewhere else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This add a SQL index not SQL Table. This index will be used by multiple queries to improve SQL performance.
CREATE INDEX IDX_NotificationIndex_UserId ON [Notification_NotificationIndex](DocumentId,UserId,IsRead,CreatedAtUtc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hishamco unless you see an issue with the PR, please approve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I just added a comment waiting for a use case for your index, I'm not sure if it's good to add something without using it in the framework
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did explain #16590 (comment) and you replaied with 👍 . This new index will be used by the database server to optimize queries that are generated by the framework.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, thanks for the explanation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gvkries
FYI: When SQL engines select an index, they prioritize included columns from left to right. The new index is likely to be used when querying by DocumentId
and UserId
, or by DocumentId
, UserId
, and IsRead
, or by DocumentId
, UserId
, and CreatedAtUtc
. Columns on the right can be optional, but including them increases the likelihood that the index will be selected for a query.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a follow-up question: What about the Content
column, isn't that probably too large for an index anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is large, but just under the limits. Check out the comments for the column length
OrchardCore/src/OrchardCore/OrchardCore.Notifications.Core/NotificationConstants.cs
Lines 7 to 13 in 51f24d7
// Maximum length that MySql can support in an inner index under utf8mb4 collation is 768, | |
// minus 2 for the 'DocumentId' integer (bigint size = 8 bytes = 2 character size), | |
// minus 2 for the 'CreatedAtUtc' (date time size = 8 bytes = 2 character size), | |
// minus 26 for 'NotificationId', 26 for 'UserId' and 1 for the 'IsRead' bool, | |
// minus 4 to allow a new integer column, for example the 'Id' column, | |
// minus 2 to allow a new date time, for example 'ReadAtUtc'. | |
public const int NotificationIndexContentLength = 705; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should have looked at the source first 🙈 Thank you.
src/OrchardCore.Modules/OrchardCore.Notifications/Migrations/NotificationMigrations.cs
Show resolved
Hide resolved
…b.com/OrchardCMS/OrchardCore into ma/add-recommended-notification-index
@hishamco anything to add here or can we merge it? |
Thanks for the explanation |
When user have lots of notifications, there is a significant degrade is query performance. This index is important to improve the speed of the notification queries used by OC