This is a follow on from #1070762: notifications_queue() slow query.

Even with the index added there, this is still quite a slow query that can add around a second to posting nodes or comments.

I can't see any obvious ways to speed the query itself up, at least not without quite a bit of work.

One idea I did have was to create an intermediary queue that just holds the node/comment id, uid, timestamp (possibly other fields but these might be enough).

hook_node_*() could then just insert items into that queue, which would be very lightweight. This queue could then be processed on cron to put items into the notifications queue itself, which would use more or less the same logic as notifications_queue() - that way you'd defer the actual hard work to cron. Not sure if or when I'd have time to work on this, but opening the issue for reference.

CommentFileSizeAuthor
#9 notifications_1075862-9.patch4.14 KBtravist
#8 notifications_1075862-8.patch4.04 KBtravist
#7 notifications_1075862-7.patch4.38 KBtravist
#5 notifications_1075862-5.patch1.87 KBtravist
#4 notifications_1075862-4.patch1.87 KBtravist
#1 notifications_1075862.patch1.75 KBcatch
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Active » Needs review
FileSize
notifications_1075862.patch1.75 KB

Here's a patch.

Adds a dependency on http://drupal.org/project/drupal_queue

notifications_event() now either adds items to the queue or sends immediately - cron will take items out of the queue and stick them into the notifications queue itself - this is the time consuming bit you don't want taking 2 seconds during comment posting.

sharplesa’s picture

sharplesa CreditAttribution: sharplesa commented
Status: Needs review » Needs work

This is labeled version 7.x-1.x-dev, but the code says its for core=6.x. Is the version mis-labeled?

If this is for D7, there's a Queue API now for D7, and no version of drupal_queue for D7, so you can't make the dependency on drupal_queue.

Sending back to "needs work" for these clarifications.

Thanks

killes@www.drop.org’s picture

killes@www.drop.org CreditAttribution: killes@www.drop.org commented
Version: 7.x-1.x-dev » 6.x-2.x-dev
Status: Needs work » Needs review

Yes, it this is for the 6.x-2 branch

travist’s picture

travist CreditAttribution: travist commented
FileSize
notifications_1075862-4.patch1.87 KB

We are going to test this patch for use in our site, will report back with those results.

In the meantime, reformatting this patch for GIT.

travist’s picture

travist CreditAttribution: travist commented
FileSize
notifications_1075862-5.patch1.87 KB

After testing, there should be a separate variable to queue immediately vs. sending immediately. Here is an updated patch.

travist’s picture

travist CreditAttribution: travist commented

Testing some more, and it seems that you will also need to apply this patch here #1567398: Notification events are getting deleted while the message is still within drupal queue. in order for this to work properly.

travist’s picture

travist CreditAttribution: travist commented
FileSize
notifications_1075862-7.patch4.38 KB

Here is a re-rolled patch to include the changes from #1567398: Notification events are getting deleted while the message is still within drupal queue. and I will just close that issue since it only applies if you have this patch applied.

travist’s picture

travist CreditAttribution: travist commented
FileSize
notifications_1075862-8.patch4.04 KB

Updated patch with a fix for some regression issues we found.

travist’s picture

travist CreditAttribution: travist commented
FileSize
notifications_1075862-9.patch4.14 KB

Need to add some protection around having an invalid query.