Cross-DB bz_add_column

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, 2 obsolete attachments)

(Assignee)

Description

14 years ago
Need to make a cross-db bz_add_column to replace bz_get_field_def, that uses the
Schema object.
(Assignee)

Comment 1

14 years ago
I mean, to replace bz_add_field. :-) It must be 2am. :-)
(Assignee)

Updated

14 years ago
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → Bugzilla 2.20
(Assignee)

Comment 2

14 years ago
Created attachment 177130 [details] [diff] [review]
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.
(Assignee)

Updated

14 years ago
Attachment #177130 - Flags: review?(Tomas.Kopal)
(Assignee)

Updated

14 years ago
Attachment #177130 - Flags: review?(edwardjsabol)
(Assignee)

Comment 3

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

Comment 4

14 years ago
Created attachment 177138 [details] [diff] [review]
v2

Oh, this version works on Pg. (Ahh, the wonders of Pg's ALTER TABLE...)
Attachment #177130 - Attachment is obsolete: true
Attachment #177138 - Flags: review?(Tomas.Kopal)
(Assignee)

Updated

14 years ago
Attachment #177130 - Flags: review?(edwardjsabol)
Attachment #177130 - Flags: review?(Tomas.Kopal)
(Assignee)

Updated

14 years ago
Attachment #177138 - Flags: review?(edwardjsabol)

Comment 5

14 years ago
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)
Attachment #177138 - Flags: review?(Tomas.Kopal) → review-
(Assignee)

Comment 6

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

Updated

14 years ago
Attachment #177138 - Flags: review?(edwardjsabol)
(Assignee)

Comment 7

14 years ago
Created attachment 177715 [details] [diff] [review]
v3

OK, I've addressed everything that you've mentioned.
Attachment #177138 - Attachment is obsolete: true
Attachment #177715 - Flags: review?(Tomas.Kopal)

Comment 8

14 years ago
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+.
Attachment #177715 - Flags: review?(Tomas.Kopal) → review+
(Assignee)

Comment 9

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

Updated

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

Comment 10

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