Last Comment Bug 347439 - Implement support for referential integrity in Bugzilla::DB::Schema and implement it on profiles_activity.
: Implement support for referential integrity in Bugzilla::DB::Schema and imple...
Status: RESOLVED FIXED
[roadmap: 3.2]
:
Product: Bugzilla
Classification: Server Software
Component: Database (show other bugs)
: 2.23
: All All
: P1 enhancement (vote)
: Bugzilla 3.2
Assigned To: Max Kanat-Alexander
: default-qa
:
Mentors:
Depends on: 347475
Blocks: bz-references
  Show dependency treegraph
 
Reported: 2006-08-04 14:45 PDT by Max Kanat-Alexander
Modified: 2007-03-19 23:08 PDT (History)
3 users (show)
mkanat: approval+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1 (15.40 KB, patch)
2006-08-04 18:45 PDT, Max Kanat-Alexander
no flags Details | Diff | Splinter Review
v1.1 (15.30 KB, patch)
2006-08-04 18:53 PDT, Max Kanat-Alexander
no flags Details | Diff | Splinter Review
v2 (16.79 KB, patch)
2007-03-09 00:09 PST, Max Kanat-Alexander
mkanat: review+
Details | Diff | Splinter Review

Description Max Kanat-Alexander 2006-08-04 14:45:42 PDT
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.
Comment 1 Max Kanat-Alexander 2006-08-04 18:45:41 PDT
Created attachment 232248 [details] [diff] [review]
v1

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.
Comment 2 Max Kanat-Alexander 2006-08-04 18:53:25 PDT
Created attachment 232250 [details] [diff] [review]
v1.1

I fixed the POD, in this one.
Comment 3 Max Kanat-Alexander 2006-08-04 18:54:55 PDT
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.
Comment 4 Max Kanat-Alexander 2006-08-04 18:59:00 PDT
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.)
Comment 5 Max Kanat-Alexander 2006-08-04 20:15:57 PDT
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.
Comment 6 Rémi Zara 2006-08-05 13:56:03 PDT
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.
Comment 7 Max Kanat-Alexander 2006-08-06 00:10:46 PDT
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.
Comment 8 Rémi Zara 2006-08-06 01:44:58 PDT
(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 !
Comment 9 Dave Miller [:justdave] (justdave@bugzilla.org) 2006-12-18 22:45:54 PST
going back to requested on approval since this is targeted at 3.2 (and sounds quite intrusive)
Comment 10 Max Kanat-Alexander 2007-03-09 00:09:25 PST
Created attachment 257936 [details] [diff] [review]
v2

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).
Comment 11 Rémi Zara 2007-03-09 01:16:40 PST
(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 12 Max Kanat-Alexander 2007-03-09 14:12:01 PST
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. :-)
Comment 13 Max Kanat-Alexander 2007-03-09 14:13:30 PST
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
Comment 14 Xiaoou 2007-03-19 02:22:10 PDT
(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}";  
}




Comment 15 Max Kanat-Alexander 2007-03-19 12:08:30 PDT
(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!
Comment 16 Xiaoou 2007-03-19 23:03:29 PDT
(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?


  
Comment 17 Max Kanat-Alexander 2007-03-19 23:08:54 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.