The NodeGrantDatabaseStorage::alterQuery() method appears to be doing extra work which could be avoided: it builds a constant array of grants in a loop on all tables in a query instead of building it just once:

    $grants = node_access_grants($op, $account);
    foreach ($tables as $nalias => $tableinfo) {
      $table = $tableinfo['table'];
      if (!($table instanceof SelectInterface) && $table == $base_table) {
        // Set the subquery.
        $subquery = $this->database->select('node_access', 'na')
          ->fields('na', ['nid']);

        // If any grant exists for the specified user, then user has access to the
        // node for the specified operation.
        $grant_conditions = static::buildGrantsQueryCondition($grants);

Some CPU use could be avoided by factoring the buildGrandsQueryCondition out of the loop, as that is a pure function which takes invariant arguments.

Similarly references and calls to \Drupal::languageManager()->isMultilingual()) are invariant and could be taken out of the loop.

Comments

fgm created an issue. See original summary.

fgm’s picture

Issue summary: View changes
fgm’s picture

Status: Active » Needs review
StatusFileSize
new3.13 KB

Suggested patch. Let's see if this causes any unexpected fail.

ndobromirov’s picture

Status: Needs review » Reviewed & tested by the community

Why the rename of subquery to sub_query this makes a lot of noise on the change that is done here.
What was the potential win here if you can share some benchmarks?

Other than than I would RTBC it, as the rename can be fixed on commit.

fgm’s picture

The rename was to apply current coding standards, since the patch was already touching the same loop, but is indeed otherwise unrelated.

The win is likely to be very small, as explained in the issue, sub-millisecond on each query, only meaningful on non-core-default sites with at least one access control implemented (ideally several e.g. domain + content_access) and pages with many node list requests, because this is where this will actually run many times.

Regarding the fails, the MySQL 8 one appears to be a bot fluke, and the PG one seems to be an error in core in the array comparison from an unordered map.

catch’s picture

Status: Reviewed & tested by the community » Needs work

Let's drop the rename here before commit and just do the main change, it's touching lines that otherwise wouldn't be affected by the patch.

init90’s picture

Status: Needs work » Reviewed & tested by the community
StatusFileSize
new2.06 KB
new2.1 KB

I've done changes according to last comment.

Replaced '$sub_query' to '$subquery'.
Also, I've added small additional change - renamed $multilingual variable to $is_multilingual for consistency with method name.
Since my changes were trivial, I'm returning status to RTBC.

tstoeckler’s picture

Thanks, looks great. RTBC++

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed 0d11203 and pushed to 8.8.x. Thanks!

  • catch committed 0d11203 on 8.8.x
    Issue #3066637 by init90, fgm, ndobromirov: Avoid repeated work in...
catch’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.