All users were logged out of Bugzilla on October 13th, 2018

Move initial table creation into the Bugzilla::DB modules

RESOLVED FIXED in Bugzilla 2.20

Status

()

P1
enhancement
RESOLVED FIXED
14 years ago
14 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)

(Assignee)

Description

14 years ago
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.
(Assignee)

Updated

14 years ago
Target Milestone: --- → Bugzilla 2.20
(Assignee)

Comment 1

14 years ago
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
(Assignee)

Updated

14 years ago
Depends on: 284172
(Assignee)

Comment 2

14 years ago
Created attachment 175993 [details] [diff] [review]
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

14 years ago
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

14 years ago
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.
(Assignee)

Comment 5

14 years ago
Created attachment 176078 [details] [diff] [review]
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.
Attachment #175993 - Attachment is obsolete: true
Attachment #176078 - Flags: review?(Tomas.Kopal)
(Assignee)

Comment 6

14 years ago
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 7

14 years ago
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+
(Assignee)

Comment 8

14 years ago
(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

14 years ago
(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 :-).
(Assignee)

Updated

14 years ago
Blocks: 284529
(Assignee)

Updated

14 years ago
Attachment #176078 - Flags: review?(bugreport) → review?(bugzilla)

Comment 10

14 years ago
+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};
}
(Assignee)

Comment 11

14 years ago
(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
(Assignee)

Updated

14 years ago
Priority: -- → P1
(Assignee)

Updated

14 years ago
Depends on: 285316
(Assignee)

Updated

14 years ago
No longer depends on: 285316
(Assignee)

Comment 12

14 years ago
*** Bug 285316 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 13

14 years ago
Created attachment 176807 [details] [diff] [review]
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.
Attachment #176078 - Attachment is obsolete: true
Attachment #176807 - Flags: review?(bugzilla)
(Assignee)

Updated

14 years ago
Attachment #176078 - Flags: review?(bugzilla)
(Assignee)

Comment 14

14 years ago
Created attachment 176825 [details] [diff] [review]
REAL v2

OK, that old one was the wrong patch.
Attachment #176807 - Attachment is obsolete: true
Attachment #176825 - Flags: review?(bugzilla)
(Assignee)

Updated

14 years ago
Attachment #176807 - Flags: review?(bugzilla)
Comment on attachment 176825 [details] [diff] [review]
REAL v2

r=glob
Attachment #176825 - Flags: review?(bugzilla) → review+
(Assignee)

Updated

14 years ago
Flags: approval?
Flags: approval? → approval+
(Assignee)

Comment 16

14 years ago
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
Last Resolved: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.