290407 - Fix up bz_alter_column and break out a bz_alter_column_raw

Status: RESOLVED FIXED

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

Description

[Max Kanat-Alexander]

bz_alter_column has a slight bug, in that we need to move its current NOT NULL
check into the inside of its "if" block. (That will make sense once you see the
patch.)

Also, we need a bz_alter_column_raw for bug 285722.

Comment 1

[Max Kanat-Alexander]

Attachment: Clean up bz_alter_column

Another part of bug 285722. :-) We need a bz_alter_column_raw function for that
bug, and I also discovered while writing it that I had to move that check for
NULLs in the column, because otherwise we might try to check a column that
doesn't exist! :-)

Comment 2

[Tomas Kopal]

(In reply to comment #1)
> Another part of bug 285722. :-) We need a bz_alter_column_raw function for that
> bug, and I also discovered while writing it that I had to move that check for
> NULLs in the column, because otherwise we might try to check a column that
> doesn't exist! :-)

Hmmm, maybe I am bit slower (it's Friday after all :-)), but I just can't grasp
why moving the check inside that condition would help if the column does not
exists? I suppose if it does not exists, the $current_def will be empty (or
undef?) so the comparison will return "not equal" and the code will still run,
failing on missing column?

Comment 3

[Max Kanat-Alexander]

(In reply to comment #2)
> Hmmm, maybe I am bit slower (it's Friday after all :-)), but I just can't 
> grasp why moving the check inside that condition would help if the column does 
> not exists?

  Because the check for $current_def will return undef if the column doesn't
exist, so we won't ever run that code. :-)

Comment 4

[Max Kanat-Alexander]

Ahhh, I figured this out, finally. I mis-stated the bug I was running into.

The ACTUAL bug that I experienced was that the NULL check would run *even if we
didn't have to alter the column*. So it would sometimes run the check when it
really didn't need to.

Comment 5

[Tomas Kopal]

Comment on attachment 180760 [details] [diff] [review]
Clean up bz_alter_column

(In reply to comment #4)
> Ahhh, I figured this out, finally. I mis-stated the bug I was running into.
> 
> The ACTUAL bug that I experienced was that the NULL check would run *even if we
> didn't have to alter the column*. So it would sometimes run the check when it
> really didn't need to.

Well, now it makes sense :-).

Comment 6

[Tomas Kopal]

Just realized another detail: In bz_alter_column_raw you are using
_bz_real_schema instead of _bz_schema, please check if that makes any difference
(like using the real schema before it exists etc.).

Comment 7

[Max Kanat-Alexander]

(In reply to comment #6)
> Just realized another detail: In bz_alter_column_raw you are using
> _bz_real_schema instead of _bz_schema, please check if that makes any 
> difference (like using the real schema before it exists etc.).

  Yeah, as I recall, get_alter_column_ddl needs to be on the _bz_real_schema,
because it needs to know the current definition as well.

  I should probably eventually modify get_alter_column_ddl to also take a
$current_def argument, because I have a feeling that this requirement of knowing
the current state of the column will happen for a lot of other DBs, also.

  That would be another bug, too.

  But this *does* mean that bz_alter_column_raw can't be used before there's a
Schema object, which might be a problem... hrm...

  I'll probably have to deal with it in another bug, I think.

Comment 8

[Max Kanat-Alexander]

Checking in Bugzilla/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v  <--  DB.pm
new revision: 1.50; previous revision: 1.49
done