Upgrade fails because of integrity constraint violation in system_update_7060

Tagging.

@mongolito404 Attaching my D6 database which fails the upgrade

Here is a new patch from marvin, mongolite404 and me.

The idea is to store the already commited nodes and look it up and decide whether to use FIELD_STORAGE_UPDATE or INSERT

You have to add #895032-8: Upgrade fails on missing file_usage table before applying this patch.

Yesssssss! let's store every nid in memory. Why not the nodes themselves too? Note the nid on this issue (even if we only use every second nid), that's why not.

This one should be faster. It does not add a new query for each field. Also needs testing. I'm on that now.

Actually I could not solve the problem, but after some little changes and suggestions of dereine I end up with _other_ error:

Failed: PDOException:
SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'id' cannot be null: INSERT INTO {file_usage} (count, fid, module, type, id) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4); Array
(
    [:db_insert_placeholder_0] => 1
    [:db_insert_placeholder_1] => 17
    [:db_insert_placeholder_2] => file
    [:db_insert_placeholder_3] => node
    [:db_insert_placeholder_4] => 

)
in file_usage_add() (line 609 of includes/file.inc).

A NULL is passed to the last parameter of file_usage_add() at

      // Add the usage entry for the file.
      $file = (object) $file;
      file_usage_add($file, 'file', 'node', $revision->nid);

@marvil07: $revision is an array and it's an array in may code...

The patch changes the following:

- Defaults to FIELD_STORAGE_UPDATE. That simply means that the relevant rows in the data and revision tables are deleted and re-inserted. If there's nothing to delete, that's perfectly fine. Probably a bit slower, but I don't think that a query to check if there is already a row for that node would be significantly faster and this is much easier.

- Only tries to create field and the instances once. In case something goes wrong with the update, it blows up when it tries to re-create the field and has lead to duplicate issues like this: #894978: FieldException: Attempt to create field name file which already exists and is active.

- Also contains the query changes from above.

- Because of the changes in the query from above, I'm pretty sure that the array_pop() part is obsolete. That didn't make any sense to me in the first place...

With this patch, the upgrade works for the provided demo database, but there is maybe another issue:

- The original site used sites/default/files to store the files. I tested this by using a separate site called sites/d7, so my file path was sites/d7u/files. The resulting URI then looks like this and this is obviously wrong: "public://sites/default/files/cocteausounds.zip". Do we need to copy the files to public:// if the path is different or is this something that's simply not supported?

Some updates...

- Updated the query that looks for uploads to join over node_revisions so that it also catches uploads that have been deleted
- Use DISTINCT and an explicit order to load the vid's
- Don't use db_query_range() in the second query that loads the uploads. Doesn't make much sense to use the limit on two different queries imho. If that gets to big result sets, we can always lower the limit on vid. This allows to process all files of a revision without any hacks.
- Instead of sorting on the weight, it's included and passed forward to the field.

I verified that this works with some test data. A current problem in the code might be that it assumes that the highest vid of a node is always the active one, I'm not sure if that is always true. It looks like clicking on "reset" to go back to an old revision actually creates a new revision with a higher vid and copies everything over, so that's actually fine. Not sure about workflows and things like that, though...

The current vid of the node isn't always the active one, revision_moderation and possibly other modules depend on that behaviour.

I can confirm that the upgrade does complete with the patch in #11.

Per #12 and #14.

Should this critical D7 upgrade path bug be one of the last two or three beta blockers or, maybe, would the patch in #11 reduce it to a clean-up issue?

@juan_g: I think it's okay to keep track of beta blockers and upgrade path bugs separately, since all upgrade path bugs automatically block a beta release.

Berdir said in #898584-6: Clean-up the upgrade path: system that he and chx decided it makes the most sense to split field creation and the data insertion into two separate updates (at least that's how I understand what he said!) so I'm working on a patch that does that, as well as handle the case where the current revision is not the newest.

Yes. The field creation and the actual data manipulation needs to be two function or else rerunning the update will die horribly when you try to recreate the field. If it's two functions, the field recreate update function will not be rerun.

ksenzee wrote:
> I think it's okay to keep track of beta blockers and upgrade path bugs separately, since all upgrade path bugs automatically block a beta release.

Probably you are referring to the critical ones. Currently, among the open and not fixed, there are 17 normal, 8 major, and 5 critical D7 upgrade path issues. 2 of these 5 critical upgrade issues are also tagged as beta blockers.

Maybe the other 3, including this one, should be tagged beta blockers as well for consistency and clarity, so that developers who are listening to Dries' Drupal 7: let's get it done can help better to fix and review these for the beta release. ;)

I'm attaching an updated patch. Here are the differences from #11:

- splits system_update_7060 into two functions (and renumbers the existing 7061 to 7062)
- adds the utility function _update_7000_field_sql_storage_write() from #898558: Clean-up the upgrade path: node and uses it to save field data
- uses the existing _update_7000 utility functions where appropriate
- changes the name of the filefield we create for upload.module files to "upload", which seemed like it would be easier for site admins to figure out

I did my best to add and update inline comments but they're still not exactly a marvel of modern literature.

I also did some testing with a D6 site that used revision_moderation, so that the current revision on a node wasn't the active revision. It left the node in an odd state, with uploads from one revision and body text from another, but nothing blew up, and reverting to any of the revisions put everything back the way it belongs. I think that's plenty for core to handle. Revisioning modules should probably set every node's current revision to active during the update, and put them back afterward.

For reviewers, I'm attaching an interdiff from #11, as well as a reviewers' version of the full patch that doesn't make any whitespace changes.

+++ modules/system/system.install
@@ -2681,132 +2681,163 @@ function system_update_7059() {
+    // Create the field.
+    $field_instance = field_info_field('file');
+    if (empty($field_instance)) {
+      _update_7000_field_create_field($field);
+    }
+    else {
+      $field = $field_instance;
+    }

Not sure if we need this anymore since the update functions are now separated. Same for instances.

+++ modules/system/system.install
@@ -2681,132 +2681,163 @@ function system_update_7059() {
+  }
+  ¶
+  $node_revisions = array();

Trailing whitespace :)

+++ modules/system/system.install
@@ -2861,23 +2889,30 @@ function system_update_7060(&$sandbox) {
+      // Update the node revision's upload file field with the file data.
+      $revision['file'][LANGUAGE_NONE][$delta] = array('fid' => $file->fid, 'display' => $file->display, 'description' => $file->description);

Just wondering, why do you explicitly define the array instead of simply using $file?

Note that at least the weight is missing here.

Other than that, this look good to me, thanks for picking this up :)

Powered by Dreditor.

That explicit array looks good because the update field storage sql writer uses the incoming data for columns not the field schema.

Add "beta blocker" tag according to http://drupal.org/community-initiatives/drupal-core

Sorry this took so long to get back to. The attached addresses Berdir's feedback plus some improvements chx suggested on IRC:

- removes the field_info checks when adding the new field and instances in 7060
- orders the fid query by weight so we're sure of adding the files to the new filefield in the right order (Berdir's concern about weight being missing)
- uses a table to keep track of which vids have uploaded files attached, which gets rid of a couple of potentially very slow queries
- adds upgrade tests for upload module

I also changed the default view mode for the filefield to show the file table. That's not ideal, but if we don't, the full page node view won't show the file table (#553298-176: Redesign the 'Manage Display' screen).

(There is a @todo in the new tests because one of the node revisions I added appears to have lost its node body. I have yet to figure out if that's a problem with the nodes added for the test, or if it's a bug with the node upgrade path. When I figure it out I'll fix one or the other.)

Turns out it's a bug with the node upgrade path - #926794: revisions of unpublished nodes lose their body field on upgrade - which means there's nothing left to fix in this issue that I'm aware of.

So what we need is a full review :-) to go to the beta.

The helper is in. The lading of field.crud is cruft now we do not use it. // Create a table to store ids of node revisions with uploads attached. well i see that you are creating that table but why? (because paging a DISTINCT query is slow and it gets slower as the paging gets deeper.)

Reroll. Go testbot!

Oops, left out the new test files. Try this one instead.

@#$! This one should be right. Sorry, testbot.

Given that #21 already said 'looks good' and i was unable to pick on anything but comments and minor stuff, this def looks good.

One line too many removed around the field.crud.inc include -- we still need to enable the poor file module, ya' know. Added that back, tests now pass.

When it took me three tries just to upload the patch, I should probably have just given up. Thanks chx. This is green and ready to go.

webchick pointed out I still have the @todo in here from before we fixed #926794: revisions of unpublished nodes lose their body field on upgrade. Here's the same patch with that removed. Let's see how fixed #926794 really is. :)

Committed to HEAD. Yay!

Don't shoot but my D6 -> D7 head failed on #7061. Do we still have unresolved issues?

* Failed: PDOException: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ') ORDER BY u.vid, u.weight, u.fid' at line 1: SELECT u.fid, u.vid, u.list, u.description, n.nid, n.type, u.weight FROM {upload} u INNER JOIN {node_revision} nr ON u.vid = nr.vid INNER JOIN {node} n ON n.nid = nr.nid WHERE u.vid IN () ORDER BY u.vid, u.weight, u.fid; Array ( ) in system_update_7061() (line 2747 of /Users/steinmb/htdocs/sites/smbjorklund.com/drupal/modules/system/system.install).

The upload module is not in use in D6 so the module is not enabled on upgrade. It might have been enabled at some point then I got the [upload] table. Here is a dump of D6 from PhpMyAdmin:

CREATE TABLE `upload` (
  `fid` int(10) unsigned NOT NULL DEFAULT '0',
  `nid` int(10) unsigned NOT NULL DEFAULT '0',
  `vid` int(10) unsigned NOT NULL DEFAULT '0',
  `description` varchar(255) NOT NULL DEFAULT '',
  `list` tinyint(3) unsigned NOT NULL DEFAULT '0',
  `weight` tinyint(4) NOT NULL DEFAULT '0',
  PRIMARY KEY (`vid`,`fid`),
  KEY `fid` (`fid`),
  KEY `nid` (`nid`)
) ENGINE=MyISAM DEFAULT CHARSET=utf8;

But the table does not contain any data.

WHERE u.vid IN () ORDER BY

Looks like we need a check to see if there actually are any vid's? However, it's weird that the update even got that far, shouldn't it just drop out in the first update function because there is no data to convert? Maybe it was enabled for some node types?

Not sure what the expected behavior is when there are no uploads. Should the field be created and be left empty or should it completely skip it?

@steinmb: You probably want to uninstall upload.module anyway if you don't want to end up with a file field on every node type that upload.module was enabled for. At least unless we decide to skip that step in this case...

The attached patch adds a check and just removes eventually existing variables and the upload table in case there is no data.

The patch fixed the problem on my system :)

Updated the inner comment, removed the first comment. I don't think that one is necessary.

This looks great and is RTBC if the tests pass.

Agree on the doc changes + the patch still works.

Agreed.

Committed to HEAD. Thanks!