Move initial table creation into the Bugzilla::DB modules

RESOLVED FIXED in Bugzilla 2.20

Status

()

enhancement
P1
normal
RESOLVED FIXED
15 years ago
15 years ago

People

(Reporter: mkanat, Assigned: mkanat)

Tracking

2.19.2
Bugzilla 2.20
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(1 attachment, 3 obsolete attachments)

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.
Target Milestone: --- → Bugzilla 2.20
I currently have a patch that's almost working. It depends on bug 146679, of
course. I'll post it when I'm ready.
Status: NEW → ASSIGNED
Depends on: 284172
Posted patch Work In Progress (obsolete) — Splinter Review
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 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
:-).
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.
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.
Attachment #175993 - Attachment is obsolete: true
Attachment #176078 - Flags: review?(Tomas.Kopal)
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. :-)
Attachment #176078 - Flags: review?(bugreport)
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.
Attachment #176078 - Flags: review?(Tomas.Kopal) → review+
(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.
(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 :-).
Blocks: 284529
Attachment #176078 - Flags: review?(bugreport) → review?(bugzilla)
+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};
}
(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
Priority: -- → P1
Depends on: 285316
No longer depends on: 285316
*** Bug 285316 has been marked as a duplicate of this bug. ***
Posted patch v2 (obsolete) — Splinter Review
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.
Attachment #176078 - Attachment is obsolete: true
Attachment #176807 - Flags: review?(bugzilla)
Attachment #176078 - Flags: review?(bugzilla)
Posted patch REAL v2Splinter Review
OK, that old one was the wrong patch.
Attachment #176807 - Attachment is obsolete: true
Attachment #176825 - Flags: review?(bugzilla)
Attachment #176807 - Flags: review?(bugzilla)
Comment on attachment 176825 [details] [diff] [review]
REAL v2

r=glob
Attachment #176825 - Flags: review?(bugzilla) → review+
Flags: approval?
Flags: approval? → approval+
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
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.