Currently, og_user_roles_user_manage() works by doing a db_query('SELECT uid FROM {users}') and user_load-ing every entry.

For medium and large sites, this means a totally unrealistic load both on the DB (one full scan table + n queries) and on the PHP (user_load-ing all user accounts).

The purpose of this code is to obtain a list of the accounts in groups managed by the current user, and only them, filtering achieved by an array_intersect() after building that monster list.

This should be done in a cheaper manner, and seems possible with just one query over the {og_uid} table.

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Active » Closed (won't fix)

With the rise of the rewritten OGUR 4.x for Drupal 6, in which many unrelated features of OGUR were removed, and nearing a release of Drupal 7, I'm closing down old issues.

somebodysysop’s picture

somebodysysop commented
Status: Closed (won't fix) » Postponed (maintainer needs more info)

Can you provde the query?

The person who changed the status of these issues to "won't fix" was not authorized to do so: http://drupal.org/node/352139#comment-2352234

OGUR 6.x-1.x will continue to be maintained by me.

fgm’s picture

fgm
Primary language French
Location Paris, France
commented
Status: Postponed (maintainer needs more info) » Active

Hi,

The problem is in og_user_roles_user_manage(). In version 1.1.2.15.2.31, it appears at lines 3130 to 3135. Quoting:

  $items = db_query("SELECT uid FROM {users} ORDER BY name");
  $output = "";

  while ($item = db_fetch_object($items)) {
	$uid = $item->uid;
    $account = user_load(array('uid' => $uid));

Notwithstanding the fact that it might make sense for this query to include a WHERE status=1 and a db_rewrite_sql($sql, 'u', 'uid') to allow for user-based access control, the performance issue lies in that :

  1. the query has no range limit, returning all users in the system
  2. user_load() in the while loop on all users in the system, causing, for each user a minimum of two additional queries in user_load(): $result = db_query('SELECT * FROM {users} u WHERE '. implode(' AND ', $query), $params); and SELECT r.rid, r.name FROM {role} r INNER JOIN {users_roles} ur ON ur.rid = r.rid WHERE ur.uid = %d, plus invocations of the hook_user implementations for each user, meaning more code, and possibly even more queries

Since the call to that function starts with a given gid, it would probably be better to rethink the queries by starting from {og_uid} or {og_users_roles} in order to limit the volume of data being handled.

somebodysysop’s picture

somebodysysop commented
Status: Active » Needs review

Absolutely sane idea. What about:

  $items = db_query("SELECT uid FROM {og_uid} ogu INNER JOIN {users} u ON ogu.uid = u.uid WHERE ogu.nid = %d ORDER BY u.name", $gid);
fgm’s picture

fgm
Primary language French
Location Paris, France
commented

Looks like a better start to me. Do you really need users with status=0 too ? And why don't you apply a db_rewrite_sql($sql, 'u', 'uid') in case someone has applied access control on user accounts ?

somebodysysop’s picture

somebodysysop commented

Want to provide a code example?

fgm’s picture

fgm
Primary language French
Location Paris, France
commented

How about this ?

$items = db_query(db_rewrite_sql("SELECT u.uid FROM {og_uid} ogu INNER JOIN {users} u ON ogu.uid = u.uid WHERE u.status = 1 AND ogu.nid = %d", 'u', 'uid'), $gid);

Note that I removed the ORDER BY. With the current index structure, MySQL 5.1 uses a temporary and filesort to achieve that sort, whereas if you remove it, the query plan is optimal: ogu uses primary index with ref:const using index, and u uses primary index too, with ref ogu.uid and using where.

Since the query does not return the names, maybe it is not needed. At any rate, if name-based ordering is needing, it can be performed in PHP before rendering, which scales better.

somebodysysop’s picture

somebodysysop commented

Note that I removed the ORDER BY. With the current index structure, MySQL 5.1 uses a temporary and filesort to achieve that sort, whereas if you remove it, the query plan is optimal: ogu uses primary index with ref:const using index, and u uses primary index too, with ref ogu.uid and using where.

This is great, thank you. One question: What happens if they are using a version of MySQL earlier than 5.1?

fgm’s picture

fgm
Primary language French
Location Paris, France
commented

I just checked, and exactly the same happens in 5.0: temp+filesort with the ORDER BY, and optimal without (tested on 5.0.51a-24+lenny2-log).