Add highly missing index: IX_CoreMessaging_MessageRecipient…

[eugene-sea]

…s_Read

Summary

CoreMessaging_GetNextMessagesForDigestDispatch SP performance was improved by near 18x, CoreMessaging_GetNextMessagesForInstantDispatch SP performance was improved by near 12x

[dnfclas]

All CLA requirements met.

[mitchelsellers]

Please for all DNN Platform items do not include internal ESW tracking ticket numbers in the subject lines of Pull Requests and/or issues. This provides confusion to the community and others that they to reference these items that we do not have exposure to.

[valadas]

@eugene-sea was it intentional to make this PR a work in progress or was that a mistake?

[eugene-sea]

Please for all DNN Platform items do not include internal ESW tracking ticket numbers in the subject lines of Pull Requests and/or issues. This provides confusion to the community and others that they to reference these items that we do not have exposure to.

@mitchelsellers, sorry, I was not acknowledged about these rules.
What about commits? They should not also contain internal tracking numbers? Is it OK if my local branch name will contain a tracking number (e.g. eugene-sea:faster/DNN-35915)? Should I recreate PR without tracking numbers?

[eugene-sea]

@eugene-sea was it intentional to make this PR a work in progress or was that a mistake?

@valadas, yes, it was intentional. When it will be ready for review I will mark the PR as such.

[valadas]

FYI, the next version will be 9.5.0 so you might want to rename that file.

[sleupold]

I would prefer creating this as filtered index instead, it should be a unique index and using a more specific name, e.g. CoreMessaging_MessageRecipients_ForDispatch.
Second issue is vw_MessagesForDispatch, which contains 2 subqueries.
3rd issue is the use of subqueries in both sprocs, one using UserID as (non-unique)
4th the sprocs should just return the columns needed, not SELECT * FROM messages JOIN recipients.

[eugene-sea]

@sleupold, I understand your points but let's not mix several problems together. The point of this PR is to add a highly missing index (it was reported by SQL Server).

I have renamed the index as you suggested and made it as filtered (very good point).

2nd, 3rd, 4th issues do not affect the index. They are a separate problem.

[sleupold]

I am currently reviewing the process of messages for subscriptions in CoreMessaging and I this needs further investigation. When splitting the send process into chunks, significant complexity was added, which might cause issues on edge cases, we should review and maybe fix, before optimizing the resulting process.

[eugene-sea]

When do you plan to finish the review?

[mitchelsellers]

@eugene-sea Do you have any detail of the size of the site/db that this recommendation was provided by SQL Server? I just want to make sure we have all of the facts before we add more DB Overhead.

[eugene-sea]

@mitchelsellers, CoreMessaging_MessageRecipients has 129696 rows.

Please keep in mind that the index is filtered by EmailSent = 0 AND [Read] = 0 AND Archived = 0. Thus just a small amount of rows (e.g. not more than hundreds) will be part of new index, so the overhead is minimal. I have measured the impact of index on inserts/updates/deletes to be just a couple of percent.

[sleupold]

@eugene-sea
I am working on the review and improvements. Unfortunately, the implementation of CoreMessaging is quite tricky, IMO.

[sleupold]

@eugene-sea
do you have a proper test set to check my suggestion:

UPDATE {databaseOwner}[{objectQualifier}CoreMessaging_MessageRecipients]
SET EmailSchedulerInstance = NULL
WHERE EmailSchedulerInstance = '00000000-0000-0000-0000-000000000000';
GO

IF EXISTS (SELECT * FROM sys.indexes
WHERE object_id = OBJECT_ID(N'{databaseOwner}[{objectQualifier}CoreMessaging_MessageRecipients]')
AND Name = N'IX_{objectQualifier}CoreMessaging_MessageRecipients_ForDispatch')
DROP INDEX [IX_{objectQualifier}CoreMessaging_MessageRecipients_ForDispatch]
ON {databaseOwner}[{objectQualifier}CoreMessaging_MessageRecipients];
GO

CREATE UNIQUE NONCLUSTERED INDEX [IX_{objectQualifier}CoreMessaging_MessageRecipients_ForDispatch]
ON {databaseOwner}[{objectQualifier}CoreMessaging_MessageRecipients]
([EmailSchedulerInstance], [UserID], [RecipientID])
INCLUDE ([LastModifiedOnDate])
WHERE [EmailSent] = 0
AND [Read] = 0
AND [Archived] = 0;
GO

IF EXISTS (SELECT * FROM sys.views WHERE object_id = OBJECT_ID(N'{databaseOwner}[{objectQualifier}vw_MessagesForDispatch]'))
DROP VIEW {databaseOwner}[{objectQualifier}vw_MessagesForDispatch];
GO

CREATE VIEW {databaseOwner}[{objectQualifier}vw_MessagesForDispatch]
AS
SELECT R.[UserID],
R.[RecipientID],
R.[EmailSchedulerInstance],
CASE
WHEN M.NotificationTypeID IS Null
THEN IsNull(P.[MessagesEmailFrequency], 0) -- direct mails are sent immediately by default
ELSE IsNull(p.[NotificationsEmailFrequency], 2) -- notifications are sent as daily digest by default
END AS EmailFrequency
FROM {databaseOwner}[{objectQualifier}CoreMessaging_MessageRecipients] AS R
INNER JOIN {databaseOwner}[{objectQualifier}CoreMessaging_Messages] AS M ON R.MessageID = M.MessageID
LEFT JOIN {databaseOwner}[{objectQualifier}CoreMessaging_UserPreferences] AS P ON R.UserID = P.UserID AND M.PortalID = P.PortalID
WHERE [EmailSent] = 0 AND [Read] = 0 AND [Archived] = 0;
GO

IF EXISTS (SELECT * FROM sys.Procedures WHERE object_id = OBJECT_ID(N'{databaseOwner}[{objectQualifier}CoreMessaging_GetNextMessagesForInstantDispatch]'))
DROP PROCEDURE {databaseOwner}[{objectQualifier}CoreMessaging_GetNextMessagesForInstantDispatch];
GO

CREATE PROCEDURE {databaseOwner}[{objectQualifier}CoreMessaging_GetNextMessagesForInstantDispatch]
@SchedulerInstance UNIQUEIDENTIFIER,
@batchsize INT
AS
BEGIN
-- reset possibly remaining records from any previous run of this SchedulerInstance:
UPDATE {databaseOwner}[{objectQualifier}CoreMessaging_MessageRecipients]
SET [EmailSchedulerInstance] = Null,
[LastModifiedOnDate] = GetDate()
WHERE [EmailSchedulerInstance] = @SchedulerInstance
AND [EmailSent] = 0 AND [Read] = 0 AND [Archived] = 0;

-- reset possibly remaining outdated records from other instances:
UPDATE {databaseOwner}[{objectQualifier}CoreMessaging_MessageRecipients]
 SET   [EmailSchedulerInstance] = Null
 WHERE [EmailSent] = 0 AND [Read] = 0 AND [Archived] = 0
   AND [EmailSchedulerInstance] Is Not Null AND [LastModifiedOnDate] < DateAdd(hh, -2, GetDate());
   

-- mark messages for dispatch, so they won't be handled by another SchedulerInstance:
UPDATE TOP (@BatchSize) R
 SET   [EmailSchedulerInstance] = @SchedulerInstance,
	   [LastModifiedOnDate]     = GetDate()
 FROM       {databaseOwner}[{objectQualifier}CoreMessaging_MessageRecipients] AS R 
 INNER JOIN {databaseOwner}[{objectQualifier}CoreMessaging_Messages]          AS M ON R.MessageID = M.MessageID
 LEFT  JOIN {databaseOwner}[{objectQualifier}CoreMessaging_UserPreferences]   AS P ON R.UserID    = P.UserID    AND M.PortalID = P.PortalID
 WHERE R.[EmailSent] = 0 AND R.[Read] = 0 AND R.[Archived] = 0 AND EmailSchedulerInstance IS NULL
   AND CASE 
	    WHEN M.NotificationTypeID IS Null 
		THEN IsNull(P.[MessagesEmailFrequency],      0) -- direct mails are sent immediately by default
		ELSE IsNull(p.[NotificationsEmailFrequency], 2) -- notifications are sent as daily digest by default
	   END = 0;

/* alternative version:
UPDATE {databaseOwner}[{objectQualifier}CoreMessaging_MessageRecipients]
SET [EmailSchedulerInstance] = @SchedulerInstance,
[LastModifiedOnDate] = GetDate()
WHERE UserID IN
( SELECT TOP (@batchsize)
UserID
FROM {databaseOwner}[{objectQualifier}vw_MessagesForDispatch]
WHERE EmailSchedulerInstance IS NULL
AND EmailFrequency = 0)
*/
SELECT M.[PortalID],
M.[NotificationTypeID],
M.[To],
M.[From],
M.[Subject],
M.[Body],
M.[SenderUserID],
M.[ExpirationDate],
M.[Context],
R.[RecipientID],
R.[MessageID],
R.[UserID]
FROM {databaseOwner}[{objectQualifier}CoreMessaging_MessageRecipients] R
JOIN {databaseOwner}[{objectQualifier}CoreMessaging_Messages] M ON R.MessageID = M.MessageID
WHERE [EmailSent] = 0 -- Filter these columms 4 to use proper index
AND [Read] = 0
AND [Archived] = 0
AND [EmailSchedulerInstance] = @SchedulerInstance
ORDER BY --[PortalID],
[UserID],
[RecipientID]
END; -- Procedure
GO

IF EXISTS (SELECT * FROM sys.Procedures WHERE object_id = OBJECT_ID(N'{databaseOwner}[{objectQualifier}CoreMessaging_GetNextMessagesForDigestDispatch]'))
DROP PROCEDURE {databaseOwner}[{objectQualifier}CoreMessaging_GetNextMessagesForDigestDispatch];
GO

CREATE PROCEDURE [dbo].[CoreMessaging_GetNextMessagesForDigestDispatch]
@frequency INT,
@SchedulerInstance UNIQUEIDENTIFIER,
@batchsize INT
AS
BEGIN
UPDATE R
SET [EmailSchedulerInstance] = @SchedulerInstance,
[LastModifiedOnDate] = GetDate()
FROM {databaseOwner}[{objectQualifier}CoreMessaging_MessageRecipients] R
JOIN (SELECT TOP (@batchsize)
UserID
FROM dbo.[vw_MessagesForDispatch]
WHERE [EmailSchedulerInstance] IS NULL
AND [EmailFrequency] = @frequency
GROUP BY UserID
ORDER BY UserID) D ON R.UserID = R.UserID

SELECT M.[PortalID],
       M.[NotificationTypeID],
	   M.[To],
	   M.[From],
	   M.[Subject],
	   M.[Body],
	   M.[SenderUserID],
	   M.[ExpirationDate],
	   M.[Context],
       R.[RecipientID],
       R.[MessageID],
	   R.[UserID]
 FROM  {databaseOwner}[{objectQualifier}CoreMessaging_MessageRecipients] R
 JOIN  {databaseOwner}[{objectQualifier}CoreMessaging_Messages]          M ON R.MessageID = M.MessageID
 WHERE [EmailSent] = 0 -- Filter these 4 columms to use proper index
   AND [Read]      = 0
   AND [Archived]  = 0
   AND [EmailSchedulerInstance] = @SchedulerInstance
 ORDER BY --[PortalID],
          [UserID],
		  [RecipientID] DESC

END; -- Procedure
GO

[eugene-sea]

@sleupold, in terms of performance it is fine, but CoreMessaging_GetNextMessagesForInstantDispatch should include MessageID column to avoid 2 key lookups. This will save IO and CPU.

[sleupold]

@eugene-sea
I don't understand your last remark - the recordset returned by my version of CoreMessaging_GetNextMessagesForInstantDispatch does include R.[MessageID].
Unfortunately, the method using it does include a lookup for message and userID instead of using this column, is this what you mean?

[sleupold]

PS: IMO, another issue:
mail digest should be separated per website (portal or portal group) . A user would not understand, why messages from multiple sites are included in a single digest

[eugene-sea]

@sleupold, sorry, I meant index should include MessageID, like:

CREATE UNIQUE NONCLUSTERED INDEX [IX_{objectQualifier}CoreMessaging_MessageRecipients_ForDispatch]
ON {databaseOwner}[{objectQualifier}CoreMessaging_MessageRecipients]
([EmailSchedulerInstance], [UserID], [RecipientID])
INCLUDE ([LastModifiedOnDate], [MessageID]) ------> Added include here <----
WHERE [EmailSent] = 0
AND [Read] = 0
AND [Archived] = 0;

[sleupold]

@eugene-sea,
oops, you are correct.
Thank you very much, I'll inclue it in the PR

[sleupold]

is there a GitHub issue number associated?

[eugene-sea]

I have not created any GitHub issues for this PR.

[sleupold]

This PR has been superseded by #3540 (associated issue: #3539)
Many thanks to @eugene-sea for bringing it up and testing it!