287986 - Bugzilla::DB::Mysql needs a way to read in a Schema object from the disk

Status: RESOLVED FIXED

(Bugzilla :: Installation & Upgrading, enhancement, P1)

Description

[Max Kanat-Alexander]

Basically, we need to be able to look at a MySQL database and create a Schema
object out of it.

This is almost impossible for PostgreSQL, but thankfully is possible for MySQL,
because the "abstract" data types have mostly a one-to-one relation with the
"database-specific" data types.

This is needed to make updates from older versions reliable.

Comment 1

[Max Kanat-Alexander]

Attachment: v1 (use -p1) (Requires bug deps)

OK, this is some of the craziest code I've ever written. :-) But it works quite
well. I did some cleverness with Data::Dumper and diff to make sure that the
schema read in by this function comes out (currently) to be the exact same as
the ABSTRACT_SCHEMA.

It is *thoroughly* commented, and fairly well tested. If you have any
questions, feel free to ask.

Comment 2

[Tomas Kopal]

Comment on attachment 178818 [details] [diff] [review]
v1 (use -p1) (Requires bug deps)

>--- mkanat4/Bugzilla/DB/Mysql.pm	2005-03-28 10:03:06.000000000 -0800
>+++ mkanat/Bugzilla/DB/Mysql.pm	2005-03-28 10:13:11.000000000 -0800
>@@ -355,7 +356,18 @@
>         } # foreach table
>     } # if old-name indexes
> 
>-    $self->SUPER::bz_setup_database();
>+    $self->SUPER::bz_setup_database(@_, "skip schema init");
>+
>+    my $schema_exists = 
>+        $self->selectrow_array("SELECT COUNT(*) FROM bz_schema");
>+    if (!$schema_exists) {
>+        print "Building Schema object from database...\n";
>+        $self->{private_real_schema} = $self->_bz_build_schema_from_disk();
>+        # XXX - The code is not ready for this yet, but once
>+        #       all the deps of bug 285111 are checked-in and
>+        #       tested, this should be uncommented.        
>+        #$self->_bz_store_real_schema;
>+    }
> }
> 
> 

I don't like this. The code here is quite similar to the base class
implementation you are overriding, which brings all problems of code
duplication.
Better approach would be to call _bz_build_schema_from_disk() to initialize the
abstract schema from disk and then call _bz_init_schema_storage() from the base
class to store it.
BTW, I am pretty sure your current patch will fail as _bz_store_real_schema()
calls UPDATE, not INSERT, on empty table...

>@@ -375,6 +387,35 @@
> 
> =over 4
> 
>+=item C<bz_column_info_real($table, $column)>
>+
>+ Description: Returns an abstract column definition for a column
>+              as it actually exists on disk in the database.
>+ Params:      $table - The name of the table the column is on.
>+              $column - The name of the column you want info about.
>+ Returns:     An abstract column definition.
>+              If the column does not exist, returns undef.
>+
>+=cut
>+
>+sub bz_column_info_real {
>+    my ($self, $table, $column) = @_;
>+
>+    # DBD::mysql does not support selecting a specific column,
>+    # so we have to get all the columns on the table and find 
>+    # the one we want.
>+    my $info_sth = $self->column_info(undef, undef, $table, '%');
>+    my $col_data;
>+    while ($col_data = $info_sth->fetchrow_hashref) {
>+        last if $col_data->{COLUMN_NAME} eq $column;
>+    }

Ummm, if you are using fetchrow_hashref(), you can directly access the hash
using the key (column name) without iterating over it, right? What am I
missing? If you need to iterate, then fetchrow_arrayref should have better
performance, according to DBI docs.

>@@ -429,6 +470,66 @@
>     return $retval;
> }
> 
>+=item C<bz_index_list_real($table)>
>+
>+ Description: Returns a list of index names on a table in 
>+              the database, as it actually exists on disk.
>+ Params:      $table - The name of the table you want info about.
>+ Returns:     An array of index names.
>+
>+=cut
>+
>+sub bz_index_list_real {
>+    my ($self, $table) = @_;
>+    my $sth = $self->prepare("SHOW INDEX FROM $table");
>+    return @{ $self->selectcol_arrayref($sth, {Columns => [3]}) };

One of the golden rules in programming is "no numbers in code" :-). Can't we
make this some named constant (even if only locally defined)? Or, at least, put
there a comment why 3 is the only right number :-).

>--- mkanat4/Bugzilla/DB/Schema/Mysql.pm	2005-03-28 10:02:45.000000000 -0800
>+++ mkanat/Bugzilla/DB/Schema/Mysql.pm	2005-03-28 09:27:24.000000000 -0800
>@@ -33,6 +33,52 @@
> 
> use base qw(Bugzilla::DB::Schema);
> 
>+# This is for column_info_to_column, to know when a tinyint is a 
>+# boolean and when it's really a tinyint. This only has to be accurate
>+# up to and through 2.19.3, because that's the only time we need
>+# column_info_to_column.

I would welcome to add something about the structure of this hash as well. Why
all members are 1, and there is no zero? I assume it's because only booleans
are included, but that's just assumption. Also, does 1 mean boolean or tinyint?
Any other values than 0,1 permited?

>+        # a 0 or an empty string. (Except for ddatetime and decimal

Typo: ddatetime

>+            $default = $dbh->quote($default) if !($default =~ /^(-)?(\d+)$/);

Nit: Shouldn't this regexp include a dot as well? I don't think we are using
floating point defaults anywhere at the moment, but just to play safe? We
probably don't have to care about time defaults now, too complex :-).

>+    # For all other db-specific types, check if they exist in 
>+    # REVERSE_MAPPING and use the type found there.
>+    if (exists REVERSE_MAPPING->{$type}) {
>+        $type = REVERSE_MAPPING->{$type};
>+    }

Can't we put this into 'else' statement and add another 'else' screaming for
help if the type was not handled by any of the preceeding cases? Sanity
checking usually pays of :-).

>--- mkanat4/Bugzilla/DB/Schema.pm	2005-03-28 10:03:06.000000000 -0800
>+++ mkanat/Bugzilla/DB/Schema.pm	2005-03-28 09:56:43.000000000 -0800
>@@ -885,7 +885,8 @@
> 
>     whine_queries => {
>         FIELDS => [
>-            id            => {TYPE => 'MEDIUMSERIAL', PRIMARYKEY => 1},
>+            id            => {TYPE => 'MEDIUMSERIAL', PRIMARYKEY => 1,
>+                              NOTNULL => 1},
>             eventid       => {TYPE => 'INT3', NOTNULL => 1},
>             query_name    => {TYPE => 'varchar(64)', NOTNULL => 1,
>                               DEFAULT => "''"},
>@@ -902,7 +903,8 @@
> 
>     whine_schedules => {
>         FIELDS => [
>-            id          => {TYPE => 'MEDIUMSERIAL', PRIMARYKEY => 1},
>+            id          => {TYPE => 'MEDIUMSERIAL', PRIMARYKEY => 1,
>+                            NOTNULL => 1},
>             eventid     => {TYPE => 'INT3', NOTNULL => 1},
>             run_day     => {TYPE => 'varchar(32)'},
>             run_time    => {TYPE => 'varchar(32)'},
>@@ -918,7 +920,8 @@
> 
>     whine_events => {
>         FIELDS => [
>-            id           => {TYPE => 'MEDIUMSERIAL', PRIMARYKEY => 1},
>+            id           => {TYPE => 'MEDIUMSERIAL', PRIMARYKEY => 1,
>+                             NOTNULL => 1},
>             owner_userid => {TYPE => 'INT3', NOTNULL => 1},
>             subject      => {TYPE => 'varchar(128)'},
>             body         => {TYPE => 'MEDIUMTEXT'},

Why is this part of this bug? It does not seem to have anything to do with the
subject?

>@@ -817,6 +819,8 @@
> =cut
> sub _bz_real_schema {
>     my ($self) = @_;
>+    # Note: Subclasses depend on the name of this variable, please
>+    #       don't change the name.

If you fix the first point I made, you don't need to add this comment :-).

Otherwise, it look pretty cool, well done.

Comment 3

[Max Kanat-Alexander]

(In reply to comment #2)


> and then call _bz_init_schema_storage() from the base class to store it.

  I can't do *exactly* that, as we're replacing _bz_real_schema, not _bz_schema.
But you're right, I ought to somehow modify something in the superclass to allow
me to do what I'm doing.

> BTW, I am pretty sure your current patch will fail as _bz_store_real_schema()
> calls UPDATE, not INSERT, on empty table...

  And you're right about that. :-)

> Ummm, if you are using fetchrow_hashref(), you can directly access the hash
> using the key (column name) without iterating over it, right?

  Yeah, if I used fetchall_hashref, then I do believe that you are correct.

> One of the golden rules in programming is "no numbers in code" :-). Can't we
> make this some named constant (even if only locally defined)? Or, at least, 
> put there a comment why 3 is the only right number :-).

  Oh, you're right, of course. :-) OK, will do.

> I would welcome to add something about the structure of this hash as well.

  OK, will be added. Basically, there's just an entry for any field that is a
BOOLEAN and not an INT1. The value is essentially discarded.

> >+            $default = $dbh->quote($default) if !($default =~ /^(-)?(\d+)$/);
> 
> Nit: Shouldn't this regexp include a dot as well? I don't think we are using
> floating point defaults anywhere at the moment, but just to play safe? We
> probably don't have to care about time defaults now, too complex :-).

  Hrm, maybe... (We actually don't care about decimal defaults now, either,
since nothing in the database has one that we need to keep, but it ought to work
anyway, you're right.)

  Looking at the code... yeah, you're right, I do believe.

> Can't we put this into 'else' statement and add another 'else' screaming for
> help if the type was not handled by any of the preceeding cases? Sanity
> checking usually pays of :-).

  If the type wasn't handled by any previous type, then it could be a raw
database type that we don't handle and needs to be passed into the Schema as
such. I'm thinking now about people who do local customizations, and I don't
want to choke on their fields. For example, if somebody has an enum in the
database, we don't want to *die*, we just want to make an enum in the Schema.

  We would then also die on the TIMESTAMP fields that this will have to
sometimes read in.

  Also, you'll notice that REVERSE_MAPPING is very small, because it contains
the only fields that need to be actually mapped back from what they're normally
called. Even most valid fields are missing from REVERSE_MAPPING.

> Why is this part of this bug? It does not seem to have anything to do with the
> subject?

  Well, it sort of is part of the bug. I tested the read-in schema, and I
noticed that certain fields that I'd read in had a different definition than
what was printed in the Schema module. In particular, all auto_increment fields
read as NOT NULL, even though some of them weren't listed as such in the Schema
module. This isn't noticeable anywhere before this patch, but it's necessary for
this, because the Schema we read in has to be *exactly* the Schema object as of
right now.

> >@@ -817,6 +819,8 @@
> > =cut
> > sub _bz_real_schema {
> >     my ($self) = @_;
> >+    # Note: Subclasses depend on the name of this variable, please
> >+    #       don't change the name.
> 
> If you fix the first point I made, you don't need to add this comment :-).

  Well, I'll still need some way to set/store the real_schema from within
Mysql.pm, so I probably will need it, one way or the other. :-)
 
> Otherwise, it look pretty cool, well done.

  Thank you very much. :-) And thanks for the review; these are all good points!
:-) I know it's not the easiest code to read in the world; I'm thankful that you
spent the time on it. :-)

Comment 4

[Tomas Kopal]

(In reply to comment #3)
> (In reply to comment #2)
> 
> 
> > and then call _bz_init_schema_storage() from the base class to store it.
> 
>   I can't do *exactly* that, as we're replacing _bz_real_schema, not _bz_schema.
> But you're right, I ought to somehow modify something in the superclass to allow
> me to do what I'm doing.

If the call to _bz_build_schema_from_disk() initialize the
abstract schema (i.e. _bz_schema variable), then you can call
_bz_init_schema_storage() directly. Backup _bz_schema beforehand, if you still
need it (not sure about it).
Or you can pass the schema as parameter, but I am not sure it's worth the trouble.

> > Ummm, if you are using fetchrow_hashref(), you can directly access the hash
> > using the key (column name) without iterating over it, right?
> 
>   Yeah, if I used fetchall_hashref, then I do believe that you are correct.

From the DBI docs for fetchrow_hashref():

"Fetches the next row of data and returns it as a reference to a hash containing
field name and field value pairs. Null fields are returned as undef values in
the hash."

So it should work with fetchrow_hashref() as well.

> > Can't we put this into 'else' statement and add another 'else' screaming for
> > help if the type was not handled by any of the preceeding cases? Sanity
> > checking usually pays of :-).
> 
>   If the type wasn't handled by any previous type, then it could be a raw
> database type that we don't handle and needs to be passed into the Schema as
> such. I'm thinking now about people who do local customizations, and I don't
> want to choke on their fields. For example, if somebody has an enum in the
> database, we don't want to *die*, we just want to make an enum in the Schema.
> 
>   We would then also die on the TIMESTAMP fields that this will have to
> sometimes read in.
> 
>   Also, you'll notice that REVERSE_MAPPING is very small, because it contains
> the only fields that need to be actually mapped back from what they're normally
> called. Even most valid fields are missing from REVERSE_MAPPING.
> 

Yeah, I was afraid you'll say exactly this :-). I just wanted to make sure I am
not missing anything :-).

> > Why is this part of this bug? It does not seem to have anything to do with the
> > subject?
> 
>   Well, it sort of is part of the bug. I tested the read-in schema, and I
> noticed that certain fields that I'd read in had a different definition than
> what was printed in the Schema module. In particular, all auto_increment fields
> read as NOT NULL, even though some of them weren't listed as such in the Schema
> module. This isn't noticeable anywhere before this patch, but it's necessary for
> this, because the Schema we read in has to be *exactly* the Schema object as of
> right now.

Fair enough, keep it there :-).

> > Otherwise, it look pretty cool, well done.
> 
>   Thank you very much. :-) And thanks for the review; these are all good points!
> :-) I know it's not the easiest code to read in the world; I'm thankful that you
> spent the time on it. :-)

Well, you are doing the same for me, aren't you :-)?

Comment 5

[Max Kanat-Alexander]

Attachment: v2 (use -p1) (requires bug 284850)

OK, I believe that I've addressed all the comments.

Comment 6

[Tomas Kopal]

Comment on attachment 179670 [details] [diff] [review]
v2 (use -p1) (requires bug 284850)

>+    $self->SUPER::bz_setup_database(@_, "skip schema init");
>+
>+    my $schema_exists = 
>+        $self->selectrow_array("SELECT COUNT(*) FROM bz_schema");
>+    if (!$schema_exists) {
>+        print "Building Schema object from database...\n";
>+        my $built_schema = $self->_bz_build_schema_from_disk();
>+        # And now we do a little hack to store the Schema we created
>+        # instead of the standard abstract Schema.
>+        my $real_abstract_schema = $self->_bz_schema;
>+        $self->{private_bz_schema} = $built_schema;
>+        # XXX - The code is not ready for this yet, but once
>+        #       all the deps of bug 285111 are checked-in and
>+        #       tested, this should be uncommented.
>+        #$self->_bz_init_schema_storage;
>+        $self->{private_bz_schema} = $real_abstract_schema;
>+    }

Well, this almost looks more complicated than before. And the code similar to
_bz_init_schema_storage function is still there :-(.

What about if we do not pass the "skip schema init" parameter to the
bz_setup_database function? Then it will call the _bz_init_schema_storage() fx
for us, just with wrong schema to store.
If we modify _bz_init_schema_storage() to call something like _get_schema()
before storing it in the db, then the _get_schema() fx could return _bz_schema
in normal circumstances, but will be overriden for MySQL to return
_bz_build_schema_from_disk() when needed. That should work with minimal code
changes and it would be cleaner. What do you think?

Nit: the typo 'ddatetime' is still there :-).

Otherwise it's ok. If you convince me the changes I suggest in the setup code
are not worth it, it's r+, but I would really like to see that cleaned up :-).

Comment 7

[Max Kanat-Alexander]

(In reply to comment #6)
> What about if we do not pass the "skip schema init" parameter to the
> bz_setup_database function? Then it will call the _bz_init_schema_storage() fx
> for us, just with wrong schema to store.

  Doesn't work; I need the setup_database function to create the tables *before*
I read in the Schema.

> If we modify _bz_init_schema_storage() to call something like _get_schema()
> before storing it in the db, then the _get_schema() fx could return _bz_schema
> in normal circumstances, but will be overriden for MySQL to return
> _bz_build_schema_from_disk() when needed. That should work with minimal code
> changes and it would be cleaner. What do you think?

  No, it wouldn't work. _bz_schema is expected to always be an abstract object,
and not a representation of the current database. We'd just be creating more
complexity.

> Nit: the typo 'ddatetime' is still there :-).

  Oh, right. :-) Will fix on checkin.

> Otherwise it's ok. If you convince me the changes I suggest in the setup code
> are not worth it, it's r+, but I would really like to see that cleaned up :-).

  The best I could do as far as cleanup would be a check_schema_storage function
that just did exactly what that one selectrow line does.

Comment 8

[Tomas Kopal]

(In reply to comment #7)
> (In reply to comment #6)
> > What about if we do not pass the "skip schema init" parameter to the
> > bz_setup_database function? Then it will call the _bz_init_schema_storage() fx
> > for us, just with wrong schema to store.
> 
>   Doesn't work; I need the setup_database function to create the tables *before*
> I read in the Schema.

That should be ok, the bz_setup_database function is first creating the DB, and
then calling _bz_init_schema_storage. You would read the schema from the DB as a
reaction to the _get_schema() call from _bz_init_schema_storage, so the tables
are already created.

> 
> > If we modify _bz_init_schema_storage() to call something like _get_schema()
> > before storing it in the db, then the _get_schema() fx could return _bz_schema
> > in normal circumstances, but will be overriden for MySQL to return
> > _bz_build_schema_from_disk() when needed. That should work with minimal code
> > changes and it would be cleaner. What do you think?
> 
>   No, it wouldn't work. _bz_schema is expected to always be an abstract object,
> and not a representation of the current database. We'd just be creating more
> complexity.

Ugh, I am afraid I do not understand. The _bz_init_schema_storage() would be
modified something like this:

    my $table_size = $self->selectrow_array("SELECT COUNT(*) FROM bz_schema");

    if ($table_size == 0) {
        print "Initializing the new Schema storage...\n";

#change here - calling _get_schema()
        my $store_me = $self->_get_schema->serialize_abstract();

        my $schema_version = $self->_bz_schema->SCHEMA_VERSION;
        my $sth = $self->prepare("INSERT INTO bz_schema "
...

Then _get_schema() would normally return bz_schema, thus working as before. For
MySQL, it would return _bz_build_schema_from_disk() when needed, which is
abstract schema as well AFAIK, only representing current DB on the disk.

What am I missing?

Comment 9

[Max Kanat-Alexander]

Attachment: v3 (use -p1) (requires bug 284850)

Oh yeah, you're right. :-) That's a better solution. I just didn't understand
until you explained it further. :-)

OK, here it is.

Comment 10

[Tomas Kopal]

Comment on attachment 179711 [details] [diff] [review]
v3 (use -p1) (requires bug 284850)

Beautiful :-).

The only last thing which worries me a bit is that (if I understand correctly
the code) for new installs on MySQL, we'll build the database and then
re-construct the abstract schema from it, which seems to be a bit awkward. It
means we'll need to keep the code building the abstract schema up to date if we
make some fundamental changes (e.g. foreign keys). It would be reall cool to
differentiate between upgrade and fresh install here...
But that's different bug anyway.

Comment 11

[Max Kanat-Alexander]

(In reply to comment #10)
> [snip] we'll need to keep the code building the abstract schema up to date if 
> we make some fundamental changes (e.g. foreign keys).

  No, because thankfully the schema-reading code only runs if the bz_schema
table is empty. This will only happen if you're upgrading from a version before
2.20. :-)

Comment 12

[Max Kanat-Alexander]

OK, nevermind. We need to fix some issues with the patch so that we don't have
to ever re-write this code in the future if we don't want to. Here's my ICQ
conversation with Tomas about it:

(16:44:03) Tomáš Kopal: Hey, I still don't get it, sorry. If we are doing a new
install, isn't it that we first create the DB and then add a record to the
schema table?
(16:44:18) Avatraxiom: Oh...
(16:44:24) Avatraxiom: Right, for the tables, yeah.
(16:44:25) Tomáš Kopal: Then after creating the DB, on MySQL, we'll read it in?
(16:45:00) Avatraxiom: Oh right, crappy.
(16:45:17) Tomáš Kopal: I was wondering about adding some 'new install'
temporary record to the schema table for this purpose, but maybe there is some
better way...
(16:45:33) Avatraxiom: There is a better way.
(16:45:46) Avatraxiom: The best way to handle it is to figure out how to do it
before we create the new tables.
(16:46:23) Tomáš Kopal: Maybe adding an empty schema to the schema table before
we start creating the DB? (but we need at least the schema table :-) ).
(16:46:28) Avatraxiom: Right.
(16:47:16) Tomáš Kopal: Then the schema still corresponds to what is on the disk
- no db, empty schema. But we have schema record, so it's basically upgrade then...
(16:53:43) Avatraxiom: Yeah. So I guess that bz_init_schema_table would just
have to create the bz_schema table.
(16:53:57) Avatraxiom: (If it didn't already exist.)
(16:54:13) Avatraxiom: OK, so we should probably do all that that way, instead
of the way we're doing it now.
(16:54:21) Avatraxiom: We should just do it right before we check in the patch,
I think.

Comment 13

[Max Kanat-Alexander]

Attachment: v4

OK, if you thought this patch was some crazy madness before... you should see
it now! :-)

Before this patch, we used to create the tables, and then assume that the
_bz_schema object represented the tables we just created.

Now, we start by initializing an empty schema object (if we're a new install),
or a built schema object (if we're an upgrading install). Then, we add the
tables one by one to that schema object.

However, for new installs, we also have to create the bz_schema table if it
doesn't exist, in a special way, before the normal table creation. So we do
that, also.

Comment 14

[Max Kanat-Alexander]

That was this, by the way:

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.386; previous revision: 1.385
done

Comment 15

[Max Kanat-Alexander]

(In reply to comment #14)
> That was this, by the way:
> 
> Checking in checksetup.pl;
> /cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
> new revision: 1.386; previous revision: 1.385
> done

  Oops, wrong bug, ignore. :-)

Comment 16

[Tomas Kopal]

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

Ahhh, I love this stuff :-). Well done Max.

>+        $self->bz_add_table($table_name, 
>+            $self->_bz_schema->get_table_abstract($table_name));

Why are you passing the second parameter here? I can't find it being used
anywhere...

>+sub bz_add_table {
>+sub _bz_add_table_raw {

These two needs POD docs.

>+        if (!defined $table_size) {
>+            $self->_bz_add_table_raw('bz_schema');
>+            $table_size = 0
>+        }
...
>+        # And now we have to update the on-disk schema to hold the bz_schema
>+        # table, if the bz_schema table didn't exist when we were called.
>+        if (!defined $table_size) {
>+            $self->_bz_real_schema->add_table('bz_schema',
>+                $self->_bz_real_schema->get_table_abstract('bz_schema'));
>+            $self->_bz_store_real_schema;
>+        }

The second condition is always false as you set $table_size = 0 in the first
one.

>+sub bz_column_info_real {
>+    my ($self, $table, $column) = @_;
>+
>+    # DBD::mysql does not support selecting a specific column,
>+    # so we have to get all the columns on the table and find 
>+    # the one we want.
>+    my $info_sth = $self->column_info(undef, undef, $table, '%');
>+    my $all_cols = $info_sth->fetchall_hashref("COLUMN_NAME");
>+    my $col_data = $all_cols->{$column};

I still think that fetchROW_hashref would be faster here, but this code does
not have to be fast, so take it as a nit only.

>+sub _bz_build_schema_from_disk {
>+    my ($self) = @_;
>+
>+    print "Building Schema object from database...\n";
>+
>+    my $schema = $self->_bz_schema->get_empty_schema();
>+
>+    my @tables = $self->bz_table_list_real();
>+    foreach my $table (@tables) {

Can't find definition of bz_table_list_real, probably part of some other patch?

>@@ -1576,7 +1579,7 @@
> 
>     # Prevent a possible dereferencing of an undef hash, if the
>     # table doesn't exist.
>-    if (exists $self->{abstract_schema}->{$table}) {
>+    if ($self->get_table_abstract($table)) {
>         my %fields = (@{ $self->{abstract_schema}{$table}{FIELDS} });
>         return $fields{$column};
>     }
>@@ -1600,13 +1603,55 @@
> 
>     # Prevent a possible dereferencing of an undef hash, if the
>     # table doesn't exist.
>-    if (exists $self->{abstract_schema}->{$table}) {
>+    if ($self->get_table_abstract($table)) {
>         my %indexes = (@{ $self->{abstract_schema}{$table}{INDEXES} });
>         return $indexes{$index};
>     }

You probably want to adjust the coments here now.

>+ Params:      $table - The name of hte table you want a definition for.

Typo: hte -> the

Comment 17

[Max Kanat-Alexander]

(In reply to comment #16)
> (From update of attachment 179803 [details] [diff] [review] [edit])
> Ahhh, I love this stuff :-). Well done Max.

  Thanks. :-)

> Why are you passing the second parameter here? I can't find it being used
> anywhere...

  Ahh, left over from another method of working it that I was trying. Will
remove that. :-)

> These two needs POD docs.

  OK, will do.

> The second condition is always false as you set $table_size = 0 in the first
> one.

  Oh, nice catch!!

> I still think that fetchROW_hashref would be faster here, but this code does
> not have to be fast, so take it as a nit only.

  I think you're missing the point that we have to grab the entire column list,
because DBD::mysql doesn't support grabbing a single column with column_info. I
*have* to use fetchall; I can't select a specific row with fetchrow_hashref.

> Can't find definition of bz_table_list_real, probably part of some other 
> patch?

  Yeah, it's from the index-renaming patch.

> You probably want to adjust the coments here now.

  Actually, they're still accurate.

> >+ Params:      $table - The name of hte table you want a definition for.
> 
> Typo: hte -> the

  Oh, thanks! :-)

Comment 18

[Max Kanat-Alexander]

Attachment: v5

OK, comments addressed.

Comment 19

[Tomas Kopal]

Comment on attachment 179826 [details] [diff] [review]
v5

Sweet.

Comment 20

[Max Kanat-Alexander]

Attachment: v6

OK, so there was a small bug in the previous patch. The interdiff should
explain it. But basically, _bz_get_initial_schema was building the Schema with
a bz_schema table in it, and then when we tried to manually add our bz_schema
table to the Schema object, we died.

Comment 21

[Tomas Kopal]

Comment on attachment 179999 [details] [diff] [review]
v6

Yeah, good catch...

Comment 22

[Max Kanat-Alexander]

Attachment: v7

OK, there was one other bug -- bz_add_table was adding an undef to the Schema.
:-)

Comment 23

[Tomas Kopal]

Comment on attachment 180003 [details] [diff] [review]
v7

Looks good.

Comment 24

[Dave Miller [:justdave]]

per status whiteboard, request approval again when 285722 is ready to land

Comment 25

[Max Kanat-Alexander]

Attachment: v8 (Activation bits only)

Now that most of this patch has been broken-off into bug 290402, here are the
bits that remain.

Carrying forward r+, because it's the same patch, just broken down into a
smaller piece.

Comment 26

[Max Kanat-Alexander]

Checking in Bugzilla/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v  <--  DB.pm
new revision: 1.53; previous revision: 1.52
done
Checking in Bugzilla/DB/Mysql.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Mysql.pm,v  <--  Mysql.pm
new revision: 1.18; previous revision: 1.17
done