683644 - Foreign keys aren't renamed correctly when DB tables are renamed

Status: RESOLVED FIXED

(Bugzilla :: Database, defect)

Description

[Frédéric Buclin]

See bug 637648 comment 22:

Tinderbox fails with:

-  CONSTRAINT `fk_bug_tag_tag_id_tags_id` FOREIGN KEY (`tag_id`) REFERENCES `tag` (`id`) ON DELETE CASCADE ON UPDATE CASCADE
+  CONSTRAINT `fk_bug_tag_tag_id_tag_id` FOREIGN KEY (`tag_id`) REFERENCES `tag` (`id`) ON DELETE CASCADE ON UPDATE CASCADE

-  KEY `fk_bug_tag_tag_id_tags_id` (`tag_id`)
+  KEY `fk_bug_tag_tag_id_tag_id` (`tag_id`)


We should fix this problem before 4.2rc1.

Comment 1

[Max Kanat-Alexander]

Attachment: v1

Turns out it also affected sequences, on PostgreSQL.

This fixes it, as tested on MySQL and PostgreSQL in all sorts of configurations. (That is, it also fixes the current tip DBs in addition to validly upgrading from DBs that had "tags" in them.)

Comment 2

[Frédéric Buclin]

Comment on attachment 557665 [details] [diff] [review]
v1

As discussed on IRC, this doesn't fix FKs for installations which already upgraded to current code. This would only fix the problem for installations which didn't upgrade yet.

Comment 3

[Frédéric Buclin]

I wonder if this bug is responsible for a crash in Oracle when running checksetup.pl twice. On the first run, it completes correctly, but on the second run, Oracle crashes:

DBD::Oracle::db do failed: ORA-00942: table or view does not exist (DBD ERROR: error possibly near <*> indicator at char 75 in 'CREATE OR REPLACE TRIGGER FK_949096678BFFBDE6CB5F_UC AFTER UPDATE OF id ON <*>tags  REFERENCING  NEW AS NEW  OLD AS OLD  FOR EACH ROW  BEGIN      UPDATE bug_tag        SET tag_id = :new.id      WHERE tag_id = :old.id; END FK_949096678BFFBDE6CB5F_UC;') [for Statement "CREATE OR REPLACE TRIGGER FK_949096678BFFBDE6CB5F_UC AFTER UPDATE OF id ON tags  REFERENCING  NEW AS NEW  OLD AS OLD  FOR EACH ROW  BEGIN      UPDATE bug_tag        SET tag_id = :NEW.id      WHERE tag_id = :OLD.id; END FK_949096678BFFBDE6CB5F_UC;"] at Bugzilla/DB/Oracle.pm line 392                                           
        Bugzilla::DB::Oracle::do(undef, 'CREATE OR REPLACE TRIGGER FK_949096678BFFBDE6CB5F_UC AFTER UP...') called at Bugzilla/DB/Oracle.pm line 664                                                          
        Bugzilla::DB::Oracle::bz_setup_database('Bugzilla::DB::Oracle=HASH(0xa607518)') called at ./checksetup.pl line 148

Comment 4

[Frédéric Buclin]

About my previous comment: in case this isn't clear, Oracle tries to update the tags table, which has been renamed to tag.

Comment 5

[Teemu Mannermaa (:wicked)]

Attachment: V2

This seems to fix the related FKs as well.

Might even fix the Oracle problem if that tries to create the "missing" FK in bug_tag that still points to the old tags table. I'm not sure why MySQL/Pg doesn't do the same..

Comment 6

[Frédéric Buclin]

(In reply to Teemu Mannermaa (:wicked) from comment #5)
> Might even fix the Oracle problem

It doesn't fix it, unfortunately. :( The error message is the same as in comment 3.

Comment 7

[Frédéric Buclin]

(In reply to Frédéric Buclin from comment #6)
> It doesn't fix it, unfortunately. :( The error message is the same as in
> comment 3.

... but I managed to fix this problem by adding the following code at line 640 of bz_setup_database() in Bugzilla/DB/Oracle.pm:

if ($table eq 'bug_tag' && $to_table eq 'tags') {
    $to_table = 'tag';
}

This is a ugly hack, but this fixes the problem. I hope either you or mkanat will have a better idea on how to fix it in a cleaner way. :)

About the patch v2, I suspect the code added into Bugzilla/Install/DB.pm is triggered too late in the upgrade process. Maybe it should be moved into bz_setup_database()?

Comment 8

[Frédéric Buclin]

Comment on attachment 577152 [details] [diff] [review]
V2

I'm really not the right reviewer for this code. But please include my hack to fix currently broken Oracle installations.

Comment 9

[Max Kanat-Alexander]

Comment on attachment 577152 [details] [diff] [review]
V2

Review of attachment 577152 [details] [diff] [review]:
-----------------------------------------------------------------

Okay, I haven't tested this, but I have some input based on how I think everything works, here:

::: Bugzilla/DB.pm
@@ +1058,5 @@
> +
> +    # FKs will all have the wrong names unless we drop and then let them
> +    # be re-created later. Under normal circumstances, checksetup.pl will
> +    # automatically re-create these dropped FKs at the end of its DB upgrade
> +    # run, so we don't need to re-create them in this method.

I already resolved this problem elsewhere, didn't I? At the very least, we don't want to do this this way, because adding an FK can be very slow, and renaming a table should be really fast.

You may need to add a bz_rename_fk method for each DB, or at least a get_bz_rename_sql method to their Schema objects.

::: Bugzilla/DB/Pg.pm
@@ +330,5 @@
>              }
>          }
>      }
> +
> +    # bz_rename_table didn't properly rename sequences.

It also didn't properly rename the FK itself, which you will have to fix. This is the other part of what's causing the Pg tinderbox to fail right now.

::: Bugzilla/DB/Schema/Pg.pm
@@ +123,5 @@
> +        my $def = $self->get_column_abstract($old_name, $column);
> +        if ($def->{TYPE} =~ /SERIAL/i) {
> +            push(@sql, "ALTER SEQUENCE ${old_name}_${column}_seq
> +                             RENAME TO ${new_name}_${column}_seq");
> +        }

Could you abstract some of this out into a method that can be shared with bz_rename_column?

::: Bugzilla/Install/DB.pm
@@ +3603,5 @@
>          $dbh->bz_add_index('tag', 'tag_user_id_idx',
>                             {FIELDS => [qw(user_id name)], TYPE => 'UNIQUE'});
>      }
> +
> +    # bz_rename_table didn't rename related FKs, see bug 683644

Yeah, this will have to be done in bz_setup_database. Also, I would have to imagine that this won't work, in general--Bugzilla can't drop the existing FK, because it has the wrong name. Most likely, you will need custom code in bz_setup_database for every DB, or you will have to create a bz_rename_fk and use it somewhere near the *top* of update_table_definitions.

Comment 10

[Max Kanat-Alexander]

Oh, nevermind my first comment about things not being needed. I didn't realize that v1 never got checked in--I thought that was already in the codebase! :-)

Comment 11

[Teemu Mannermaa (:wicked)]

Heh, mkanat is mostly commenting on his own code. :)

(In reply to Max Kanat-Alexander from comment #9)
> ::: Bugzilla/DB/Pg.pm
> @@ +330,5 @@
> > +    # bz_rename_table didn't properly rename sequences.
> It also didn't properly rename the FK itself, which you will have to fix.

Oh, indeed there's that additional Pg bustage.

> ::: Bugzilla/Install/DB.pm
> @@ +3603,5 @@
> > +    # bz_rename_table didn't rename related FKs, see bug 683644
> Yeah, this will have to be done in bz_setup_database. Also, I would have to
> imagine that this won't work, in general--Bugzilla can't drop the existing

It did seem to work for my own install. Although, TBH, I first used bz_drop_fk() that correctly dropped the FK but it never read the new table from the schema so I had to switch to bz_alter_fk() to manually change schema*. Internally, this used bz_drop_fk() followed by bz_add_fk() so it should do pretty much the same. The dropping is done using the current information, only adding uses the changed FK information.

*Isn't this a bug of itself? There's a comment that indicates the FK information is stored somewhere (why? it's in the schema anyway) so that we can add it back and the code only handles dropping of FKs permanently (so not renaming/changed FK info). :( (And I can't even see where it does the dropping part.)

> bz_setup_database for every DB, or you will have to create a bz_rename_fk
> and use it somewhere near the *top* of update_table_definitions.

Sounds a lot of rearchitecting for a blocker. But if we must, we must. :)

Except, I can't find any syntax to directly rename a FK on MySQL. Is there such a thing? Quickly looking for this suggest I need to drop and add the FK, which is exactly what bz_alter_fk() does.

I wonder why Oracle is the only one adding FKs during bz_setup_database() while MySQL/Pg does it at the end of the update_table_definitions() sub by calling bz_setup_foreign_keys() sub.

Comment 12

[Max Kanat-Alexander]

Yeah, I think you're right that bz_alter_fk's *SQL* actually should work properly for this situation. The problem is that we put the database into an inconsistent state where bz_schema won't believe there's an old FK with the right name, for the current bugzilla_tip database on landfill.

Comment 13

[Max Kanat-Alexander]

To be clear, there's two situations we need to handle: (1) People upgrading from an earlier release of Bugzilla (2) People upgrading from the current trunk/4.2 tip.

Comment 14

[Frédéric Buclin]

(In reply to Max Kanat-Alexander from comment #9)
> don't want to do this this way, because adding an FK can be very slow, and
> renaming a table should be really fast.
> 
> You may need to add a bz_rename_fk method for each DB, or at least a
> get_bz_rename_sql method to their Schema objects.

AFAIK, there is no way to rename an existing FK. You must delete and re-create it. If it's slow, that's our fault, because if we did things right the first time, we wouldn't have to run this code. Also, this code will be triggered only once, not at each run of checksetup.pl. So instead of trying some dangerous SQL to rename the FK which will probably not work correctly, let's simply drop and re-create the FK.


> Could you abstract some of this out into a method that can be shared with
> bz_rename_column?

Could we do that in a separate bug? It's high time to fix this nasty bug and forget about everything else.


> or you will have to create a bz_rename_fk
> and use it somewhere near the *top* of update_table_definitions.

As said above, I don't think there is any SQL code available to rename a FK.

Comment 15

[Frédéric Buclin]

Attachment: patch, v3

This fixes the problem with PostgreSQL and Oracle. AFAIK, there is nothing to do for MySQL and SQLite.

Comment 16

[Teemu Mannermaa (:wicked)]

(In reply to Frédéric Buclin from comment #15)
> AFAIK, there is nothing to do for MySQL and SQLite.

You still need to rename the old FK on MySQL (that's the code I had in Bugzilla/Install/DB.pm) somewhere or the Tinderboxen will continue to burn (and this affects anyone that updated same way).

After taking a quick look these changes look good to me but I'll do some testing on MySQL and Pg with this patch. I can't test on Oracle since we have no test install we can use.

Comment 17

[Frédéric Buclin]

Attachment: patch, v4

MySQL has been patched too. And the FK is now correctly named.

Comment 18

[Frédéric Buclin]

Attachment: patch, v4

Oops, I didn't include all files in the patch.

Comment 19

[Teemu Mannermaa (:wicked)]

Comment on attachment 581980 [details] [diff] [review]
patch, v4

This won't fully fix the TB Orange on PG because you'd need to handle the primary key in the renamed tag table that still has the name tags_pkey. That's seen as a mismatch on the checksetup test output, which is the only reason it's "wrong".

Additinally on Pg, upgrade from 4.2 branch w/non-converted state (w/tags table) fails:
--!--
Adding new table tag...
Fixing tags_id_seq sequence...
DBD::Pg::db do failed: ERROR:  relation "tag_id_seq" already exists
 at Bugzilla/DB/Pg.pm line 323
        Bugzilla::DB::Pg::_fix_bad_sequence('Bugzilla::DB::Pg=HASH(0xc8596c0)', 'tag', 'id', 'tags_id_seq', 'tag_id_seq') called at Bugzilla/DB/Pg.pm line 287
        Bugzilla::DB::Pg::bz_setup_database('Bugzilla::DB::Pg=HASH(0xc8596c0)') called at ./checksetup.pl line 148
--!--

And on MySQL, when updating on a 4.2 branch w/non-converted state, this awful thing happens:
--!--
Adding new table tag...
Dropping foreign key: bug_tag.tag_id -> tags.id...

WARNING: There were invalid values in bug_tag.tag_id that have been
deleted: <all ids>
Adding foreign key: bug_tag.tag_id -> tag.id...
Fixing file permissions...
Checking for             GraphViz (any)       ok
Dropping the 'tag' table...
Failed SQL: [DROP TABLE tag] Error: DBD::mysql::db do failed: Cannot delete or update a parent row: a foreign key constraint fails [for Statement "DROP TABLE tag"] at Bugzilla/DB.pm line 1013
        eval {...} called at Bugzilla/DB.pm line 1013
        Bugzilla::DB::bz_drop_table('Bugzilla::DB::Mysql=HASH(0xc1626ec)', 'tag') called at Bugzilla/Install/DB.pm line 3604
        Bugzilla::Install::DB::_rename_tags_to_tag() called at Bugzilla/Install/DB.pm line 650
        Bugzilla::Install::DB::update_table_definitions('HASH(0x9f3b48c)') called at ./checksetup.pl line 199
Dropping foreign key: tags.user_id -> profiles.userid...
Removing index 'tags_user_id_idx' from the tags table...
Renaming the 'tags' table to 'tag'...
DBD::mysql::db do failed: Table 'tag' already exists [for Statement "ALTER TABLE tags RENAME TO tag"] at Bugzilla/DB.pm line 1077
        Bugzilla::DB::bz_rename_table('Bugzilla::DB::Mysql=HASH(0xc1626ec)', 'tags', 'tag') called at Bugzilla/Install/DB.pm line 3606
        Bugzilla::Install::DB::_rename_tags_to_tag() called at Bugzilla/Install/DB.pm line 650
        Bugzilla::Install::DB::update_table_definitions('HASH(0x9f3b48c)') called at ./checksetup.pl line 199
--!--
In addition to failing the upgrade, it also removes everything from bug_tag table. :( Reason seems to be that you try to do the bz_alter_fk call in bz_setup_database(), which is not correct place even though mkanat said so.

That code is only meant to fix the case where a previous rename had left a bad FK around, which doesn't happen anymore with the rename in this patch. I tested and this works correctly when the bz_alter_fk is done at the end of _rename_tags_to_tag() sub like in V2 patch.

For optional bonus points, please fix the confusing update_queries_to_tags install message in template/en/default/setup/strings.txt.pl to not talk about the old tags table.

For the record, I tested both MySQL and Pg. On both I tested an upgrade from 1) 4.0 branch tip schema, 2) 4.2 branch r7912 non-converted schema (just before tags to tag rename), 3) 4.2 branch r7913 converted schema (where the tags table was renamed to tag), and 4) 4.2 branch tip schema.

I'll see if I can do some tests on Oracle but 12h on a Sunday is enough Bugzilla work for now.. don't you think? :)

>=== modified file 'Bugzilla/DB/Pg.pm'
>--- Bugzilla/DB/Pg.pm	2011-12-05 17:04:47 +0000
>+++ Bugzilla/DB/Pg.pm	2011-12-12 23:32:27 +0000

>+sub _fix_bad_sequence {
...
>+        $self->do("ALTER SEQUENCE $old_seq RENAME TO $new_seq");

I guess it's fine to use "ALTER SEQUENCE" even though it's a non-standard Pg extension as it's only used in Pg modules and we also require v8.3 that introduced this extension. It sure is more descriptive than the generic "ALTER TABLE".. :)

Comment 20

[Frédéric Buclin]

Attachment: patch, v5

I tested upgrading Bugzilla 4.0.2 (no tag(s) table), Bugzilla 4.2-branch rev 7912 (tags table) and Bugzilla 4.2-branch rev 7913 (tag table) to 4.3 with both MySQL and PostgreSQL. All errors reported by wicked, which I could initially reproduce, have all been fixed, and the new DB schema is correct AFAICS.

I will test my patch with Oracle and SQLite tomorrow.

Comment 21

[Frédéric Buclin]

Attachment: patch, v5.1

I forgot to fix the case where rev 7912 is first upgraded to rev 7913 before being upgraded to 4.3.

Comment 22

[Teemu Mannermaa (:wicked)]

Comment on attachment 583029 [details] [diff] [review]
patch, v5.1

Awesome, with bonus points as well! I tested on both MySQL and Pg like previously explained.

I'll try to test Oracle later (unless LpSolit managed to do basic tests on it) but any problems for Oracle can be fixed in follow-up bugs. This patch already complicated enough.

>=== modified file 'Bugzilla/Install/DB.pm'
>@@ -3611,6 +3611,12 @@
>+    if (my $bug_tag_fk = $dbh->bz_fk_info('bug_tag', 'tag_id')) {

To clarify what happens here, maybe add a comment about bz_rename_column failing previously for this as well?

Comment 23

[Frédéric Buclin]

Thanks a lot for the testing and the review!! :)

Comment 24

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla/DB.pm
modified Bugzilla/DB/Oracle.pm
modified Bugzilla/DB/Pg.pm
modified Bugzilla/DB/Schema/Oracle.pm
modified Bugzilla/DB/Schema/Pg.pm
modified Bugzilla/Install/DB.pm
modified template/en/default/setup/strings.txt.pl
Committed revision 8048.

Committing to: bzr+ssh://[e-mail]/bugzilla/4.2/
modified Bugzilla/DB.pm
modified Bugzilla/DB/Oracle.pm
modified Bugzilla/DB/Pg.pm
modified Bugzilla/DB/Schema/Oracle.pm
modified Bugzilla/DB/Schema/Pg.pm
modified Bugzilla/Install/DB.pm
modified template/en/default/setup/strings.txt.pl
Committed revision 7986.