Skip to:
Content

BuddyPress.org

Opened 7 weeks ago

Last modified 6 weeks ago

#8426 new defect (bug)

Update queries when new message notifications mark as read

Reported by: oztaser Owned by:
Milestone: 8.0.0 Priority: normal
Severity: normal Version:
Component: Messages Keywords: has-patch
Cc:

Description

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

Attachments (4)

new-message-notification-update-queries.png (194.9 KB) - added by oztaser 7 weeks ago.
8426.patch (1.7 KB) - added by oztaser 7 weeks ago.
8426.2.patch (1.5 KB) - added by oztaser 6 weeks ago.
8426.3.patch (3.9 KB) - added by imath 6 weeks ago.

Download all attachments as: .zip

Change History (10)

@oztaser
7 weeks ago

#1 @oztaser
7 weeks ago

  • Keywords has-patch added

#2 @imath
6 weeks ago

  • Keywords reporter-feedback added
  • Milestone changed from Awaiting Review to 8.0.0
  • Type changed from enhancement to defect (bug)

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?

#3 @oztaser
6 weeks ago

  • Keywords reporter-feedback removed

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.

@oztaser
6 weeks ago

#4 @imath
6 weeks ago

  • Keywords reporter-feedback added

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.

#5 @oztaser
6 weeks ago

  • Keywords reporter-feedback removed

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
6 weeks ago

#6 @imath
6 weeks ago

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

PS: I haven't tested it ;)

Note: See TracTickets for help on using tickets.