Remove database locking
chx - March 23, 2006 - 17:45
| Project: | Drupal |
| Version: | 6.x-dev |
| Component: | database system |
| Category: | task |
| Priority: | critical |
| Assigned: | David Strauss |
| Status: | closed |
Description
This patch removes the locks from Drupal. Backwards compatibility is kept (for a change).
| Attachment | Size |
|---|---|
| no_locks.patch | 6.03 KB |
#1
#2
mysqli db_next_id was forgotten.
#3
patched the files, ran update.php. I saw 'unspecified error' flicker briefly in the progress bar just before the page reloaded and said things had gone well. Everything SEEMS to be working though, including inserting/updating/etc in situations that previously locked tables. so far so good, further updates as events warrant.
#4
This is practically a dup of the postponed Getting rid of the sequences table issue. Although, that one proposed a db_insert_id() to replace db_next_id(), this new approach looks promising.
Would you mind to continue in that issue?
#5
For postgresql, this just won't happen. At least not easily. Problem here is that if we have two requests that want to replace the same row, then the second will fail and that's bad. Cvbge, please comment.
#6
#7
May I ask why do you keep the {sequences} table? As far as I can see it is only used by the core itself, and only for MySQL.
#8
Because I want this in 4.7. For 4.8 the issue you linked is a possibility. For 4.7 this is a huge performance benefit for MySQL users while PostreSQL users neither lose neither gain anything. And finally, many contrib uses db_next_id -- why break BC when you can get away with it?
#9
I think I see, db_next_id is kept for backwards compatibility, though I believe this implementation wouldn't work.
<?phpfunction db_next_id() {
db_query('INSERT INTO {sequences} VALUES (NULL)');
return mysql_insert_id();
}
?>
That is, db_next_id() is supposed to return the key of the specified table (argument $name removed here), not the next key of the {sequences} table.
#10
In the PG version of db_query_replace()
<?php$queries['insert'][0] = 'INSERT '. $queries['insert'][0]
?>
1) Is the keyword INTO missing?
2) semi-colon is missing.
#11
What you need is a unique indentifier. And this is unique. It's just that it's unique not just for nodes but users, menu items, comments etc. But fear not that we will run out of numbers, I am using a 63 bit counter.
And now database.mysql joins the patched files.
#12
INTO is optional, but why not add it? Yes, that ; was missing thanks for catching.
#13
Are all the related tables using BIGINTs as well? ;-)
#14
Are there performance issues (for joins, sorts, etc.) when using BIGINTs?
#15
Maybe you could still keep the existing design of the sequences table and use the REPLACE trick to get rid of the LOCK table in db_next_id?
#16
Well, I could use integers, yes, four billion objects are ought to be enough for any Drupal site. (no, we do not support all google pages as nodes until you donate a server farm comparable to Google's for development purposes.)
#17
Ops, the user module patch does not belong here.
#18
Some good idea and some good time wasted.
#19
#20
critical?
#21
There we go. We have two new functions, _db_query_sprintf which is just sprintf for mysql(i) and the complex horror for pgsql it was. blob placeholder is now a define. REPLACE implemented. The postgresql implementation is based on the SQL by Joshua Drake (whee! did you know that he is into Drupal?)
Be wary: completely untested.
#22
a little cleanup...
#23
after testing, a new version (thx Eaton).
#24
Haven't had a chance to look too deeply at the code, but it appears to be working finr, going through both an update to an existing database and a full fresh install cycle. It's bit disorienting to create a node with $nid = 47, but it seems to be working smoothly.
Becuse the ids are shared between tables we now need to pay attention to modules that churn through lots of IDs -- aggregator.module, for example, can be cron'ing away in the background creating hundreds of records (and using hundreds of IDs) an hour. Also, it might be worth looking for cases where we can use updates instead of deletes/inserts etc. In tables like {system} it doesn't matter, but in others it might.
#25
Mine db_next_id solution is clumsy compared to http://drupal.org/node/77312 . I removed the error handling from there as we do not check ever whether the database disappeared or not... I added mysqli, though.
#26
related patch - http://drupal.org/node/77312
#27
To make it clear: when this gets in, plumbley must be mentioned as the db_next_id is his. This is critical as locking tables are really not nice.
#28
sammys corrected db_query_replace for postgresql. the
for updateis not after SELECT but at the end. I feel so good that many other people help this patch.#29
Someone cares to benchmark this? :)
#30
Here's a patch for 4.7 for people who want to test it against a 4.7 install.
#31
I've done some preliminary testing and can't see much of a difference.
#32
I am 100% that sprintf is faster than preg_replace_callback... even if you can't bench a difference, given enough queries there will be a difference however small. Ah and there is no disadvantage as far as I can see.
On another note, db_query_replace is sorely needed. Just look at http://drupal.org/node/77924 .
#33
The 4.7 patch didn't apply totally cleanly so I re-rolled that patch with a fix for the block.module in there so people could test that out too.
This patch is for DRUPAL-4-7.
#34
Oops, forgot to remove the DELETE in block.module.
Here is another for DRUPAL-4-7.
#35
I can confirm that for high traffic sites, the locking of tables is a severe disadvantage. I'll see if we can test this patch on the site in mind.
#36
Plus I can confirm that using LOCKs is not possible on Hungarian shared hosts, so those using Drupal on those hosts need to comment these out, which makes for a possibly vulnerable system without the solutions presented here. Unfortunately I don't have the time to test the patch though, but definitely agree that it is critical for the next Drupal release.
#37
Per killes' request I mutilated my beautiful patch.
#38
Reroll with mysqli junk removed.
#39
Going to benchmark this today (if all goes well). I already spent 2 hours setting up a good benchmark infrastructure for this.
#40
I spent a couple hours benchmarking this patch, but it doesn't seem to make any difference regardless of my benchmark configuration.
Attached is the result of my last benchmarks. All accesses were done by 20 concurrent anonymous visitors with page caching enabled. One out of the x accesses resulted in a cache miss, which will cause the page to get cached. In order to put a page in the cache, we need to take a lock. So when x is 2, for example, 1 request out of 2 will put a page in the cache and will require the lock to proceed. With 20 concurrent visitors, this means there must be some contention.
I don't think that's the case because (i) the graph is fairly linear (which suggest that lock contention isn't a problem) and (ii) the no-lock patch didn't improve performance.
#41
On the other hand, removing database locks from core expands the number of hosts Drupal can run on, doesn't it?
#42
Drupal.org:
show status;| Table_locks_immediate | 648542721 |
| Table_locks_waited | 12206055 |
On drupal.org less than 2% of the locks have to wait ...
My test setup:
| Table_locks_immediate | 15392266 || Table_locks_waited | 20863 |
On my test machine less than 0.2 of the locks had to wait ...
Looks like I might have to tweak my configuration setup some more so that at least 2% of the locks have to wait.
#43
I wish we could benchmark this on a client's site of mine. There, when we look at the MySQL processes at peak times, the whole page are stuck waiting on locks (100%). I can't confirm for sure whether this is due to some shady programming in a contributed module or not, but the queries that we see in the queue are the same ones addressed in this patch.
#44
I figured out what is wrong with my benchmark setup. Fixed it. The benchmarks are running again but they take 3 hours to complete.
Robert: could you check which table is asking for the lock (instrument db_lock_table and have it write its table names to a file):
<?php
static $file = NULL;
if (!$file) {
$file = fopen('/tmp/lock.txt', 'a+');
}
fwrite($file, "$table\n");
?>
variable_set() asks for a table lock so if modules are abusing the variables table to store data (eg. per-page counters), you might spent a lot of time waiting for locks ... if that is the case, you might want to write a stack trace to disk in order to find the culprit.
#45
chx, robert: looking at the code ... it doesn't make 100% sense to me. If we're going to have backend specific code, we might as well simplify this and use MySQL's REPLACE function. Introduce a
db_replace()and get rid of all the ugly regex? Both the variables table, the session table and cache table could take advantage of that function.If you provide a patch, I'll benchmark it and I'll compare it to the existing no-lock patch! :)
#46
IMO, neglible performance change is a *good* thing. This patch is about removing a big support problem.
#47
This patch is also about avoiding critical data loss issues which is why I think it belongs in 4.7. See my post about two users administering blocks at the same time http://drupal.org/node/77924.
#48
Rob, data losses can be prevented with locking. We try to avoid that now.
I would like to point out that the "ugly regexp change" I do in db_query_replace is modelled straight from db_query_temporary. But the cost of one regexp change is so minimal that it's dwarved by writing the database anyways.
#49
Well, I think it is a lot of ugly code (regex here, regex there) to work around this problem. I mean, if it gets this ugly, we might as well do a:
<?phpif (db_type == 'mysql') {
// MySQL specific code
}
else {
// PostgreSQL specific code
}
?>
That is ugly as well, but at least it is (i) faster and (ii) readable.
#50
Here is another graph. 20 threads are requesting cached pages, and every once in a while (x-axis), the cache is invalidated. This will result in one thread regenerating the page, and grabbing the global table lock to store it in the cache.
The no-lock patch improves performance a little bit.
#51
I thought I asked about locking on IRC for the admin > blocks data loss problem at http://drupal.org/node/77924 and you said that it wouldn't help; that this patch was what needed to get in for that critical (IMHO) data loss issue.
But it seems for my issue of two admins deleting all block data, moving the DELETE and INSERTS next to each other and locking the blocks table would prevent data loss, no? I will refactor the patch there to work with the current db implementation and let me know if that would fix it.
#52
Dries, one preg_replace per a REPLACE type query is not bad at all. Especially because the regexp is very, very simple it's almost str_replace just a safer one.
What you suggest is not a generic solution -- you will need to write MySQL / PgSQL specific code for variable_set, for cache_set and so on for any such function. I provide an API. And when tostinni finishes (if he ever does) his Oracle specific code, you need to go over again, and add that. And so on. I am very much against that solution.
Another solution would be a query builder for insert query builder. I am not sure whether I want that or not but you really do not want that so that's a moot point.
#53
OK, we have a small performance improvement, we have a cleaner API, we have better support for free hosts (which disable LOCK for you). What is missing? Is it only some code cleanup?
#54
Code clean-up would be nice (I don't like the regex -- the code is quite ugly).
Also not that we'll always need table locks. For example, RobRoy's block.module fix I just committed introduces table locks in the block.module. This renders the non-performance argument of this patch moot.
I'll look at this some more as time permits -- I already spent hours testing/benchmarking this patch. I'll try to do some more testing next week. (I think this is very much about performance.)
#55
The problem with locking is tied more to MyISAM than InnoDB type tables.
There is a patch: http://drupal.org/node/78503 ... it introduces a new function "db_engine_type" . If we use that function, we can see if InnoDB is supported (I'm guessing this will be almost always the case) and then switch the following tables to InnoDB:
variable
cache
blocks
sequences
Those are the only tables that we actually lock in core. As InnoDB types, we should see a nice performance boost on heavily trafficed sites with lots of concurrent users.
#56
I think the regexp is okay.
#57
Based on #53 and #56 , really what's missing?
#58
There is a lock in block.module which renders the non-performance argument moot.
#59
Sorry, I disagree, the lock in block.module is only executed if somebody saves the block admin page. For the cache_set function it is quite nice to not have the locks. Due to the many times the cache_set function is executed, there are quite a lot of lock queries done. These queries don't take a lot of time, but why not avoid them?
I think that we will never be able to entirely work without db locks.
#60
Actually, that is incorrect. The locks in block.module execute when somebody views the block admin page as _block_rehash() is called on each view of that page to refresh the list of blocks for the user to modify.
But I still think that removing some locks is better than not removing any.
#61
I agree, moving the other locks are much more important--those are ones that can affect lots of visitors to your site which can really slow down a *major* site.
The block lock is certainly needed, but in terms of performance, that should be of little effect, since it would only be admins visiting that page.
#62
If this goes in I will implement a variable-based semaphor for the block page. But that will cause a big debate which shall not hold this up.
#63
I suspect the whole block rehash madness could be better designed so it doesn't delete everything and readd it on viewing the page. The point is, these problems can be solved and we should concentrate on single changes and get them done. We have to start somewhere and every lock removal needs to stand on it's own merits anyway.
#64
now there is a conflict in cache_set(). sigh.
#65
re-rolled
#66
I'm going to postpone this patch. We'll work on it later.
#67
If you only concern is that this patch doesn't remove all locking instances, look at:
http://drupal.org/node/80963
This removed the last remainning lock in core. I'd prefer to keep the db_lock function nevertheless.
#68
That other one isn't in yet (although it seems like a good idea, and not because of locking).
Can we break this into two patches since it removes two unrelated use of locking? It would be nice to review each on their own merits.
On the next_id one- why not just use auto increment? I postgres has something suitable for this.
On the replace one- that code looks annoying to write, but I can't think of any way to make it better.
#69
this patch missed two releases. i don't care any more. not my problem. if someone wants to fight blatant ignorace masked in unfounded requests, then do it.
#70
no comment
#71
For the record, I think removing use of database locking from core is a good idea because many smaller hosts don't allow that permission.
I think having a less-clunky way of replacing rows is a good idea. And I think having a better way of dealing with sequences is a good idea (although I'm not sure why we are keeping that table around when, as far as I know, don't really need it).
#72
Agree
#73
After 4 hours, i finally patched files, but why I still get the error message even I applied the patch?
This is the error message when I tried to create a new page.
user warning: Access denied for user: 'xxxxxx@%' to database 'xxxxx' query: LOCK TABLES cache WRITE in /home/content/............/html/includes/database.mysql.inc on line 120.
Please help, thanks!
#74
Using auto_increment in the sequences table and returning
mysql_insert_id()indb_next_id()also allow us to setup mysql replication with multiple masters. This sort of replication requires insert in more than one database at the same time.We want to setup mysql for high availability using a master-master replication with following settings:
In the Master A server my.cnf file we would have:
auto_increment_increment=10auto_increment_offset=1
And on the Master B:
auto_increment_increment=10auto_increment_offset=2
This way the keys inserted in one database never match with the other, i.e. the keys inserted in Master A database would be 1, 11, 21 etc and in Master B 2, 12, 22 etc. But for this to work with drupal, we need the keys to be generated by the auto_increment feature.
-1 for the last patch.
+1 for the patch in comment #23.
#75
You can tell me what patch to apply, because there are many and I need its since I am a slave of Godaddy and a lover of Drupal
#76
don't hijack issues.
#77
Following this discussion. +1 for getting rid of table locks.
#78
Not because I do not care -- I still do not and will not change ever -- but because Dries said so, this is postponed.
Just clearing the Drupal5 queue.
#79
I also agree we should get rid of the table locks. Two of our biggest sites are suffering from this issue at the moment and we have no quick fix for them. @Dries, this is a serious problem for us. Please review this issue again.
#80
This won't happen in 4.7.x... 6.x at the earliest.
#81
Thanks to chx and niosop it is possible to avoid using LOCK TABLE and CREATE TEMPORARY TABLE.
I know I'm not alone in having a shared hosting package which doesn't allow these statements.
Here is a minimal patch for 4.7.4 which removes both safely. There are still some calls to LOCK TABLES but these are in admin places so will simply give an error message but not cause problems in the usual case of a single admin. The comments in the patch say where the code comes from - none of it is mine.
Note this only works for MySQL.
HTH, Peter
#82
Oops - forgot to name the cache table.
#83
Subscribing to this issue.
#84
Is there a patch available for Drupal 4.7.5?
It would be nice if you could also add a patched version of bootstrap.inc, database.mysql.inc, search.module. I realise this is a rather stupid question, but somewhere on this site said quite correctly that even experienced users (admittedly w*** users) sometimes have problems patching their files.
If anyone could post the patched 4.7.5 files, it would be really nice.
Thank you very much, great job, I'm impressed,
Felix.
PS: you should make more publicity for your thread, it's by far not the first one you find when performing a search
#85
Restoring title...
felixsmile, please do not change the title of an issue when you comment on it. The title is for the whole thread, not for your comment. Thanks.
#86
Doesn't need to be postponed either now that 6.x branch exists.
#87
I'm sorry, I noticed it a bit too late, and then, I thought a double post would be worse. Next time I'll fix it myself. Thank you for telling me.
#88
subscribing
#89
I've updated chx' latest patch.
#90
Subscribing.
#91
chx has pointed out, that my patch has cruft in it and that we should look into using INSERT ... ON DUPLICATE KEY UPDATE
http://dev.mysql.com/doc/refman/4.1/en/insert-on-duplicate.html
#92
We looked at it and it's hard to convert INSERT syntax into that. Still I leave the issue at CNW because there is cruf.t
#93
Updated the patch and remove the cruft.
I have been running this with a test system of mine. The tests consist of letting 5 rampaging wgets on an unsuspecting Drupal site. The filter cache was cleared before this. With this patch, the script took 15m15s without 16m16s. About 10000 filter strings were generated and stored.
#94
+1 for getting rid of table locks.
#95
This looks like it's really working great. Please excuse me if this is a naive question, but I couldn't find any answer to this here:
what are the plans for versions 4.7.5 and 5.0?
If I understood all the discussions around the topic, this is particularly interesting as even when a user has the lock tables privilege, Drupal runs faster without locking the tables. You should do more advertisement, for sure :)
Thanks
Felix.
#96
There are no plans to backport this patch (as usual). However, you can probably safely apply it to your site's installation. I am running it on drupal.org now and haven't yet seen any problems related to it. I'll wait a bit longer and then mark it as rtbc.
#97
I just had an experience on site that converted some of its write-heavy tables to InnoDB which actually caused more overhead since the LOCK TABLES queries are still run since it causes InnoDB and MySQL to both perfom locks. Once I commented out the LOCK and UNLOCK queries since InnoDB was handling it's own transactions, we saw a performance gain.
#98
Maybe I didn't understand what you said, I thought adapting the patch for versions 4.7.5 and 5.0 would not really be backporting but updating. That's what I really meant: does anyone have any plans to update the patches or, even better, has already done it and could post the patch or the "finished products"?
If no one has, I will try and look at it, comparing the files of version 4.7.5 and 4.7.4. If the lines concerned are identical (or similar), I'll patch it "by hand", line by line :) (if you tell me there is a better method, please feel free too =D).
Thank you for the experience about the performance issue. So, rephrasing my previous post: in a "pure" MySql environment, the patch might/will bring some performance improvements.
Thank you very much for your replies,
Felix
#99
Update: maybe I've understood your post now: do you mean that now that the task has been assigned to version 6.x, you will no longer think about the versions 4.7.x and 5.x, which would also be the reason why you called it backporting?
I'm not a freak in matters of php, but I think this is a really useful patch for many of us. If anyone can explain me roughly what to do to update the patch for 4.7.4 posted above to 4.7.5, I can write a detailed tutorial for the people interested, if you like.
Thank you, and sorry for the double post,
Felix
#100
It means that the patch will only be applied to Drupal 6 (if it's accepted), and not previous versions of Drupal. So you will never be able to download a 4.7.7 or a 5.2 or whatever that has this change in it.
However, you can apply the patches to an existing 4.7.x/5.x installation, as long as you know what you're doing and remember to re-apply the patch each time you upgrade (say, from 4.7.5 to 4.7.6). More info is available on applying patches here: http://drupal.org/node/60108
#101
Still not a fan of the regex ...
Is there a reason we can't clean up db_next_id(). It's in the MySQL specific database backend so it should be OK to use MySQL specific constructs (like REPLACE).
#102
preg_replace('/^INSERT /', 'REPLACE ', $insert_query),
This poor regexp? This so harmless that we almost could convert it to str_replace, but only almost.
The main concern (performace wise of course) of this patch is not db_next_id btw, but cache_set.
#103
OK, while you are talking about really interesting stuff, I'll try to describe for newbies like me how to use your nice patch. Please tell me if something is missing, I don't want to be responsible for the destruction of a site =D
This description is valid for 4.7.5, but I'm sure it will work with other versions, too. Don't forget to make tons of BACKUPS before everything you try.
Thank you for your previous reply, I'll repeat it here: every time you will update your Drupal installation, you'll have to patch the updated files again.
No-Lock-Patch in 10 easy steps.
1) As a newbie is more probably a Windows user :) go here http://drupal.org/node/32875 and download cygwin. (otherwise http://drupal.org/node/60108 will be good for you). Don't forget patchutils during installation.
2) If you leave all the options as they are, cygwin will be in C:\cygwin. There is a home directory in there, with another directory inside containing your username. In there, you copy the entire includes folder of your Drupal installation. (that's just one way or another, if you have bandwith problems, do it differently, copying just the four or five files to be patched.
3) Copy the patchfile in the same directory.
4) Open cygwin, go to your includes folder (typing cd includes).
5) Then, type patch < no_lock_0.patch
6) He will complain:
7) Tell him bootstrap.inc, cache.inc does not exist for drupal 4.7.5, you will get a little error file later.
8) The rest will work fine, and you will have 5 new little files. On the screen, you might see something like this:
9) One thing is still missing, though: the error he produced before. Now, in the file bootstrap.inc.rej he tells you what the problem is. So, you have to copy and paste manually the missing code in your new bootstrap.inc. Here is an example with the files I had now.
Get rid of this (opening bootstrap.inc with a text editor):
function cache_set($cid, $data, $expire = CACHE_PERMANENT, $headers = NULL) {db_lock_table('cache');
db_query("UPDATE {cache} SET data = %b, created = %d, expire = %d, headers = '%s' WHERE cid = '%s'", $data, time(), $expire, $headers, $cid);
if (!db_affected_rows()) {
@db_query("INSERT INTO {cache} (cid, data, created, expire, headers) VALUES ('%s', %b, %d, %d, '%s')", $cid, $data, time(), $expire, $headers);
}
db_unlock_tables();
}
and replace it with
function cache_set($cid, $table = 'cache', $data, $expire = CACHE_PERMANENT, $headers = NULL) {db_query_replace(
array("DELETE FROM {%s} WHERE cid = '%s'", $table, $cid),
array("INSERT INTO {%s} (cid, data, created, expire, headers) VALUES ('%s', %b, %d, %d, '%s')", $table, $cid, $data, time(), $expire, $headers)
);
}
10) Copy everything back to your site (i.e. ideally your test installation) and pray it's going to work. :)
If anyone of the more experienced users gets a heart attack reading this, please tell me why you had it. Otherwise, I think these files should work well.
I attach the result of my little experiment so that if the experienced users feel my method was OK, someone else might get around the hassle (well, in reality, not that much...) of patching the files. It's a zip file, I'm sorry, please tell me if there is a better way to post the files.
Good luck to everyone, and thank you for that really nice piece of work and your help in the forum,
Felix.
#104
Update: little (or big?) problem. If I do this on my live site (don't worry, all is backuped =D), I will get:
I'm running on MySql 5, and the lines that must be concerned are the following (as there is no db_query_replace in the "original" non-patched version):
function db_query_replace($delete, $insert) {$insert_query = array_shift($insert);
db_query(preg_replace('/^INSERT /', 'REPLACE ', $insert_query), $insert);
}
Sorry if I'm making a dumb mistake, I just wanted to make sure no ones patches as I said :) Nonetheless, of course, I would be quite, not to say very grateful if you could help me out.
Thank you again,
Felix.
#105
Is anyone using this patch successfully on 4.7.5? As I explain in the post:
http://drupal.org/node/115634#comment-198815
I applied the patch and don't get any errors, but my cache table is still locking and I still have horrible latency when adding nodes and comments. If anyone has any updates or alternative solutions (aside from moving to 5.0), I would love to know.
#106
If anybody messes this issue any further up by adding "I want this for Drupal 4.4" I'll get really unfriendly.
I've re-rolled the patch for offsets. It runs now for two months on drupal.org and we didn't encounter any problems. IMO this is good to go.
#107
OK, how about 4.3 then?
Seriously, here is something I don't understand:
For the sequences, database.mysql.inc, says:
return mysql_insert_id();However, for database.mysqli.inc, it says:
return db_result(db_query("SELECT LAST_INSERT_ID() FROM {sequences} WHERE name = '%s'", $name));Is this intentional, or an oversight? I think the mysqli version is good enough for mysql.
#108
mysqli_insert_id only exists in PHP5. using mysql_insert_id is the better option, I believe.
#109
The php docs say mysqli_connect is also PHP5 only, so I guess we can stick with
mysqli_insert_id()indatabase.mysqli.inc.#110
Patch no longer applied. This is just a reroll of #106 but with mysqli_insert_id(), since it is guaranteed on the same PHP version as mysqli itself is.
I rather like the side effect that we get an alternative to the delete/insert cycle to use elsewhere such code would be needed. Spiffy.
#111
I'll think about this patch some more.
#112
A few problems:
#113