Problem/Motivation

This issue is brought about by the changes to system_theme_data() function which were introduced in 6.24. Anyone upgrading from 6.22 or 6.23 (or prior?) will run into this issue. Please note that there are two separate triggering conditions of these messages, with a slightly different message for each, but both are caused by this function.

The first issue, which is attempting to INSERT duplicate keys into the system table is caused drush with any command that flushes the caches before doing anything (update, pm-update, enable). drush sets MAINTENANCE_MODE to 'update' and then calls system_theme_data() which is unhandled by the logic. It is a booby trap so changing drush is not appropriate; more later.

The second issue, is attempting to UPDATE the system table so that two entries have the same filename which is the primary key. This is caused by the system table have more than one instance of say the 'minelli' theme in it. One in the original location, and one in an overridden location. It is unknown how these two entries end up in the system table, however they have remained persistent and many people do have it occurring. At this point, I don't think it's all that important.

The function formally ran a DELETE on all type = 'theme' items in the system table, without race protection which was causing all themes on the system to enter the disabled state if the race occurred.

The replacement of this function adds a semaphore so that the function can only ever run single threaded and all others wait, but also the logic of the database manipulation was replaced. The replacement logic is based on the assumption that the row will not be in the database if MAINTENANCE_MODE is defined, and is set not to 'install', which is where the drush setting differs.

if (isset($theme->status) && !(defined('MAINTENANCE_MODE') && MAINTENANCE_MODE != 'install')) {
  UPDATE  .. name = $theme->name AND type = 'theme'
}
else {
  INSERT
}

This logic is flawed as the setting of MAINTENANCE_MODE has absolutely nothing to do with a particular row being present in the database or not. It also does not give any indication if there are two rows in the database with the same name with type theme. This means that the INSERT is called when it should not be, and the UPDATE does not take into account duplicate rows, both because the function does not know the actual state of the database at the time.

Prior to 6.24, this further node from David_Rothstein

By the way, to give us a little more confidence here I decided to look into the Git history and find out why the MAINTENANCE_MODE checks were apparently needed in December 2010 (when they were added to the patch in #147000: Rewrite module_rebuild_cache() and system_theme_data()) but don't seem to be needed anymore.

It turns out this was all a side effect of #632080: _system_theme_data() should clone the returned objects. should clone the returned objects. (a bug which was only fixed later, in April 2011). Basically, there was a static cache of theme objects that was getting polluted, and it seems like that's what led objects that didn't exist in the database yet to have $theme->status set during install. The MAINTENANCE_MODE check that was introduced therefore effectively turned out to work around that bug.

Therefore, with that analysis (plus the above testing that people did), I think we can be very confident that this change in the patch from #83 is a safe and correct one:

-      if (isset($theme->status) && !(defined('MAINTENANCE_MODE') && MAINTENANCE_MODE != 'install')) {
+      if (isset($theme->status)) {

Proposed resolution

#83 - http://drupal.org/node/1425868#comment-5607720

This patch provides a fix which uses the provided semaphore locking to ensure that only one thread can update the theme data in the system table at a time, removing the race condition in the same way. The MAINTENANCE_MODE check is no longer needed. It relies on the status variable of the theme data to determine if the information is in the database or not, which is now accurate. Any duplicate entries are removed prior to leaving the function, meaning that while an error is throw the first time this is run after an update, it will not error again.

Remaining tasks

This solution addresses all known issues.

The this. Oops.

Original report by lordzik

Hello,
after update from 6.22 to 6.24 my dblog is flooded with alerts like that:

user warning: Duplicate entry 'sites/site.domain.com/themes/greenhouse/greenhouse.info' for key 1 query: UPDATE system SET owner = 'themes/engines/phptemplate/phptemplate.engine', info = 'a:10:{s:4:\"name\";s:22:\"Greenhouse for Dupal 6\";s:11:\"description\";s:34:\"Piekna skorka dla WUMowskich stron\";s:4:\"core\";s:3:\"6.x\";s:6:\"engine\";s:11:\"phptemplate\";s:7:\"regions\";a:5:{s:4:\"left\";s:12:\"Left sidebar\";s:5:\"right\";s:13:\"Right sidebar\";s:7:\"content\";s:7:\"Content\";s:6:\"header\";s:6:\"Header\";s:6:\"footer\";s:6:\"Footer\";}s:8:\"features\";a:10:{i:0;s:20:\"comment_user_picture\";i:1;s:7:\"favicon\";i:2;s:7:\"mission\";i:3;s:4:\"logo\";i:4;s:4:\"name\";i:5;s:17:\"node_user_picture\";i:6;s:6:\"search\";i:7;s:6:\"slogan\";i:8;s:13:\"primary_links\";i:9;s:15:\"secondary_links\";}s:11:\"stylesheets\";a:1:{s:3:\"all\";a:1:{s:9:\"style.css\";s:49:\"sites/site.domain.com/themes/greenhouse/style.css\";}}s:7:\"scripts\";a:1:{s:9:\"script.js\";s:49:\"sites/site.domain.com/themes/greenhouse/script.js\";}s:10:\"screenshot\";s:54:\"sites/site.domain.com/themes/greenhouse/screenshot.png\";s:3:\"php\";s:5:\"4.3.5\";}', filename = 'sites/site.domain.com/themes/greenhouse/greenhouse.info' WHERE name = 'greenhouse' AND type = 'theme' in /var/www/html/drupal6/modules/system/system.module on line 837.

user warning: Duplicate entry 'sites/site.domain.com/themes/wnoz_own_framework/wnoz_own_framewo' for key 1 query: UPDATE system SET owner = 'themes/engines/phptemplate/phptemplate.engine', info = 'a:13:{s:4:\"name\";s:14:\"Framework WNoZ\";s:11:\"description\";s:57:\"Framework zmodyfikowany na potrzeby WNoZ, w katalogu WNoZ\";s:7:\"version\";s:7:\"6.x-2.6\";s:4:\"core\";s:3:\"6.x\";s:6:\"engine\";s:11:\"phptemplate\";s:11:\"stylesheets\";a:2:{s:3:\"all\";a:1:{s:9:\"style.css\";s:57:\"sites/site.domain.com/themes/wnoz_own_framework/style.css\";}s:5:\"print\";a:1:{s:9:\"print.css\";s:57:\"sites/site.domain.com/themes/wnoz_own_framework/print.css\";}}s:7:\"regions\";a:6:{s:4:\"left\";s:12:\"Left sidebar\";s:5:\"right\";s:13:\"Right sidebar\";s:7:\"content\";s:7:\"Content\";s:6:\"header\";s:6:\"Header\";s:3:\"nav\";s:10:\"Navigation\";s:6:\"footer\";s:6:\"Footer\";}s:7:\"project\";s:9:\"framework\";s:9:\"datestamp\";s:10:\"1247529671\";s:8:\"features\";a:10:{i:0;s:20:\"comment_user_picture\";i:1;s:7:\"favicon\";i:2;s:7:\"mission\";i:3;s:4:\"logo\";i:4;s:4:\"name\";i:5;s:17:\"node_user_picture\";i:6;s:6:\"search\";i:7;s:6:\"slogan\";i:8;s:13:\"primary_links\";i:9;s:15:\"secondary_links\";}s:7:\"scripts\";a:1:{s:9:\"script.js\";s:57:\"sites/site.domain.com/themes/wnoz_own_framework/script.js\";}s:10:\"screenshot\";s:62:\"sites/site.domain.com/themes/wnoz_own_framework/screenshot.png\";s:3:\"php\";s:5:\"4.3.5\";}', filename = 'sites/site.domain.com/themes/wnoz_own_framework/wnoz_own_framework.info' WHERE name = 'wnoz_own_framework' AND type = 'theme' in /var/www/html/drupal6/modules/system/system.module on line 837.

Also during drush updatedb i'm flooded with alerts like:

Duplicate entry 'themes/async/async.info' for key 1 ^[[1;33;40m^[[1m[warning]^[[0m
query: INSERT INTO system (name, owner, info, type, filename,
status, throttle, bootstrap) VALUES ('async',
'themes/engines/phptemplate/phptemplate.engine',
'a:13:{s:4:\"name\";s:5:\"async\";s:11:\"description\";s:15:\"A
Sync
Template\";s:7:\"version\";s:7:\"6.x-1.9\";s:4:\"core\";s:3:\"6.x\";s:6:\"engine\";s:11:\"phptemplate\";s:7:\"regions\";a:9:{s:6:\"header\";s:6:\"Header\";s:13:\"preface_first\";s:13:\"Preface
First\";s:14:\"preface_middle\";s:14:\"Preface
Middle\";s:12:\"preface_last\";s:12:\"Preface
Last\";s:7:\"content\";s:7:\"Content\";s:14:\"content_bottom\";s:14:\"Content
Bottom\";s:13:\"sidebar_first\";s:13:\"Sidebar
First\";s:12:\"sidebar_last\";s:12:\"Sidebar
Last\";s:6:\"footer\";s:6:\"Footer\";}s:11:\"stylesheets\";a:1:{s:6:\"screen\";a:2:{s:9:\"reset.css\";s:22:\"themes/async/reset.css\";s:9:\"style.css\";s:22:\"themes/async/style.css\";}}s:7:\"project\";s:5:\"async\";s:9:\"datestamp\";s:10:\"1259153703\";s:8:\"features\";a:10:{i:0;s:20:\"comment_user_picture\";i:1;s:7:\"favicon\";i:2;s:7:\"mission\";i:3;s:4:\"logo\";i:4;s:4:\"name\";i:5;s:17:\"node_user_picture\";i:6;s:6:\"search\";i:7;s:6:\"slogan\";i:8;s:13:\"primary_links\";i:9;s:15:\"secondary_links\";}s:7:\"scripts\";a:1:{s:9:\"script.js\";s:22:\"themes/async/script.js\";}s:10:\"screenshot\";s:27:\"themes/async/screenshot.png\";s:3:\"php\";s:5:\"4.3.5\";}',
'theme', 'themes/async/async.info', 0, 0, 0)
database.mysql.inc:135

What the hell is going on?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dane Powell’s picture

Priority: Normal » Major

I'm pretty sure this is the same bug... I'm getting similar warnings, just for themes it seems (even themes that are not enabled). I think this is major or even critical since it prevents upgrades using Aegir...

WD php: Duplicate entry 'themes/garland/minnelli/minnelli.info' for key 'PRIMARY' query: INSERT INTO system (name, owner, info, type, filename, status, throttle, bootstrap) VALUES ('minnelli', 'themes/engines/phptemplate/phptemplate.engine', 'a:14:{s:4:\"name\";s:8:\"Minnelli\";s:11:\"description\";s:56:\"Tableless, recolorable, multi-column, fixed width theme.\";s:7:\"version\";s:4:\"6.24\";s:4:\"core\";s:3:\"6.x\";s:10:\"base theme\";s:7:\"garland\";s:11:\"stylesheets\";a:1:{s:3:\"all\";a:1:{s:12:\"minnelli.css\";s:36:\"themes/garland/minnelli/minnelli.css\";}}s:7:\"project\";s:6:\"drupal\";s:9:\"datestamp\";s:10:\"1328134267\";s:7:\"regions\";a:5:{s:4:\"left\";s:12:\"Left sidebar\";s:5:\"right\";s:13:\"Right sidebar\";s:7:\"content\";s:7:\"Content\";s:6:\"header\";s:6:\"Header\";s:6:\"footer\";s:6:\"Footer\";}s:8:\"features\";a:10:{i:0;s:20:\"comment_user_picture\";i:1;s:7:\"favicon\";i:2;s:7:\"mission\";i:3;s:4:\"logo\";i:4;s:4:\"name\";i:5;s:17:\"node_user_picture\";i:6;s:6:\"search\";i:7;s:6:\"slogan\";i:8;s:13:\"primary_links\";i:9;s:15:\"secondary_links\";}s:7:\"scripts\";a:1:{s:9:\"script.js\";s:33:\"themes/garland/minnelli/script.js\";}s:10:\"screenshot\";s:38:\"themes/garland/minnelli/screenshot.png\";s:3:\"php\";s:5:\"4.3.5\";s:6:\"engine\";s:11:\"phptemplate\";}', 'theme', 'themes/garland/minnelli/minnelli.info', 0, 0, 0) in /var/aegir/platforms/live_recipe_175/modules/system/system.module on line 841.
lort’s picture

FileSize
39.25 KB

Getting the same thing here. Only for themes.

I see the errors only when I try to enable a module with drush.

attached screen output.

lort’s picture

Comment #173 in this post http://drupal.org/node/147000 has a fix.

Dane Powell’s picture

@lort - thanks for the tip, but I wouldn't really consider a patch that's 'for amusement only' a good fix :)

jmuessig’s picture

Issue occurred for me on an upgrade from drupal core 6.23 to 6.24 using drush.

greg.harvey’s picture

Agreed with Dane, at the very least that patch should be moved here as a work in progress, as it's an independent issue and the original issue has been committed to Drupal 6.

greg.harvey’s picture

Version: 6.24 » 6.x-dev
Status: Active » Needs review
FileSize
1.17 KB

In fact, to that end, here is that patch, so there is at least a start, even if it needs a re-write. Feel free to mark back to "needs work" if you feel this patch is not adequate. =)

(See link in #3 for the original post and patch.)

ELC’s picture

Thanks Greg.

The "for amusement only" remark was because I don't know why there is a MAINTENANCE_MODE check, and why it has to be NOT-NOT "install". I made the condition NOT-NOT "install" and "update", the later is what drush sets the variable to when it runs. Since this neither fixes all issues with this new version of the function, and does not take into account why this check is made, it's more than likely not feature complete.

In reality, the $themes array should be able to include a marker that the particular entry is in the database or not, and as a result subject to an UPDATE or INSERT. The condition would then be based on that marker instead of the state of the MAINTENANCE_MODE define.

Neither of these allow for more specific overrides of themes to be deleted (when upgrading I found someone had been playing so I cleaned it up). For that to work properly, the $themes array must check itself for uniqueness and delete any entries from the database that are no longer needed; hence why I thought the entire system_theme_data needed to be reworked, again and posted to #147000: Rewrite module_rebuild_cache() and system_theme_data().

If someone could point me to the reason for the MAINTENANCE_MODE check and critique my thoughts on changing how the array is built and saved to the database, that would be good. With more understanding I might be able to make something with more of a chance of working for all situations.

Gábor Hojtsy’s picture

Can someone please summarize the conditions under which this occurs?

danmed’s picture

Sorry that I cannot participate in finding a fix, but I do need to know the consequences of these erros on a site in production: shall I revert to a backed up DB + Drupal filesystem (prior to the 6.22 --> 6.24 update), or is my system still stable?

Thank you for your advice.

Gábor Hojtsy’s picture

@danmed: the errors reported are about issues *in the update process* so if that ran fine for you, you'd not be affected by this issue.

danmed’s picture

I understand, and I got those errors too! But the system "seems" to run fine.
Can I therefore ignore these errors or is it safer to revert to a previous version?

Anonymous’s picture

@ Gabor in #9:

Not much to it: Run drush updatedb on a site after it's moved onto the latest D6 release (6.24) to apply the database changes.

Simple as that.

greg.harvey’s picture

@Gábor: seems related to pretty much any updates with drush. See http://twitter.com/_mig5/status/165391939951276032 and http://twitter.com/_mig5/status/165392108222545921 plus many other reports appearing.

Gábor Hojtsy’s picture

@greg.harvey: if *everybody* who uses drush to update sites had this issue, I'd expect a lot more feedback here.

@danmed: please be very clear. Did you get the errors in the update or did you get the errors outside of the update or are you continually getting these errors? It makes a huge difference both for debugging and announcements.

@all: I've added this to the http://drupal.org/drupal-7.12 announcement for now at the end ("might experience" due to it being not clear that everybody using drush will experience it(?)):

In Drupal 6.24 if you run your updates with Drush, you might experience duplicate entry errors in your system table. See the ongoing discussion at http://drupal.org/node/1425868

lordzik’s picture

Version: 6.x-dev » 6.24
Priority: Major » Normal
Status: Needs review » Active

Well, this isn't happening only at Drush. I also see errors in Drupal admin interface and dblog:

user warning: Duplicate entry 'sites/site.domain.com/themes/greenhouse/greenhouse.info' for key 1 query: UPDATE system SET owner = 'themes/engines/phptemplate/phptemplate.engine', info = 'a:10:{s:4:\"name\";s:22:\"Greenhouse for Dupal 6\";s:11:\"description\";s:34:\"Piekna skorka dla WUMowskich stron\";s:4:\"core\";s:3:\"6.x\";s:6:\"engine\";s:11:\"phptemplate\";s:7:\"regions\";a:5:{s:4:\"left\";s:12:\"Left sidebar\";s:5:\"right\";s:13:\"Right sidebar\";s:7:\"content\";s:7:\"Content\";s:6:\"header\";s:6:\"Header\";s:6:\"footer\";s:6:\"Footer\";}s:8:\"features\";a:10:{i:0;s:20:\"comment_user_picture\";i:1;s:7:\"favicon\";i:2;s:7:\"mission\";i:3;s:4:\"logo\";i:4;s:4:\"name\";i:5;s:17:\"node_user_picture\";i:6;s:6:\"search\";i:7;s:6:\"slogan\";i:8;s:13:\"primary_links\";i:9;s:15:\"secondary_links\";}s:11:\"stylesheets\";a:1:{s:3:\"all\";a:1:{s:9:\"style.css\";s:49:\"sites/site.domain.com/themes/greenhouse/style.css\";}}s:7:\"scripts\";a:1:{s:9:\"script.js\";s:49:\"sites/site.domain.com/themes/greenhouse/script.js\";}s:10:\"screenshot\";s:54:\"sites/site.domain.com/themes/greenhouse/screenshot.png\";s:3:\"php\";s:5:\"4.3.5\";}', filename = 'sites/site.domain.com/themes/greenhouse/greenhouse.info' WHERE name = 'greenhouse' AND type = 'theme' in /var/www/html/drupal6/modules/system/system.module on line 836.

I can confirm that this happens only with themes and only with some of them (not all that are in "themes" directory). Probably only with those which were enabled at least one time on particular site (multisite configuration).

lordzik’s picture

Priority: Normal » Major
Status: Active » Needs review
andypost’s picture

Status: Needs review » Reviewed & tested by the community

Confirm this bug and fix. drush updb without patch throws this warnings about theme duplicates

ELC’s picture

Status: Reviewed & tested by the community » Needs review

@Gábor Hojtsy This error occurs ANY time a drush instruction somehow calls system_theme_data(). drush updatedb does this every time. There were a few other instructions I ran into that triggered it, but they escape me right now.

As I said in the other thread, the error occurs because of these conditions

  • status is set (either 0 or 1, thus the row exists in the database)
  • MAINTENANCE_MODE is defined
  • MAINTENANCE_MODE is set to "update"

(full details http://drupal.org/node/147000#comment-5548754) which results in the INSERT code being run even though only the UPDATE code should be.

The error @lordzik is experiencing in #16 is because of the second bug - if the same theme is present twice on the system, once in the normal position, and once in some more specific location (site specific directory for example), the present condition will attempt to update both entries in the database to the same path which causes yet another duplicate key error - filename being the same no two themes when they should be different. I got this occuring from having the same theme twice in the drupal tree and then deleting it; like duplicating garland so that it can be edited for a specific site (wasn't me!).

So, to replicate the first one, just run drush updatedb. You'll get as many warnings are you do themes.

I have been unable to replicate the second again but to fix it, I had to delete the offending row out of the system table so that there was only one "minnelli" row. Prior to that I had two; one for the original location, and one for the overridden location. The SQL attempts to update both at the same time. I do not know how two rows with the same occurred, but I had 2 sites with the error out of ~20 in that container. It seems that at some time in the past, the theme in question was moved and two rows resulted?
Suffice to say, it does occur. The original version of this code DELETE'd all of the entries and inserted them again and this event did not throw errors; now the error persists until the second record is removed.

Gábor Hojtsy’s picture

Why is it not happening outside of drush?

greg.harvey’s picture

According to #16 it *is* happening outside of drush. It so happens we haven't tried an update without drush, so can't verify.

lordzik’s picture

Status: Needs review » Needs work

@ELC #19
I confirm that deleting one of two rows for particular theme makes error to dissapear.

The problem is how to do it quickly if you have 100 sites/databases with few involved themes... :)
So this is good as workaround but not as the solution.

ELC’s picture

The drush bootstrap somehow calls system_theme_data() with MAINTENANCE_MODE set to 'update'; update.php does not call the function, but MAINTENANCE_MODE is set to 'update' as its very first task.

Essentially anything calling that function with MAINTENANCE_MODE set, but not set to 'install' will cause this problem which I am calling the INSERT/PRIMARY key issue.

The patch on this thread does fix the issue for the INSERT/PRIMARY key issues, but not the UPDATE/PRIMARY.

The UPDATE/PRIMARY key issue happens when the contents of database includes something like this:

mysql> select filename,name from system where type = 'theme';
+---------------------------------------------------------------------+------------+
| filename                                                            | name       |
+---------------------------------------------------------------------+------------+
| sites/all/themes/zen/STARTERKIT/STARTERKIT.info                     | STARTERKIT |
| sites/all/themes/zen/zen/zen.info                                   | zen        |
| themes/bluemarine/bluemarine.info                                   | bluemarine |
| themes/chameleon/chameleon.info                                     | chameleon  |
| themes/chameleon/marvin/marvin.info                                 | marvin     |
| themes/garland/garland.info                                         | garland    |
| themes/garland/minnelli/minnelli.info                               | minnelli   |
| themes/pushbutton/pushbutton.info                                   | pushbutton |
| sites/FULLDOMAIN/themes/THEMENAME/THEMENAME.info                    | THEMENAME  |
| sites/DEVDOMAIN/themes/THEMENAME/THEMENAME.info                     | THEMENAME  |
+---------------------------------------------------------------------+------------+

.. which gives two copies of THEMENAME (censored) which are actually the exact same file, but one of them is symlinked. BUT don't think that's the only time it happens. I also had it happen to a renamed garland theme that contained the "minelli" folder. It somehow ended up with two rows in the database too but for two different files. I have no idea how this manages to get into the database, but it's persistent! The old function that ran DELETE on anything of type 'theme' even managed to put them back in each time. Now, it gives a dup key warning.

Tersely; the database contains information which does not match the final $themes array which is generated, and when writing to the database, no care is taken to avoid hitting the dup key. It only include 1 copy of the theme with that name; whatever the last one experienced was and it attempts set that filename on all entries with type = 'theme' and name = 'THEMENAME'.

danmed’s picture

Status: Needs work » Needs review

Replying to #15.
- I got the errors during the update process, using "drush up" command. These appeared in the console output.
- These same errors appear in the DBlog (same date/time the drush command was issued)
- No such errors in the dblog since then (a few hours ago).

Anonymous’s picture

I also confirm this bug (using Drush 3.3) and fix in #7. Thanks.

lordzik’s picture

To me, patch from #7 solves most warnings in Drush. However, still remains those which are also displayed in Drupal admin panel and dblog:

Duplicate entry 'sites/site.domain.com/themes/greenhouse/greenhouse.info' for key 1 [warning]
query: UPDATE system SET owner = 'themes/engines/phptemplate/phptemplate.engine', info =
'a:10:{s:4:\"name\";s:22:\"Greenhouse for Dupal 6\";s:11:\"description\";s:34:\"Piekna skorka dla
WUMowskich
stron\";s:4:\"core\";s:3:\"6.x\";s:6:\"engine\";s:11:\"phptemplate\";s:7:\"regions\";a:5:{s:4:\"left\";s:12:\"Left
sidebar\";s:5:\"right\";s:13:\"Right
sidebar\";s:7:\"content\";s:7:\"Content\";s:6:\"header\";s:6:\"Header\";s:6:\"footer\";s:6:\"Footer\";}s:8:\"features\";a:10:{i:0;s:20:\"comment_user_picture\";i:1;s:7:\"favicon\";i:2;s:7:\"mission\";i:3;s:4:\"logo\";i:4;s:4:\"name\";i:5;s:17:\"node_user_picture\";i:6;s:6:\"search\";i:7;s:6:\"slogan\";i:8;s:13:\"primary_links\";i:9;s:15:\"secondary_links\";}s:11:\"stylesheets\";a:1:{s:3:\"all\";a:1:{s:9:\"style.css\";s:51:\"sites/site.domain.com/themes/greenhouse/style.css\";}}s:7:\"scripts\";a:1:{s:9:\"script.js\";s:51:\"sites/site.domain.com/themes/greenhouse/script.js\";}s:10:\"screenshot\";s:56:\"sites/site.domain.com/themes/greenhouse/screenshot.png\";s:3:\"php\";s:5:\"4.3.5\";}',
filename = 'sites/site.domain.com/themes/greenhouse/greenhouse.info' WHERE name = 'greenhouse' AND type = 'theme'
database.mysql.inc:135

ELC’s picture

Now that I know a bit more about what is going on, I have this proposal.

The old code that had a wonderful race condition where it would have different processes with different versions of the theme data and whatever the last one to execute would have the deleted settings and save those to the database meaning that all themes would end up disabled. I did actually run into this bug once or twice.

By wrapping the entire function in the database table/semaphore lock, the race condition should have been eliminated and the old behaviour of DELETE everything that is a 'theme' and INSERT new entries would have worked just fine. In that case, the new more complicated code that attempts to determine if it should be INSERT or UPDATE a row of data when it doesn't really know if it's in the database or not is simply that; too complex and not doing everything it needs to.

So, I have attached a patch which returns the behaviour to DELETE + INSERT inside the $write_database section of the code. It solves both of the issues for me. It will fix it for @lordzik too, but please keep in mind ...

For this to be approved, someone who knows the conditions that cause the "all themes disabled" issue needs to make sure that it doesn't happen with this patch. That looks to be jbrauer or greg.harvey?

fen’s picture

I had the problem updating one site with drush 4.4 and another using the update.php URL

Status: Needs review » Needs work

The last submitted patch, 1425868-system-duplicate-key-theme-data-27.patch, failed testing.

ELC’s picture

Status: Needs work » Needs review
FileSize
2.36 KB

Patch based on official 6.x tree instead of my private one

Status: Needs review » Needs work

The last submitted patch, 1425868-system-duplicate-key-theme-data-30.patch, failed testing.

ELC’s picture

Title: Duplicate entry of themes primary key in systems table of Drupal 6.24 (using Drush or Ægir) » user warning: Duplicate entry after update from Drupal 6.22 to 6.24
Version: 6.x-dev » 6.24
Issue tags: -hotfix
FileSize
2.51 KB

Simpletest ends up with a array where $theme->status is not defined. I think this also would have happened when new themes were being installed so this was a good catch by the QA.

UPDATE: This is deprecated. Use http://drupal.org/node/1425868#comment-5607720 instead.

donquixote’s picture

Status: Needs work » Needs review

the new more complicated code that attempts to determine if it should be INSERT or UPDATE a row of data when it doesn't really know if it's in the database or not

This code in 6.24 is simply flawed. It uses the MAINTENANCE_MODE as a criterion to determine whether a row is in the database or not. This is simply a flawed logic, that doesn't make sense.

Whether that is to be fixed with a DELETE + INSERT ? I imagine it could also be fixed by improving the logic.

Another thing I noticed:
function _system_theme_data() has a static cache, but no $reset argument.
Do we need one? I dunno.

I think these function are a mess.

the "all themes disabled" issue

Hooray, if this can be fixed as a side effect!

lorlarz2’s picture

Title: user warning: Duplicate entry after update from Drupal 6.22 to 6.24 » Question: Any database changes in going from 6.22 to 6.24?

Had a strange problem where I had to use the backup to the whole drupal6 database, a backup which was made before I updated 6.22 to 6.24. Can I expect there to be any problems. I.E.: Question: Any database changes in going from 6.22 to 6.24?

xjm’s picture

Title: Question: Any database changes in going from 6.22 to 6.24? » user warning: Duplicate entry after update from Drupal 6.22 to 6.24

#34: Just run update.php again. Also, this issue is specifically regarding the error messages in the original post, so let's not hijack it. :)

ELC’s picture

@lorlarz2 Please do not hijack an open issue for your question. This is an attempt to fix a bug in core.

If you have a question, please open a new one in the support forum instead.

@donquixote DELETE + INSERT is how version 6.22 used to work, it just didn't have a database lock around it thus ended in the race condition. This is essentially a revert to that behaviour but keeping the lock.

The logic can't be improved without giving the function attempting to insert into the database more information. It has to know if a row exists, if there's already another row it's about to conflict with and about to overwrite (so it has to delete the spares anyway). The system_get_ functions aren't really the place for any modification so essentially we're stuck with the information we have unless they also get changed which means changing hundreds of lines of code to deal with a new returned array structure. Not going to happen in D6.

StuddMan’s picture

Confirmed Patch #32 worked for us. We upgrade core and about 10 modules via Drush 4.4 and were getting the errors in the admin interface. We applied the patch and everything appears to be running normally again.

juliangb’s picture

I can also confirm this bug and that the patch in #32 worked for me.

xmacinfo’s picture

Status: Needs review » Reviewed & tested by the community

Looks like we have an RTBC based on the two previous comments!

DanGarthwaite’s picture

I can also confirm this bug and that the patch in #32 worked for me. This is a complicated drupal site that has been upgraded and upgraded since the 4.7 days.

jscm’s picture

Patch #32 works for me now! :D

ELC’s picture

@jscm There are a couple of parts to this issue. Could you provide a little more detail? What is the error message you're getting? Does that patch command actually work? What are the results of this sql command: select filename,name from system where type = 'theme'; ?

Essentially, I need to know how I could reproduce your version of the problem so I can see what might be causing it.

jscm’s picture

Hi,
now it works properly.
I had forgotten to update the path with the new release of Drupal, inside a script I created to manage all websites. I ran drush in directory drupal 6.22. I'm sorry it was my fault.

izmeez’s picture

no errors on update with the patch applied, thanks.

lordzik’s picture

i also confirm that patch from #32 solves problems with warnings and errors. IMO should be commited ASAP.

Thank you ELC!

pfrenssen’s picture

Status: Reviewed & tested by the community » Needs work

Review for patch in #32: Logic seems fine, it gets rid of the maintenance mode check and simply deletes and then recreates the theme records.

It does need some minor cleanups:

  • Comment "Make sure there is a status and if it's missing [...]" seems to contradict itself, and should capitalize "Garland".
  • The INSERT query would be better on one line as it was before. It is not usual to split long lines of code over multiple lines.

Thanks for the great work.

ELC’s picture

  • Removed the superfluous $names = array(); declaration. This was part of the 6.24 logic.
  • Change comment to
          // If status is missing, set it based on this theme name matching the
          // configured default theme.
    
  • Made UPDATE query one line.

UPDATE: This is deprecated. Use http://drupal.org/node/1425868#comment-5607720 instead.

pfrenssen’s picture

Status: Needs work » Reviewed & tested by the community

Looks good to me.

sciencex’s picture

Thanks for working on this.

I get the following when testing the patch from #47. Is this my (user) error or the patch?

# patch -p0 < 1425868-system-duplicate-key-theme-data-47.patch
patching file b/modules/system/system.module
Hunk #1 FAILED at 826.
1 out of 1 hunk FAILED -- saving rejects to file b/modules/system/system.module.rej
xjm’s picture

Try using patch -p1 instead of -p0.

sciencex’s picture

Thanks XJM, that did the trick. The patch at #47 appears to be working for me.

ChrisLaFrancis’s picture

So should we expect a 6.25 release with this fix included in the near future, or will the next release just follow the normal schedule?

xmacinfo’s picture

Issue tags: +hotfix

@fivefrank: Only Gábor Hojtsy can answer this. I have not seen a lot of hotfix releases for Drupal, but I’d also like to see a new release sooner than later.

larowlan’s picture

+1 for patch at #47 - fixes my problem

roball’s picture

Title: user warning: Duplicate entry after update from Drupal 6.22 to 6.24 » Duplicate entry of themes primary key in systems table of Drupal 6.24

+1 - patch #47 solves the drush problems.

REo’s picture

+1 patch #47. It seems to work. I'm using drush 4.5.

Gábor Hojtsy’s picture

In response to @fivefrank, etc. about a 6.25 release with this fix, I'm looking at both the extent to which people experience this issue and more validation that (a) the fix worked and (b) the fix is complete. Drupal 6 does not get very good testing before releases (no automated testing system, very few people running off of -dev versions), so all I can depend on is plenty actual testing feedback. The patch was RTBC several times above and yet people came here to report it did not work for them or was not complete and kept work on it. If it settles with satisfaction among people needing the fix, that would be a great time to put in Drupal 6. Also, I'm trying to figure out how wide is this affecting people (it would be great to see how many people follow this issue) to gauge if we should release a Drupal 6.25 ASAP.

Hope this helps clear up the core maintainer perspective.

donquixote’s picture

Ok, I can confirm:
1) problem applies to me
2) #47 patch fixes the problem
3) The original code not just behaves wrong, it also looks wrong, and should not have survived a code review.

So I'd say #47 is clearly an improvement.

sterndata’s picture

I would like to see an update to 6.25. I have the problem on my site when using Drush and, maybe coincidentally, a theme related problem. Having a clean update would help me track down the other problem.

ronaldmulero’s picture

I can confirm that this patch also fixes an issue that causes the Aegir hosting system "Migrate" feature to fail when trying to migrate sites from Drupal 6.23 to Drupal 6.24.

  1. Before patching Drupal-6.24 platform: Migrate from D6.23 to D6.24 FAILS for all sites
  2. After applying #47 patch to Drupal-6.24 platform and re-verifying platform: Migrate from D6.23 to D6.24 SUCCEEDS for all sites

Drush version: 4.4
Aegir version: 1.6

xmacinfo’s picture

@Gábor Hojtsy: I am waiting for a 6.25 release before upgrading the Drupal 6 sites I am still running. And for all these sites I am using Drush.

I should be able to test out a -dev version as soon as this patch is commited.

As for testing the patch, a lot of users have already done it.

Cheers!

Dane Powell’s picture

#47 eliminates the errors for me, allowing me to migrate sites from 6.22 to 6.24 in Aegir without trouble.

liquid06’s picture

I don't have enough experience to understand or review the code changed by the patch, but I am using drush version 4.5 to manage multiple Drupal 6 sites and I experienced this problem consistently when using drush to update sites to 6.24. I'm avoiding updating sites to 6.24 because I worry, as sterndata mentions in #59, that this issue could obscure other problems or cause undue confusion when using drush for regular site management.

After applying the patch in #47, the scary group of "Duplicate entry..." messages no longer appears when enabling modules or running updatedb.

liquid06’s picture

Issue summary: View changes

Issue summary by ELC

Robin van Emden’s picture

#47 fixes aegir & warnings for me

Jbrydolf’s picture

#47 worked great for me too. No more errors when using Drush.

xmacinfo’s picture

Title: Duplicate entry of themes primary key in systems table of Drupal 6.24 » Duplicate entry of themes primary key in systems table of Drupal 6.24 (using Drush or Ægir)
fuerst’s picture

#47 fixes the problem described in the issue summary.

jbrauer’s picture

Working on taking a look at this.

A few questions have been raised about why MAINTENANCE_MODE is checked ... That came about in http://drupal.org/node/147000#comment-3833014 which is a response to an earlier version of a fix for this that prevented installation and subsequently installed but did not insert theme records in the system table.

I've also tested #47 in the following configurations:

Upgrade core-only site from 6.22->6.24+patch via web browser.
Upgrade core-only site from 6.22->6.24+patch via drush.
Install new site on 6.24+patch.

In all cases I've also changed the themes/default settings before each run and seen that the settings are preserved.

Testing duplicate themes in paths and a more full-featured D6 site now as well for confirmation.

selfuntitled’s picture

#47 worked for me as well fixed error in Drush in both Dev and production environments for me.

pingers’s picture

Ran into this on a production site - doh! :)
- local copy of course ;)

So like #68, I tried in maintenance mode, not in maintenance mode, web UI, drush.
All failed to perform the locale update 6007.

After applying the patch in #47...

$ drush updb -y
The following updates are pending:

 comment module                                                  
 6004 - Add index to to node_comment_statistics on comment_count 
 6005 - Add indices to uid fields.                               


 locale module                     
 6007 - Fix Drupal.formatPlural(). 


Do you wish to run all pending updates? (y/n): y
The external command could not be executed due to an application error. [error] 
Executing comment_update_6004 [success] 
ALTER TABLE {node_comment_statistics} ADD INDEX comment_count (comment_count) [success]
Executing comment_update_6005 [success]
ALTER TABLE {comments} ADD INDEX comment_uid (uid) [success]
ALTER TABLE {node_comment_statistics} ADD INDEX last_comment_uid (last_comment_uid) [success]
Executing locale_update_6007 [success]
Drush command terminated abnormally due to an unrecoverable error. [error]
Error: Call to undefined function locale_inc_callback() in /var/www/.../modules/locale/locale.install, line 236
Output from failed command : [error]
 
Fatal error: Call to undefined function locale_inc_callback() in /var/www/.../modules/locale/locale.install on line 236

So of course, the locale module is actually disabled. I enabled, ran the update - no more errors. Then disabled locale again.
I think we should fix this - I'd dig in, but don't have time right now...

I suspect the locale module was installed at some point in time, but now disabled.

pingers’s picture

I don't think we can call module functions from hook_update_6xxx() - I can't see any other example of this after a quick look in core.

ELC’s picture

@pingers That seems like a completely separate issue, unrelated to system_theme_data? Perhaps it should have it's own issue?

pingers’s picture

andreic’s picture

+1 - patch #47 solves the drush problems.
Drush: 4.5

dstol’s picture

Version: 6.24 » 6.x-dev
Status: Reviewed & tested by the community » Needs work

The patch in #47 is an incomplete rollback/improvement on http://drupal.org/node/147000#comment-5395906 as it leaves the stub lock file and some other code from #147000.

I think the better approach here would be #7.

Now that jbrauer has explained MAINTENANCE_MODE in #68 it may be a good idea to pursue and expand on the work in 147000.

ñull’s picture

Version: 6.x-dev » 6.24
Status: Needs work » Reviewed & tested by the community

+1
patch #47 fixes it for me (still) with drush 4.4 .

ñull’s picture

Version: 6.24 » 6.x-dev
Status: Reviewed & tested by the community » Needs work

Sorry that was caused by a race condition too :-)

cknoebel’s picture

#47 solves it for me. Thanks! (Problem occurred using Drush 4.5 when moving to D6.24 from D6.22)

ELC’s picture

@dstol The locking is needed to prevent more than one process from calling this function at a time. At present it works during install (which is completely faked), and most certainly during a normal working site run. Without this, the original race condition returns.

The checking of MAINTENANCE_MODE to determine if a row is in the database is an outright error in logic and should not be used. The value of this define has no bearing on a record being in the database or not; it was pure co-incidence. The fix in #7 was based upon this flawed logic and did not fix all of the issues present.

Perhaps another approach would be to not check for locking while in install mode which would negate the need for the lock-install.inc file.

However, in its current state, I think the database locks being there during install is a good thing as it simplifies handling of the database for modules that are being enabled during install time. They don't need to take into account the special situation that they are in and can now use the database locking functions without an error. The manual for these functions would need to updated to reflect that they actually do nothing during install, but are available. Or are the normal full locking facilities available by the time contributed modules are enabled? In that case, this last paragraph is mute.

It still makes sense to me to have the database locking at least faked when the database is not available for locking.

Why is locking not available when the database is clearly available when this function is run during install? After all, it INSERTs into the database in the current (6.24) state even with the faked locking which means there must be something there.

dstol’s picture

Checking of MAINTENANCE_MODE isn't used to determine if a row is in the database. It is used to determine Drupal's current state at the time and act appropriately.

This all started at http://drupal.org/node/147000#comment-3830452 after it was reverted from the 6.20 release. During the install process, when MAINTENANCE_MODE is 'install' there is nothing in {system}. Check out http://drupal.org/node/147000#comment-3831870 through #79

ELC’s picture

Status: Needs work » Reviewed & tested by the community

That's exactly the flawed logic though. The state that Drupal is in has no relevance to what is in the database - it is a co-incidence. At install time, there is nothing in the database, and at update time, there is something in the database, however the setting of MAINTENANCE_MODE makes no difference to there being something in the database or not as it is pure co-incidence. Yes, it's a very strong co-incidence (during install, MAINTENANCE_MODE == 'install' and the database is empty always, but the opposite is NOT true at any other time) but it is just that. Relying on the context in which is was called to infer what is in the database is leading to the bugs that are being manifest.

The point in case is that drush sets MAINTENANCE_MODE to 'update', just like update.php does, but also calls system_theme_data() because it always flushes caches and rebuilds to ensure everything is up to date. In this case, (TRUE && !(TRUE, TRUE)) means that each entry will be INSERT into the database even though it should be UPDATE.

The second issue from the existing code (6.24) related to that UPDATE is that it also doesn't deal with two versions of the same named module being in the database with more than 2 filenames. When the filename is updated, a duplicate key error occurs. This is also fixed by #47. I have no idea how this occurs, but I had it happen on about 20% of 96 sites and others have mentioned it too; it seems common enough to deal with it. The new building of the theme list does not have this bug any longer as that seems to have been the source.

The #47 form of the function deals with the issues stated in http://drupal.org/node/147000#comment-3831870 through #79 because it doesn't care if there's anything in the database or not. An empty database does not care that a DELETE is run, and one that has entries will remove all the type = 'theme' ones and the database will now be in a safe and known state without having to infer that state of the database from the context in which function was called. So this works from both a blank slate, and when rebuilding the theme list.

The database locking, both faked during install (only one request anyway) and real after that, make sure that only one thread at a time will be DELETE/INSERT into the system table which gets rid of the "all themes disabled" bug (hopefully, still waiting on feedback on that but from my understanding it was the race condition that caused it and the locking stops that).

The combination of $themes = _system_theme_data(); and system_get_files_database($themes, 'theme') will result in the currently available list of themes based on what is in the database (install - nothing, later - something), plus everything on the file system (same at install and later).

Going from a known state of the database and INSERT all the values means that no matter what the state of Drupal, the state of the database is known and does not need to be inferred from context.

So #47 ..

  • works at install time
  • works at update time, regardless of the unrelated context in which it was called
  • doesn't cause the "all themes disabled" bug due to the non-faked database locking (wont happen at install)
  • if it gets a database lock (faked or not) no matter what the state of the database is at the start, it will be in a known state at the end of the call

It covers the issues from #147000: Rewrite module_rebuild_cache() and system_theme_data() and then fixes the bugs that it introduced. I believe this should be returned to RTBC.

keithm’s picture

subscribe

dasjo’s picture

#47 fixed duplicate errors with updatedb for me, still getting them with drush site-upgrade

schoene_idee’s picture

  • both #32 and #47 seem to fix the issue (my sites were affected)
  • using drush 4.5
  • after reverting the patch the warnings re-appeared (reversibly) with drush updatedb
ELC’s picture

@schoene_idee #32 should be considered deprecated as #47 is a direct, non-functional improvement.

@dasjo Could you post the error you are getting during site-upgrade, and how the error can be reproduced? I'm ponder whether the same issue might be present in Drupal 7 but I do not currently have the time to investigate the issue from scratch.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.23 KB

A lot of great work has gone into this issue, but the most recent patch still looks very dangerous to me. The reason is that it leaves a window where there are no themes in the database at all. So what happens to a visitor who hits the site during that window? Or worse, what happens if the database connection drops in the middle of those queries?

I think the attached patch will work better. I can't see any reason why the MAINTENANCE_MODE check needs to be there at all (maybe it needed to be there a year ago when the code in #147000: Rewrite module_rebuild_cache() and system_theme_data() was originally written, but I can't find a use for it now) so I simply removed it and left the logic otherwise the same. Also, by keying deletion off of filename rather than name, we can solve the other half of the bug.

I've tested this patch under several scenarios and did not see any problems:

  1. Fresh install of Drupal 6. This needs extra testing since this is what apparently required the MAINTENANCE_MODE addition a year ago, but again, I tried it and had no problems.
  2. Running updates via the UI.
  3. Running updates via "drush updatedb".
  4. General enabling/disabling of themes via the UI.
  5. Moving themes in the filesystem.
  6. Deleting themes in the filesystem.
  7. The "two themes with the same name in the database" issue (e.g. as described in #23). Note that a good way to trigger this scenario is to start wth Drupal 6.24 (i.e. before the patch is applied) and do something like this:
    # Put two copies of the same theme in the codebase.
    cp -r themes/pushbutton sites/all/themes/
    # Trigger the bug.
    drush --yes updatedb
    

    The patch takes care of cleaning up sites that found themselves in that situation, although note it takes two tries to do it. So if your site is in that situation and you apply the patch, the first time you rebuild the theme data (e.g. by visiting admin/build/themes) you'll still get the UPDATE-related primary key errors, but the second time everything will be OK, and only one copy (the sites/all/themes copy in this case) will remain in the database.

ELC’s picture

@David_Rothstein
If someone visits the site during the time when there are NO entries in the database, they sit on the database locking wait for 30 seconds. This is the entire point of the database locking. One thread does the work, all others wait and get the completed information.

I'll look at the rest when I get the time, but I still haven't seen good reason to knock out #47. It handles all situations described.

In your post about, number 7 does not create the correct situation. The entries must be in the database, not simply sitting on the file system. It is unknown how to have this situation replicate itself, but it was present in sites made between 6.19 and 6.22.

ELC’s picture

Status: Needs review » Needs work

Correction, #86 FAILS for number 7 with corrected situation. It still triggers the duplicate key error as there are TWO entries in the database with the same theme name and the filename is attempted to be set to the same for both.

How on earth do I now suggest that #47 is still the way to go and the current patch should be ignored, but still put it back to RTBC? Post the patch again?

If you wish to discuss the patch, please find me on IRC sometime during the day, Australian Eastern Standard Time.

David_Rothstein’s picture

If someone visits the site during the time when there are NO entries in the database, they sit on the database locking wait for 30 seconds.

A lock is only acquired if the site visitor triggers a call to system_theme_data(). That does not happen on a typical page request (or at least, it better not!). I'm talking about normal page requests, which read from the {system} table only. There is nothing to block them from reading, so they'll simply fail to get a theme at all.

In your post about, number 7 does not create the correct situation. The entries must be in the database, not simply sitting on the file system.

The "drush --yes updatedb" call in my example is what triggers putting them in the database... I have tested this and it definitely creates the correct situation.

David_Rothstein’s picture

Status: Needs work » Needs review

Correction, #86 FAILS for number 7 with corrected situation. It still triggers the duplicate key error as there are TWO entries in the database with the same theme name and the filename is attempted to be set to the same for both.

Did you try it twice (as I described), or once? Please list the exact steps you took.

ELC’s picture

A lock is only acquired if the site visitor triggers a call to system_theme_data().

True, #47 is deprecated.

Did you try it twice (as I described), or once? Please list the exact steps you took.

That does not seem like a suitable solution. I can confirm that it will fix it on the first run through, throwing a duplicate key error, and all subsequent runs will no longer run into the issue as the duplicate has been removed. But, it does seem to be a left over from some ancient weirdness which is no longer manifesting itself so perhaps this is acceptable. It will require a note on release that drush updatedb may throw a dup key error when upgrading, but it can be ignored if a second run does not throw errors.

Any code for handling this especially would be run once and never again, so a single error with explanation might be enough. If you concur, it sounds like it's time to update the issue summary and request people test this fix. The pool of people available may be quite shallow by now.

David_Rothstein’s picture

Title: user warning: Duplicate entry after update from Drupal 6.22 to 6.24 » Duplicate entry of themes primary key in systems table of Drupal 6.24 (using Drush or Ægir)
Version: 6.24 » 6.x-dev
Issue tags: +hotfix

OK, cool.

I wonder if it would be possible to write an update function that fixes that issue specifically (although it might be that the update system calls system_theme_data() before actually running updates, in which case there would be no point). But that might not be worth the effort since the issue will fix itself eventually. The way I see it, anyone experiencing that part of the bug has probably already seen those duplicate key errors a bunch of times anyway... so this will just mean they see it one more time and then it's done :)

I agree it may be confusing for them to see it even after updating their code, but it sounds like the number of people affected by that aspect of the bug is relatively small? So hopefully it isn't a big deal. And I think your idea of adding a note about this in the release notes would help a lot too.

ELC’s picture

I thought about using hook_update_N to fix the problem before it occurred, but system_theme_data() is called before hook_update_N when doing a drush updatedb. Flush and rebuild everything (error happens) -> run the update_N which fixes the now non-existent problem -> oops.

With this present code, there's just going to be an error message once. It'll happen when the update is run and the caches are flushed. For drush this happens before the update and after, for update.php I think it happens after and the message would end up in the watchdog.

I have no idea how to make sure that a note is included in the update notes. There's no text file in the code that can be updated as a source of them, it's entirely up to the person creating the next Drupal release during the release creation process when they're more than likely not going to be looking at some text in the issue summary.

There's "change records" - http://drupal.org/list-changes/drupal - but I have no idea how to create or request one of those.

Anyway, might be getting ahead of ourselves here, but certainly something to plan for.

Shai’s picture

I'd like to test the patch and report back.

I've been getting the errors described when I run drush up drupal on 6.22 sites.

But I'm a little bit confused as to what I should do to test this. The following is what I think, but please verify.

The following assumes that I'm starting with an installed Drupal 6.22 that needs updating:

  1. run: drush --yes pm-update drupal (and expect to see errors)
  2. apply patch from comment #86
  3. run: drush --yes updatedb and expect no errors
  4. run: drush cc all and expect no errors

Please confirm and or adjust.

Thanks for all the great work!

Shai Gluskin

ELC’s picture

Status: Needs review » Reviewed & tested by the community

Please note that this bug is in Drupal 6.24 .. AFAIK you will need that version for this error to occur.

  • Have a working site running 6.22
  • replace the core with 6.24
  • run drush updatedb - expect errors every time this is run
  • patch with #86
  • run drush updatedb - expect errors about duplicate key but they have now been fixed
  • run again, no errors

If you are getting these errors on 6.22, I do not know if they are related. As for #86, I have installed it on all servers/containers I have running 6.24 and am getting no more errors.

It seems like the best solution at this point even though it will cause errors the first time it is run. Since any code to avoid that error has to be in the code itself instead of an update, avoiding it seems like overkill. Setting RTBC.

Shai’s picture

@ELC,

Great, thanks.

Yes, the errors I'm getting are from 6.24.

I wrote,

I've been getting the errors described when I run drush up drupal on 6.22 sites

drush up drupal includes installing Drupal 6.24 and running drush updatedb, which is when the errors are triggered. I was not suggesting that if I ran drush updatedb on Drupal 6.22 code that I would get errors.

My experience is just as you describe. The patch in #86 is working for me.

Shai

donquixote’s picture

@Shai,
with drush pm-updatecode drupal or drush upc drupal, you can update the code without the db. So you can apply the patch before you run updb.
So, you need to choose, whether you want to reproduce the bug or avoid it :)

Shai’s picture

@donquixote,

Thanks! My rigid brain memorizes a few key drush commands and then I forget there are others. Your suggestion is perfect. Using drush ups drupal instead of drush up drupal I'll happily patch system.module in advance of updating the database!

Thanks again,

Shai

David_Rothstein’s picture

By the way, to give us a little more confidence here I decided to look into the Git history and find out why the MAINTENANCE_MODE checks were apparently needed in December 2010 (when they were added to the patch in #147000: Rewrite module_rebuild_cache() and system_theme_data()) but don't seem to be needed anymore.

It turns out this was all a side effect of #632080: _system_theme_data() should clone the returned objects. (a bug which was only fixed later, in April 2011). Basically, there was a static cache of theme objects that was getting polluted, and it seems like that's what led objects that didn't exist in the database yet to have $theme->status set during install. The MAINTENANCE_MODE check that was introduced therefore effectively turned out to work around that bug.

Therefore, with that analysis (plus the above testing that people did), I think we can be very confident that this change in the patch from #86 is a safe and correct one:

-      if (isset($theme->status) && !(defined('MAINTENANCE_MODE') && MAINTENANCE_MODE != 'install')) {
+      if (isset($theme->status)) {
Ammaletu’s picture

I just ran into this issue when trying to upgrade a large number of sites from 6.22 to 6.24 via drush. Lots of duplicate key insert error messages and the update stopped with an error. I applied patch #86 and then the update ran through just fine, no further errors.

Before applying the patch I visited admin/build/themes as suggested, but didn't see any error messages in the site itself. There aren't any now after the update either, so I think the theme data in the database is fine. We don't have the same theme copied to two locations.

I'd like to vote for a release 6.25 or at least for committing this patch. I can't put random patches on the production installations (this was just the test machine), so they stay at 6.22 for now.

guy_schneerson’s picture

subscribe

xmacinfo’s picture

@guy_schneerson: Do not subscribe by posting a comment. You must use the Follow button at the top right of this page.

@Ammaletu: I would also like to see this patch committed (even if we don't see a 6.25 release yet). Only Gábor Hojtsy can take the decision.

I am staying at 6.22 also for all my Drupal 6 sites.

ELC’s picture

Issue summary: View changes

Updated current active patch by ELC.

ELC’s picture

It should be noted that 6.23 does not have this error issue, and only patches the security issues from 6.22. You should probably investigate updating to at least this level.

6.23 release notes
SA-CORE-2012-001 - Drupal Core - Multiple vulnerabilities http://drupal.org/node/1425084

ELC’s picture

Issue summary updated. Adding Issue summary initiative tag.

x_v’s picture

subscribe

xmacinfo’s picture

@thexman: Please do not subscribe by posting an empty comment. Use rather the Follow button at the top right of the issue.

claar’s picture

Confirming #86 removes these error messages without apparent side-effect. Commit ASAP.

rootwork’s picture

Also confirming that #86 works great!

fr34ck’s picture

Confirming, the #86 works to me too..

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Thanks, committed #86 to Drupal 6. Thanks for all the testing and feedback, it really helps! Currently planning to release Drupal 6.25 on Feb 29th with this fix.

lambic’s picture

Great news, thanks Gabor!

xmacinfo’s picture

Tested the 6.x-dev update (from 6.22 to the dev version) with Drush.

Everything went perfectly and Drupal reports using version 6.25-dev.

Thank you all for fixing this.

Gábor Hojtsy’s picture

Drupal 6.25 with this fix should be available in a matter of minutes at http://drupal.org/node/1461656 and at http://drupal.org/drupal-6.25

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Gerhard Killesreiter’s picture

Issue summary: View changes

Update issue summary with details of #86 fix. by ELC.