Fix up bz_alter_column and break out a bz_alter_column_raw

RESOLVED FIXED in Bugzilla 2.20

Status

()

Bugzilla
Installation & Upgrading
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
Bug Flags:
approval +

Details

Attachments

(1 attachment)

(Assignee)

Description

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

Updated

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

Comment 1

13 years ago
Created attachment 180760 [details] [diff] [review]
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! :-)
Attachment #180760 - Flags: review?(Tomas.Kopal)

Comment 2

13 years ago
(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?
(Assignee)

Comment 3

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

Comment 4

13 years ago
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

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

Updated

13 years ago
Flags: approval?

Comment 6

13 years ago
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.).
Flags: approval? → approval+
(Assignee)

Comment 7

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

Comment 8

13 years ago
Checking in Bugzilla/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v  <--  DB.pm
new revision: 1.50; previous revision: 1.49
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.