Make bz_get_index_def cross-DB compatible

RESOLVED WONTFIX

Status

()

enhancement
RESOLVED WONTFIX
15 years ago
14 years ago

People

(Reporter: mkanat, Assigned: mkanat)

Tracking

Dependency tree / graph
Bug Flags:
blocking2.20 +

Details

Attachments

(1 attachment)

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.
Assignee: email-notifications → mkanat
Component: Email Notifications → Installation & Upgrading
Target Milestone: --- → Bugzilla 2.20
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.
Attachment #176347 - Flags: review?(Tomas.Kopal)
Blocks: 284850
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 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.
Attachment #176347 - Flags: review?(Tomas.Kopal)
Flags: blocking2.20+
No longer blocks: 284850
WONTFIXed in favor of the new methods being created in bug 285111.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → WONTFIX
Target Milestone: Bugzilla 2.20 → ---
You need to log in before you can comment on or make changes to this bug.