Okay, so this is the smallest possible start that I can take on referential integrity. Basically, Bugzilla::DB::Schema and all the DB drivers need to understand referential integrity, and then I'll implement it on one of the least-used tables in Bugzilla, profiles_activity.

Okay, here it is! Some of this code is currently unused (the code to drop FKs), but I tested it all anyway, and it works on both MySQL and Pg. It works for adding a new FK, dropping an FK, checking the values before adding the FK, creating new tables, and everything else.

I fixed the POD, in this one.

I've tested this code pretty well, and I'm pretty confident about it. Anyway, who could review it?? Requesting approval directly as module owner.

It should be noted that at this time only PostgreSQL will enforce these, as we still use MyISAM tables in MySQL. I tested it all with InnoDB tables on my local installation, and it does work, when we switch to InnoDB tables. (At that time we'll also have to re-create all the FKs for all the tables.)

I've decided to wait until bug 347475 gets checked in to check this in, because then I won't have to go back and re-implement all the referential integrity checks for MySQL.

Why don't you explicitly name your foreign key constraints in all cases, so that you don't have to "guess" or digg for the name ? By my tests, you can do create table test2 (col1 integer, constraint fk_name foreign key (col1) references test (col1), col2 integer); in both mysql (4.1) and postgresql (8.1). So in get_type_ddl, you can put ", constraint fk_name foreign key (col1) references test (col1)" in create table context, or ", add constraint fk_name foreign key (col1) references test (col1)" in alter table context instead of just "references test (col1)", passing the name of the field as an extra argument. The code coud then be generic and brought up in Schema.pm.

I found it kept the code simpler to not have to specify a name for the constraint except when absolutely necessary. The MySQL code works. I'll consider your suggestion, though.

(In reply to comment #7) > I found it kept the code simpler to not have to specify a name for the > constraint except when absolutely necessary. Well you can compute a name, for instance of the form postgresql use. The only case a predetermined name might be necessary would be for some DBMS (like Oracle) which impose tight limits on identifier name. A reference definition could contain an optional "name" key, to override the computed name when it would exceed the limit. I admit that it might complicate the code a little, but perhaps not much more that parsing "show create table" output, and not having common code for PG and Mysql. > The MySQL code works. Of course, but I'm not sure Mysql guarantees that the output of "show create table" won't change, or that Postgresql guarantees that they will name their foreign keys always the same. Of course that's an hypothetic risk :) On the other hand, naming constraints is part of the SQL spec... > I'll consider your suggestion, though. > Cool !

going back to requested on approval since this is targeted at 3.2 (and sounds quite intrusive)

I decided that Remi was right, and that we should explicitly name all of our keys. I like this code much more. Also, Remi, if you want to show me some code for sorting tables in the correct creation order, I'd be totally willing to take it (in a separate bug).

(In reply to comment #10) > Also, Remi, if you want to show me some code for sorting tables in the correct > creation order, I'd be totally willing to take it (in a separate bug). > see attachment #232346 [details] [diff] [review] of bug 109473

Comment on attachment 257936 [details] [diff] [review] v2 Okay, I've tested this more thoroughly, including upgrades from very old versions, and it currently works just fine. :-)

Checking in Bugzilla/DB.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v <-- DB.pm new revision: 1.94; previous revision: 1.93 done Checking in Bugzilla/DB/Schema.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v <-- Schema.pm new revision: 1.81; previous revision: 1.80 done Checking in Bugzilla/DB/Schema/Mysql.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema/Mysql.pm,v <-- Mysql.pm new revision: 1.16; previous revision: 1.15 done Checking in Bugzilla/Install/DB.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/DB.pm,v <-- DB.pm new revision: 1.27; previous revision: 1.26 done

(In reply to comment #12) I find you use fk_${table}_${column}_${to_table}_${to_column} to form a foreign key name, and this key name may always exceed a length of 30 characters. But in Oracle, an indentifier must be less then 30 characters, so I think we've better have another naming rule here. sub _get_fk_name { my ($self, $table, $column, $references) = @_; my $to_table = $references->{TABLE}; my $to_column = $references->{COLUMN}; return "fk_${table}_${column}_${to_table}_${to_column}"; }

(In reply to comment #14) > (In reply to comment #12) > I find you use fk_${table}_${column}_${to_table}_${to_column} to form > a foreign key name, and this key name may always exceed a length of 30 > characters. Xiaoou--That's why I made _get_fk_name into a separate subroutine, so that you can override it!

(In reply to comment #15) > Xiaoou--That's why I made _get_fk_name into a separate subroutine, so that > you can override it! yes, I see. But I think we can not get a simple way to generate a fk_name less than 30 characters with table names and column names, because it may easily cause names conflict. So I think it is better for us to specify the fk_name in the schema, by adding a field in REFERENCES. Any idea?

(In reply to comment #16) > So I think it is better for us to specify the fk_name in the schema, by adding > a field in REFERENCES. No, that adds needless complexity. You can use a short hash function to generate a unique but short string from the table and columns. Anyhow, this discussion should happen in another bug.