move users.access into its own table

Very nice trick.

I don't get it. Do we have performance benchmarks?

Benchmarking this is quite hard. However, not invalidating the query cache for users is obviously a benefit.

Gerhard, awesome!

Correct me if I'm wrong but I don't see any reason why this should wait for 6.x, would be a nice performance benefit for 5.x.

Why this should wait for D6? Because today is the Drupal 5 release date and this would need a database upgrade which is a particularly bad idea past RC...

I think it would make sense to move the "login" field also to the users_access table (and rename that table). Currently, a user logging in blocks other users from getting a new page or loading a node or ...

subscribed

nice patch. subscribing

Wicked; this has bugged me for a LONG time.

On the mailing list, I expressed that I am ambivalent about this.

Updating the access field only happens in session.inc, and hence no one else is affected. That is good.

On the other hand, any module that wants to print out the last access time for a user will now have a penalty of an extra join for a 1:1 relationship. Breaking a 1:1 relationship into a different table is often against normalization, and usually proves problematic down the road.

Here are the modules affected by this are:

./buddylist/buddylist.module
./devel/devel.module
./erp/erp_home/erp_home.module
./evaluation/evaluation.module
./hof/hof.module
./im/im.module
./inactive_user/inactive_user.module
./knosos/node_access/node_access.module
./og2list/og2list.module
./print/print.module
./profile-ng/profile.module
./syndication/syndication.module
./user_list/user_list.module
./welcome/welcome_demosite.inc

If everyone else decides to go with this patch, then you need to modify this part too in user.module, and add an extra join for the who is new block.

$result = db_query_range('SELECT uid, name FROM {users} WHERE status != 0 AND access != 0 ORDER BY created DESC', 0, variable_get('user_block_whois_new_count', 5));

Setting this to needs work for the above query.

@kbahey - there isn't much question about this rightness of this patch. the current situation penalizes everyone so that the handful of modules that you mention can avoid a JOIN.

Point taken.

Other alternatives:

Has the users table been converted into InnoDB on drupal.org?

Perhaps playing with various locking mechanisms with it being InnoDB may alleviate the problem, since only one row at a time need to be locked when updated.

Details here http://dev.mysql.com/doc/refman/5.0/en/innodb-lock-modes.html

Here is an updated list of modules.

./affiliate/affiliate.module
./buddylist/buddylist.module
./devel/devel.module
./erp/erp_home/erp_home.module
./hof/hof.module
./im/im.module
./inactive_user/inactive_user.module
./print/print.module
./profile-ng/profile.module
./syndication/syndication.module
./tasks/tasks.module
./user_list/user_list.module

Here is the command used:

$ cd HEAD/contributions/modules
$ find . -name "*.module" -exec grep "{users}" {} /dev/null \; | grep -w access |awk -F: '{print $1}' | sort -u

This is interesting. We don't explicitly lock the users table so I don't understand the write lock problem. Perhaps this is an internal MySQL thing I don't know about?

I benchmarked this patch locally and didn't see any performance improvements, but I only benchmarked as a single user with (ab -n 50 -c 5 -C cookie_value localhost). My benchmark only really shows there's not a performance degradation for low-traffic sites, really.

Thanks for the reference material. I tested the patch differently now and can confirm the LOCK contention. I'm using a slightly different patch, one with the login table still a part of the users table. Moving the login column is such a small performance gain compared to the access column that I prefer to keep it with the rest of its sibling data.

Before the patch (using ab -n 50 -c 5)

mysql> SHOW STATUS LIKE 'Table%';
+-----------------------+-------+
| Variable_name         | Value |
+-----------------------+-------+
| Table_locks_immediate | 2111  | 
| Table_locks_waited    | 34    | 
+-----------------------+-------+

After the patch:

mysql> SHOW STATUS LIKE 'Table%';
+-----------------------+-------+
| Variable_name         | Value |
+-----------------------+-------+
| Table_locks_immediate | 974   | 
| Table_locks_waited    | 1     | 
+-----------------------+-------+

Subscribing.

I had chance to revisit this patch and cleaned some things up. Should be ready for another review now. I added in the system.install table changes and merged the user.install updates to a single function. I also added documentation and a little bug fix to user.module.

And the user install file.

Killes found an error in my last patch. Let's try this one.

i read through the patch and looks good. i think this comment is still valid and should remain.:

"// TODO: this can be an expensive query. Perhaps only execute it every x minutes. Requires investigation into cache expiration."

These two queries are the only writes that we perform on a typical page view. We should aim to get rid of them. Especially the one for access time. I would think that if we write that once every 5 minutes it is enough. As it says, we just have to work through cache expire stuff.

anyway, i am arguing about a comment which is obviously quite minor. nice work.

This patch is ready to go, anything holding up for 6.x commit?

i tested this. rtbc.

fyi, you can create diffs including new files with fakeadd.sh

Dries, I bet the drupal.org infrastructure team will be able to provide numbers probably, if the above is not convincing.

Is it me, or isn't there an upgrade path? Where is the users_access table created?

bumping, this should still be fine, although it probably needs a reroll

Rerolled. Moved things to user.schema since that is where we define schemas now. user.install is a new file and provides an update path.

FYI, I have patch in which signficantly reduces writes into users.access. see http://drupal.org/node/40545

my patch got in - http://drupal.org/node/40545. i am tempted to mark this won't fix. we are now not writing into this field very often. at leats, we should postpone ot D7 IMO and see if this is needed after some real world experience. if someone agrees, please change the issue.

Given that Moshe's patch went in, I'd like to see us re-evaluate the need for this patch. Moshe's patch should help both MyISAM as InnoDB users, and is likely to buy us more cycles than killes' patch.

Seeing that updating the login timestamp can be that slow, makes me wonder though. The strange part is that only 38 queries (out of hundreds of similar queries?) were slow -- that doesn't seem like consistent behavior. Are these queries slow day after day or was it a one time observation?

All in all, it's probably best to focus our time on more pressing performance patches, and to re-evaluate this patch during the Drupal 7 development cycle.

Subscribing...

i think killes agrees thats this patch is no longer desireable for D6 and beyond. if i'm wrong, please reopen.

killes brought this patch up again on irc. It'd be cleaner to normalize this information properly I think, and we're firmly into Drupal 7 now.

Avoiding a write query on every page is hardly a hack or a band-aid. Personally, I think a new table just adds a join for little benefit. But feel free try to get this in. I won't block it.

Is there any solution to this problem for 5? We have a lot of users/nodes/pageviews and are seeing long wait times for users table (and sessions table) related UPDATES. Since there is an update to the users table for latest access time on each page load I am wondering if this lockout problem is in fact the biggest contributor to database lag. Thanks!

-Drew

Moving to D8.

#1835496: Factor {users_field_data}.access out of the table and entity caches is a duplicate. It's newer but has a better issue summary (this one is outdated since comment ~#41), so closing this one.