Problem and workaround described. I suggest the SQL below could be added to this update to help future users.
---
My upgrade got stuck at update #7061. At least, I stopped it after it had been sat at that point for four hours. No errors were reported.
Checking the upload table showed many rows that had no matching rows in the files table.
I used phpmyadmin to run the following SQL to show the problem:
SELECT `upload`. *
FROM `upload`
WHERE NOT EXISTS (
SELECT * FROM `files`
WHERE `upload`.`fid` = `files`.`fid`
)
and the following SQL to delete them:
DELETE
FROM `upload`
WHERE NOT EXISTS (
SELECT * FROM `files`
WHERE `upload`.`fid` = `files`.`fid`
)
The upload table had 1,314 rows before, and 130 rows after running the SQL.
Then I deleted the system_update_7061 table, truncated the file_managed table, and ran update.php again. This time it ran through 7061 successfully and completed the upgrade.
The site was originally built using drupal 5, and upgraded to drupal 6 in Dec 2008. I note that no rows were added to the upload table after that d5 -> d6 upgrade.
Regards, David
Comment | File | Size | Author |
---|---|---|---|
#65 | 1586542-system-7061-65.patch | 1.11 KB | andypost |
#57 | system-1586542-57.patch | 684 bytes | Eric_A |
#52 | system-1586542-52.patch | 2.15 KB | xjm |
#52 | interdiff-41-52.txt | 736 bytes | xjm |
#41 | system-1586542-39-tests.patch | 1.04 KB | xjm |
Comments
Comment #1
jenlamptonI experienced exactly the same problem, and this solution worked for me too.
I also have an old D5 site upgraded to D6, and now on to D7.
Comment #2
xjmAha, looks like system_update_7061() fails to increment the batch counter when the file is not defined.
Should be a simple enough fix.
Comment #3
xjmOops, didn't mean to do that.
So, two tasks:
db_delete()
version of the query above to the top of system_update_7061().Comment #4
carwin CreditAttribution: carwin commentedComment #5
chx CreditAttribution: chx commentedconditionInterface has exists and not exists methods and exposed to delete queries: http://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Database%2...
Comment #6
carwin CreditAttribution: carwin commentedWill upload the patch soon, I'm currently still trying to get tests to pass locally.
Comment #7
carwin CreditAttribution: carwin commentedAlright, so here's a partial patch.
Comes complete with: 60 passes, 2 fails, 1 exception, and 28 debug messages
Mainly, there's an exception thrown from taxonomy_vocabulary and upload upgrade path test complains about sql syntax.
Comment #8
carwin CreditAttribution: carwin commentedGah, I made the patch from the wrong branch - here it is again:
Comment #9
webchickMarking needs review!
Comment #11
carwin CreditAttribution: carwin commentedOk, I got the syntax issues figured out. However, running the upload upgrade path still fails - part of that is that it can't seem to find the Upload table. Looking at both the Drupal 6 and Drupal 7 databases on fresh install, I can't seem to find this table. So I'm not sure what about this I'm missing but I'm uploading another partial patch.
This can probably wait until Monday's COH, but I wanted to post it up before I forget.
Comment #13
BTMash CreditAttribution: BTMash commentedI'm looking at this as well from a testing perspective and I'm a bit stumped right now. I looked at the test and it *seems* like this test is supposed to be accounted for (entry in upload table, same entry not in files table).
Regarding the patch, db_delete does not take aliases from what I see. I get some strange behavior from the table prefixing which I'm digging into a bit more.
Comment #14
xjmSo the exception on QA is:
Argument 2 passed to db_delete() must be an array, string given
So this should be
db_delete('upload')
and thenupload.fid
later in the subquery, I think.Comment #15
xjmExample usage of
notExists()
with a select:core/modules/system/lib/Drupal/system/Tests/Database/SelectSubqueryTest.php
Comment #16
xjmOh also, the
else { }
is not necessary, since we're returning when theif
is met.Comment #17
carwin CreditAttribution: carwin commentedUploading a modified patch, even though we know it'll fail.
I removed the unnecessary else statement (thanks for pointing that out xjm), and cleaned up the query by removing the "u" from db_delete().
Additionally, thanks for looking at this BTMash.
Comment #18
xjmSetting NR to expose the fail. Apparently it's not jiving with the prefixing. We may need to explicitly call the query on the appropriate connection, rather than using the procedural function.
Comment #20
BTMash CreditAttribution: BTMash commentedI'm not sure if running a query like this is correct but I **believe** it is correct:
Alternately, I had this working on my machine as well (it is, however, slower):
I'll test them both out locally and should be able to give an answer tonight.
Comment #21
BTMash CreditAttribution: BTMash commentedHere is the modification of the patch by @carwin which should continue passing the tests. Note, however, that I have not been able to replicate the issue encountered by @gwulo or @jenlampton which is hampering me from confirming the issue. So regardless, we're still on 'needs tests'.
Comment #22
xjmExcellent, thanks @carwin and @BTMash.
Comment #23
xjmRegarding #21, is it that you are not encountering the error if you manually add an entry to the upload table that does not match anything in the files table?
Comment #24
BTMash CreditAttribution: BTMash commented@xjm, yes. In fact, that test is already in drupal-6.upload.database.php at line 14. I tried to add a larger number with no luck. I am starting to suspect it has something to do with the vid so I'm going to try something down that route.
Comment #25
BTMash CreditAttribution: BTMash commentedHunting through this more, I have been able to figure out a test that brings up a failure. However, I think this is really a symptom of some stranger behavior that is happening in the 7061 update. I had a suspicion that this was linked to something from the batch with the vids. So what my test does is create a series of vids in the {upload} table that are not in the node_revisions table (something larger than the batch will work with). Because the node revision does not contain that item, the loop moves on without updating the sandbox to the last vid that it processed. So while we can add in the delete statement, I don't know if there is any scenario where a file may be in the upload table and in the files table, but the corresponding node will not be in the node_revisions table (I suppose a possibility for this could be shared files if someone used something like the filefield sources module). So we should have that delete statement, but we likely also need something else.
Note to the testing system maintainers: this fail patch is very likely to cause you guys headaches.
Comment #26
xjmI think it's pretty clear this is no longer novice. ;)
Comment #27
BTMash CreditAttribution: BTMash commentedHere is a more full fledged test. This will add 1000 new entries to the table. Half of them are invalid fid, invalid vid. The other half are valid fid, invalid vid. The first patch addresses the behavior of the first bad use case. Does not address the second (I'm only attaching the test patch). Also a bad-behavior test. So our patch needs to go back to 'needs work'.
Comment #29
BTMash CreditAttribution: BTMash commentedThis patch will need a thorough review. I pass it through on deleting stale fid references AND later on deleting stale vid references. But I want to make sure this is not going overboard with deleting anything more than it should. Includes the test.
Comment #30
BTMash CreditAttribution: BTMash commentedRemoving whitespace.
Comment #31
xjmI do hope we can find a way to test this that doesn't add 40 minutes to our test runs. ;)
Comment #32
xjm#30: 1586542.combined.patch queued for re-testing.
Comment #33
BTMash CreditAttribution: BTMash commentedAlternately, we can trim it down to 250 items instead of 1000 as it should still run into the same issue.
Comment #34
xjmI keep retesting this to see if the super-long test run was a fluke and then forgetting to check back on the same day. :) Let's go ahead and cut it to 250. Want to upload a test-only and a combined with 250 and we can check the times?
Comment #35
xjmActually I'll just do that.
Comment #36
BerdirI'm a bit confused as to why this is necessary.
Have you tried where('f.fid = {upload}.fid')?
Once the query is built, it runs through query() just like a normal db_query() and any remaining curly braces in there should be prefixed correctly.
Or am I missing something?
Comment #37
BTMash CreditAttribution: BTMash commentedI have tried {upload}.fid and that is how it ends up inside the query (ie it does not substitute). I'm not sure why it does not, but had not explored into that.
Comment #38
Berdir@xjm is currently trying it locally.
We've also discussed performance of the query in IRC and it's probably OK, althought it would not hurt to maybe execute the query on a database with a really large amount of revisions. Maybe we could run it on one of those d.o test installations or a d7 upgrade test installation?
Comment #40
xjmOops.
Comment #41
xjmComment #42
BerdirOk, so the patches with the fix pass fine with {upload}, the test-only patches are taking a very long time, which makes sense as this is exactly the pattern that we're seeing here.
Which is ok, because we won't be committing them :)
So if we can answer the performance problem, then I think we're good here.
Comment #43
BTMash CreditAttribution: BTMash commentedLooking at the logs:
4,618,468 Result retrieved by project client. 15 min 56 sec ago
4,618,458 Result received from test client #973. 16 min 26 sec ago
4,618,333 Requested by test client #973. 59 min ago
4,618,323 Test request received. 59 min 7 sec ago
Seems like it still took about 45 mins?
Comment #44
Berdirthe passed test took "Test run duration: 42 min 1 sec].", the last 7.x "Test run duration: 38 min 40 sec].". That may or may not mean anything, because they were run by different testbots and these take a different amount of time.
I'll run the upgrade tests locally to see if there's a speed difference in a controlled environment.
Comment #45
xjm@berdir, I think you're right and it's fine now.
Comment #46
BerdirYes, confirmed that UploadUpgradePathTestCase takes 7s for me, with and without this patch.
Still would like to see the result of the following query being executed on a large database with lots of files and revisions like e.g. d.o before RTBC'ing this:
DELETE FROM upload WHERE NOT EXISTS (SELECT nr.vid FROM node_revision nr ON nr.vid = upload.vid}
Can we get that? :)
Comment #47
gregglesmysql> select count(1) FROM upload WHERE NOT EXISTS (SELECT nr.vid FROM node_revisions nr WHERE nr.vid = upload.vid);
+----------+
| count(1) |
+----------+
| 7 |
+----------+
1 row in set (8.56 sec)
mysql> select * FROM upload WHERE NOT EXISTS (SELECT nr.vid FROM node_revisions nr WHERE nr.vid = upload.vid);
+--------+-------+-------------+------+-----+--------+
| fid | vid | description | list | nid | weight |
+--------+-------+-------------+------+-----+--------+
| 172036 | 0 | | 0 | 0 | 0 |
| 56327 | 37660 | | 0 | 0 | 0 |
| 56328 | 37660 | | 0 | 0 | 0 |
| 56329 | 37660 | | 0 | 0 | 0 |
| 171345 | 55888 | | 0 | 0 | 0 |
| 171346 | 55888 | | 0 | 0 | 0 |
| 171347 | 55888 | | 0 | 0 | 0 |
+--------+-------+-------------+------+-----+--------+
Comment #48
BerdirThanks @greggles.
That doesn't sound too bad and is I think a no-brainer compared to how long some schema-changing queries probably take on d.o.
RTBC, let's fix this :)
Comment #49
BerdirWait a second :(
That's a node_revision in there, not a node_revisions. Why does that work?
Also, the fields('f') should be extend with an array('fid') to be consistent with the second query.
Comment #50
xjm@berdir and I talked about this in IRC; the table is
node_revision
(no s) in D7. It's renamed in node_update_7001() which runs before this. The comment is a typo.Comment #51
xjmFixing #49 / #50.
Comment #52
xjmComment #53
BerdirGreat. Really RTBC now, if the tests pass.
Comment #54
BTMash CreditAttribution: BTMash commentedReviewed on my end and tested as well. +1 from me.
Comment #55
webchickExcellent!!
I feel ok just going ahead and committing this one, because I've witnessed multiple reviews in IRC by people going over it very thoroughly, including manual testing. David, if you want a closer look though before the next release, I won't be offended if you decide to roll back.
Committed and pushed to 7.x! Thanks!!
Comment #56
Eric_A CreditAttribution: Eric_A commentedJust asking: why do we not add this as an explicit update dependency?
Comment #57
Eric_A CreditAttribution: Eric_A commentedComment #58
BerdirNote that the dependency is nothing new, there are many more similar queries in the same update function, this just confused us a bit so we documented it. So it probably doesn't hurt to have this, although it doesn't need to be major anymore.
Comment #59
xjmAs far as I can tell this is a good improvement.
Comment #60
BerdirAgreed. I looked through the dependencies to see if this is somehow already explicitly covered but I didn't find anything, so it just seems to by ordered the correct way round by default. Better to make it explicit.
Comment #61
webchickCommitted and pushed that follow-up too. Thanks!
Comment #62
David_Rothstein CreditAttribution: David_Rothstein commentedLooks awesome to me. In retrospect I wonder if some database API pain would have been avoided here by using a NOT IN query rather than notExists()?... but in any case, now I really know how notExists() works if I ever need it in the future :)
This also looks like an important enough fix to add to the release notes and CHANGELOG.txt.
Comment #64
mdowsett CreditAttribution: mdowsett commentedI'm upgrading a D6 site to D7 and is getting stuck on this exact same issue. I am using an absolute fresh 7.17 download and looking at the /modules/system/system.install file, it has all the same lines as in the .patch file...should this mean I shouldn't get these same errors?
If I apply the patch, I can't see it changing anything in my files....
....or do I have to wait to 7.18?
Comment #65
andypostRunning 'drush updb' for 7061 also fails with some memory leak and there's not ability to start again.
So please add to this issue a follow-up
Comment #66
xjmWrapping in a
db_table_exists()
is never a bad thing, and we can't add test coverage for a memory leak, so this should be good.Comment #67
David_Rothstein CreditAttribution: David_Rothstein commentedHow do you reproduce this? The code already looks like it should only ever run once...
If updates fail due to a memory leak we should fix that, but after it fails you can't run it again: #1462144-7: Update hook #7002 renames the D6 'comments' table to 'comment' without testing to see if the 'comments' table even exists.
Comment #68
andypostThis could be easily reproduced when converting a site with a lot of files.
This caused a tremendous size of {upload} table so php just hangs waiting to insert data in temporary table
@David_Rothstein the only part that supposes to run once is db_create_table() so better to wrap it
Comment #69
andypostThe better way to solve this trouble is to split this update function into parts: create temporary table, convert data, drop tables
But there's no room for the split
Comment #70
David_Rothstein CreditAttribution: David_Rothstein commentedWell, if it's really true that you can hit this issue, rerun updates, and recover without any side effects at all, then I guess it's OK to go in. Seems unusual, to say the least...
Anyway, fixing tags. (Drupal 7.21 is already out and this patch has not been committed yet.)
Comment #71
David_Rothstein CreditAttribution: David_Rothstein commentedAlright, still a little unsure about this one, but committed so we can hopefully never have to think about system_update_7061 again - somehow seems unlikely, though :)
http://drupalcode.org/project/drupal.git/commit/92d2299
Thanks!
Comment #73
cruell CreditAttribution: cruell commentedSorry but I still have this problem.
I mounted D7.22 upgrading from D6.28 and from D5.23.
No problem from D5 to D6 but when I try to upgrade from D6 to D7.22 I have che same problem.
After upgrade.php the result is:
I checked the patch files but i saw that D7.22 have integrated it.
So how can I do for resolve it.
My problem is tha in my articles there isn't the file attached.
My site hase about 620 pages and I can't renew it by handle.
Help please!
Comment #74
David_Rothstein CreditAttribution: David_Rothstein commentedDoes this happen the first time you run the Drupal 6 to 7 update, or does the update stall and then you try it again and that's when this error happens (i.e., as in the scenario described in #65 onwards)?
I don't see how this can happen except in the latter case since the query only inserts distinct values into the table.
If the initial part of this update function is taking too long to run on some systems we may need to break up the batch processing within it further.
Comment #75
cruell CreditAttribution: cruell commentedEvery time I execute "update.php" I see that tere is 17 upgrade pending.
I tried it 3 or 4 time.
I did try this too:
I deleted the system_update_7061 table, truncated the file_managed table, and ran update.php again.
But nothing to do, coz this is the result:
And after this I did verify my tables and
-table system_update_7061 has 1719 rows,
-table upload has 2,208 rows
-table files has 2,209 rows (difference is that there is a file named ".po" for the translation)
I hope it can help.
Comment #76
David_Rothstein CreditAttribution: David_Rothstein commentedOK, but what happened the first time you ran it? I assume there were more than 17 upgrades pending then (since there are something like 130 which run during the Drupal 6-to-7 upgrade).
Once the upgrade fails once, I don't really believe it's possible to try to restart it generally (see my earlier comments above); I think accepted wisdom is that you have to restore from a database backup at that point.
So that's why I think it's important to understand how this failed the first time you tried it. Whatever failure happened then, that's the real issue.
Comment #77
cruell CreditAttribution: cruell commentedObviure I have backup of all and I can repeat the conversion all time I need.
When I did try the update from D6 to D7 first time I had 147 pending.
At the end of first update the result is
What must I change in my tables for not have that problem during conversion?
Maybe must I move my question in an other thread?
Comment #78
David_Rothstein CreditAttribution: David_Rothstein commentedAh, in that case it looks like #1260938: d6 to d7 update fails on file duplicates '#7061 Integrity constraint violation' is your issue (same update function, different bug).
So I'll close this again, but take a look at that issue for ideas on how to fix this problem. (And if you know how your Drupal 6 files table wound up in a state to trigger this, please comment on that issue since it seems like several people experience this but no one knows exactly what the root cause is.)
Comment #79
cruell CreditAttribution: cruell commentedThank you David
Comment #80
alberto_zurita CreditAttribution: alberto_zurita commentedHi cruell,
I was wondering if you found a solution to this problem. I have exactly the same one, and can't go past the 17 pending updates
Please let me know. I would really appreciate it.
************************************
The following updates returned messages
system module
Update #7061
Failed: PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '12' for key 1: INSERT INTO {system_update_7061} (vid) SELECT DISTINCT u.vid AS vid FROM {upload} u; Array ( ) in system_update_7061() (line 2775 of /data/_b/azurita-LAUCZ/modules/system/system.install).
Comment #81
kenorb CreditAttribution: kenorb commentedComment #82
kenorb CreditAttribution: kenorb commentedComment #83
jessZ CreditAttribution: jessZ commentedI have a similar block this is a drupal 5 site that was just upgraded to 6 and we are trying to get to 7. there are about 2000 attachments that were uploaded under the old scheme
DOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '70' for key 'PRIMARY': INSERT INTO {system_update_7061} (vid) SELECT DISTINCT u.vid AS vid FROM {upload} u; Array ( ) in system_update_7061()