290089 - Move timestamp updates to happen before any other Schema updates

Status: RESOLVED FIXED

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

Description

[Max Kanat-Alexander]

The new checksetup-upgrade-by-Schema code involves some silent UPDATE statements
in order to make it work properly. (Yes, they are very much needed.)

This is fine, except for the fact that old Bugzillas have timestamp fields, so
these silent updates will change the delta_ts on various tables when they really
shouldn't.

It should actually be very straightforward to move the timestamp updates to
happen before any other Schema updates, by moving them into
Bugzilla::DB::Mysql::bz_setup_database.

Comment 1

[Max Kanat-Alexander]

Attachment: Move timestamp changes

OK, this is practically just a straight cut-and-paste. I did change the names
of the $fielddef variables, and I had to change all the $dbh vars to $self. But
those are the only changes in the moved code.

The rest of the patch is just a cleanup of "delta_ts = delta_ts", which no
longer needs to exist in checksetup.

Comment 2

[Tomas Kopal]

Comment on attachment 180520 [details] [diff] [review]
Move timestamp changes

>--- checksetup.pl	12 Apr 2005 01:30:44 -0000	1.390
>+++ checksetup.pl	12 Apr 2005 22:08:23 -0000

You missed one occurence at line 2177:

	    if ($table eq "bugs") {
		$extra = ", delta_ts = delta_ts";
	    }

Also, there are places where we are currently modifying the bugs table, but the
statement does not contain "delta_ts = delta_ts". I am pretty sure we should
not change the timestamp by these updates so it's more an old bug you are
fixing here, but it's a change in functionality so I would like you to look at
this and confirm that not adding "delta_ts = NOW()" is correct and intentional.

Other tables which had timestamp columns sometime in the past (attachments,
logincookies) does not seem to be affected.

>--- Bugzilla/DB/Mysql.pm	6 Apr 2005 00:18:04 -0000	1.8
>+++ Bugzilla/DB/Mysql.pm	12 Apr 2005 22:08:23 -0000
>@@ -371,6 +371,79 @@
>         } # foreach table
>     } # if old-name indexes
> 
>+    # The old timestample fields need to be adjusted here instead of in

Typo: timestample => timestamp

>+    # 2002-08-14 - [e-mail] - bug 153578
>+    # attachments creation time needs to be a datetime, not a timestamp
>+    my $attach_creation = 
>+        $self->bz_get_field_def("attachments", "creation_ts");
>+    if ($attach_creation->[1] =~ /^timestamp/) { # If type is timestamp

You changed the code here: are your sure the bz_get_field_def() can't return
undef?

>+    my $login_lastused = $self->bz_get_field_def("logincookies", "lastused");
>+    if ($login_lastused->[1] =~ /^timestamp/) {

Dtto.

>+    my $bugs_deltats = $self->bz_get_field_def("bugs", "delta_ts");
>+    if ($bugs_deltats->[1] =~ /^timestamp/) {

Dtto.

Otherwise, it looks pretty good and I will be happy to add my r+.

Comment 3

[Max Kanat-Alexander]

Attachment: v2

OK, you were right about the undef checks, and I also removed the other
delta_ts bit. :-) Anywhere else in checksetup that was missing a "delta_ts =
delta_ts" was a bug. :-)

Comment 4

[Tomas Kopal]

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

>+        while (my ($bug_id, $attach_id, $submitter_id) 
>+                  = $sth->fetchrow_array()) 
>+        {
>+            $sth2->execute($bug_id, $submitter_id, 
>+                "Created an attachment (id=$attach_id)%");
>+            my ($when) = $sth2->fetchrow_array();
>+            if ($when) {
>+                $self->do("UPDATE attachments " .
>+                             "SET creation_ts='$when' " .
>+                           "WHERE attach_id=$attach_id");

Hmmm, maybe this is a good time for a bit of cleanup too. This should be
probably prepare()d before the loop starts.
But that's minor optimization and I am happy if you ignore it.

Comment 5

[Max Kanat-Alexander]

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.393; previous revision: 1.392
done
Checking in Bugzilla/DB/Mysql.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Mysql.pm,v  <--  Mysql.pm
new revision: 1.9; previous revision: 1.8
done

Comment 6

[Max Kanat-Alexander]

OK, backed out the patch, it causes the test-checksetup tinderbox to burn.
Creating a new database is broken, because bz_get_field_def dies if you call it
on a table that doesn't exist. (Which is silly.)

DBD::mysql::st execute failed: Table 'bugs_checksetup.attachments' doesn't exist
[for Statement "SHOW COLUMNS FROM attachments"] at Bugzilla/DB.pm line 593
	Bugzilla::DB::bz_get_field_def('Bugzilla::DB::Mysql=HASH(0xa253944)','attachments','creation_ts')
called at Bugzilla/DB/Mysql.pm line 381
	Bugzilla::DB::Mysql::bz_setup_database('Bugzilla::DB::Mysql=HASH(0xa253944)')
called at ./checksetup.pl line 1579

Comment 7

[Max Kanat-Alexander]

Attachment: v3 (Also fix bz_get_field_def)

This is so silly, since I'm going to *remove* this function in a few days, but
I guess I have to fix it for now. :-)

Comment 8

[Max Kanat-Alexander]

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.395; previous revision: 1.394
done
Checking in Bugzilla/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v  <--  DB.pm
new revision: 1.44; previous revision: 1.43
done
Checking in Bugzilla/DB/Mysql.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Mysql.pm,v  <--  Mysql.pm
new revision: 1.11; previous revision: 1.10
done