Cross-DB bz_rename_column and bz_drop_column

RESOLVED FIXED in Bugzilla 2.20

Status

()

Bugzilla
Bugzilla-General
P1
enhancement
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: Max Kanat-Alexander, Assigned: Max Kanat-Alexander)

Tracking

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

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

13 years ago
Just like bz_rename_field, but cross-database compatible! Wow!
(Assignee)

Updated

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

Comment 1

13 years ago
It turns out that I'll pretty much have to implement DROP at the same time, so
let's just do that. They're both easy anyhow.
Summary: Cross-DB bz_rename_column → Cross-DB bz_rename_column and bz_drop_column
(Assignee)

Comment 2

13 years ago
Created attachment 177730 [details] [diff] [review]
bz_rename_column and bz_drop_column

Tested on PostgreSQL and on MySQL. Once again, this contains various fixes for
previous patches. I apologies for mixing them together, but I think that this
method of doing things is better than taking out a branch and then having to
merge it back in.
Attachment #177730 - Flags: review?(Tomas.Kopal)
(Assignee)

Comment 3

13 years ago
Created attachment 177731 [details] [diff] [review]
v1.1

Oops, I'd left two debug "print" statements in there.
Attachment #177730 - Attachment is obsolete: true
Attachment #177731 - Flags: review?(Tomas.Kopal)

Comment 4

13 years ago
Comment on attachment 177731 [details] [diff] [review]
v1.1

>+        # Handle any fields that were NULL before. We *must* have a DEFAULT.
>+        push(@statements, "UPDATE $table SET $column = $default"
>+                        . "  WHERE $column IS NULL");

Just for the record, you just created the column so there can't be any sensible
data, so leaving out the WHERE and changing all values would be as good as
this. But we don't need extra performance so let's play safe.

>+    my $abstract_fields = $self->{abstract_schema}{$table}{FIELDS};
>+    my $name_position = lsearch($abstract_fields, $column);
>+    # Delete the key/value pair from the array.
>+    splice(@$abstract_fields, $name_position, 2);
>+

I would really like to see a check for sensible value in $name_position here,
as a safeguard. I don't like trusting output from functions like lsearch to be
always valid, i.e. the substring always found :-).

>--- mkanat4/Bugzilla/DB.pm	2005-03-17 06:14:53.000000000 -0800
>+++ mkanat/Bugzilla/DB.pm	2005-03-17 06:20:06.000000000 -0800

>+sub bz_drop_column {

>+sub bz_rename_column {

Missing PODs for these.

Also, I was just thinking if it wouldn't be handy if these methods in DB.pm
returned true/false if any change were performed. Then you could easily do some
migration code (or rollback some chages, or whatever is needed) based on the
return value. Does it make sense, in the context of your idea how it should be
used?
Attachment #177731 - Flags: review?(Tomas.Kopal) → review-

Updated

13 years ago
Attachment #177730 - Flags: review?(Tomas.Kopal)
(Assignee)

Comment 5

13 years ago
Created attachment 177837 [details] [diff] [review]
v2

OK, I addressed all the comments.
Attachment #177731 - Attachment is obsolete: true
Attachment #177837 - Flags: review?(Tomas.Kopal)

Comment 6

13 years ago
Comment on attachment 177837 [details] [diff] [review]
v2

>--- Bugzilla/DB/Schema.pm	18 Mar 2005 03:28:52 -0000	1.13
>+++ Bugzilla/DB/Schema.pm	18 Mar 2005 07:09:21 -0000
>+            push(@statements, "UPDATE $table SET $column = $default"
>+                            . "  WHERE $column IS NULL");

>--- Bugzilla/DB/Schema/Pg.pm	18 Mar 2005 03:28:53 -0000	1.6
>+++ Bugzilla/DB/Schema/Pg.pm	18 Mar 2005 07:09:21 -0000
>+        push(@statements, "UPDATE $table SET $column = $default");

Is there any reason you changed this only for Postgres?
Just to be clear, I don't mind having the WHERE there, quite the opposite, we
are not shooting for extra speed here (it's just setup, nothing run by
thousands of users every second).

Otherwise, it looks perfect.
Attachment #177837 - Flags: review?(Tomas.Kopal) → review+
(Assignee)

Comment 7

13 years ago
(In reply to comment #6)
> Is there any reason you changed this only for Postgres?

  Yeah, one of those is ADD COLUMN and the other one is ALTER COLUMN.

Comment 8

13 years ago
(In reply to comment #7)
> (In reply to comment #6)
> > Is there any reason you changed this only for Postgres?
> 
>   Yeah, one of those is ADD COLUMN and the other one is ALTER COLUMN.

Ooops :-). Ok, then, no problems with the patch :-).
(Assignee)

Updated

13 years ago
Flags: approval?
(Assignee)

Comment 9

13 years ago
Created attachment 178791 [details] [diff] [review]
v2.1 (bitrot fix)

Just a bitrot fix; carrying forward r+.
(Assignee)

Updated

13 years ago
Attachment #177837 - Attachment is obsolete: true
Attachment #178791 - Flags: review+
(Assignee)

Updated

13 years ago
Blocks: 287986
(Assignee)

Updated

13 years ago
Blocks: 288539
(Assignee)

Updated

13 years ago
No longer blocks: 288539
Flags: approval? → approval+
(Assignee)

Comment 10

13 years ago
Checking in Bugzilla/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v  <--  DB.pm
new revision: 1.42; previous revision: 1.41
done
Checking in Bugzilla/DB/Schema.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v  <--  Schema.pm
new revision: 1.18; previous revision: 1.17
done
Checking in Bugzilla/DB/Schema/Mysql.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema/Mysql.pm,v  <--  Mysql.pm
new revision: 1.5; previous revision: 1.4
done
Checking in Bugzilla/DB/Schema/Pg.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema/Pg.pm,v  <--  Pg.pm
new revision: 1.7; previous revision: 1.6
done
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.