#5451 closed enhancement (fixed)
Groups query overhaul
Reported by: | boonebgorges | Owned by: | boonebgorges |
---|---|---|---|
Milestone: | 2.7 | Priority: | high |
Severity: | normal | Version: | |
Component: | Groups | Keywords: | has-patch commit |
Cc: |
Description
In the past few versions of BP, we've overhauled the main members and activity queries. This has given us huge boosts in flexibility, consistency, cacheability, and performance.
Next I would like to tackle the Groups component. At a minimum:
- Refactor the main query so that it's split into two parts: one to get matching IDs, and another to fill in the data for the matched IDs.
- Once the query is split, implement some object caching for the main
get()
method.
It may also be worth thinking about reorganizing into a BP_Group_Query
class, akin to BP_User_Query
.
Attachments (4)
Change History (49)
This ticket was mentioned in IRC in #buddypress-dev by boonebgorges. View the logs.
10 years ago
#3
@
10 years ago
total_member_count
is a good candidate to be moved into the main bp_groups
table since group template queries rely on it.
I'm not so sure about invite_status
.
This ticket was mentioned in Slack in #buddypress by mamaduka. View the logs.
10 years ago
#9
@
8 years ago
- Milestone changed from Future Release to 2.7
Let's split the query and add caching for 2.7. See #6643.
#10
@
8 years ago
See also #6358. Making BP_Groups_Group::get()
return an array of BP_Groups_Group
objects will help to eliminate some unneeded bits of this query.
#11
@
8 years ago
- Keywords has-patch 2nd-opinion added
5451.diff contains the first steps toward overhauling the query. It's the "first steps" because so far I've only refactored what we've currently got. No additional caching or explicit performance improvements are included here (with one exception - see below). I figured it'd be easier to get a review and/or a second opinion on the approach if I broke things up. Here's what the patch does:
- Instead of doing a single query to fetch all groups matching the
get()
params, there are now two queries: an ID query and a cache-priming query. The old query looked like this:
SELECT DISTINCT g.id, g.*, gm1.meta_value AS total_member_count, gm2.meta_value AS last_activity FROM wp_bp_groups_groupmeta gm1, wp_bp_groups_groupmeta gm2, wp_bp_groups g WHERE g.id = gm1.group_id AND g.id = gm2.group_id AND gm2.meta_key = 'last_activity' AND gm1.meta_key = 'total_member_count' ORDER BY last_activity DESC LIMIT 0, 20
while the new ones look like this:
SELECT DISTINCT g.id FROM wp_bp_groups g JOIN wp_bp_groups_groupmeta gm_last_activity on ( g.id = gm_last_activity.group_id ) WHERE gm_last_activity.meta_key = 'last_activity' ORDER BY gm_last_activity.meta_value DESC LIMIT 0, 20 SELECT g.* FROM wp_bp_groups g WHERE g.id IN (63,68,71,70,69,18,66,46,20,61,44,32,45,62,67,24,58,65,27,2)
- As you can see, a costly table join has been removed. This table join was previously used to get the
total_member_count
. We only need this join whentype=popular
- ie, when we're sorting bytotal_member_count
. See the section with the comment// Ordering by 'total_member_count' requires another table join.
BP_Groups_Group::get()
now returns an array ofBP_Groups_Group
objects. See #6358. This has lots of benefits. The important one for the purposes of this ticket is that it centralizes cache management.
last_activity
andtotal_member_count
are no longer loaded into the group object by default. Instead, they've been madeprotected
and are accessible by magic method. This reduces overhead when the values are not needed, and points the way to a future where we can do away withpopulate_extras
altogether.
- The rebuilding of the query syntax means that we can do away with all the funky string manipulation we were doing with
tax_query
andmeta_query
. So 5451.diff fixes #5099 as well.
Next steps, once this patch is in:
- Incrementor-based caching for the ID query. See #6643.
- Better pre-loop caching for group admins. With object caching disabled,
bp_get_group_join_button()
is causing a query for every group in the loop. #5711 added cache support forget_group_administrator_ids()
, so all we'd need is single query at the beginning of a group loop that pre-fetches these admin lists and puts them into the cache. - Move some/all of the
populate_extras
stuff into magic methods, where they can be lazy-loaded. This will make our cache treatment more standardized, since we won't be maintaining separate versions of the object.
This ticket was mentioned in Slack in #buddypress by boone. View the logs.
8 years ago
#13
@
8 years ago
This first patch is great. It makes the SQL query building much easier to read. Far and away the most exciting change from my perspective is that groups_get_groups()
returns honest-to-goodness BP_Groups_Group
objects without incurring any meaningful overhead.
The extra query that is introduced is very fast and well worth the cache-friendliness for the future.
Thanks for the work on this! Let me know if there's any part of this update I can help with.
This ticket was mentioned in Slack in #buddypress by boone. View the logs.
8 years ago
This ticket was mentioned in Slack in #buddypress by boone. View the logs.
8 years ago
#16
@
8 years ago
I've found a couple uses of the filters. Only a few in public plugins:
- Break: https://plugins.trac.wordpress.org/browser/bp-sticky-groups/trunk/bp-sticky-groups.php#L254 will break completely. It's @imath's plugin, so I assume he'd be happy to accept a patch.
- OK: https://plugins.trac.wordpress.org/browser/cac-featured-content/trunk/cac-featured-autocomplete.php?marks=52-62#L16 will not break at all!
On GitHub (I've excluded anything that I wrote or maintain myself). Many of these appear to be copy-paste duplicates of each other:
- OK: https://github.com/misfist/nycga5/blob/d2a02fb311e7a5b5604dd3f3936e1d72a2dd5a2c/wp-content/plugins/.temp-bp/ga-group-status/status.php#L40
- OK: https://github.com/ajency/Seedeis/blob/9c6b9198850e0d67a406dfcd4ce18d2d69df55c5/wp-content/themes/seedeis/business_proposals/Seed_business_proposals.php#L125
- OK: https://github.com/mlaa/levitin/blob/f291f2fa84839fea930cc86b6e8ef945fa461843/lib/mla/group-filters.php#L27
- OK: https://github.com/karysto/cord-network/blob/c5c5327e5f5059cb6302fd0209daea9ee422f45a/wp-content/themes/cbox-mla-master/bp-custom.php#L72
- Break: https://github.com/slaFFik/BP-Groups-Extras-Search/blob/faf7afdfa1aafcb69dffabd06fa189e40b444edf/bpge-search.php#L180
- Break: https://github.com/imath/bp-groups-taxo/blob/db0f7b6257b2f5e1d1ffe41a737ef9ce9fe15f02/bp-groups/bp-groups-tag.php#L180
- Break: https://github.com/rackdevt/noticeboard-master/blob/3355db63535b73617bc74db12e5e981ce9a77312/wp-content/plugins/gmw-groups-locator/includes/gmw-gl-component-search-functions.php#L361
- OK: https://github.com/tamriel-foundry/apocrypha/blob/2dd18713f752926199d8d88137892ea1ebbc185f/library/extensions/buddypress.php#L1087
- Break: https://github.com/paulmedwal/edxforumspublic/blob/e77124c8f32c6f092537c748fbb70ace1f2c63a5/wp-content/themes/wp-knowledge-base-child/functions.php#L491
This covers most of the uses I could spot in the wild.
So, some things will break. It *might* be possible to salvage some of these by continuing to assemble the $sql
array that gets passed to the filter, even if it's never used in BP itself; but this will require lots of weird code.
Given the above, I think we're OK to make the change, with associated documentation and posts on bpdevel. I'd like to hear what others think about that - especially @DJPaul and @r-a-y (@dcavins has already voiced his support :) )
#17
@
8 years ago
Thumbs up to the patch here as well.
One thing to add is maybe we can query for groups without the last_activity
groupmeta JOIN altogether if orderby
is empty or something like that.
#19
@
8 years ago
Thanks for the look, @r-a-y! Good catch about the last_activity
join - I took your suggestion in [11071]. I think that'll have a pretty huge positive effect in certain use cases.
#24
@
8 years ago
Better pre-loop caching for group admins. With object caching disabled, bp_get_group_join_button() is causing a query for every group in the loop. #5711 added cache support for get_group_administrator_ids(), so all we'd need is single query at the beginning of a group loop that pre-fetches these admin lists and puts them into the cache.
5451.2.diff implements this. It saves 19 queries when viewing the group directory without persistent caching, which is nice :-D @r-a-y - you're the author of the original caching in #5711. Would you mind giving my approach a sanity check? The weirdness with stdClass
is to ensure compatibility with the existing format of the cache values.
#25
@
8 years ago
- Keywords 2nd-opinion removed
I like the general approach, @boone. Patch works as advertised!
A couple of things:
- This problem only occurs for people who are logged-in that are members of the group in the loop: https://buddypress.trac.wordpress.org/browser/trunk/src/bp-groups/bp-groups-template.php?marks=3245#L3219. So the query problem isn't that bad unless all the groups in the Groups Directory are all yours or if you are a member of all of them :)
- Can we be conservative with the default value of the
update_admin_cache
parameter? We should default totrue
if on the Groups Directory or on a user's groups page (bp_is_groups_directory() || bp_is_user_groups()
) - Likewise, we could also add moderator ID caching, so we can save a query on a single group page.
#26
@
8 years ago
This problem only occurs for people who are logged-in that are members of the group in the loop
Quite right! Can you tell what kind of installation I've been testing on? :)
Can we be conservative with the default value of the update_admin_cache parameter?
Good idea. Let's do it.
Likewise, we could also add moderator ID caching, so we can save a query on a single group page.
OK. I'll do it as part of the next part, where I try to eliminate the scourge that is populate_extras()
.
This ticket was mentioned in Slack in #buddypress by boone. View the logs.
8 years ago
#34
@
8 years ago
- Resolution set to fixed
- Status changed from new to closed
I think I've done all I wanted on this ticket. Let's handle future improvements in another release.
#35
@
8 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
Oops, I broke some group membership checks because of our ! empty()
zealotry. Fix coming up.
This ticket was mentioned in Slack in #buddypress by boone. View the logs.
8 years ago
#37
@
8 years ago
Regarding comment:35, I've fixed this by calling the magic __get()
method and casting the result as a boolean
. Might make sense to do the same for all the other keys.
#38
@
8 years ago
Thanks for the patch, @r-a-y!
If I'm reading 5451.isset.patch correctly, it'll cause problems with falsey values.
// Say the user is *not* a member of the group $group = new BP_Groups_Group(); // bool( false ) var_dump( $group->is_member ); // bool( true ) var_dump( empty( $group->is_member ) ) // bool( false ) var_dump( isset( $group->is_member ) )
I think this last part is incorrect - the value is set, but it happens to be empty. Does that seem right to you? If so, I think the right solution is to return true for the keys that are accessible by magic method.
#39
@
8 years ago
If so, I think the right solution is to return true for the keys that are accessible by magic method.
Yeah, you're right. Carry on!
#40
@
8 years ago
- Owner set to boonebgorges
- Resolution set to fixed
- Status changed from reopened to closed
In 11103:
#42
@
8 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
I've been playing with the legacy forums today.
And the magic methods broke legacy group forum functionality (topics did not show up under a group's "Forum" page) due to:
https://buddypress.trac.wordpress.org/browser/tags/2.6.2/src/bp-forums/bp-forums-template.php?marks=538,542#L536
Attached patch fixes this up.
Let's consider moving
last_activity
out of meta and into thebp_groups
table proper with an additional column. I don't love that it's inconsistent with Member activity, but there's no sense in trying to query meta if we don't have to.We could do similar with
invite_status
andtotal_member_count
since queries are ran against both also.