Skip to:
Content

BuddyPress.org

Opened 3 years ago

Closed 3 years ago

Last modified 17 months ago

#7245 closed defect (bug) (fixed)

Updating user last_activity doesn't bust activity query cache

Reported by: boonebgorges Owned by: boonebgorges
Milestone: 2.7 Priority: normal
Severity: normal Version:
Component: Members Keywords: has-patch dev-feedback
Cc:

Description

User last_activity data is stored in the activity table, but the activity API (bp_activity_add()) is not used to add the data there. I don't recall the exact reasoning; I think it's because we didn't want to trigger activity-related filters for 'last_activity' bumping, out of backward-compatibilty concerns.

Because of this, bumping a user's last_activity doesn't bust activity query caches, and queries that include last_activity can be incorrect.

Previously: #7237

Attachments (2)

7245.diff (3.1 KB) - added by boonebgorges 3 years ago.
7245.2.diff (6.3 KB) - added by boonebgorges 3 years ago.

Download all attachments as: .zip

Change History (11)

@boonebgorges
3 years ago

#1 @boonebgorges
3 years ago

  • Keywords has-patch dev-feedback added

7245.diff is a straightforward fix for the problem. It ensures that the bp-activity incrementor is busted every time a user's last_activity value is bumped.

The downside of this naive approach is that it may result in a more shortlived cache than we might otherwise have. Consider a community where lots of people log in to view content, but don't create much new content. Currently, activity queries are cached until a new activity item (other than last_activity) is created. With 7245.diff, even a single logged-in user visiting the site will invalidate the caches every five minutes. This may be somewhat of an edge case, since lots of logged-in users are probably likely to produce more "actual" activity, but it could still be an issue.

A workaround could be to have a separate cache group for "normal" activity queries than for those that include last_activity. So the activity query cache would look something like this:

if ( false === strpos( $sql, "'last_activity'" ) ) {
    $group = 'bp_activity';
} else {
    $group = 'bp_activity_with_last_activity';
}

$cached = bp_core_get_incremented_cache( $sql, $group );

The regular increment-busting function bp_activity_reset_cache_incrementor() would reset incrementor for *both* groups. But when updating last_activity, only the bp_activity_with_last_activity group would have its incrementor bumped. So the caches remain mostly separate.

I'd like a second opinion about the following questions:
(a) Is it overkill to worry like this about short-lived, shared cache groups?
(b) If the concern is valid, does the approach described here seem like a sensible (if weird) one?

#2 @DJPaul
3 years ago

Off-topic: the phpdoc for bp_core_reset_incrementor is incorrect. Was that function MEANT to return a bool, or is it the documentation that's wrong?

This ticket was mentioned in Slack in #buddypress by djpaul. View the logs.


3 years ago

#4 @DJPaul
3 years ago

Okay, for the record, the initial implementation: #5128

User last_activity data is stored in the activity table, but the activity API (bp_activity_add()) is not used to add the data there. I don't recall the exact reasoning; I think it's because we didn't want to trigger activity-related filters for 'last_activity' bumping, out of backward-compatibilty concerns.

Not quite. We set this in bp_core_user->update_last_activity() which is inside the Core component, so it can't rely on the Activity component always being active.

#5 @DJPaul
3 years ago

Such a short-lived cache would have little value to the significant majority of BuddyPress of sites. However, there is no harm in keeping it. Having it stored in a separate cache group surely is better than everything getting flushed so frequently. I think using strpos to identify the query is going to be too naive, but the idea's sound.

Another idea is updating the last_activity timestamps less frequently. Say, every 15 minutes instead of every 5 minutes. We'd still not want this to destroy all the cached activity groups, so the work here is still useful/required. But maybe that's a discussion to be had in its own ticket if it's something we want to pursue with this batch of changes.

#6 @boonebgorges
3 years ago

In 11060:

bp_core_reset_incrementor() should return a boolean.

Props DJPaul.
See #7237, #7245.

@boonebgorges
3 years ago

#7 @boonebgorges
3 years ago

Thanks for the review, @DJPaul.

Off-topic: the phpdoc for bp_core_reset_incrementor is incorrect. Was that function MEANT to return a bool, or is it the documentation that's wrong?

Fixed in [11060].

Such a short-lived cache would have little value to the significant majority of BuddyPress of sites.

I'm not sure enough about numbers to say that it'd affect a "majority" or "significant majority", but I agree that the cache will be of little for low-traffic sites, or sites where all members are logged in. But sites that have any significant number of logged-out users - as many of my client sites do - will see meaningful performance benefits from having last_active queries cached.

I think using strpos to identify the query is going to be too naive, but the idea's sound.

Yes, that was meant to be an illustration only. Unfortunately, we do have to do string sniffing (rather than argument detection) because the SQL statement goes through a filter. 7245.2.diff uses a regular expression to detect clauses of the form a.type NOT IN ( ... 'last_activity' ... ); this clause should exist in all default activity queries, as BP excludes 'last_activity' items from activity queries unless specifically requested.

Another idea is updating the last_activity timestamps less frequently. Say, every 15 minutes instead of every 5 minutes.

5 minutes is pretty arbitrary. You're right that we should probably consider this separately, though, since what I'm proposing here is required either way.

#8 @boonebgorges
3 years ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from new to closed

In 11101:

Activity: Invalidate query caches when a user's 'last_activity' is updated.

User 'last_activity' is stored in the activity table, but not updated
via the Activity API, so regular invalidation techniques do not work.

In addition, 'last_activity' is quite frequently updated, meaning that
the activity query cache becomes less valuable. To mitigate this, the
current changeset separates the cache for queries containing
'last_activity' from the cache for those that do not.

Fixes #7245.

#9 @r-a-y
17 months ago

In 12152:

Activity: After r11101, fix notices when using the legacy activity query.

r11101 introduced incremented cache improvements when fetching activity
items. However, if the legacy activity query is force-enabled and if the
'count_total' parameter is passed, a few notices are thrown due to
the $cache_group variable not being defined.

This commit addresses this by declaring the $cache_group variable during
the legacy activity query block.

See #7245.

Fixes #7848 (3.x branch).

Note: See TracTickets for help on using tickets.