Implement support for referential integrity in Bugzilla::DB::Schema and implement it on profiles_activity.

RESOLVED FIXED in Bugzilla 3.2

Status

()

Bugzilla
Database
P1
enhancement
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Max Kanat-Alexander, Assigned: Max Kanat-Alexander)

Tracking

(Blocks: 1 bug)

2.23
Bugzilla 3.2
Dependency tree / graph
Bug Flags:
approval +

Details

(Whiteboard: [roadmap: 3.2])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

11 years ago
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.
(Assignee)

Comment 1

11 years ago
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.
(Assignee)

Comment 2

11 years ago
Created attachment 232250 [details] [diff] [review]
v1.1

I fixed the POD, in this one.
Assignee: database → mkanat
Attachment #232248 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Assignee)

Comment 3

11 years ago
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.
Flags: approval?
(Assignee)

Comment 4

11 years ago
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.)
Flags: approval? → approval+
(Assignee)

Comment 5

11 years ago
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.
Depends on: 347475

Comment 6

11 years ago
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.
(Assignee)

Comment 7

11 years ago
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

11 years ago
(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 !
(Assignee)

Updated

11 years ago
Whiteboard: [roadmap: 3.2]
(Assignee)

Updated

11 years ago
Target Milestone: Bugzilla 3.0 → Bugzilla 3.2
going back to requested on approval since this is targeted at 3.2 (and sounds quite intrusive)
Flags: approval+ → approval?
(Assignee)

Comment 10

11 years ago
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).
Attachment #232250 - Attachment is obsolete: true

Comment 11

11 years ago
(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
(Assignee)

Comment 12

11 years ago
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. :-)
Attachment #257936 - Flags: review+
(Assignee)

Comment 13

11 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Flags: approval? → approval+
Priority: -- → P1
Resolution: --- → FIXED

Comment 14

11 years ago
(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}";  
}




(Assignee)

Comment 15

11 years ago
(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

11 years ago
(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?


  
(Assignee)

Comment 17

11 years ago
(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.
You need to log in before you can comment on or make changes to this bug.