329537 - [PostgreSQL] Bugzilla::DB::Pg can't alter a column to be SERIAL

Status: RESOLVED FIXED

(Bugzilla :: Database, defect)

Description

[Max Kanat-Alexander]

We have discovered in bug 322285 that DB::Pg can't change a column to a SERIAL type.

Comment 1

[Max Kanat-Alexander]

Okay, I give in, I think we can just require Pg 8.0 for our next release. There's too much hacking now to support a version that will be ancient by the time we release.

Comment 2

[Frédéric Buclin]

The error message pasted in bug 322285 was generated by Pg 8.0.7. I don't see how requiring Pg 8.0 would fix the problem.

Comment 3

[Max Kanat-Alexander]

(In reply to comment #2)
> The error message pasted in bug 322285 was generated by Pg 8.0.7. I don't see
> how requiring Pg 8.0 would fix the problem.

  It'll be easier to fix in DBD::Pg 8.

Comment 4

[Frédéric Buclin]

Maybe make it creates a temporary column with mediumserial format, remove the old column and rename this temporary column to the deleted name.

/me wants to make some progress this week on Flag.pm :(

Comment 5

[Frédéric Buclin]

(In reply to comment #4)
> /me wants to make some progress this week on Flag.pm :(

mkanat, ping? :(

Comment 6

[Max Kanat-Alexander]

Attachment: v1

Okay, this modifies Bugzilla to use the Pg-8 way of changing a column's type. It also adds some custom code to deal with the fact that the column is a SERIAL type.

Theoretically, we *maybe* could have done this on Pg 7, but it would have been hideously complex. Imagine all the crazy code we had before, *and* all the code in this patch.

I also removed Bugzilla::DB::Schema::Pg::get_add_column_ddl, because we don't need it anymore. Pg 8 does that properly for us with the code that we have.

Comment 7

[Frédéric Buclin]

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

>Index: Bugzilla/DB/Schema.pm

>@@ -1538,8 +1505,8 @@
>     # If we went from no default to a default, or we changed the default,
>     # or we have a default and we changed the data type of the field
>     elsif ( (defined $default && !defined $default_old) || 
>+            ($default ne $default_old) ) 
>+    {

The last line of the comment should be removed as you removed the corresponding check.


>@@ -1547,9 +1514,7 @@
>     # If we went from NULL to NOT NULL
>     # OR if we changed the type and we are NOT NULL
>+    if (!$old_def->{NOTNULL} && $new_def->{NOTNULL}) {

Same comment here.


>@@ -1572,8 +1537,7 @@
>     # If we went from not being a PRIMARY KEY to being a PRIMARY KEY,
>     # or if we changed types and we are a PK.
>+    if (!$old_def->{PRIMARYKEY} && $new_def->{PRIMARYKEY}) {

Same comment here.



>Index: Bugzilla/DB/Schema/Pg.pm

>+    if ($new_def->{TYPE} =~ /serial/i && $old_def->{TYPE} !~ /serial/i) {
>+        push(@statements, "CREATE SEQUENCE ${table}_${column}_seq");
>+        push(@statements, "ALTER TABLE $table ALTER COLUMN $column 
>+                           SET DEFAULT nextval('${table}_${column}_seq')");
>     }

Pg restarts the sequence from 1, meaning that it will fail when someone will try to INSERT something because there is already an entry with ID = 1. You should set the sequence to MAX($column) using: SELECT setval('${table}_${column}_seq', MAX($table.$column));

http://www.postgresql.org/docs/8.0/static/functions-sequence.html

Else your patch looks good.

Comment 8

[Frédéric Buclin]

Looks like you can even set the initial value when creating the sequence:

http://www.postgresql.org/docs/8.0/static/sql-createsequence.html

Comment 9

[Max Kanat-Alexander]

Attachment: v2

Okay, I fixed it.

Comment 10

[Frédéric Buclin]

Comment on attachment 218272 [details] [diff] [review]
v2

>Index: Bugzilla/DB/Schema/Pg.pm
>+    if ($new_def->{TYPE} =~ /serial/i && $old_def->{TYPE} !~ /serial/i) {
>+        push(@statements, "CREATE SEQUENCE ${table}_${column}_seq");
>+        push(@statements, "SELECT setval('${table}_${column}_seq',
>+                                         MAX($table.$column))");

Looks good. Works correctly, even when $table.$column is empty (I don't know what max() really returns, but the sequence correctly starts with 1 in this case). When we move from an auto-increment column to something else, maybe it would be good to remove the corresponding sequence. But that's another bug. r=LpSolit (yay!)

Comment 11

[Max Kanat-Alexander]

(In reply to comment #10)
> When we move from an auto-increment column to something else, maybe it
> would be good to remove the corresponding sequence. But that's another bug.

  Yeah. We actually can't, because if we were created as a SERIAL type then Pg won't let us delete the sequence without deleting the column first. I think it's a bug, but it still happens in Pg 8.0.7.

Comment 12

[Frédéric Buclin]

Checking in Bugzilla/DB/Pg.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Pg.pm,v  <--  Pg.pm
new revision: 1.19; previous revision: 1.18
done
Checking in Bugzilla/DB/Schema.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v  <--  Schema.pm
new revision: 1.48; previous revision: 1.47
done
Checking in Bugzilla/DB/Schema/Pg.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema/Pg.pm,v  <--  Pg.pm
new revision: 1.11; previous revision: 1.10
done