284845 - Make bz_get_index_def cross-DB compatible

Status: RESOLVED WONTFIX

(Bugzilla :: Installation & Upgrading, enhancement)

Description

[Max Kanat-Alexander]

The only information it needs to return, really, is the name of the index, the
fields that the index is on, and if the index is unique or fulltext.

Comment 1

[Max Kanat-Alexander]

Attachment: v1 (Contains bits of other patches)

As usual, this contains bits of other patches, because I'm so far into work
that depends on other patches.

I'm just posting it up here to get any comments that anybody might have.

Comment 2

[Tomas Kopal]

Comment on attachment 176347 [details] [diff] [review]
v1 (Contains bits of other patches)

>Index: Bugzilla/DB.pm
>===================================================================
>@@ -359,18 +421,6 @@
>     return ($sth->rows);
> }
> 
>-# XXX - Needs to be made cross-db compatible.
>-sub bz_get_index_def ($$) {
>-    my ($self, $table, $field) = @_;
>-    my $sth = $self->prepare("SHOW INDEX FROM $table");
>-    $sth->execute;
>-
>-    while (my $ref = $sth->fetchrow_arrayref) {
>-        next if $$ref[2] ne $field;
>-        return $ref;
>-   }
>-}
>-

If you remove it from DB and put it to db specific, add it to the list of
abstract method.

>@@ -503,8 +589,9 @@
> 
>   # Schema Information
>   my @fields    = $dbh->bz_get_field_defs();
>-  my @fieldinfo = $dbh->bz_get_field_def($table, $column);
>-  my @indexinfo = $dbh->bz_get_index_def($table, $index);
>+  my $fieldinfo = $dbh->bz_get_field_def($table, $column);
>+  my $indexinfo = $dbh->bz_get_index_by_field($table, $field);
>+  my $indexinfo = $dbh->bz_get_index_by_name($table, $index);
>   my $exists    = $dbh->bz_table_exists($table);

I am sure you don't want to assign to $indexinfo twice here.

>@@ -769,24 +891,36 @@
>  Params:      $table = the table that you want to count indexes on
>  Returns:     The number of indexes on the table.
> 
>-=item C<bz_get_index_def>
>+=item C<bz_get_index_by_field($table, $field)>

We were not using parameter names elsewhere in the PODs, do we want to start to
do it now?

>+ Implementation Note: This function is ABSTRACT in Bugzilla::DB.

I was usually adding this to the description. I am not saying it's better, but
I would prefer to be consistent.

>Index: Bugzilla/DB/Mysql.pm
>===================================================================
>@@ -159,4 +163,103 @@
>+#####################################################################
>+# Database Setup
>+#####################################################################
>+
>+sub bz_setup_database {
>+    my ($self) = @_;
>+
>+    # Figure out if any existing tables are of type ISAM and convert them
>+    # to type MyISAM if so.  ISAM tables are deprecated in MySQL 3.23,
>+    # which Bugzilla now requires, and they don't support more than 16
>+    # indexes per table, which Bugzilla needs.
>+    my $sth = $self->prepare("SHOW TABLE STATUS");
>+    $sth->execute;
>+    my @isam_tables = ();
>+    while (my ($name, $type) = $sth->fetchrow_array) {
>+        push(@isam_tables, $name) if $type eq "ISAM";
>+    }
>+
>+    if(scalar(@isam_tables)) {
>+        print "One or more of the tables in your existing MySQL database are\n"
>+              . "of type ISAM. ISAM tables are deprecated in MySQL 3.23 and\n"
>+              . "don't support more than 16 indexes per table, which \n"
>+              . "Bugzilla needs.\n  Converting your ISAM tables to type"
>+              . " MyISAM:\n\n";
>+        foreach my $table (@isam_tables) {
>+            print "Converting table $table... ";
>+            $self->do("ALTER TABLE $table TYPE = MYISAM");
>+            print "done.\n";
>+        }
>+        print "\nISAM->MyISAM table conversion done.\n\n";
>+    }
>+
>+    $self->SUPER::bz_setup_database();
>+}

Different patch.

>+
>+#####################################################################
>+# Schema Information Functions
>+#####################################################################
>+
>+sub bz_get_index_by_field {
>+    my ($self, $table, $field) = @_;
>+    my $sth = $self->prepare("SHOW INDEX FROM $table");
>+    $sth->execute;
>+
>+    my @index_data;
>+
>+    while (my $row = $sth->fetchrow_arrayref) {
>+        push(@index_data, $row) if $row->[4] eq $field;

You may want to replace [4] by a constant here.

>+   }
>+
>+   return _build_index_hash(\@index_data);
>+}
>+
>+sub bz_get_index_by_name {
>+    my ($self, $table, $index) = @_;
>+    my $sth = $self->prepare("SHOW INDEX FROM $table");
>+    $sth->execute;
>+
>+    my @index_data;
>+
>+    while (my $row = $sth->fetchrow_arrayref) {
>+        push(@index_data, $row) if $row->[2] eq $index;

Dtto.

>+    }
>+
>+    return _build_index_hash(\@index_data);
>+}
>+
>+# SHOW INDEX returns the following data:
>+# 0 = name of the table that the index is on
>+# 1 = 0 if unique, 1 if not unique
>+# 2 = name of the index
>+# 3 = seq_in_index (either 1 or 0)
>+# 4 = Name of ONE column that the index is on
>+# 5 = 'Collation' of the index. Usually 'A'.
>+# 6 = Cardinality. Either a number or undef.
>+# 7 = sub_part. Usually undef. Sometimes 1.
>+# 8 = "packed". Usually undef.
>+# 9 = comments. Usually an empty string. Sometimes 'FULLTEXT'.

Shouldn't this comment be attached to the previous functions, which are
actually calling SHOW INDEX?

>+sub _build_index_hash ($) {
>+    my ($array) = @_;
>+    return undef if !scalar(@$array);
>+
>+    my @col_list;
>+    foreach my $row (@$array) {
>+        push(@col_list, $row->[4]);
>+    }
>+
>+    my $hashref = {
>+        NAME => $array->[0]->[2],
>+        COLUMNS => \@col_list,
>+        UNIQUE => ($array->[0]->[1] eq '1' ? 1 : 0),
>+        FULLTEXT => ($array->[0]->[9] =~ /FULLTEXT/ ? 1 : 0),
>+    };

Again, indexes should be replaced by constants.


Otherwise, it looks good, but similarly to the other patch, I was able to just
scratch the surface, I need to read it few times more to fully understand :-).

Comment 3

[Max Kanat-Alexander]

Comment on attachment 176347 [details] [diff] [review]
v1 (Contains bits of other patches)

Don't worry too much about this anymore, now that we're going in a different
direction with it.

Comment 4

[Max Kanat-Alexander]

WONTFIXed in favor of the new methods being created in bug 285111.