User-Agent: Mozilla/5.0 (Macintosh; U; PPC Mac OS X; fr) AppleWebKit/418.8 (KHTML, like Gecko) Safari/419.3 Build Identifier: Some series use a creator id of 0, which won't work when referential integrity constraints are enabled (see bug 109473). It would be better to make this column nullable in the DB, and use NULL for when the creator of seriesis unknown Reproducible: Always

Yes, that makes total sense. I'm surprised I didn't file this before.

unbitrot

Right - presumably we need to do this because 0 is not a valid userid? Gerv

(In reply to comment #4) > Right - presumably we need to do this because 0 is not a valid userid? Exactly. It would break foreign key checks.

Comment on attachment 231991 [details] [diff] [review] patch v1.1 >Index: Bugzilla/Install/DB.pm >+ # 2006-08-03 remi_zara@mac.com bug 3462411, make series.creator nullable >+ $dbh->bz_alter_column('series', 'creator', {TYPE => 'INT3'}); >+ $dbh->do("UPDATE series SET creator = NULL WHERE creator = 0"); >+ > _copy_old_charts_into_database(); Any reason to put this change so early in this script? Usually, we put changes chronologically to not break upgrades. Are you sure this doesn't break anything? mkanat, is this OK for you (double check)?

(In reply to comment #6) > Any reason to put this change so early in this script? Usually, we put changes > chronologically to not break upgrades. Are you sure this doesn't break > anything? mkanat, is this OK for you (double check)? Ideally I'd like to see it later in the script, unless there's some very good reason for it to be there. Also, I'd like to see it wrapped in a bz_column_info on series.creator, so that UPDATE doesn't run every time we run checksetup. I know it's harmless, but I expect checksetup to only do its work *once*, and then not do it again. Running the update every time checksetup runs could hide bugs. (That is, something setting series.creator to 0 and us not noticing.)

Comment on attachment 231991 [details] [diff] [review] patch v1.1 >Index: Bugzilla/Install/DB.pm >=================================================================== >+ # 2006-08-03 remi_zara@mac.com bug 3462411, make series.creator nullable >+ $dbh->bz_alter_column('series', 'creator', {TYPE => 'INT3'}); >+ $dbh->do("UPDATE series SET creator = NULL WHERE creator = 0"); >+ > _copy_old_charts_into_database(); To me it looks like this change here because it needs to happen before _copy_old_charts_into_database() sub is called. This is because that sub already expects to be able write NULL values. But I do agree with mkanat that this should be wrapped inside a if ($dbh->bz_column_info('series', 'creator')->{NOTNULL}) { } block to keep it executing all the time. This is same logic as in line 321 for user id in quips table. Please add this if block and a comment to say that this block needs to happen before _copy_old_charts_into_database() sub before commit.

Approval granted with the changes requested in comment 8.

How can we have series with an ID of 0?? IMO, these series should be removed instead of changing the field to NULL. Probably this was due to some bug in an older version of Bugzilla.

(In reply to comment #10) > How can we have series with an ID of 0?? IMO, these series should be removed > instead of changing the field to NULL. Probably this was due to some bug in an > older version of Bugzilla. No, the creator is 0. It's from migrating old charts into the DB.

The IF block has been added, as well as the comment about why to put this code here. Checking in sanitycheck.cgi; /cvsroot/mozilla/webtools/bugzilla/sanitycheck.cgi,v <-- sanitycheck.cgi new revision: 1.122; previous revision: 1.121 done Checking in Bugzilla/Series.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Series.pm,v <-- Series.pm new revision: 1.13; previous revision: 1.12 done Checking in Bugzilla/DB/Schema.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v <-- Schema.pm new revision: 1.63; previous revision: 1.62 done Checking in Bugzilla/Install/DB.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/DB.pm,v <-- DB.pm new revision: 1.9; previous revision: 1.8 done