Improve cache usage and storage.

I just found exactly the same issue with devel query module. The typo is also in the last stable release (version 6.x-1.3)

There is something else with this function. The static variable $users used for caching is now broken. It's set in _realname_make_name() and checked in realname_make_name().
See: http://drupal.org/node/518862

Tracking. Thanks

subscribing

@ayalon - please open a new issue about #1. I think you are right.

@ayalon: I have just committed the patch, along with several others. It, however, did not address whether that statement was needed at all; that is why I asked you to open the other issue.

Committed to 6.x-1.x-dev.

Even with the patch, the logic in this function is wrong. It should be something like:

  if (empty($result)) {
    return $account->name;
  }

  $users[$account->uid] = $result;
  $name_in_db = db_result(db_query("SELECT realname FROM {realname} WHERE uid=%d", $account->uid));
  if (!$name_in_db) {
    db_query("INSERT INTO {realname} (uid, realname) VALUES(%d, '%s')", $account->uid, $result);
  }
  elseif ($name_in_db != $result) {
    db_query("UPDATE {realname} SET realname='%s' where uid=%d", $result,$account->uid);
  }

I agree that it is weird to update the name every times. I though it was some sort of optimisation, something like a "Select take the same times as an Update" (I'm pretty sure it's not the case) so you can always do an update...

I'm not sure exactly what the author was trying to do here.

As Nancy said, this part of the code is probably not needed at all...

Having just chased thru the logic to solve another issue (with content_profile), I can say that some sort of update needs to happen after the _realname_make_name() function is called.

This is the code that calls the different realname "plug-ins" that handle different profile methods. The plugs can return a new name value calculated from the source. This might have been updated thru a method realname doesn't monitor (e.g direct DB change/ content_profile node being updated / maybe even a custom LDAP call ).

Bottom line is that this update check need to be here... in a correct form.

That said, maybe there should be a check to verify that the current name matches the results returned by _realname_make_name(). Then, do the update/insert if it doesn't.

One question to the person who had multiple queries occuring on the homepage. Was this with the -dev version? The caching mechanism has been fixed in the -dev. With this version, this code should only execute once per page. All other calls get resolved via the cache.

So, if it was happening multiple times in the -dev version, there is still a bug in the cache system.... (FWIW - My recent tracing thru the code show it was working... but... ).

@cgmonroe I had multiple queries but fixing the typo (which is the main purpose of this Bug) actually fix that. I now have a descent number of queries. I'm using the last stable release, not the dev one.

We only need to update if the generated realname differs from what is saved in the database. Here is a patch which is basically the same as in #10, with some clearer documentation. This reduces all those unnecessary "UPDATE" statements.

Changing title to be more relevant to the current issue.

@joelstein's patch in #14 makes sense and looks good to me.

I don't think this is really necessary at this point. We don't check if someone actually changed a field on a node when someone presses 'Save' on a existing node form, but we just save it anyway. I don't see the harm here.

I occasionally get into a performance mood and have looked at this. I don't think the update should be there at all. The only time the table should be updated is in hook_user. But I haven't had the time to fully investigate to see if it would cause any problems.

Frankly, while there is a performance hit, I can hardly justify "major" let alone "critical."

Regardless of the priority, I still think it's worth committing. On an intranet for a major health system, we have an "activity stream" of sorts, which (before this patch was applied) resulted in lots of user_load() calls, and the number of unnecessary UPDATEs was not insignificant. This patch is working great for us.

Agreed with @joelstein above - I'm considering using this module on my own Intranet, and initial performance testing on pages listing more than 20 or 30 users at a time get a huge performance hit, and can't easily be cached...

So probably the proper solution is to:
1. Have realname_make_name() first check if there is a realname in the database. If there is, store it in the static cache and return it.
2. Else generate the realname, insert into the database (since if it wasn't already found, we can assume we can just insert).
3. Ensure that hook_user('update') just removes the realname and resets the static variable in realname_make_name().

I was going to comment on the necessity of the database update on each page-load as well, but figured it was better to close this issue (that's already changed direction once), and further optimize in a follow up (which I suppose I really should have commented to).
I agree with @NancyDru that the update shouldn't be there at all: RealName is going to the effort of creating a cache table but only using it for autocomplete calls. The first load of the realname for each account on every page always regenerates the realname and updates the cache (which doesn't help MySQl query caching at all either).

My though was that realname_make_name() seems somewhat overloaded, fetching and storing the generated name. I would suggest separating out generation and retrieval into separate functions.
As well, if an entry is known to exist in the cache table for every user (add / remove on user creation / deletion) we can always just do an update rather than having to worry about inserts as well.

My psuedo-ish-code thoughts for separation:

/**
 * Retrieve the realname, using static or DB cache if available
 */
function realname_get_name($account){
  $result = fetch_from_static();
  if(!result){
    $result = fetch_from_db();
  }
  if(!result){
    $result = realname_make_name();
  }
  update_static_cache();
  return $result;
}
/**
 * Generate and cache the realname
 */
function realname_make_name($account){
  $result = _realname_make_name($account);
  update_db_cache();
  update_static_cache();
  return $result;
}

This would require quite a few more changes, but I think it makes sense.

Here's a patch that tries to improve the cache handling, though keeps it in realname_make_name() still.

An update hook makes sure that every user has an entry in the realname table, and records are now only deleted on user deletion.

realname_make_name() has a reset parameter which will cause it to regenerate the real name and store it in both caches. This is used when a user or content_profile node is updated.

status & title

Is this patch really for the 6.x ? I'm wondering about he use of &drupal_static()

I think this is critical. The current *stable* version has no caching at all, not even static.

Maybe major... I'm using the module on a decent-traffic site without much of a problem... and I will continue using the patch in #10 until a better long-term solution is accepted. As Nancy said earlier:

Frankly, while there is a performance hit, I can hardly justify "major" let alone "critical."

I think we should respect her wishes. A review would get this patch in quicker than a status change ;-)

@franz: drupal_static is conditionally defined by the realname module in order to backport its functionality:

realname.module line 16

if (!function_exists('drupal_static')) {
  function &drupal_static($name, $default_value = NULL, $reset = FALSE) {
    // .... snip ....
  }
}

I think this is also one of several significant outstanding issues in the current stable (and development) release for 6.x, but it is only a performance issue rather than broken functionality or corrupt data and so 'major' is more than enough.
As @geerlingguy said, a patch review is what is most needed to move this issue along.

Sorry about that, 'major' seems more reasonable.

I think this piece of code should be at least better documented, as it can get confusing.

realname_make_name($node, TRUE);

I have nothing else to add to this patch, it looks great!

Here's an updated patch to keep up with head and clean up some formatting.

The only functional change is that when a content profile is updated

  realname_make_name($node, TRUE);

is changed to

  realname_make_name(user_load($node->uid), TRUE);

Hopefully this should be clearer in intent, and will avoid any potential issues of realname_make_name() and _realname_make_name() expecting a user object.

#29: realname-805526-27.patch queued for re-testing.

Here's an updated patch. Not sure if EGit will play nice, so here's both of it's patch output formats.

Or, pull from github: https://github.com/gapple/realname/tree/bug/805526-cache-performance

Status

I believe there is a typo in the database update code. First, update 6103 clashes, so I've changed it to 6104. Second, it should be FROM {users} not FROM {user} which barfs when one tries to run update.php.

In realname.install:

/**
 * Implementation of hook_update_N().
 *
 * Make sure every user has an entry in the {realname} table.
 */
function realname_update_6104() {
  // Create entries in the realname table for all users
  // Values will be generated and stored on first use
  db_query('INSERT INTO {realname} (uid) SELECT uid FROM {users}');
}

Otherwise looks great. +1 for reviewed and tested if this is fixed. I would patch this myself if I knew how (always more to learn). This was pretty critical for me, as I am using CiviCRM and having RealName force a load of CiviCRM for every username was a pretty big performance hit. (I've since rewritten my CiviCRM interface for RealName to be faster as well.)

Thank you for testing the patch and finding those bugs @uayebforever.

Here's an updated patch. I changed the update hook query some more to only insert values not already present in the {realname} table in order to avoid duplicate key errors.

New users never get realname set cause the insert never creates a record. working on patch now

For those using this patch already, another update function needs to be added somewhere-else that is same as one added in this patch (assuming you've had new users since then).

Er, I don't think that testbot failure relates to anything I did, does it?