637648 - Rename the "tags" table to "tag"

Status: RESOLVED FIXED

(Bugzilla :: Database, enhancement)

Description

[Max Kanat-Alexander]

For consistency with how our other multi-select-style fields work, we should rename the "tags" table to "tag".

Comment 1

[Frédéric Buclin]

Does this need to be done before 4.2rc1? If yes, it should block (it's a DB schema change), else it must be retargetted to 5.0.

Comment 2

[Max Kanat-Alexander]

Yeah, I would rather not release this with the current table name.

Comment 3

[Stephanie Daugherty [:sdaugherty]]

Status update: This is almost done, having issues with the table rename in checksetup.pl

Comment 4

[Max Kanat-Alexander]

Just this should work:

$dbh->bz_rename_table('tags', 'tag');

Comment 5

[Stephanie Daugherty [:sdaugherty]]

Attachment: almost working

Work in progress so far, almost works but the bz_rename_table is called too late by checksetup if called in the recommended place.

Comment 6

[Max Kanat-Alexander]

Comment on attachment 539141 [details] [diff] [review]
almost working

You'll also need to rename the index.

Ah, put bz_rename_table before _migrate_user_tags.

Comment 7

[Stephanie Daugherty [:sdaugherty]]

Attachment: patch for bug 637648

Ok. Working patch now! :)

Comment 8

[Frédéric Buclin]

Comment on attachment 539443 [details] [diff] [review]
patch for bug 637648

Please leave the _populate_bug_see_also_class() subroutine where it is currently. There is no need to move it, and you are going to make code history more confusing (because the routine will look like to be created by you, in Bonsai). Please revert that and resubmit another patch.

Comment 9

[Stephanie Daugherty [:sdaugherty]]

It's showing up as moved because I moved _migrate_user_tags(); down below _migrate_from_tags_to_tag(); to prevent the possibility of problems caused by both DB migrations hitting at the same time.

Comment 10

[Frédéric Buclin]

I'm not talking about the calls to the subroutines, but the subroutines themselves.

Comment 11

[Max Kanat-Alexander]

Comment on attachment 539443 [details] [diff] [review]
patch for bug 637648

This looks a bit more complicated than I expected, in Install::DB. Why couldn't you just move the bz_rename_table and keep everything else in place?

Comment 12

[Stephanie Daugherty [:sdaugherty]]

Attachment: cleaned up patch for bug 637648

same patch, but the functions themselves aren't moved around this time, only the calls to them.

Comment 13

[Max Kanat-Alexander]

Comment on attachment 539609 [details] [diff] [review]
cleaned up patch for bug 637648

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

::: Bugzilla/Install/DB.pm
@@ +647,5 @@
>          {TYPE => 'MEDIUMSERIAL', NOTNULL => 1, PRIMARYKEY => 1});
>  
>      # 2011-01-29 [e-mail] - Bug 616185
> +    
> +    _populate_bug_see_also_class();

Why did this move? I'd suggest just inserting your _migrate_from_tags_to_tag above the existing _migrate_user_tags.

@@ +3583,4 @@
>      $dbh->bz_commit_transaction();
>  }
>  
> +sub _migrate_from_tags_to_tag {

I'd call it "_rename_tags_to_tag".

@@ +3593,5 @@
> +    # copy the tags table into tag before dropping tags 
> +    if ($dbh->bz_table_info('tags') and $dbh->bz_table_info('tag')) {
> +        print "Migrating data from 'tags' to 'tag'...\n";
> +        $dbh->do("INSERT INTO tag SELECT * from tags");
> +        $dbh->bz_drop_table("tags");

Ah, this isn't necessary, just check if there is a tags table, and if so, drop the "tag" table and rename it to "tags". You'll also have to rename the index.

Also, just as a note: the SELECT * isn't safe because the schema of "tag" could change in the future, but the schema of "tags" will never change.

@@ +3598,5 @@
> +    }
> +    elsif ($dbh->bz_table_info('tags')) {
> +        # This probably won't be able to be hit, but in case checksetup.pl
> +        # gets changed to do table updates before creating missing tables,
> +        # we'll still do the right thing.'

Oh, you don't need to worry about how checksetup might be changed, so we don't need this block. (In general, we don't include code that isn't used.)

Comment 14

[Stephanie Daugherty [:sdaugherty]]

Attachment: patch again

attempt to address review concerns, this one uses bz_drop_table before bz_rename_table

Comment 15

[Max Kanat-Alexander]

Comment on attachment 539615 [details] [diff] [review]
patch again

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

::: Bugzilla/Install/DB.pm
@@ +647,5 @@
>          {TYPE => 'MEDIUMSERIAL', NOTNULL => 1, PRIMARYKEY => 1});
>  
>      # 2011-01-29 [e-mail] - Bug 616185
> +    
> +    _populate_bug_see_also_class();

Why does the diff think you are moving this? Really, don't move this.

@@ +3590,5 @@
> +    # unfortunately this isn't a perfect world, and checksetup creates missing
> +    # tables from the schema before update_table_definitions() ever gets called.
> +    # So, we check to see if both the old and the new of these exist, and then
> +    # copy the tags table into tag before dropping tags 
> +    if ($dbh->bz_table_info('tags') and $dbh->bz_table_info('tag')) {

You don't need to check if the "tag" table exists, we know it does.

@@ +3596,5 @@
> +        # If we get here, it's because the schema created tag as an empty
> +        # table while tags still exists. We'll just get rid of the empty
> +        # tag table so we can do the rename over the top of it.
> +        $dbh->bz_drop_table("tag");
> +        $dbh->bz_rename_table('tags','tag');

You still have to rename the index. That means bz_drop_index and then bz_add_index.

@@ +3598,5 @@
> +        # tag table so we can do the rename over the top of it.
> +        $dbh->bz_drop_table("tag");
> +        $dbh->bz_rename_table('tags','tag');
> +    }
> +    elsif ($dbh->bz_table_info('tags')) {

Don't add code that isn't used (remove this block).

Comment 16

[Frédéric Buclin]

Stephanie, ping?

Comment 17

[Max Kanat-Alexander]

Attachment: v3

Okay, since this is a blocker, I'm going to take it and fix it myself. Also, there was a problem with Sqlite that probably only I would have been able to fix right now, so I had to go and fix that anyhow, as part of this patch. (Otherwise the index disappears from the table entirely when it's being renamed, it's very weird.)

Comment 18

[Frédéric Buclin]

Comment on attachment 553638 [details] [diff] [review]
v3

>=== modified file 'Bugzilla/Install/DB.pm'

>+sub _rename_tags_to_tag {
>+    my $dbh = Bugzilla->dbh;
>+    # 2011-06-13 [e-mail] - Bug 637648 - Rename the "tags" table
>+    # to "tag".

This comment should go right above the caller, not here, for consistency. Also, we usually never put the bug description in the comment, only its ID.


buglist.cgi crashes, because you forgot to fix Bugzilla/Search.pm around line 2529:
  $args->{full_field} = 'tags.name';

Also, global/field-descs.none.tmpl must also be fixed:
  "tags.name"               => "Tags",


Otherwise looks good.

Comment 19

[Max Kanat-Alexander]

Attachment: v4

Cool, thanks for the review. Fixed all the things you pointed out.

Comment 20

[Frédéric Buclin]

Comment on attachment 555869 [details] [diff] [review]
v4

Looks good now. r=LpSolit

Comment 21

[Max Kanat-Alexander]

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/Bug.pm
modified Bugzilla/DB.pm
modified Bugzilla/Search.pm
modified Bugzilla/User.pm
modified Bugzilla/DB/Schema.pm
modified Bugzilla/DB/Sqlite.pm
modified Bugzilla/Install/DB.pm
modified template/en/default/global/field-descs.none.tmpl
Committed revision 7940.

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/4.2/
modified Bugzilla/Bug.pm
modified Bugzilla/DB.pm
modified Bugzilla/Search.pm                                                    
modified Bugzilla/User.pm
modified Bugzilla/DB/Schema.pm
modified Bugzilla/DB/Sqlite.pm                                                 
modified Bugzilla/Install/DB.pm
modified template/en/default/global/field-descs.none.tmpl
Committed revision 7913.

Comment 22

[Frédéric Buclin]

Tinderbox now 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`)

Comment 23

[Max Kanat-Alexander]

(In reply to Frédéric Buclin from comment #22)
> Tinderbox now fails with:
> [snip]

  Okay. Sounds like FKs don't get properly renamed on some DB when tables get renamed, so we should fix that.