Improve sessions performance

This patch should be extension to fix user_load() where dynamic queries look like: SELECT u.* FROM users u WHERE u.uid = 0 AND LOWER(u.status) = LOWER('1') AND u.status < 3 LIMIT 0, 1

Seems to be very related queries which would boost things up a bit indeed.

Above patch doesn't apply to CVS :-)

Here's a new patch that applies and also fixes the only other u.status < 3 query that I could find in core. Both should be eliminated now and minor performance improvements as a result.

By the way, that dynamic query above now is:

SELECT u.* FROM users u WHERE u.uid = '1' AND u.status = '1' AND 1

1 is needed because various parameters in there, not consistent, only way to always work.

Here's an even better patch, we don't need the slower db_range in there anymore either.

Ok even better patch, using implode thanks to killes suggestion to further simplify things.

looks much nocer now. i am still wondering why we had the LOWER and the db_query_range there in the first place, though.

Yeah I was scratching my head about that but I see no reason for it to be used, my best guess is that it was some legacy code that just was never really ever updated, I'm going to set this to commit then, Dries, I'm sure you'll be able to enlighten us on that db_range() stuff. :-)

I guess this LOWER() stuff was because the author wanted to match some fields ignoring case. You could use it for matching e.g. mail (the local [account] part *can* be case-sensitive, but usually isn't, so it'd be good to compare it case-insensitive) or other text columns (themes, signature, language and others)

Correct sollution would be not to build the query autmatically, but explicitly using column names.

This does not make any sense, the query needs to be made dynamically, if you take a look at all of the user_load() calls in user.module, you'll see that they each pass in various paramaters at various times (uid, status, etc...) no way to *not* build this dynamically. This patch also *isnt* changing anything, it is merely simplifying things and possibly introducing some small performance tweaks (simpler queries).

This does make a lot of sense, the query does not need to be made dynamically, if you take look at the user_load() definition you'll see that it uses the parameters with {user} only so we know what columns are possibile (uid, status, etc...) so there *is* a way to build this dynamically. The patch is also removing LOWER() form values which can change the behaviour.

so there *is* a way to build this dynamically

Of course I meant "to build this not dinamically".

code looks good to me.

code looks good to me.

Cvbge: are you saying this should be committed as is, or not? Did you test this patch on PostgreSQL? Is using '%s' for integers a problem?

As for the db_query_range(); I vaguely remember that sessions.sid wasn't always a unique or primary key.

I'll commit this patch as soon Cvbge is happy with it. :-)

Dries, I do remember that sessions sid IS a unique key and before we made it primary key, we inspected Drupal.org database and it had VERY few number of duplicate sids which we deemed as error.

What I ment is that this patch changes the user_load() behaviour.

Currently the values are lowecased, so e.g. user_load(array('mail' => 'Joe@Doe.com')) will load user with mail Joe@doe.com, but after this patch it won't do that.

This could create problems.

Or course, the questions is, do we want to lowercase all, or some values? If yes, or if we want to do it as it's done now, we should change the patch.

I don't have other problems with this patch.

Cvbge, ok I understand what you are saying now. I just checked core, we have 9 out of 40 calls to user_load() that use a string: name or mail, which *might* need the lower(). Ok, I have a new idea for a patch that should fix this and still simplify things...

Ok here we go, an updated query that still leaves the LOWER() in there but changes up the logic a bit. Since a majority of the calls to user_load() are based on uid or status, I put these as the first checks in the if/else bracket since this will save time parsing through the others. Since there are only a few calls to name and mail, this is the last check and LOWER() is used on these and all others, assuming any other calls to user_load() might be calling fields that need this.

Overall, this patch should be solid now and ready to commit, so someone please review and set the status as needed!

ok with me.

Applied to local test site, clicked here and there, created user, logged in/out, haven't seen any problems.

BTW, wouldn't db_query_range() be faster, or at least as fast as db_query()? Just wondering, really ;)

An after-note, just to educate people ;)

Is using '%s' for integers a problem?

yes, it can be. If you pass a NULL or FALSE to '%s' it'll be changed to ''. This value will be inserted into mysql and automagically changed to 0 (or default value, don't remember). But it will produce error for postgresql.
So it's better to use %d, NULL/FALSE will be changed to 0 in both mysql and postgresql.

Committed. Good catch, good work. Thanks guys.