Skip to:
Content

BuddyPress.org

Opened 5 months ago

Last modified 7 weeks ago

#8426 new defect (bug)

Update queries when new message notifications mark as read

Reported by: oztaser Owned by:
Milestone: Up Next Priority: normal
Severity: normal Version:
Component: Messages Keywords: has-patch reporter-feedback
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 5 months ago.
8426.patch (1.7 KB) - added by oztaser 5 months ago.
8426.2.patch (1.5 KB) - added by oztaser 5 months ago.
8426.3.patch (3.9 KB) - added by imath 5 months ago.

Download all attachments as: .zip

Change History (12)

@oztaser
5 months ago

#1 @oztaser
5 months ago

  • Keywords has-patch added

#2 @imath
5 months 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
5 months 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
5 months ago

#4 @imath
5 months 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
5 months 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
5 months ago

#6 @imath
5 months ago

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

PS: I haven't tested it ;)

#7 @imath
2 months ago

  • Keywords reporter-feedback added

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 ?

#8 @imath
7 weeks ago

  • Milestone changed from 8.0.0 to Up Next

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

Note: See TracTickets for help on using tickets.