715870 - [Oracle] Related sequences and triggers must be removed when dropping a table

Status: RESOLVED FIXED

(Bugzilla :: Database, defect)

Description

[Frédéric Buclin]

Attachment: patch, v1

When a table is created, several sequences and triggers are created. But bz_drop_table() doesn't remove them, meaning that you cannot recreate the table because it will complain that the sequence already exists. This is the reason why a user got this exact problem in the support mailing-list when upgrading from 4.1.3 to 4.2rc1. We remove the newly created "tag" table, and then rename "tags" to "tag". But bz_drop_table("tag") didn't remove all its related stuff correctly, and so bz_rename_table("tags", "tag") cannot run correctly because stuff is already there.

This patch also fixes a typo in get_rename_table_sql() introduced by bug 683644.

Comment 1

[Frédéric Buclin]

In the URL field, the original description of the problem.

Comment 2

[Max Kanat-Alexander]

Comment on attachment 586399 [details] [diff] [review]
patch, v1

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

If this bug has corrupted existing databases, there will also have to be some code in DB::Oracle::bz_setup_database to fix existing installs.

Comment 3

[Frédéric Buclin]

(In reply to Max Kanat-Alexander from comment #2)
> If this bug has corrupted existing databases, there will also have to be
> some code in DB::Oracle::bz_setup_database to fix existing installs.

I would have to investigate even more, but I would like to keep this bug specific to fix bz_drop_table() correctly, and keep for a separate bug the problem you mention, if it's still present. AFAIK, this only affects upgrades from 4.1.x to 4.2rc1, not from a stable release to 4.2rc1. Do you agree with this proposal?

Comment 4

[Frédéric Buclin]

Comment on attachment 586399 [details] [diff] [review]
patch, v1

wicked doesn't seem to be around these days.

Comment 5

[Max Kanat-Alexander]

Comment on attachment 586399 [details] [diff] [review]
patch, v1

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

Looks great! I'm assuming you tested and those trigger names are right. (I know that Oracle does this hashing thing for identifiers--I'm not sure if it applies here?)

::: Bugzilla/DB/Schema/Oracle.pm
@@ +410,5 @@
> +        if ($def->{TYPE} =~ /varchar|text/i && $def->{NOTNULL}) {
> +            push(@sql, "DROP TRIGGER ${name}_${column}");
> +        }
> +    }
> +    push(@sql, "DROP TABLE $name");

Maybe get this by calling $self->SUPER::get_drop_table_ddl?

Comment 6

[Frédéric Buclin]

(In reply to Max Kanat-Alexander from comment #5)
> Looks great! I'm assuming you tested and those trigger names are right.

Yes, those names are right. I checked them one by one.


> Maybe get this by calling $self->SUPER::get_drop_table_ddl?

Sure, I will do this change.


Thanks for the review!

Comment 7

[Frédéric Buclin]

Attachment: patch, v2

I realized that my previous patch didn't correctly remove all indexes attached to tables, and so the deletion could sometimes fail. To remove all indexes correctly, I have to write "DROP TABLE $name CASCADE CONSTRAINTS PURGE" instead of "DROP TABLE $name" only. CASCADE CONSTRAINTS also removes all triggers itself, so we do not need to do it ourselves anymore. But it doesn't remove sequences at all, which is why we still have to do it ourselves. This makes the patch shorter and cleaner.

I tested, and everything works very cleanly now.

Comment 8

[Max Kanat-Alexander]

Comment on attachment 589060 [details] [diff] [review]
patch, v2

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

Beautiful! Very nice. :-)

Comment 9

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla/DB/Schema/Oracle.pm
Committed revision 8091.

Committing to: bzr+ssh://[e-mail]/bugzilla/4.2/
modified Bugzilla/DB/Schema/Oracle.pm
Committed revision 8012.