285723 - Cross-DB bz_add_column

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement, P1)

Description

[Max Kanat-Alexander]

Need to make a cross-db bz_add_column to replace bz_get_field_def, that uses the
Schema object.

Comment 1

[Max Kanat-Alexander]

I mean, to replace bz_add_field. :-) It must be 2am. :-)

Comment 2

[Max Kanat-Alexander]

Attachment: bz_add_column

This is the first patch that actually makes use of most of the serialization
work before this, so it also corrects a few bugs in that code.

Comment 3

[Max Kanat-Alexander]

Oh, and I tested it by adding some columns. It adds them correctly, updates the
serialization correctly, and doesn't add them if they already exist (correctly).

Comment 4

[Max Kanat-Alexander]

Attachment: v2

Oh, this version works on Pg. (Ahh, the wonders of Pg's ALTER TABLE...)

Comment 5

[Tomas Kopal]

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

>--- mkanat/Bugzilla/DB/Schema/Pg.pm	2005-03-11 01:08:53.000000000 -0800
>+++ mkanat4/Bugzilla/DB/Schema/Pg.pm	2005-03-11 07:40:23.000000000 -0800
>+    if ($definition->{NOTNULL}) {
>+        push(@statements, "ALTER TABLE $table ALTER COLUMN $column "
>+                         . " SET NOT NULL");
>+    }

There is one more thing in the Pg docs you maybe overlooked: "If you want to
mark the column non-null, use the SET NOT NULL form ***after you've entered
non-null values for the column in all rows***." (Emphasis mine).
I understand it that when you add a new column, it will contain NULL in all
existing rows. So you can't just set NOT NULL without adding a value for all
existing rows... I am not sure how to deal with it ATM, but this would beat us
on upgrades...

>--- mkanat/Bugzilla/DB/Schema.pm	2005-03-11 04:38:59.000000000 -0800
>+++ mkanat4/Bugzilla/DB/Schema.pm	2005-03-11 07:36:35.000000000 -0800

>+    my $field_position = lsearch($fields, $column) + 1;
>+    # If the column doesn't exist, then add it.
>+    if (!$field_position) {
>+        push(@$fields, $column);
>+        push(@$fields, $new_def);
>+        push(@$abstract_fields, $column);
>+        push(@$abstract_fields, $new_def);
>+    }
>+    # We're modifying an existing column.
>+    else {
>+        splice(@$fields, $field_position, 1, $new_def);
>+        splice(@$abstract_fields, $field_position, 1, $new_def);

Don't you want to use "$field_position - 1" in these two rows? splice() works
with zero base index, doesn't it?

>+=item C<bz_add_column($table, $name, \%definition)>
>+
>+ Description: Adds a new column to a table in the database. Prints out
>+              a brief statement that it did so, to stdout.
>+ Params:      $table = the table where the column is being added
>+              $name  = the name of the new column
>+              \%definition = SQL for defining the new column

Is \%definition really pure SQL? I tought that you are expecting abstract
definition here... (and the param is $new_def in the actual function header)

Comment 6

[Max Kanat-Alexander]

(In reply to comment #5)
> I understand it that when you add a new column, it will contain NULL in all
> existing rows. So you can't just set NOT NULL without adding a value for all
> existing rows... I am not sure how to deal with it ATM, but this would beat us
> on upgrades...

  Hrn. Seems like it's the problem for the caller. They have to add the column
as NULL and then modify it, and then set it to NOT NULL, if it doesn't have a
default.

  That's true in any real database system, anyhow, if you want to create a NOT
NULL column with no default.

  I'll add a note to the docs about it, on bz_add_column.


> >+        splice(@$fields, $field_position, 1, $new_def);
> >+        splice(@$abstract_fields, $field_position, 1, $new_def);
> 
> Don't you want to use "$field_position - 1" in these two rows? splice() works
> with zero base index, doesn't it?

  Yes, but this is Perl Magic (TM). I've made a hash into an array, so the key
is in the first place (0), and the value is in the second place (1). The
definition goes into $field_position. Trust me. :-)

 
> Is \%definition really pure SQL? I tought that you are expecting abstract
> definition here... (and the param is $new_def in the actual function header)
 
  Oh yeah, you're right. :-) I think I might have fixed that in my local copy... :-)

Comment 7

[Max Kanat-Alexander]

Attachment: v3

OK, I've addressed everything that you've mentioned.

Comment 8

[Tomas Kopal]

Comment on attachment 177715 [details] [diff] [review]
v3

>+# Overridden because Pg has such weird ALTER TABLE problems.
>+sub get_add_column_ddl {
>+    my ($self, $table, $column, $definition) = @_;
>+
>+    my @statements;
>+    my $specific = $self->{db_specific};
>+
>+    my $type = $definition->{TYPE};
>+    $type = $specific->{$type} if exists $specific->{$type};
>+    push(@statements, "ALTER TABLE $table ADD COLUMN $column $type");
>+
>+    my $default = $definition->{DEFAULT};
>+    # Replace any abstract default value (such as 'TRUE' or 'FALSE')
>+    # with its database-specific implementation.
>+    if (defined $default) {
>+        $default = $specific->{$default} if exists $specific->{$default};
>+        push(@statements, "ALTER TABLE $table ALTER COLUMN $column "
>+                         . " SET DEFAULT $default");
>+    }
>+
>+    if ($definition->{NOTNULL}) {
>+        push(@statements, "ALTER TABLE $table ALTER COLUMN $column "
>+                         . " SET NOT NULL");
>+    }
>+

I am still not sure this does what we want. Addition of a new column don't know
anything about future default, so it will probably set all the new values to
NULL. Setting the default later will not modify the existing (NULL) values.
Adding NOT NULL constraint will probably throw an error about violating the
constraint.
I would guess that you need to update all the existing rows and set the default
value explicitely, before adding NOT NULL. Yes, it sucks :-).
This is just my estimate, and I am no postgres expert, so if you tested this
and it works, you have my r+.

Comment 9

[Max Kanat-Alexander]

Oh yeah, you're right. I'll deal with it in my next patch, though. :-) (Sorry,
this is just an endless stream of patches to implement one thing, at this point.
:-) )

Comment 10

[Max Kanat-Alexander]

Checking in Bugzilla/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v  <--  DB.pm
new revision: 1.37; previous revision: 1.36
done
Checking in Bugzilla/DB/Schema.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v  <--  Schema.pm
new revision: 1.12; previous revision: 1.11
done
Checking in Bugzilla/DB/Schema/Pg.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema/Pg.pm,v  <--  Pg.pm
new revision: 1.5; previous revision: 1.4
done