Issue status update for http://drupal.org/node/20114 Project: Drupal Version: cvs Component: base system Category: bug reports Priority: critical Assigned to: Ricetons Reported by: Ricetons Updated by: Dries Status: patch The proper fix is table/column locking. Dries Previous comments: ------------------------------------------------------------------------ April 7, 2005 - 05:53 : Ricetons This bug was exposed by the poormancron module. We can see that the module itself contains no bug. But in the function variable_get(), the function just execute SQL like this: DELETE bar WHERE key="foo" INSERT INTO bar VALUES(foo, bar2ooo) but suppose this: thread A and thread B has executed the DELETE sentence. And they do INSERT at the same time. then an SQL error like this occured: duplicated key 'foo' in table "variables" It's a critical bug. It should be fixed before the time that drupal 4.6.0 released. ------------------------------------------------------------------------ April 8, 2005 - 10:01 : robertDouglass I also experience this on a site that has many different cron cycles ranging from 2 minutes to 6 hours. The query errors which arise (as noted above) are impossible to reproduce through normal testing yet occur fairly (surprisingly) often in the logs. This doesn't just apply to cron jobs but rather to all code that calls variable_set. Setting to CVS since this hasn't been addressed since the issue was opened. ------------------------------------------------------------------------ April 8, 2005 - 15:10 : jhriggs Attachment: http://drupal.org/files/issues/variable_set_update.patch (922 bytes) I'm not sure that there is a "right" way to fix this. In mysql, we could use a REPLACE rather than DELETE/INSERT, but that wouldn't work for pgsql. And, if we are really seeing this type of race, there is no telling which REPLACE would actually make it last and be saved. Instead, we could do a check to see if the row exists and do an UPDATE if it does and an INSERT otherwise; however, this would be prone to the same problem if we are seeing such a race. That is, they may both see that the row does not exist and both attempt to do an INSERT. This will only happen the first time a variable is created, though, which may be more acceptable than what we are seeing now. If the row does already exist, they would both do UPDATES and would "succeed" in the sense that there is no duplicate key error. There is still no telling which one actually sticks, though. The attached patch takes the second approach. ------------------------------------------------------------------------ April 8, 2005 - 15:37 : Jeremy@kerneltrap.org What keeps another session from doing a DELETE after your session does its SELECT COUNT but before it does its UPDATE? Or, what keeps another session from doing an INSERT after your session does its SELECT COUNT but before it does its INSERT? All you've done is add another query... I think you'll need to use locking to fix this right. ------------------------------------------------------------------------ April 8, 2005 - 16:46 : jhriggs 1. Assuming that all we are talking about is variable_set() calls, DELETE is not an issue, because the patch removes deletes. It only does updates and inserts. Now if you throw in variable_del() calls, that's another story, but it's also outside the scope of this issue. It would likely be much more rare than the delete/insert problems we see now. 2. Your second example is the only lingering downfall. As I mentioned in my post: "...they may both see that the row does not exist and both attempt to do an INSERT. This will only happen the first time a variable is created, though, which may be more acceptable than what we are seeing now." 3. This doesn't add any queries. Right now it does DELETE and INSERT. With the patch it does SELECT and UPDATE/INSERT. Two queries either way. As I alluded, this may not be a true fix -- I don't think there is one for race conditions like this, even with table-locking -- but it addresses the problem in a way that makes sense and eliminates an error in the majority of cases. ------------------------------------------------------------------------ April 8, 2005 - 17:42 : killes@www.drop.org @Jeremy: db locking will become available with the revisios patch. I don't advise to use it too mch because it is very slow... I actually do not see why this needs a fix. All it is going to produce is a message in the error log... ------------------------------------------------------------------------ April 9, 2005 - 00:06 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/variable_set.patch (708 bytes) The attached patch makes more sense to me. The same method is used by cache_set() and statistics_exit(). Once a variable already exists in the variable table, we only need to do one db_query to update it... ------------------------------------------------------------------------ April 9, 2005 - 05:39 : Ricetons variable_set() is a function being used almost everywhere in drupal. INSERT/DELETE caused a serious performance problem. Maybe we should come to a soultion which uses a miminum number of queries. In my opinion, it seems that the third patch can solve the problem, but it's not good enough. By the way, table locking costs too much. I don't agree to use that method to solve this case. ------------------------------------------------------------------------ April 9, 2005 - 10:37 : Steven Jeremy: cache_set(), and thus this patch, suffer from an annoying bug: db_affected_rows() only returns which rows were changed, not which rows matched the WHERE clause. This means that if you set a variable to exactly the same value as it already is, that a db error will be generated. This is rarely the case with cache and only occurs in a race condition there, but with variables this problem is quite important. ------------------------------------------------------------------------ April 9, 2005 - 16:07 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/variable_set_0.patch (978 bytes) Okay, here's a fix for variable_set. It has the added advantage of not requiring any db_queries if we're trying to set the variable to a value it's already equal to. (Granted, without using locks there's always a tiny race window... But this will always be true.) Costs: - if the variable is not previously set: two db queries - if the variable is previously set, and changes: one db query - if the variable is previously set, and not changing: no db query ------------------------------------------------------------------------ April 9, 2005 - 17:12 : Jeremy@kerneltrap.org Attachment: http://drupal.org/files/issues/variable_set_1.patch (1.04 KB) Just because it's possible, here's an alternative patch that never costs more than 1 db_query. Costs: - if the variable is not previously set: one db query - if the variable is previously set, and changes: one db query - if the variable is previously set, and not changing: no db queries