419581 - [Oracle] Enable ALTER COLUMN for Oracle

Status: RESOLVED FIXED

(Bugzilla :: Database, enhancement)

Description

[Xiaoou]

Attachment: v1

To enable ALTER COLUMN for Oracle.

Comment 1

[Max Kanat-Alexander]

Comment on attachment 305687 [details] [diff] [review]
v1

Wow, it really sucks that we have to copy all the logic into this function, but I guess I see why.

>+sub _get_alter_type_sql {
>+        $type =~ s/serial/integer/i;

  Is that actually necessary for Oracle? That was a PostgreSQL hack.

>+    if ( $old_def->{TYPE} =~ /LONG/i && $new_def->{TYPE} !~ /LONG/i) {

  Why not just match specifically against LONGTEXT? There are no other types with LONG in them.

>+        # LONG to VARCHAR is no allowed in Oracle, just a way to work around.

  This is very clever! :-)

>+        push(@statements, 
>+            "ALTER TABLE $table ADD ${column}_temp $type");  
>+        push(@statements, "UPDATE $table SET ${column}_temp = $column");

  Will that always work, or do you have to 

>+        push(@statements, "COMMIT");

  How do you know you're in a transaction? What if you're not? Won't the ALTER TABLE statements force a commit anyway?

>+    # If this column is changed to type TEXT/VARCHAR, we need to deal with
>+    # empty string.
>+    if ( $old_def->{TYPE} !~ /varchar|text/i 
>+            && $new_def->{TYPE} =~ /varchar|text/i 
>+            && $new_def->{NOTNULL} ) {

  Nit: Put the { on the next line, aligned with the "i" in "if".

>+sub get_rename_column_ddl {
>+    my ($self, $table, $old_name, $new_name) = @_;
>+        my $serial_sql =
>+                       "CREATE OR REPLACE TRIGGER ${table}_${new_name}_TR "
>+                     . " BEFORE INSERT ON ${table} "
>+                     . " FOR EACH ROW "
>+                     . " BEGIN "
>+                     . "   SELECT ${table}_${new_name}_SEQ.NEXTVAL "
>+                     . "   INTO :NEW.${new_name} FROM DUAL; "
>+                     . " END;";

  Um, don't we already have a function for that SQL? If we don't, we'll want to add one. We don't want that SQL twice in Bugzilla anywhere.

Comment 2

[Xiaoou]

Attachment: v2

> >+        push(@statements, 
> >+            "ALTER TABLE $table ADD ${column}_temp $type");  
> >+        push(@statements, "UPDATE $table SET ${column}_temp = $column");
>   Will that always work, or do you have to 
> >+        push(@statements, "COMMIT");
>   How do you know you're in a transaction? What if you're not? Won't the ALTER
> TABLE statements force a commit anyway?
Yes, ALTER TABLE statements force a commit, and rollback is not possible here. I add a "COMMIT" there just to make sure the UPDATE statement commit before dropping the column.
Anyway, according to ORA-22858, this is the way to  modify the column type to LOB.

> >+sub get_rename_column_ddl {
> >+    my ($self, $table, $old_name, $new_name) = @_;
> >+        my $serial_sql =
> >+                       "CREATE OR REPLACE TRIGGER ${table}_${new_name}_TR "
> >+                     . " BEFORE INSERT ON ${table} "
> >+                     . " FOR EACH ROW "
> >+                     . " BEGIN "
> >+                     . "   SELECT ${table}_${new_name}_SEQ.NEXTVAL "
> >+                     . "   INTO :NEW.${new_name} FROM DUAL; "
> >+                     . " END;";
>   Um, don't we already have a function for that SQL? If we don't, we'll want to
> add one. We don't want that SQL twice in Bugzilla anywhere.

Yes, we have a  _get_create_seq_ddl, but it is not what I need, this part of code do a rename sequence stuff.

Comment 3

[Max Kanat-Alexander]

(In reply to comment #2)
> Yes, ALTER TABLE statements force a commit, and rollback is not possible here.
> I add a "COMMIT" there just to make sure the UPDATE statement commit before
> dropping the column.

  Ahh, okay. But how do you know you're in a transaction? What if you're not?

> Yes, we have a  _get_create_seq_ddl, but it is not what I need, this part of
> code do a rename sequence stuff.

  But it all uses new_name, so it doesn't look like it's any different from the normal code...would it be possible to just add a parameter to _get_create_seq_ddl to generate the right code here?

Comment 4

[Xiaoou]

(In reply to comment #3)
>   Ahh, okay. But how do you know you're in a transaction? What if you're not?
This is not in a transaction, because ALTER TABLE always forces a commit.

Comment 5

[Xiaoou]

Attachment: v3

To allow VARCHAR to LONG

Comment 6

[Max Kanat-Alexander]

Comment on attachment 319240 [details] [diff] [review]
v3

Okay. I'm unhappy that we had to copy so much of DB::Schema into this file, but I tested this and it seems to work fine.

Comment 7

[Max Kanat-Alexander]

  Note that all I tested was upgrading from 3.1.2+ to 3.1.4+--so if something is broken that's not here, we'll have to deal with that later.

Checking in Bugzilla/DB/Schema/Oracle.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema/Oracle.pm,v  <--  Oracle.pm
new revision: 1.4; previous revision: 1.3
done