284348 - Move initial table creation into the Bugzilla::DB modules

Status: RESOLVED FIXED

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

Description

[Max Kanat-Alexander]

Right now, creation of the initial tables in checksetup happens with a lot of
MySQL-specific stuff.

Instead, we should use the cross-db schema and the Bugzilla::DB structure to do
the initial table creation.

We aren't going to fix all of checksetup for this bug, just the initial table
creation part.

Comment 1

[Max Kanat-Alexander]

I currently have a patch that's almost working. It depends on bug 146679, of
course. I'll post it when I'm ready.

Comment 2

[Max Kanat-Alexander]

Attachment: Work In Progress

OK, here's my work in progress. Of course, it depends on bug 146679 and bug
284172, and it's not yet complete. There are still a few bugs, but it gets
through schema creation nicely.

Comment 3

[Tomas Kopal]

Comment on attachment 175993 [details] [diff] [review]
Work In Progress

>Index: Bugzilla/DB.pm
>@@ -173,7 +174,8 @@
> # List of abstract methods we are checking the derived class implements
> our @_abstract_methods = qw(new sql_regexp sql_not_regexp sql_limit
>                             sql_to_days sql_date_format sql_interval
>-                            bz_lock_tables bz_unlock_tables);
>+                            bz_lock_tables bz_unlock_tables
>+                            REQUIRED_VERSION PROGRAM_NAME);
> 
> # This overriden import method will check implementation of inherited classes
> # for missing implementation of abstract methods

This is from a different patch :-). But otherwise it looks damn good so far
:-).

Comment 4

[Ed Sabol]

I would rather you call the Bugzilla::DB method for accessing the Schema object
bz_schema() instead of _bz_schema(), but I'm having difficulty thinking of a
really good reason, so feel free to ignore this.

Heads up: In the forthcoming Schema implementation, the method get_table_ddl()
can return an array of statements, not just a single statement.

Comment 5

[Max Kanat-Alexander]

Attachment: Use Bugzilla::DB to create the tables

OK, this uses Bugzilla::DB::Schema and Bugzilla::DB to create the database.

It runs perfectly on MySQL. (checksetup cannot yet complete on PostgreSQL,
however, it also creates the tables perfectly on PostgreSQL).

The Bugzilla::Schema module that is currently posted on bug 146679 is not quite
up-to-date -- I've sent Ed a few corrections, and he will post a new patch
tonight. However, with those corrections this patch creates a schema identical
to the current schema.

Comment 6

[Max Kanat-Alexander]

Comment on attachment 176078 [details] [diff] [review]
Use Bugzilla::DB to create the tables

Hey Joel. If Tomas likes this, could you look it over soon? I have a lot of
work to do in this area to make PostgreSQL installation work before feature
freeze. :-)

Comment 7

[Tomas Kopal]

Comment on attachment 176078 [details] [diff] [review]
Use Bugzilla::DB to create the tables

I love this. Just small details:

>Index: Bugzilla/DB.pm
>@@ -332,6 +356,21 @@
>     }
> }
> 
>+#####################################################################
>+# Schema Information
>+#####################################################################
>+
>+sub _bz_schema {
>+    my ($self) = @_;
>+    eval("require $self->{private_schema_class}")
>+        or die $self->{private_schema_class} . " is not a valid "
>+               . " Bugzilla::Schema:\n  $@";
>+    # Can't use ||= on private_ DBI vars. See DBI docs.
>+    $self->{private_bz_schema} = $self->{private_schema_class}->new
>+        unless $self->{private_bz_schema};
>+    return $self->{private_bz_schema};
>+}
>+

Maybe we can check if $self->{private_bz_schema} is initialised at the begining
and skip even the eval? But this code is not performance critical, so it
probably does not matter...

>@@ -366,7 +405,7 @@
>     $sth->execute;
> 
>     while (my $ref = $sth->fetchrow_arrayref) {
>-        next if $$ref[2] ne $field;
>+        next if $$ref[4] ne $field;
>         return $ref;
>    }
> }

Hmmm, what is this? Is it fixing some other bug? Or is it caused by moving the
code from checksetup to separate module? If it is fixing different bug, maybe
it should be separate patch? Although it's simple one-liner, so if you explain
it here and there is no bug already raised for it, I am ok with it as it is.

Comment 8

[Max Kanat-Alexander]

(In reply to comment #7)

> >@@ -366,7 +405,7 @@
> >     $sth->execute;
> > 
> >     while (my $ref = $sth->fetchrow_arrayref) {
> >-        next if $$ref[2] ne $field;
> >+        next if $$ref[4] ne $field;
> >         return $ref;
> >    }
> > }
> 
> Hmmm, what is this? Is it fixing some other bug? Or is it caused by moving the
> code from checksetup to separate module? If it is fixing different bug, maybe
> it should be separate patch? Although it's simple one-liner, so if you explain
> it here and there is no bug already raised for it, I am ok with it as it is.

  Actually, it fixes a bug that we never noticed. It technically is another bug,
but it was critical to getting this fixed, because otherwise MySQL checksetup
breaks when we start using Bugzilla::DB::Schema.

  The bug is that we ask for the *field name* in checksetup when we use
get_index_def, but the function used the *index name*. This wasn't a problem
when they were the same, but now that indexes have real names it breaks checksetup.

Comment 9

[Tomas Kopal]

(In reply to comment #8)
>   Actually, it fixes a bug that we never noticed. It technically is another bug,
> but it was critical to getting this fixed, because otherwise MySQL checksetup
> breaks when we start using Bugzilla::DB::Schema.
> 
>   The bug is that we ask for the *field name* in checksetup when we use
> get_index_def, but the function used the *index name*. This wasn't a problem
> when they were the same, but now that indexes have real names it breaks
checksetup.

I tought so :-). Well, I am happy with it as it is. FWIW, it r+ from me :-).

Comment 10

[Ed Sabol]

+sub _bz_schema {
+    my ($self) = @_;
+    eval("require $self->{private_schema_class}")
+        or die $self->{private_schema_class} . " is not a valid "
+               . " Bugzilla::Schema:\n  $@";
+    # Can't use ||= on private_ DBI vars. See DBI docs.
+    $self->{private_bz_schema} = $self->{private_schema_class}->new
+        unless $self->{private_bz_schema};
+    return $self->{private_bz_schema};
+}

How about if I change the Bugzilla::DB::Schema constructor to die instead of ThrowCodeError? That 
would eliminate this eval, remove some code redundancy, and simplify a lot of this. Something like this:

sub _bz_schema {
    my ($self) = @_;
    # Can't use ||= on private_ DBI vars. See DBI docs.
    unless (ref $self->{private_bz_schema}) {
        $self->{private_bz_schema} = Bugzilla::DB::Schema->new($self->{private_db_module});
    }
    return $self->{private_bz_schema};
}

Comment 11

[Max Kanat-Alexander]

(In reply to comment #10)
> How about if I change the Bugzilla::DB::Schema constructor to die instead of 
> ThrowCodeError?

  Yeah, that's probably a good idea. I was trying to avoid the whole "superclass
instantiates subclass" mechanism, but I suppose it's OK here.

  -Max

Comment 12

[Max Kanat-Alexander]

*** Bug 285316 has been marked as a duplicate of this bug. ***

Comment 13

[Max Kanat-Alexander]

Attachment: v2

OK, this version is up-to-date with the current patch on the Schema bug that
will be checked-in.

This works on MySQL and on PostgreSQL, tested with Pg 7.3.4.

Comment 14

[Max Kanat-Alexander]

Attachment: REAL v2

OK, that old one was the wrong patch.

Comment 15

[:glob ✱]

Comment on attachment 176825 [details] [diff] [review]
REAL v2

r=glob

Comment 16

[Max Kanat-Alexander]

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.362; previous revision: 1.361
done
Checking in Bugzilla/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v  <--  DB.pm
new revision: 1.28; previous revision: 1.27
done
Checking in Bugzilla/DB/Mysql.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Mysql.pm,v  <--  Mysql.pm
new revision: 1.5; previous revision: 1.4
done
Checking in Bugzilla/DB/Pg.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Pg.pm,v  <--  Pg.pm
new revision: 1.4; previous revision: 1.3
done