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

Status: RESOLVED FIXED

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

Description

[Max Kanat-Alexander]

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.

Comment 1

[Max Kanat-Alexander]

Attachment: Re-Add indexes when altering a column in "ANSI" ALTER COLUMN

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.

Comment 2

[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.

Comment 3

[Tomas Kopal]

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?

Comment 4

[Max Kanat-Alexander]

(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. :-)

Comment 5

[Tomas Kopal]

>  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 :-).

Comment 6

[Max Kanat-Alexander]

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

Comment 7

[Max Kanat-Alexander]

Attachment: Patch as checked-in

Here's the version that was checked-in with the checkin fixes.