1320936 - Differences in the performance_alert_summary schema between environments

Status: RESOLVED FIXED

(Tree Management :: Perfherder, defect, P2)

Description

[Ed Morley [:emorley]]

--- vagrant.sql	2016-11-25 17:22:53.439965200 +0000
+++ prod.sql	2016-11-25 17:31:49.775087600 +0000
...
 --
 -- Table structure for table `performance_alert_summary`
 --
 
 DROP TABLE IF EXISTS `performance_alert_summary`;
 /*!40101 SET @saved_cs_client     = @@character_set_client */;
 /*!40101 SET character_set_client = utf8 */;
 CREATE TABLE `performance_alert_summary` (
   `id` int(11) NOT NULL AUTO_INCREMENT,
   `prev_result_set_id` int(10) unsigned DEFAULT NULL,
-  `result_set_id` int(10) unsigned DEFAULT NULL,
+  `result_set_id` int(11) DEFAULT NULL,
   `last_updated` datetime(6) NOT NULL,
   `status` int(11) NOT NULL,
   `bug_number` int(10) unsigned DEFAULT NULL,
   `framework_id` int(11) DEFAULT NULL,
   `repository_id` int(11) NOT NULL,
   `manually_created` tinyint(1) NOT NULL,
-  `prev_push_id` int(11) NOT NULL,
+  `prev_push_id` int(11) DEFAULT NULL,
   `push_id` int(11) NOT NULL,
   PRIMARY KEY (`id`),
+  UNIQUE KEY `<INDEX_NAME>` (`repository_id`,`framework_id`,`prev_result_set_id`,`result_set_id`),
   UNIQUE KEY `<INDEX_NAME>` (`repository_id`,`framework_id`,`prev_push_id`,`push_id`),
   KEY `<INDEX_NAME>` (`framework_id`),
   KEY `<INDEX_NAME>` (`last_updated`),
   KEY `<INDEX_NAME>` (`prev_push_id`),
   KEY `<INDEX_NAME>` (`push_id`),
-  KEY `<INDEX_NAME>` (`repository_id`),
   CONSTRAINT `<INDEX_NAME>` FOREIGN KEY (`framework_id`) REFERENCES `performance_framework` (`id`),
   CONSTRAINT `<INDEX_NAME>` FOREIGN KEY (`repository_id`) REFERENCES `repository` (`id`),
   CONSTRAINT `<INDEX_NAME>` FOREIGN KEY (`prev_push_id`) REFERENCES `push` (`id`),
   CONSTRAINT `<INDEX_NAME>` FOREIGN KEY (`push_id`) REFERENCES `push` (`id`)
 ) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_bin;
 /*!40101 SET character_set_client = @saved_cs_client */;

Comment 1

[GitHub Autolander Bot]

Attachment: [treeherder] wlach:1320936 > mozilla:master

Comment 2

[William Lachance (:wlach)]

Comment on attachment 8815452 [details] [review]
[treeherder] wlach:1320936 > mozilla:master

We can fix the result_set_id problems just by dropping the columns from the db (they aren't used anymore).

We should not allow null values for the prev_push_id value (i.e. the vagrant configuration is correct), but we need to make sure we don't generate such summaries first.

Comment 3

[Robert Wood [:rwood]]

I know the change to the actual model to disallow null values in prev_push_id isn't in this patch (just ensuring alert summaries with null prev_push_ids aren't generated first). Although question, where is the actual prev_push_id field specified in the model, I can't seem to find it...

Comment 4

[William Lachance (:wlach)]

(In reply to Robert Wood [:rwood] from comment #3)
> I know the change to the actual model to disallow null values in
> prev_push_id isn't in this patch (just ensuring alert summaries with null
> prev_push_ids aren't generated first). Although question, where is the
> actual prev_push_id field specified in the model, I can't seem to find it...

We specify a reference to the ORM class "Push" as "prev_push" in the model, but Django is smart enough to know what you mean when you specify "prev_push_id" when creating an object (sometimes you don't want to get a whole ORM object when you already know the integer id that references it).

Comment 5

[Robert Wood [:rwood]]

Comment on attachment 8815452 [details] [review]
[treeherder] wlach:1320936 > mozilla:master

Ah that's pretty cool, ok thanks Will and the patch LGTM btw.

Comment 6

[Treeherder GitHub Bugbot]

Commits pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/1df5b7957d61dd524910989635caf9a7b96e5014
Bug 1320936 - Drop result_set_id columns from alert summary table

https://github.com/mozilla/treeherder/commit/32ca04ef414f43e02a40106341020c1b8644dbce
Bug 1320936 - Disallow performance alert summaries with null previous pushes

This is an indication that we are creating a regression alert without much
previous data, which is a red flag.

Comment 7

[Ed Morley [:emorley]]

Updated diff:

--- schema-vagrant.sql
+++ schema-prod.sql
...
 CREATE TABLE `performance_alert_summary` (
   `id` int(11) NOT NULL AUTO_INCREMENT,
   `manually_created` tinyint(1) NOT NULL,
   `last_updated` datetime(6) NOT NULL,
   `status` int(11) NOT NULL,
   `bug_number` int(10) unsigned DEFAULT NULL,
   `framework_id` int(11) NOT NULL,
-  `prev_push_id` int(11) NOT NULL,
+  `prev_push_id` int(11) DEFAULT NULL,
   `push_id` int(11) NOT NULL,
   `repository_id` int(11) NOT NULL,
   PRIMARY KEY (`id`),
   UNIQUE KEY `<NAME>` (`repository_id`,`framework_id`,`prev_push_id`,`push_id`),
   KEY `<NAME>` (`last_updated`),
   KEY `<NAME>` (`framework_id`),
   KEY `<NAME>` (`prev_push_id`),
   KEY `<NAME>` (`push_id`),
   CONSTRAINT `<NAME>` FOREIGN KEY (`framework_id`) REFERENCES `performance_framework` (`id`),
   CONSTRAINT `<NAME>` FOREIGN KEY (`repository_id`) REFERENCES `repository` (`id`),
   CONSTRAINT `<NAME>` FOREIGN KEY (`prev_push_id`) REFERENCES `push` (`id`),
   CONSTRAINT `<NAME>` FOREIGN KEY (`push_id`) REFERENCES `push` (`id`)
 ) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_bin;

Comment 8

[Ed Morley [:emorley]]

Fixed on dev/stage/prod using:

ALTER TABLE treeherder.performance_alert_summary MODIFY `prev_push_id` int(11) NOT NULL;

On stage only, I had to delete half a dozen or so `performance_alert_summary` rows that had a NULL `prev_push_id` along with rows that referenced them in `performance_alert`.