[buddypress-trac] [BuddyPress Trac] #7130: bp_notifications_get_all_notifications_for_user gets all records and can cause memory leak

[buddypress-trac]

#7130: bp_notifications_get_all_notifications_for_user gets all records and can
cause memory leak
-------------------------------------+------------------
 Reporter:  hberberoglu              |       Owner:
     Type:  defect (bug)             |      Status:  new
 Priority:  normal                   |   Milestone:  3.0
Component:  Toolbar & Notifications  |     Version:
 Severity:  normal                   |  Resolution:
 Keywords:  has-patch                |
-------------------------------------+------------------

Comment (by boonebgorges):

 @m_uysl After [11851] I took a second, closer look at your patch, and I
 think I really like the idea. In my very rough tests, the `GROUP BY` query
 is much faster than doing similar logic in PHP. Can you look at the new
 patch to see if I've captured it correctly? (I've regenerated after
 [11851] and done a small amount of documentation changes.)

 I agree that it might be worthwhile to rethink the notification toolbar a
 bit more radically, but you're correct that this is a larger project, and
 it shouldn't hold up improvements like this one.

 @mauteri Before we go any further down the road of limiting the query,
 could you have an initial look at [attachment:7130.3.diff]? The important
 change here is that the heavy work of grouping has been handed off to
 MySQL in `get_grouped_notifications_for_user()`. If by any change you have
 a test rig that has the amounts of data you've discussed here (5
 figures!!), it would be hugely helpful if you could apply the patch to see
 what kind of numbers you see against a very large notifications table.

--
Ticket URL: <https://buddypress.trac.wordpress.org/ticket/7130#comment:15>
BuddyPress Trac <http://buddypress.org/>
BuddyPress Trac