Updating user last_activity doesn't bust activity query cache

[boonebgorges]

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

[boonebgorges]

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?

[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?

[DJPaul]

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.

[DJPaul]

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.

[boonebgorges]

In 11060:

bp_core_reset_incrementor() should return a boolean.

Props DJPaul.
See #7237, #7245.

[boonebgorges]

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.

[boonebgorges]

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.

[r-a-y]

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).