Cbioportal: DB update script fails on DBs seeded as documented

[fedde-s]

When running the database schema update script on any database initialised with the current (2016-07) or previous (2015-05) seed database, it does not update the schema to the version compatible with the current 1.3.x release, but instead leaves it in a partially updated state. This effectively makes the current release of cBioPortal uninstallable for anyone who is not a SQL developer.

Output of the update script:

% migrate_db.py -p $PORTAL_SRC/src/main/resources/portal.properties -s $PORTAL_SRC/core/src/main/resources/db/migration.sql
WARNING: This script will alter your database! Be sure to back up your data before running.
Continue running DB migration? (y/n) y
Running statements for version: 1.3.0
        Executing statement: DROP TABLE IF EXISTS `clinical_trial_keywords`;
        Executing statement: DROP TABLE IF EXISTS `clinical_trials`;
        Executing statement: ALTER TABLE `users` CONVERT TO CHARACTER SET utf8;
        Executing statement: ALTER TABLE `info` CONVERT TO CHARACTER SET utf8;
        Executing statement: ALTER TABLE `sample_profile` ADD COLUMN PANEL_ID int(11) DEFAULT NULL, ADD FOREIGN KEY (PANEL_ID) REFERENCES `gene_panel` (PANEL_ID) ON DELETE RESTRICT;
        Executing statement: ALTER TABLE `gene` MODIFY COLUMN `ENTREZ_GENE_ID` int(11) NOT NULL;
        Executing statement: ALTER TABLE `gene_alias` MODIFY COLUMN `ENTREZ_GENE_ID` int(11) NOT NULL;
        Executing statement: ALTER TABLE `uniprot_id_mapping` MODIFY COLUMN `ENTREZ_GENE_ID` int(11) NOT NULL;
        Executing statement: ALTER TABLE `genetic_alteration` MODIFY COLUMN `ENTREZ_GENE_ID` int(11) NOT NULL;
        Executing statement: ALTER TABLE `mutation_event` MODIFY COLUMN `ENTREZ_GENE_ID` int(11) NOT NULL;
        Executing statement: ALTER TABLE `mut_sig` MODIFY COLUMN `ENTREZ_GENE_ID` int(11) NOT NULL;
        Executing statement: ALTER TABLE `protein_array_target` MODIFY COLUMN `ENTREZ_GENE_ID` int(11) NOT NULL;
        Executing statement: ALTER TABLE `sanger_cancer_census` MODIFY COLUMN `ENTREZ_GENE_ID` int(11) NOT NULL;
        Executing statement: ALTER TABLE `gistic_to_gene` MODIFY COLUMN `ENTREZ_GENE_ID` int(11) NOT NULL;
        Executing statement: ALTER TABLE `cna_event` MODIFY COLUMN `ENTREZ_GENE_ID` int(11) NOT NULL;
        Executing statement: ALTER TABLE `cosmic_mutation` MODIFY COLUMN `ENTREZ_GENE_ID` int(11) NOT NULL;
        Executing statement: ALTER TABLE `mutation` MODIFY COLUMN `ENTREZ_GENE_ID` int(11) NOT NULL;
        Executing statement: ALTER TABLE `mutation` ADD KEY (`MUTATION_EVENT_ID`);
/usr/local/bin/migrate_db.py:195: Warning: Duplicate index 'MUTATION_EVENT_ID_2' defined on the table 'cbioportal.mutation'. This is deprecated and will be disallowed in a future release.
  cursor.execute(statement.strip())
        Executing statement: ALTER TABLE `sample_profile` ADD KEY (`SAMPLE_ID`);
/usr/local/bin/migrate_db.py:195: Warning: Duplicate index 'SAMPLE_ID_2' defined on the table 'cbioportal.sample_profile'. This is deprecated and will be disallowed in a future release.
  cursor.execute(statement.strip())
        Executing statement: CREATE TABLE gene_panel ( INTERNAL_ID int(11) NOT NULL auto_increment, STABLE_ID varchar(255) NOT NULL, DESCRIPTION mediumtext, PRIMARY KEY (INTERNAL_ID), UNIQUE (STABLE_ID) );
        Executing statement: CREATE TABLE gene_panel_list ( INTERNAL_ID int(11) NOT NULL, GENE_ID int(255) NOT NULL, PRIMARY KEY (INTERNAL_ID, GENE_ID), FOREIGN KEY (INTERNAL_ID) REFERENCES gene_panel (INTERNAL_ID) ON DELETE CASCADE, FOREIGN KEY (GENE_ID) REFERENCES gene (ENTREZ_GENE_ID) ON DELETE CASCADE );
(1215, 'Cannot add foreign key constraint')

[fedde-s]

@pieterlukasse, did you have a solution for this already?

[pieterlukasse]

@fedde-s the temporary workaround for now (if you have MyISAM) is to remove the foreign key constraints from the .sql statements and run them manually.

[fedde-s]

The cause of the issue might be that most tables, including gene, are defined in the seed database as using the MyISAM engine rather than the now-standard InnoDB one, and MyISAM does not support foreign keys. Currently the only table in the seed database that does not use MyISAM is info.

[pieterlukasse]

@sheridancbio @zheins what do you think? Maybe for now we need to provide 2 migration.sql scripts: one for MyISAM one for InnoDB. The migration.py script could take the engine as a parameter and then call the correct .sql script...

[fedde-s]

Or maybe add the lines

ALTER TABLE gene ENGINE = InnoDB;
ALTER TABLE cancer_study ENGINE = InnoDB;
ALTER TABLE sample ENGINE = InnoDB;
ALTER TABLE genetic_profile ENGINE = InnoDB;

to the 1.3.0 block? That's a no-op if the tables already use the InnoDB engine.

[pieterlukasse]

👍 if that works, than it could be a nice solution!

[fedde-s]

The output of the migration script here suggests that this might be a separate issue from #1799, as this error message only occurs on a line that adds a foreign key in an InnoDB table, referencing a MyISAM table. Adding a foreign key in a MyISAM table is documented to be a no-op, but adding a foreign key _referencing_ a MyISAM table might be an error.

[sheridancbio]

We should check this ... if an InnoDB table referencing a parent MyISAM table in a FOREIGN KEY constraint causes an error, maybe we can add tests into the migration.py script that catches this case dynamically and refuses to run under these conditions. The idea of auto-updating some tables to InnoDB in the migration step so that the DB has some tables under one engine and some under another feels unwise, and it is also mysql specific. Some deployers may not want to use a mysql backend at all, and we ourselves may want to migrate away from mysql.

Since knowing which engine is applied to each table is now becoming important, I think we should consider whether or not we should support arbitrary mixtures of engine use on the deployment platform. Perhaps we should think about what database configuration (engines / system) we want to support (both on the deployment platform and on the database importing/building platform). One option might be to use the foreign key constraints for all import operations, but to deploy the databases without the foreign key constraints (when going into production use, which is basically read-only)

[fedde-s]

Most people's databases already have a mix of engines as it is, since the seed database explicitly defines most tables as MyISAM but the info table as InnoDB, and the migration script adds new tables with the default storage engine, which since MySQL 5.5.5 (July 2010) has been InnoDB (unless overridden using a system variable).

[pieterlukasse]

Regarding: _"One option might be to use the foreign key constraints for all import operations, but to deploy the databases without the foreign key constraints (when going into production use, which is basically read-only)"_ : I think that foreign key constraints will be a performance bottleneck during _import_ process, but not for querying. So I would see no reason for dropping them at a read-only deployment.

[sheridancbio]

I think it is a good point that the overhead of the constraint checks come during update and not retrieval. So if databases are built with InnoDB (and they probably should be) there is probably little reason to switch to MyISAM for production (unless there are MyISAM features not present in InnoDB .. or if there is significant performance differences (even when using read-only operation))

Still .. I wonder if we should be mandating the engine to end users in such a mysql-specific manner. Some organizations may want to deploy under a different DBMS. If we _do_ want to specify InnoDB in the cgds and migration scripts, then I would favor making the transition to InnoDB for all tables (to avoid future confusion when creating new tables or new foreign keys)

[sheridancbio]

Note following up from slack comments: I lean towards recommending a full switchover to InnoDB engine as standard for all tables. First need to build a consistent full database for ourselves under InnoDB and measure any performance slowdown for queries.

Second Note: one possible explanation / fix for the failure of the migration script is the difference in data type for the foreign key linking gene_panel_list.gene_id (int(255)) to gene.entrez_gene_id (int(255)). Maybe by making gene_id match (int(11)) it would be able to create the key. (testing needed)

[mandawilson]

The unit tests require InnoDB so making it standard for all tables makes sense to me.

[fedde-s]

Do I read things right if I think that #1696 should have fixed this when it's released?

[morungos]

This isn't just an InnoDB issue (although that resolves some of it). I'm just attempting to use 1.3.2 master and seeing a problem in the same place (tables all InnoDB):

    Executing statement: ALTER TABLE `sample_profile` ADD COLUMN PANEL_ID int(11) DEFAULT NULL, ADD FOREIGN KEY (PANEL_ID) REFERENCES `gene_panel` (PANEL_ID) ON DELETE RESTRICT;
(1005, "Can't create table 'cbioportal.#sql-ad0_44' (errno: 150)")

The issue is simple: there is no PANEL_ID column in the gene_panel table.

Maybe this should be REFERENCES gene_panel(INTERNAL_ID)?

[morungos]

Follow up on this, the current import process is extremely twitchy about server collations because the seed file wires in a charset, and collations appear to be completed by magic and not from the database. I am sure if you have everything set right it is all smooth, but it isn't if you don't.

I'd recommend tweaking the dump seed file so it doesn't set:

• the engine
• the character set
• the collation (it doesn't do this already)

This is because the migration environment also doesn't contain these settings, and the two need to be consistent. Otherwise, if a server/DB is set to, e.g., latin1_general_ci, somehow the seed ends up in latin1_swedish_ci, and then the foreign keys break during the migration.

Generally, the fewer settings wired into the seed data, the easier it will be to deploy.

[pieterlukasse]

How much of this is now solved by #1696?

[morungos]

It's hard to say. I don't know why the seed was ending up in latin1_swedish_ci but it was using this basic framework. I'm wondering whether there was a default that differed between the python migration and the mysql data loading.

The ALTER TABLE sample_profile issue is still present in 1.4.0

[sheridancbio]

we are in the process now of transitioning to an all InnoDB production database. After that is done we will put into place the working assumption that all tables will be InnoDB and introduce a migration.sql version step with @fedde-s 's suggestion. We are shooting for feb 15

[pieterlukasse]

@sheridancbio I just checked a recent version of rc and the statement @morungos identified here is still wrong in migration.sql:

ALTER TABLE `sample_profile` ADD COLUMN PANEL_ID int(11) DEFAULT NULL, ADD FOREIGN KEY (PANEL_ID) REFERENCES `gene_panel` (PANEL_ID) ON DELETE RESTRICT;

as @morungos stated, gene_panel does not have a field called PANEL_ID

This will cause migration to fail regardless of engine...

[sheridancbio]

Thanks @morungos ... migration.sql had a plain bug which was not caught .. presumably because the migration.sql script was not tested under the InnoDB engine and so the foreign key constraint was silently ignored. That issue was fixed in the hotfix branch in #2126, which should be reviewed shortly by @zheins

[sheridancbio]

2126 has been merged. The other factors to address here are to solve the issue of mixed engine types and the use / non-use of foreign key constraints. We are still working on this currently, and are leaning towards stripping references to engine type and character sets from the database creation and migration scripts and providing recommendations / guides / documentations in the installation pages --- basically saying that we expect installers to select an engine & character set configuration which makes sense for their environment and that they should take care to keep their database defaults set appropriately to avoid the creation of mixed table engine types or mixed character encodings. (the main suggested install platform will probably be all tables using InnoDB under mysql 5.7 with all text fields being utf8 encoded) cgds.sql and migration.sql will create appropriate schemas with minimal adjustments to system defaults. But we will probably not handle automated recovery of systems which have a mixture of engine type in use.

[sheridancbio]

The largest primary MSKCC database has been transitioned fully to InnoDB with utf8 encoded text fields and all foreign key constraints (and unique and other indexes) in place.

The schema of the production database is nearly identical to cgds.sql. The only semantic differences are:

• in order to create the foreign key relationship on the TYPE_OF_CANCER_ID field, the width of the field in different tables varied (some varchar(25) some varchar(63)) .. so we have expanded to varchar(63) for this field in tables: cancer_study and sample
• in order to allow a unique key to exist in mutation_event with utf8 encoded text fields, we have reduced the width of fields TUMOR_SEQ_ALLELE and REFERENCE_ALLELE to varchar(255). We are also considering how to better handle long indels, since there are cases of up to length 6000nt in our data sources. A new approach may be needed for very long indels.

There are also some new discrepancies between cgds.sql and migration.sql ... such as the "ON DELETE" clause for the foreign key to genetic_entity in the table gene ... in cgds.sql it cascades but in migration.sql it does not. Also, the position of the field GENETIC_ENTITY_ID in the gene table definition is not consistent, being located in the third position in cgds.sql but being added in the last position in migration.sql.

So some changes / fixes to the .sql scripts will come along with the dropping of hardcoded character sets or engine references. [PR to come]

[sheridancbio]

The production databases are all now transitioned to the latest schema, with the addition of these changes: #2242

The issue of the long field TUMOR_SEQ_ALLELE causing a problem with utf8 character set will be addressed in the future with a refactoring to allow proper handling of very long inserts/deletes (up to thousands of nucleotides). So for now, that part of the schema is unchanged in cgds.sql but the production databases (which use utf8 for all text fields) have reduced the width of that column to 255. For now, installers can either use a one-btye encoding for that column or can reduce the column width (manually).

In #2242 we also make the schema files independent/ignorant of character set choice or engine choice. Installers will need to choose and manage their own DEFAULT CHARACTER SET and ENGINE settings. Choosing MyISAM will cause FOREIGN KEYS to not be applied in the database, which will have implications for functionality (for example, DaoCancerStudy.deleteCancerStudy() has a new parallel function which relies on the cascade of deletion provided by foreign keys, but the original delete function must be used for MyISAM installations) A future PR will add instructions to the installation page detailing the choices and making recommendations.

[sheridancbio]

I believe that this issue has been substantially resolved. The actual bug should have been corrected by #2126 and we have altered the cgds.sql and migration.sql to be non-specific about character sets and storage engine. The final piece to resolving this issue is to update the installation documentation to explain the choices for installers [foreign key / engine / character set] and to provide recommendations / guidance for one or two default paths, maybe one for a "heavy" InnoDB / foreign keys install with utf8 encoded text fields, and another for a "light" MyISAM / latin1 install.

We have a task on our backlog to prepare such an update to the documentation.

[jjgao]

@sheridancbio Thanks! Could you create another issue for the final piece -- updating the installation documentation and then close this issue?