290405 - bz_add_column needs a way to specify an initial value

Status: RESOLVED FIXED

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

Description

[Max Kanat-Alexander]

Right now, bz_add_column dies if you try to create a NOT NULL column with no
default.

This makes life really difficult, because it means that checksetup would be full
of bz_add_column with a bz_alter_column immediately afterward. (Add the column
with a default, and then immediately remove it.) That's not friendly to
developers, and it also causes a few problems with certain types of columns.
(For example, it's really difficult to call bz_alter_column on any column which
is a PRIMARY KEY in MySQL, currently.)

Instead, bz_add_column should be able to specify an "initial value" for the
column. Then, Bugzilla::DB::Schema implementations can just create the column as
NULL, set it to a default value, and then set it to NOT NULL.

That's much cleaner from the caller's perspective.

Comment 1

[Max Kanat-Alexander]

Attachment: Add the init_value parameter

OK, this is directly part of bug 285722; I pretty much just edited the diff
down.

Comment 2

[Tomas Kopal]

Comment on attachment 180758 [details] [diff] [review]
Add the init_value parameter

>     # You can't add a NOT NULL column to a table with
>-    # no DEFAULT statement.
>-    if ($new_def->{NOTNULL} && !exists $new_def->{DEFAULT}) {
>+    # no DEFAULT statement, unless you have an init_value.
>+    if ( $new_def->{NOTNULL} && !exists $new_def->{DEFAULT} 
>+         && !defined $init_value) 
>+    {
>         die "Failed adding the column ${table}.${name}:\n  You cannot add"
>-            . " a NOT NULL column with no default to an existing table.\n";
>+            . " a NOT NULL column with no default to an existing table,.\n"
>+            . "  unless you specify something for \$init_value." 
>+            # SERIAL types are an exception, though, because they can
>+            # auto-populate.
>+            unless $new_def->{TYPE} =~ /SERIAL/;

Please, move this to the if condition above, it's a bit convoluted to have the
condition split like this.

>+    # XXX - Note that although this works for MySQL, most databases will fail
>+    # before this point, if we haven't set a default.
>+    (push(@statements, "UPDATE $table SET $column = $init_value"))
>+        if defined $init_value;

Judging by the XXX, this is something you plan to fix in the future? Or will
this stay as it is an be fixed by overriding this method for every DB, as is
the case of Pg? Why the XXX then?

Comment 3

[Max Kanat-Alexander]

(In reply to comment #2)
> Judging by the XXX, this is something you plan to fix in the future? Or will
> this stay as it is an be fixed by overriding this method for every DB, as is
> the case of Pg? Why the XXX then?

  Yes, I do plan to fix it in the future, if it's ever necessary to fix. The
comment is in the code so that if somebody runs into the bug when they're trying
to implement the driver, they'll know what they have to do. It's not necessary
to fix for 2.20, though.

Comment 4

[Max Kanat-Alexander]

Attachment: v2

OK, I fixed the condition. I agree; it's much clearer this way.

Comment 5

[Tomas Kopal]

Comment on attachment 180761 [details] [diff] [review]
v2

Nice.

Comment 6

[Max Kanat-Alexander]

Checking in Bugzilla/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v  <--  DB.pm
new revision: 1.45; previous revision: 1.44
done
Checking in Bugzilla/DB/Schema.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v  <--  Schema.pm
new revision: 1.21; previous revision: 1.20
done
Checking in Bugzilla/DB/Schema/Pg.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema/Pg.pm,v  <--  Pg.pm
new revision: 1.8; previous revision: 1.7
done