Closed Bug 289139 Opened 15 years ago Closed 15 years ago

Bugzilla::DB::Schema::Pg needs to re-create indexes when it has to drop them for a rename/alter

Categories

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

2.19.2
enhancement

Tracking

()

RESOLVED FIXED
Bugzilla 2.20

People

(Reporter: mkanat, Assigned: mkanat)

References

Details

Attachments

(1 file, 1 obsolete file)

Because of Pg's wonderful ALTER TABLE limitations, we have to actually drop and
re-create columns when we want to alter them or rename them.

Unfortunately, this drops all indexes that were on that column, so they need to
be regenerated.

It should be pretty easy; we just need a function to tell us all the indexes
that are on a particular column, and then we need to get the index_ddl to
re-create them.
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → Bugzilla 2.20
Assignee: installation → mkanat
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
OK, here it is. I've tested this on many different columns, and it works quite
well.

I also fixed a bug that was causing an "uninitialized value" message to pop up
during some ALTER TABLE statements on Pg.
Attachment #181950 - Flags: review?(Tomas.Kopal)
Comment on attachment 181950 [details] [diff] [review]
Re-Add indexes when altering a column in "ANSI" ALTER COLUMN

>     my $default = $new_def->{DEFAULT};
>     my $default_old = $old_def->{DEFAULT};
>+    # This first condition prevents "uninitialized value" errors.
>+    if (!defined $default && !defined $default_old) {
>+        # Do Nothing
>+    }
>     # If we went from having a default to not having one
>-    if (!defined $default && defined $default_old) {
>+    elsif (!defined $default && defined $default_old) {
>         push(@statements, "ALTER TABLE $table ALTER COLUMN $column"
>                         . " DROP DEFAULT");
>     }

I do not understand how this could make any difference. I believe you that this
fixed the problem you had, but I would like to understand why :-). Can you
explain why this works?

>+=item C<get_indexes_on_column_abstract($table, $column)

You forgot to close the C<> block (I have seen this on couple more places in
the same module, feel free to fix all of them as part of this patch).


r+, but fix the comments before checkin.
Attachment #181950 - Flags: review?(Tomas.Kopal) → review+
BTW, this made me think where else we need to drop/recreate indexes when
touching the DB schema. What about drop_column? Shouldn't we drop/recreate the
index there as well? If some databases will remove the column from multi column
index and some will drop the index, we don't have consistent behaviour we need.
What about rename_column?
(In reply to comment #3)
> What about drop_column? [snip]

  Yeah, it's possible that we should also do that, but it's not as high-priority
as this one is. I've only once ever run into the problem with drop_column, and
it was in a pretty edge case.

> What about rename_column?

  We don't currently have any databases that have to drop the column for a
rename, so I'm not too worried about that.(In reply to comment #2)

> (From update of attachment 181950 [details] [diff] [review] [edit])
> Can you explain why this works?

 Yeah. Look at the logic for what would have happened before if we had an
undefined $default *and* an undefined $default_old. There's a "||
($default ne $default_old)" outside of the patch context that would have run,
and now never is seen.

> >+=item C<get_indexes_on_column_abstract($table, $column)

  OK, will fix on checkin. :-)
Flags: approval?
Whiteboard: requires minor checkin fixes
>  Yeah. Look at the logic for what would have happened before if we had an
> undefined $default *and* an undefined $default_old. There's a "||
> ($default ne $default_old)" outside of the patch context that would have run,
> and now never is seen.

Ahhh, outside of the context... That was the trick :-). All clear now :-).
Flags: approval? → approval+
Checking in Bugzilla/DB/Schema.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v  <--  Schema.pm
new revision: 1.28; previous revision: 1.27
done
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Here's the version that was checked-in with the checkin fixes.
Attachment #181950 - Attachment is obsolete: true
Attachment #182474 - Flags: review+
Whiteboard: requires minor checkin fixes
You need to log in before you can comment on or make changes to this bug.