As mentioned in #1976696: Polyfill permissions for the Node module, we need to make sure we don't create a huge performance problem with our node access system.

CommentFileSizeAuthor
#10 group-node_access_performance-2048397-10.patch21.78 KBkristiaanvandeneynde
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde commented
Status: Active » Closed (duplicate)

See #2048393: Workflow for creating nodes in groups

chaby’s picture

chaby CreditAttribution: chaby commented
Priority: Normal » Critical
Status: Closed (duplicate) » Active

Re-open it and set it as critical because I fear that what I have mentionned here is still topical...no ? (having X conditions with query node access altered with grant system for outsiders with a lot of groups he doesn't belong too and groups are from the same group type with allowed ACL for outsider).

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde commented

In reply of #1976696-39: Polyfill permissions for the Node module: For every grant you have, there will be a condition added to the query.

  • If you have 100 groups of a group type
  • Said group type allows view access to 5 different node types
  • You are outsider to all 100 groups

You'll get 100 * 5 = 500 view grants or condition for a node_access query.

The only upside to this performance beast is that single node access queries first filter on nid:

SELECT 1 AS expression
FROM node_access node_access WHERE
(grant_view >= '1') AND
( (nid = '1') OR (nid = '0') ) AND
(
    ( (gid = '0') AND (realm = 'all') ) OR
    ( (gid = '1') AND (realm = 'group:page:view') ) OR
    ( (gid = '2') AND (realm = 'group:page:view') ) OR
    ( (gid = '1') AND (realm = 'group:article:view') )
)
LIMIT 1 OFFSET 0

Where the query tag filters on the permissions first:

SELECT COUNT(*) AS expression
FROM node_access node_access WHERE
(nid = '0') AND
(grant_view >= '1') AND
(
    ( (gid = '0') AND (realm = 'all') ) OR
    ( (gid = '1') AND (realm = 'group:page:view') ) OR
    ( (gid = '2') AND (realm = 'group:page:view') ) OR
    ( (gid = '1') AND (realm = 'group:article:view') )
)

This still sucks major balls though.. :/
I'm going to Google a bit on the consequences of having queries with a lot of conditions.

chaby’s picture

chaby CreditAttribution: chaby commented

You are right. These two queries (from node_access() and node_access_view_all_nodes()) are first filtered by nid and op. So we could hope that SQL interpreter will only apply grant conditions on it (gid + realm) after filtering it with indexed nid. So only with grant module group, it should apply it to 3 rows for node_access (realm : bypass, admin, view) and no one for node_access_view_all_nodes.
So as you say, probably the question is : is that support a lot of conditions ?

But I'm curious about the query executed in _node_query_node_access_alter() :
If the original query is a listing of 20 nodes without explicit filter (node type only for e.g), it will fetch nodes tables , filter per type and make a correlated subquery on nid for each rows as limit should be applied later ? And as you know, this subquery could have a lot of conditions...
TBH, I don't know SQL interpreter well and need some explain query with a real-life database test (lot of groups, node access and so on) to check it !

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde commented

See #106721: Optimize node access query building

chaby’s picture

chaby CreditAttribution: chaby commented

Huuum...very interesting ! Using 'IN' clause instead of adding a lot of conditions should improve performance. And maybe having a IN condition with 3000 integer (gid) should not be very dangerous.
However, this patch must be applied to the core to resolve this issue (returning a lot of gids for outsider if permissions are set).
If this issue could not be resolve inside group module itself, a minimum precaution would be to warn end-user about this performance issue (for e.g on the group permission overview, like 'be careful, set outsider permissions could have performance issue with grant system')...no ?

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde commented

Ok so from what I've gathered (hours of research), we could reduce the amount of access records significantly by setting only 4 records per node. All of those records could grant full access to a node. However, because hook_node_grants() provides us with an $op parameter, we would only supply grants for said all-access locks when the user actually has the permission to perform the provided operation.

Currently for node 2, group 2, user 3 this is written:

nid gid realm view update delete
2 2 group:administer 1 1 1
2 2 group:webform:delete 0 0 1
2 2 group:webform:update 0 1 0
2 2 group:webform:view 1 0 0
2 3 group:2:webform:delete 0 0 1
2 3 group:2:webform:update 0 1 0
2 1986 group:bypass 1 1 1

 

Proposed change for node 2, group 2, user 3:

nid gid realm view update delete
2 2 group:administer 1 1 1
2 2 group:webform 1 1 1
2 2 group:3:webform 1 1 1
2 1986 group:bypass 1 1 1

 

Important changes:

  • All access records could (but in practice won't) grant full access.
  • We use the group id as the grant id for 'OPERATION own NODETYPE' instead of the user id.
    That way, if #106721: Optimize node access query building makes it in, we'll get a lot more performance out of the IN() condition.

 

Alternative solution:

Screw node access and we write gnode_query_node_access_alter() instead.

chaby’s picture

chaby CreditAttribution: chaby commented

Ok, if I understand :
- less node records but with grant hook, it returns grants only if account have permission for this op (by using user access and group_access) ? If true, why not, we 'll loose visibility but decrease number of rows in node_access table on which could be applied a lot of conditions. So it should be a good improvement.
By the way, why not removed also grant 'administer' and 'bypass' ? In hook grant, one could be checked with group_access and other with user_access ? If at least one match for the op, returns appropriate grants, no (so all perm) ? So if I'm not wrong, we could reduce it to 2 rows only :

nid gid realm view update delete
2 2 group:webform 1 1 1
2 2 group:3:webform 1 1 1

- however as you say, we will loose the real ownership of the node : is that the author = the group ? Is that meaning a member of the same group could edit a node of another member if he have the permission edit own and not the global perm edit any ?

Screw node access and we write gnode_query_node_access_alter() instead.

Is that won't be real nightmare with other grant modules ? (just a question, I have no point of view for now ;-) ).

chaby’s picture

chaby CreditAttribution: chaby commented

Ouch, not really...didn't think about node unpublished...so we needs at least 3 grants (one more for unpublished node allowed for bypass and admin).

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde commented
Status: Active » Needs review
FileSize
group-node_access_performance-2048397-10.patch21.78 KB

Okay, I've thoroughly reworked the node access system for Group Node and came up with this:

  • use hook_node_access for every operation
  • use our own 'node_access' query tag alter for list queries

Please review, the todo is about the fact that I still hardcodedly assume the node table is aliased with 'n'.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde commented 
$user->uid == $account->uid

should of course be

$account->uid == $node->uid

My bad

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde commented
Status: Needs review » Fixed

Right, I'm committing this. We can always revert to node access if necessary.

Status: Fixed » Closed (fixed)

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

  • Commit 315fb86 on 7.x-1.x, ctools_plugins, inheritance by kristiaanvandeneynde: 
    Issue #2048397 by kristiaanvandeneynde: Kicked node grant system to the...