Skip to content
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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Aug 21, 2024
Merged

Add a perf-importent Notification index #16590

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f6f9bea
Add an pref-importent missing Notification index
MikeAlhayek Aug 20, 2024
b65ecce
Merge branch 'main' into ma/add-recommended-notification-index
MikeAlhayek Aug 20, 2024
796ae4a
add the index to the create
MikeAlhayek Aug 21, 2024
58071c2
Merge branch 'ma/add-recommended-notification-index' of https://githu…
MikeAlhayek Aug 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 24 additions & 1 deletion src/OrchardCore.Modules/OrchardCore.Notifications/Migrations/NotificationMigrations.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,29 @@ await SchemaBuilder.AlterIndexTableAsync<NotificationIndex>(table => table
collection: NotificationConstants.NotificationCollection
);

return 1;
await SchemaBuilder.AlterIndexTableAsync<NotificationIndex>(table => table
.CreateIndex("IDX_NotificationIndex_UserId",
"DocumentId",
"UserId",
"IsRead",
"CreatedAtUtc"),
collection: NotificationConstants.NotificationCollection
);

return 2;
}

public async Task<int> UpdateFrom1Async()
{
await SchemaBuilder.AlterIndexTableAsync<NotificationIndex>(table => table
.CreateIndex("IDX_NotificationIndex_UserId",
Copy link
Member

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?

Copy link
Member Author

@MikeAlhayek MikeAlhayek Aug 20, 2024 •
edited
Loading

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)

Copy link
Member Author

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.

Copy link
Member

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

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Member Author

@MikeAlhayek MikeAlhayek Aug 21, 2024 •
edited
Loading

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.

Copy link
Contributor

@gvkries gvkries Aug 21, 2024 •
edited
Loading

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?

Copy link
Member Author

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;

Copy link
Contributor

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.

"DocumentId",
"UserId",
"IsRead",
"CreatedAtUtc"),
collection: NotificationConstants.NotificationCollection
);

return 2;
}
}