Fixed XF\Pub\Controller\Member::actionRecentContent() triggers multiple database query per node

[Lukas W.]

Affected version

2.2

This is happening on an installation with all add-ons disabled and the default theme with no further modifications activated.

The page does not appear to preload node content permissions, and subsequently triggers multiple database queries to individually fetch the node permission cache each time a node permission check is performed.

The following query is performed around 6500 times on my dataset:

SELECT cache_value
FROM xf_permission_cache_content
WHERE permission_combination_id = ?
    AND content_type = ?
    AND content_id = ?

[B]Params:[/B] <permission_combiniation_id>, node, <node_id>

I've used a simple workaround hotfix for now of preloading the node permission cache and the total query count for the page dropped to 10:

class Member extends XFCP_Member
{
    public function actionRecentContent(ParameterBag $params)
    {
        $user = \XF::visitor();
        $user->PermissionSet->cacheAllContentPerms('node');

        return parent::actionRecentContent($params);
    }
}

There's probably a different/connected issue here, so this probably not an actual solution to the issue.

 

[Xon]

You can also do something like \XF::visitor()->cacheNodePermissions(); which is the same.

I'm sure that report centre can trigger this sort of thing with permission checks

 

[Lukas W.]

This issue appears to be affecting other routes as well. We're currently seeing a traffic volume of up to 95% of all queries hitting that table as some massive blocks on a number of subsites that we can't identify hit it with huge volumes.

I've looked into a more cohesive fix, and determined a workaround in \XF\PermissionCache with @Xon's help:

class PermissionCache extends \XF\PermissionCache
{
    /**
     * @param int $permissionCombinationId
     * @param string $contentType
     * @param int $contentId
     * @return array|null
     */
    public function getContentPerms($permissionCombinationId, $contentType, $contentId)
    {
        $this->cacheAllContentPerms($permissionCombinationId, $contentType);

        if (
            empty($this->contentPerms[$permissionCombinationId][$contentType])
            || !array_key_exists($contentId, $this->contentPerms[$permissionCombinationId][$contentType])
        ) {
            $this->contentPerms[$permissionCombinationId][$contentType][$contentId] = null;
        }

        return parent::getContentPerms($permissionCombinationId, $contentType, $contentId);
    }
}

getContentPerms() is currently triggering at least one query per content ID, when the cache is not or not sufficiently loaded, instead of doing a single-query lookup in these situations. If the queried content ID doesn't exist, it'll trigger one query on each execution instead. There may be a more elegant solution where the cache is only loaded on an actual miss, but I've not been able to see any partial loads other than on the last activity email in the code, so this seemed like a trade-off that was worth it for now.
I don't think there will be a situation where preloading the entire content permission cache as fallback on a cache miss is not a suitable solution. It's a good indicator that the preloading of the subset has been done wrong, and it's probably so rare that the function will be called exactly once, that that's an acceptable trade-off.

 

[Mike]

I'm pretty sure this is more or less one query per node (plus a small number of queries independent of number of nodes). I'm also pretty sure this is more of a bug in the search system when it's going to apply its node constraints to the search request. I think the fix is to just load the node permissions in \XF\Search\Data\Post::getTypePermissionConstraints.

As a general purpose thing, I don't think we'd proactively load the entire set of node permissions on a cache miss. This particular instance is essentially the worst case because we're actually looping through nodes, but the most common case would involve the individual bits of content being shown (like the reports referenced), and there the theoretical maximum number of queries is the number of content items being shown. So there the trade off would be 1 query loading 6500 rows (in the case where there's 6500 nodes) vs 20 queries (if we're showing that many items) loading 1 row each, which probably isn't going to be 100% clear in either direction and might vary depending on system setup (latency to the DB server being one major point).

 

[Lukas W.]

So there the trade off would be 1 query loading 6500 rows (in the case where there's 6500 nodes) vs 20 queries (if we're showing that many items) loading 1 row each

My search result page was showing 1 result, yet 6.5k queries were being performed in the search system (due to XF\Search\Post\Data::getTypePermissionConstraints() querying all nodes for permissions before performing the search.

The problem is, that if this ever happens, it is very costly. Queries to xf_permission_cache_content currently make up for more than 90% of all database calls performed without the fix I applied above. It also will only ever perform this lookup when there's an actual cache miss, so it's already a fallback mechanism for a mistake somewhere else. In this case, the safest approach would be to just look them all up, rather than risk an unknown number of database calls. Especially in the average set of 20-50 nodes it's practically negligible.

 

[XF Bug Bot]

Thank you for reporting this issue, it has now been resolved. We are aiming to include any changes that have been made in a future XF release (2.2.3).

Change log:

Fetch the list of unviewable forums more efficiently when searching.

There may be a delay before changes are rolled out to the XenForo Community.