290581 - Checksetup fails emailprefs migration if the database has been manually modified

Status: RESOLVED WONTFIX

(Bugzilla :: Installation & Upgrading, defect)

Description

[Joel Peshkin]

Migrating email preferences to new table ...
DBD::mysql::st execute failed: Duplicate entry '1-0-2' for key 1 [for Statement
"INSERT into email_setting (user_id, relationship, event) VALUES (1, ?, ?)"] at
./checksetup.pl line 3792

Comment 1

[Jacob Steenhagen]

Did you do any testing on the emailprefs patch? If so, you may need to drop (or
just empty) your email_setting table. If not, then there is some oddity in your
current email prefs inside the profiles table.

Comment 2

[Joel Peshkin]

No previous testing.  I am going directly from a clone of a production 2.17.1 to
a fresh cvs checkout.

Comment 3

[Max Kanat-Alexander]

I've seen this happen, but only after checksetup failed previously on the same
database, or was interrupted.

Comment 4

[Joel Peshkin]

OK, I see the problem...

When I clone the old database to load the new one, email_settigns still exists
but did not get emptied (there was nothing else to write into it).  We should
probably not fail with an error if this happens.

Comment 5

[Max Kanat-Alexander]

(In reply to comment #4)
> When I clone the old database to load the new one, email_settigns still exists
> but did not get emptied (there was nothing else to write into it).  We should
> probably not fail with an error if this happens.

  Hrm -- I don't fully understand what happened, here. Could you explain to me
how a normal user who wasn't testing emailprefs could run into this situation?

Comment 6

[Joel Peshkin]

What triggered this is...  I ran the upgrade, then did a "mysqldump | mysql" to
reload my old db.  That caused all the old data to be restored, but the newly
created tables still hung around.

So, the issue is not so much that this would happen in a normal flow.  The
question is...  should we have a fatal when the extra tables exist or should we
do something more graceful?

Comment 7

[Max Kanat-Alexander]

(In reply to comment #6)
> So, the issue is not so much that this would happen in a normal flow.  The
> question is...  should we have a fatal when the extra tables exist or should we
> do something more graceful?

  Well, generally, my policy is that checksetup shouldn't have to deal with a
database state that checksetup never could have created. Of course, if people
have common problems (like their indexes being upgraded to PRIMARY KEYs) that
checksetup needs to deal with in an upgrade, then I think it's OK to make
exceptions.

  Still, it's exactly these sorts of schema changes that checksetup is
triggering off of. If the old emailflags column exists, and the email_settings
table is empty... then we do the migration.

  Perhaps it *is* possible that the email_settings migration should deal more
smoothly with failure, and that it should be rewritten to be faster anyhow
(right now it's O(n^2)), but it's not so critical that I'd want to fix it for 2.20.

Comment 8

[Max Kanat-Alexander]

Okay, thinking about it, I'm going with my earlier position that we don't handle states that checksetup never could have created (unless they're extremely common or trivial to fix).