Update queries when new message notifications mark as read

[oztaser]

Every single message page view triggers an update queries for mark the new_message notifications even if there are no unread new_message notifications. The worse part is it runs the queries as much as the total number of recipients times the number of messages.

To reproduce the problem;

• Enable MySQL query logs
• Create a thread and send some messages
• Refresh the message view and look at query logs

[imath]

Hi @oztaser

Thanks a lot for your report and patch: good catch! I wonder if we could also avoid the query to check if there are unread notifications.

In bp_messages_mark_notification_on_mark_thread() instead of getting all thread messages, maybe we could get all thread unread messages?

[oztaser]

Hi @imath,

Thanks for your feedback. What you think about the new path? It selects unread messages by notifications table because we don't store read data for messages. It looks the only way finding unread messages is this.

[imath]

Thanks for your new patch @oztaser.

After looking a bit more into it, I believe we need to think a bit more at it :)

A thread can have multiple messages. I guess what's happening in your example is this: it loops on every message of the thread to make sure the corresponding notifications are being updated as not new for the logged in user only. I'm not sure it does it for every recipient.

To improve this, we could probably run the notification query once using a WHERE IN( list of message_ids ) clause.

I guess the problem we have is in the end into the BP_Notifications_Notification->_update() private method we are using ​$wpdb->update(), so we cannot pass an array so that it uses the IN operator.

Do you think there's a substantial performance win using a WHERE IN( list of message_ids ) clause.

[oztaser]

Yes, you are right. It runs the queries only for logged-in users I know that but it looks like a one-page view triggers queries multiple times. You can take a look query logs screenshot I attached. Whole queries triggered after one-page view. I'm just a little bit confused about it right now, I need to debug it again :)

You also mentioned another good point. I believe running just one update query could be better. I didn't want to run a custom update query because I've seen that only the bp_notifications_mark_notifications_by_item_id function is used when I was working on the patch.

If we decide to run one query with the WHERE IN clause, we also could consider improving other notification updates for the message component.

[imath]

In ​8426.3 I'm suggesting to at least avoid marking notifications if nothing was updated.

PS: I haven't tested it ;)

[imath]

Hi @oztaser

We are ~2 weeks before 8.0.0-beta release, what should be done about this ticket in your opinion? Should we try to have it fixed or do we need more time to think about it ?

[imath]

I think we'll need more time, punting it to up/next.

[oztaser]

Hi @imath,

Sorry for my very late reply. Let's try to fix this in 10.0.0.

Your patch works fine. We avoid run update queries if there are no notifications that need to be updated.

I've also thought about update queries for every thread message notifications. In addition to your where in clause suggestion, what you think about less than or equal condition? Can't we use update query like below:

UPDATE `wp_bp_notifications` SET `is_new` = 0 WHERE `user_id` = 2 AND `item_id` <= (MAX_UNREAD_ITEM_ID) AND `component_name` = 'messages' AND `component_action` = 'new_message'

P.S: I've updated your patch because we've added $user_id parameter to messages_thread_mark_as_read action.

[oztaser]

Please ignore the query suggestion part. I was thinking secondary_item_id is thread_id but it is not :(

I've written new query. It finds message id by thread_id and sender_id from bp_messages_messages table. What you think about my new query? I think we get closer the solution, what you think?

 update wp_bp_notifications set is_new = 0 where user_id = {current_user_id} and component_action='new_message' and is_new = 1 and item_id in (select id from wp_bp_messages_messages where thread_id = {$thread_message->thread_id} and sender_id={$thread_message->sender_id});

[imath]

Hi, no problem.

Is your suggestion using this query into/as a replacement of the BP_Notifications_Notification::_update() method?

[oztaser]

Hi @imath,

Yes that what I suggest and I think it works pretty well.

[imath]

Ok, thanks for confirming 👍.

Then we need to think wider than the messages component to make the need to update notifications of a list of item IDs available more broadly. This means:

1. adding a new BP_Notifications_Notification::update_items protected static method making it possible to pass an array of item IDs. I think we might even think of something more generic like BP_Notifications_Notification::update_many to update a list of notification for a list of users, components, etc...
2. adding a new function bp_notifications_mark_notifications_for_item_ids() to use the above method.
3. Updating bp_messages_mark_notification_on_mark_thread() to use this new function.

Is this something you want/have time to keep on working on?

[oztaser]

I've updated the patch based on your comment but It isn't finished yet, I need your opinion about updating get_query_clauses function. Is it ok to edit that function as I've done? Or should we prepare query clauses on our new function update_many.

Right now it supports array of ids and item_ids on where clause.

<?php
BP_Notifications_Notification::update_many(
    array(
        'is_new' => 0
    ),
    array(
        'id' => array( 1, 2, 3 )
    ),
);

<?php
BP_Notifications_Notification::update_many(
    array(
        'is_new' => 0
    ),
    array(
        'item_ids' => array( 1, 2, 3 )
    ),
);

I am open any other suggestions about the patch.

[oztaser]

We can also pass field and items array to update_many function If we do that we don't need to edit get_query_clauses function. I believe this way is better.

[imath]

Hey, this is pretty nice 👏.

.8.patch is including a unit test. I was able to check with this test this improvement replaces the ( number of message thread ) x update queries by 1 query.

In .8.patch I also updated the method name from update_many() to update_id_list().

You're getting close to have this committed. I have 2 last questions:

• Shouldn't we also use bp_notifications_mark_notifications_by_item_ids() inside bp_messages_screen_conversation_mark_notifications()?
• Maybe we should also improve bp_messages_message_delete_notifications() the same way?

I haven't tested it yet manually, I'll try to do it asap.

[oztaser]

Hey, thanks for the update.

Shouldn't we also use bp_notifications_mark_notifications_by_item_ids() inside bp_messages_screen_conversation_mark_notifications()?

Yes, we should. I've updated.

Maybe we should also improve bp_messages_message_delete_notifications() the same way?

More performance improvements I liked that :) I've also updated the patch for this.

I've added new functions called bp_notifications_mark_notifications_by_ids and bp_notifications_delete_notifications_by_ids. I thought these functions could be helpful.

[imath]

Awesome! Thanks a lot for your new patch. I'm looking at it asap.

[imath]

Thanks again for your work on this ticket @oztaser

I've improved a bit these new functions because we don't need the component name / action or other data when we have the ids :)

I've also used these new functions to handle Notification bulk delete/mark actions this way we're avoiding some more queries!

I finally added unit tests about these new functions.

I believe the patch is ready to be committed, so I'll probably do it in a few days after another round of testing.

Maybe you could look to some other parts of BuddyPress where we are using a loop to delete notifications one by one to see if we can add more performance improvements 😉. I'm thinking of activity comments maybe...

[oztaser]

I've improved a bit these new functions because we don't need the component name / action or other data when we have the ids :)

Ah, you are absolutely right. How I missed that? :facepalm:

Maybe you could look to some other parts of BuddyPress where we are using a loop to delete notifications one by one to see if we can add more performance improvements 😉. I'm thinking of activity comments maybe...

I was looking for something to keep contribute. I'll take a look that, thanks for advice.

[imath]

In 13112:

Improve SQL performance when updating/deleting a list of notifications

Instead of looping into a list of notifications to run an update/delete query for each notification, update/delete a list of notifications using a single query. Use primarly this change for message thread notifications and bulk notification actions.

Mainly introduces 2 new static methods to BP_Notifications_Notification:

• BP_Notifications_Notification::update_id_list() let you update a list of notifications using their notification IDs or component item IDs.
• BP_Notifications_Notification::delete_id_list() let you delete a list of notifications using their notification IDs or component item IDs.

Props oztaser

Fixes #8426