Hi, I've had this error when upgrading to Drupal 7.16.

The following updates returned messages
system module
Update #7075

* Failed: PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '2-create blog content' for key 'PRIMARY': UPDATE {role_permission} SET permission=:db_update_placeholder_0 WHERE (permission = :db_condition_placeholder_0) ; Array ( [:db_update_placeholder_0] => create blog content [:db_condition_placeholder_0] => create blog entries ) in system_update_7075() (line 3050 of/home/mysite/public_html/modules/system/system.install).

Any help in completing this task manually to the database (7075 - Fix renamed queries from system_update_7000().) would be appreciated, thanks!

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

David_Rothstein’s picture

Title: Error in upgrade to Drupal 7.16, Update #7075 » Error in upgrade due to system_update_7075()
Version: 7.16 » 7.x-dev
Status: Active » Postponed (maintainer needs more info)

system_update_7075() is not included in the Drupal 7.16 release. (It's on the 7.x branch and scheduled for a future release, but it hasn't been released yet.)

So it sounds like you aren't updating to the code you think you are... What method are you using to get the Drupal 7.16 release and upgrade to it?

In any case, system_update_7075() is scheduled for release soon, so if there's a bug in it we should fix it. Can you provide a bit more information to help? - i.e., what do the Node and Blog module sections of your permissions page look like on the current site (before the upgrade)? What is the full list of permissions that are listed there?

Thanks!

David_Rothstein’s picture

Adding tag - I'm not sure if this is actually a release blocker, but would like to understand it more before we do release this code in an official release.

Irous’s picture

Ahh crap! You're right, I thought I had downloaded v7.16, but it was actually the dev version! :( I'm going to revert back to a previous db back up and update properly to v7.16 now..

Would you still like the information about the Node and Blog permissions I have after I've reverted back and before I do the 7.16 update?

David_Rothstein’s picture

Would you still like the information about the Node and Blog permissions I have after I've reverted back and before I do the 7.16 update?

If possible, that would be great.. thanks! (Since the code currently in 7.x-dev is likely going to be released as Drupal 7.17 pretty soon, so it's definitely important to track down any bugs in the update function.)

BTMash’s picture

I didn't run into this issue when I first wrote out the patch, but my suspicion is that somehow, the following kind of entry is in the database:

1|create blog entries|blog
1|create blog content|node

So when the update runs, we run into an issue since we have "duplicate" entries based on the primary key:

1|create blog content|blog
1|create blog content|node

Since the primary key is on the first 2 columns, they are seen as duplicate which would cause the PDOException. So the task is to now figure out how to fix up 7075 around this issue.

Irous’s picture

FileSize
511.35 KB

Here's my permissions page as it is, before I upgraded to the latest dev or v7.16, attached. I've renamed 'Blog' to "Journal entry", btw, if you're looking for the blog permissions. I'm guessing this might be what caused the problem? I'm pretty sure I renamed them that in admin sometime years ago.. Their machine name is still 'blog', however. Let me know if you want to see db table info (and which ones exactly).

BTMash’s picture

FileSize
107.85 KB

I looked at the image and while it didn't quite help me, it helped verify what I wrote in #5. I ran through the steps right now:

  1. Create D6 site
  2. Install Blog, Forum module
  3. Set up role permissions
  4. Update codebase and upgrade db to stable version of D7 (pre-update-7075)
  5. Save new role permissions (blog, forum, etc are now under node)
  6. Update codebase to post-update-7075 version
  7. See error on upgrade.

The table entries look like the following:

|   3 | edit any blog content           | node     |
|   3 | edit any blog entry             |          |

Which means the old entry ('edit any blog entry') still resides. I am attaching a gzip of my sql dump just so there is an easier starting point for folks to verify the upgrade failure (until we can show in a patch that 7075 is failing) - user 1 credentials are admin/password.

BTMash’s picture

Status: Postponed (maintainer needs more info) » Needs review
FileSize
888 bytes

This is a tentative patch (it is late here so I will be trying to see if a test is possible tomorrow - I have an idea on how to simulate it) where I first check if the new permission already exists in the system (if it does not, then create the update) followed by deletion of the old permission. I haven't used fetchField() before hence it definitely needs a review.

jhodgdon’s picture

Status: Needs review » Needs work

Looking at the original error reported by the user, it looks like the problem that is happening is not what you think it is. There is a DBTNG problem, because the same placeholder is appearing twice in the query:

UPDATE {role_permission} SET permission=:db_update_placeholder_0 WHERE (permission = :db_condition_placeholder_0) ; Array ( [:db_update_placeholder_0] => create blog content [:db_condition_placeholder_0] => create blog entries )

It used :db_update_placeholder_0 twice in the query. One of them should have a different placeholder number. If that query had two different placeholders, I believe it would be fine, and it would be changing "create blog entries" to "create blog content", as it should have been doing.

The code that generates this query looks fine:

foreach ($permission_changes as $permission_key => $permission_change) {
    db_update('role_permission')
      ->fields(array('permission' => $permission_change))
      ->condition('permission', $permission_key)
      ->execute();

I don't know what's going on under the hood in DBTNG here, but that looks like the problem. I don't think we need to change the code as suggested in #8.

BTMash’s picture

@jhodgdon, db_update_placeholder is only being used once. SET permission=:db_update_placeholder_0 is followed by permission = :db_condition_placeholder_0. Basically, there are 4 scenarios to this upgrade (as far as I can see).

  1. User creates a D7 website.
  2. User upgrades a site from D6 directly to D7 post-update-7075.
  3. User upgrades a site from D6 to D7 pre-update-7075, never changes the role permissions page and later updates to post-update-7075.
  4. User upgrades a site from D6 to D7 pre-update-7075, changes role permissions and later updates to post-update-7075.

(1) never runs into the issue. (2) properly upgrades since the new permissions don't exist in the system. Same goes for (3). (4) has both permissions (from the above example, 'edit any blog content' and 'edit any blog entry') in the system. Renaming the latter to the former will cause duplicate entry exceptions if the rid is the same as well. If you try from the db dump I provided above, you should run into the issue as well without the patch. Working on the test right now.

BTMash’s picture

And here is the test and the test + patch.

BTMash’s picture

Status: Needs work » Needs review

Setting to 'needs review'.

jhodgdon’s picture

RE #10 - oops, I misread that query. Good analysis, and this patch/test look good to me (though I didn't yet give them an extremely careful review).

David_Rothstein’s picture

FileSize
7.1 KB

I did some testing of this and it seemed to work well. Here's a minor reroll just to remove t() from the test assertion message.

However, the tricky thing is that for a site which already upgraded from Drupal 6 a long time ago, and is now doing a regular update from Drupal 7.16 to 7.17, is it really a good idea to bring their old permissions "back from the dead" like that? The patch looks like it sort of handles that, in the sense that for a particular permission, if it finds the new permissions assigned in the database it will completely respect them (and ignore whatever the old ones were). But if you assigned some permissions but deliberately left others unassigned, it will still overwrite the others with the old Drupal 6 values.

That's a little opaque, but as a concrete example, let's suppose you used to allow some users with an "editor" role to delete blog entries on the site, but sometime after your Drupal 7 upgrade you decided that was too much power and took the permission away (or perhaps just looked at the permissions page and confirmed it was already gone, due to this bug :) So now, nobody on your site except user 1 can delete blog entries. But after this update function runs, those users will be magically re-granted the permission to delete blog entries again, which really doesn't sound good to me.

In short, I'm wondering if we don't just want to get rid of system_update_7075() entirely (or at least have it only do a db_delete() to clean up the stale permissions)? Since #1685110: Upgrade of forum.module d6-d7 loses permission fixed system_update_7000(), the bug should already be fixed for a site upgrading from Drupal 6 today. But for a site that has been on Drupal 7 a while, I think maybe we just have to accept that it's impossible to know what their actual intentions are any more, and not try to fix this bug for them.

In any case, if it turns out this is the last issue blocking the release of Drupal 7.17, I think we should just fix it by killing system_update_7075() for now. (We can always add it back as a new update function later on, after more discussion.)

Damien Tournoud’s picture

I agree that we should not touch existing sites.

David_Rothstein’s picture

FileSize
1.26 KB

OK, here's a patch that removes the update function entirely (for now).

In the past when doing this kind of thing we've sometimes left the update function in place but made it a no-op, in case there are people running 7.x-dev on live sites (even though they shouldn't) who may already have run it; we don't want those people to have an update function numbering conflict later on.

But in this case, we already have higher-numbered update functions in the codebase, so I think we can just remove this one with no problems. (Which seems preferable, since otherwise it appears in the user interface as an update function which needs to run and confuses people as a result.)

David_Rothstein’s picture

Status: Needs review » Reviewed & tested by the community

Marking this RTBC with the intention of committing it so we can get Drupal 7.17 released.

Reviews/comments still welcome, even afterwards, though :)

David_Rothstein’s picture

Priority: Major » Normal
Status: Reviewed & tested by the community » Needs work

Committed the rollback for now: http://drupalcode.org/project/drupal.git/commit/6d462a30f69abf96cc05ceff...

Let's leave this open for further discussion, to see if we want to add it back (or an abbreviated version of it back) going forward.

BTMash’s picture

I'm fine with the rollback to avoid touching existing sites per your outline in #16 (I hadn't thought of that scenario in the latest patch and thinking on it more...I don't believe there is a way to resolve that use-case). However, since we will still have users coming in from D6 after this release goes out, the permissions for blog, forum content types are going to have issues. Perhaps for the 7075 update, there should be a message in the upgrade process to the user to doublecheck and resave their permissions for this?

David_Rothstein’s picture

Hm... Didn't system_update_7000() get fixed such that anyone coming from Drupal 6 in the future won't have this problem anymore?

The problem does still exist for anyone who already upgraded previously, but it's existed for a while and now will just keep on existing :)

I think the idea of having the update function display a message is an interesting one. It would probably makes sense to combine it with whatever else we do here (i.e., if we add back an update function to delete the obsolete permissions from the database, then the message could be set at the same time; and in fact, I think it means we could even check the permissions before deleting them, which would allow the message to tell people specifically what their previous permissions looked like, if we want to go through the trouble of making it do that).