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.

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

Performance fix for CoreMessaging_GetMessageConversations and Journal_ListForGroup Procedure #2342

Merged
merged 1 commit into from
Oct 3, 2018

Conversation

ChrisHammond
Copy link
Contributor

@ChrisHammond ChrisHammond commented Sep 26, 2018

Summary

Fixes performance issues experienced with CoreMessaging_GetMessageConversations procedure. The where clause for filtering paging could take 5+ minutes to properly return the result set due to the way the query was written. Added additional CTE to not have to calculate the row count range inside of the where clause.

Fixes issue with Journal_ListForGroup procedure where the page size is set, but the result set is less any deleted journal entries, so the page size is almost always greater, and thus you can never "get more" via the get more button. Proc change allows for Row_number to work properly depending on if you want deleted posts or not.

…size fix for Journal_ListForGroup procedures
@dnfclas
Copy link

dnfclas commented Sep 26, 2018

CLA assistant check
All CLA requirements met.

@david-poindexter
Copy link
Contributor

@ChrisHammond have you created a GitHub issue? For a quick reference on the process, see this document.

@ChrisHammond
Copy link
Contributor Author

I didn't because someone....... @ohine..... told me not to bother in these two cases :D

@ohine
Copy link
Contributor

ohine commented Sep 26, 2018

I am a fan of having a descriptive pull request description instead of just saying fixes #xyz and putting the info in an issue.

@mitchelsellers
Copy link
Contributor

@ChrisHammond Do you have any metrics on the sizes of tables that you used to validate these changes against? I want to make sure we test/validate based on a proper sized data set.

@mitchelsellers mitchelsellers added this to the 9.3 milestone Sep 26, 2018
@mitchelsellers mitchelsellers added the Community Legacy label used to identify community contributions label Sep 26, 2018
@sleupold
Copy link
Contributor

IMO one major performance issue with CoreMessaging: there is no "delete" option, i.e. the tables are growing indefinately. This also conflicts with GDPR.

@mitchelsellers
Copy link
Contributor

@sleupold I would agree, the size of these tables over time is a concern. At this point if we can get some incremental improvement it would be appreciated and a longer-term review of architecture or removal of data would be welcomed via an additional pull request.

@ChrisHammond
Copy link
Contributor Author

@mitchelsellers customer that I was addressing the CoreMessaging issue fix for had

~11k records in Messages
~156 unread messages for the user who was attempting to "load more"

When the load more request fired, it would time out server side, manually running the proc with the same parameters would take 5+ minutes.

After my modification, <1 second for result.

Copy link
Contributor

@mitchelsellers mitchelsellers left a comment

Choose a reason for hiding this comment

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

I have tested this change on 3 installations, one small, one medium and one large. This change doe appear to improve performance greatly. I acknowledge that future changes might need more work, but this seems to make a good improvement over today.

@ChrisHammond
Copy link
Contributor Author

Thanks for the confirmation @mitchelsellers

@ohine ohine merged commit cd8119b into dnnsoftware:development Oct 3, 2018
@ChrisHammond ChrisHammond deleted the HammondProcFixes branch October 3, 2018 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community Legacy label used to identify community contributions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants