DBIx::Class has the ability to load a scheme defined by a set of DBIC schema classes into an empty database. We can use this to gain the ability to use a single script to deploy to either MySQL or PostgreSQL and set the stage for using DBIx::Class::Schema::Versioned to manage updates.
WIP can be found on the following Git branch: http://git.librarypolice.com/?p=koha-galen.git;a=shortlog;h=refs/heads/pg/bug11390_db_deploy_script
I've been playing around with DBIx::Class::Schema... and looking at DBIx::Class::Schema::Versioned. 0) We probably want to add "our $VERSION" to Koha::Schema. The DBIx::Class::Schema::schema_version() method uses this variable when figuring out the "current version" for the database as represented by the DBIC classes. While you won't necessarily get any warnings, it can be used in Koha::Database::create_ddl_dir() (http://cpansearch.perl.org/src/RIBASUSHI/DBIx-Class-0.082810/lib/DBIx/Class/Storage/DBI.pm). Plus, I believe DBIx::Class::Schema::Versioned counts on this $VERSION variable to know the current version as well: A table called I<dbix_class_schema_versions> is created and maintained by the module. This is used to determine which version your database is currently at. Similarly the $VERSION in your DBIC schema class is used to determine the current DBIC schema version. (http://cpansearch.perl.org/src/RIBASUSHI/DBIx-Class-0.082810/lib/DBIx/Class/Schema/Versioned.pm) 1) Itemtypes and Statistics both use the "double" data type. It looks like the Schema::Loader recorded these as "double precision", which is fine if you're just doing a dump via Koha::Schema->create_ddl_dir($databases, $current_version, $directory). However, if you're trying to do a diff via Koha::Database->create_ddl_dir($databases, $current_version, $directory, $previous_version), you'll get fatal errors like so: ERROR (line 2890): Invalid statement: Was expecting comment, or use, or set, or drop, or create, or alter, or insert, or delimiter, or empty statement DBIx::Class::Storage::DBI::create_ddl_dir(): translate: Error with parser 'SQL::Translator::Parser::MySQL': no results at dbic_controller.pl line 6 If we can't generate diffs, then we can't use DBIx::Class::Schema::Versioned::upgrade(). 2) I'm running into a strange issue when I am finally able to run a SQL diff. So I tried dumping two different versions of the DBIC schema using "Koha::Database->create_ddl_dir($databases, $current_version, $directory)". I then used the CLI tool "sqlt-diff", and it worked perfectly. It only detected the actual difference between the two. However, if I tried "Koha::Database->create_ddl_dir($databases, $current_version, $directory, $previous_version", I got weird results. The reason was that I wasn't comparing the two dumps. Rather, create_ddl_dir() was using a previous dump and the current version as it is stored in DBIC. (I confirmed this when I emulated create_ddl_dir() but removed the possibility of using the current schema in favour of a current dump.) For some reason, SQL::Translator::Producer::MySQL acts quite differently when using the method SQL::Translator::Producer::MySQL->can('preprocess_schema') returns true. I haven't dug deep enough to figure out what is causing the problem... whether it's the SQL::Translator::Parser::MySQL or SQL::Translator::Parser::DBIx::Class or SQL::Translator::Producer::MySQL or... I don't know what. In any case, I get the following set of SQL statements: BEGIN; ALTER TABLE accountlines CHANGE COLUMN timestamp timestamp timestamp NOT NULL DEFAULT current_timestamp; ALTER TABLE accountoffsets CHANGE COLUMN timestamp timestamp timestamp NOT NULL DEFAULT current_timestamp; ALTER TABLE action_logs CHANGE COLUMN timestamp timestamp timestamp NOT NULL DEFAULT current_timestamp; ALTER TABLE aqbudgets CHANGE COLUMN timestamp timestamp timestamp NOT NULL DEFAULT current_timestamp; ALTER TABLE aqorders CHANGE COLUMN timestamp timestamp timestamp NOT NULL DEFAULT current_timestamp; ALTER TABLE aqorders_items CHANGE COLUMN timestamp timestamp timestamp NOT NULL DEFAULT current_timestamp; ALTER TABLE aqorders_transfers CHANGE COLUMN timestamp timestamp timestamp NOT NULL DEFAULT current_timestamp; ALTER TABLE biblio DROP INDEX biblionumber, CHANGE COLUMN timestamp timestamp timestamp NOT NULL DEFAULT current_timestamp; ALTER TABLE marc_subfield_structure CHANGE COLUMN seealso seealso text NULL; ALTER TABLE message_queue CHANGE COLUMN time_queued time_queued timestamp NOT NULL DEFAULT current_timestamp; ALTER TABLE messages CHANGE COLUMN message_date message_date timestamp NOT NULL DEFAULT current_timestamp; ALTER TABLE misc_files CHANGE COLUMN date_uploaded date_uploaded timestamp NOT NULL DEFAULT current_timestamp; ALTER TABLE patronimage DROP INDEX borrowernumber; [...there was more but you get the idea] -- I have a theory about why "marc_subfield_structure" is there. I think it's because it ACTUALLY has a VARCHAR datatype and a size of 1100 characters in the DBIC schema. According to MySQL's documentation "Values in VARCHAR columns are variable-length strings. The length can be specified as a value from 0 to 255 before MySQL 5.0.3, and 0 to 65,535 in 5.0.3 and later versions." If you go to the source for SQL::Translator::Producer::MySQL, you can find the following lines: elsif ( $data_type =~ /char/i && $size[0] > 255 ) { unless ($size[0] <= 65535 && $mysql_version >= 5.000003 ) { $data_type = 'text'; @size = (); } } We can see that the data_type has been changed to 'text' from 'varchar', so it's clear that $mysql_version isn't being passed in (correctly). -- Unfortunately, I didn't notice anything in SQL::Translator::Producer::MySQL to signify why all those "timestamp" columns are being changed. -- I think I've sort of figured out the dropped indexes as well. The dropped index for every column happens where the index is for a field that is A) the primary key, and B) also a foreign key (in the DBIC schema). It looks like "X Module" automatically creates an index for every foreign key, but it runs into a problem when the foreign key is also the primary key. The SQL dump will still create some SQL for it like so: "INDEX (`borrowernumber`)," which is in stark contrast to how DBIC creates indexes normally: "INDEX `patroncards_idx_borrowernumber` (`borrowernumber`)," I think technically the `patroncards_idx_borrowernumber` name is optional in MySQL, but I think this is what's causing the strange result in the SQL diff. 3) Since the SQL being dumped from DBIC isn't the same SQL that we currently have in kohastructure.sql, I think that we'll want to be careful before using Koha::Database::deploy(); 4) I've tried doing SQL diffs between DBIC dumps and kohastructure.sql. However, it looks like there are characters in kohastructure.sql that are causing problems for SQL::Translator::Diff. I think it croaked whenver it got to "CREATE TABLE `borrowers`". I think I was able to do a trace and it thought it was written as "\nCREATE TABLE `borrowers`", which... I couldn't see in my text editor... but that will probably be a problem. 5) I'm to do some more poking around, but these are some of the things I've encountered over the past 24 hours.
I did think about this... deploying a DBIC schema to a database and doing a MySQL dump to get the structure of the deployed database. Probably also do a MySQL dump of a database populated using kohastructure.sql. Then those two dumps can be compared using regular diff tools or SQL::Translator::Diff. At the very least, it would be nice to start using Koha::Database::deploy() rather than kohastructure.sql...
(In reply to David Cook from comment #3) > I did think about this... deploying a DBIC schema to a database and doing a > MySQL dump to get the structure of the deployed database. > > Probably also do a MySQL dump of a database populated using > kohastructure.sql. > > Then those two dumps can be compared using regular diff tools or > SQL::Translator::Diff. > > At the very least, it would be nice to start using Koha::Database::deploy() > rather than kohastructure.sql... To that end, how might we factor in data that we want to import into the database? We could additionally run our other SQL as we currently do (for system preferences and the like), although then I wonder what we'll do for upgrades when we're not using upgradedatabase.pl. I suppose we could either manually add to the SQL diffs (release managers responsibility) or have versioned upgrade scripts or...
I've solved the "ALTER TABLE marc_subfield_structure CHANGE COLUMN seealso seealso text NULL;" issue. It turns out that you need to pass your MySQL version manually through the $sqltargs in create_ddl_dir() so that it can pass through SQL::Translator to SQL::Translator::Producer::MySQL. I did this with the following line within the $sqltarg hashref: producer_args => {mysql_version => '5.5.33'} Of course, I still have the issues with the "timestamp" columns and the dropped indexes. I tried removing the foreign key from `patronimage` but that just caused even more weirdness... Anyway... perhaps I'll keep searching tomorrow...
Muahaha. I've solved the "timestamp" issue. I ran the following SQL: SELECT * FROM information_schema.`COLUMNS` where TABLE_SCHEMA = 'kohadev' and DATA_TYPE = 'timestamp'; I got back 43 columns, but I could only count 41 in the SQL diff. So I isolated the two that didn't appear... 1) BorrowerDebarment->updated 2) Accountline->timestamp The reason? BorrowerDebarment->updated didn't have a default_value. I had a feeling yesterday that it was going to be a problem with the default_value of \"current_timestamp". Well, I looked at Accountline->timestamp, and it did have a default_value... of "\CURRENT_TIMESTAMP". Sure enough, when I changed the default_value on a few other Result classes, they no longer showed up as spurious diffs in the SQL diff! I haven't isolated which module is responsible for this case sensitivity issue (I would say bug), but the workaround for us is just to use uppercase SQL in the default_value. -- I've investigated DBIx::Class a bit and SQL::Abstract, and while SQL::Abstract does do some things with case sensitivity... I don't think it's an issue in this case. I think the DBIC schema isn't having anything done to its case. This is reinforced by the fact that the spurious diffs use lowercase "current_timestamp". I'm thinking... when it's parsing the past DBIC dump (rather than the current DBIC schema) that it's uppercasing "current_timestamp" to "CURRENT_TIMESTAMP". Of course, the time is now 5:17pm on a Friday, so I'm not going to investigate this any further until next week. OK, I lied. Check out http://cpansearch.perl.org/src/ILMARI/SQL-Translator-0.11020/lib/SQL/Translator/Parser/MySQL.pm and search the page a bit for all instances of "current_timestamp". As I suspected, the MySQL Parser is uppercasing current_timestamp and now() to CURRENT_TIMESTAMP and CURRENT_TIMESTAMP when used as a default_value. Ergo... the only thing we can do is uppercase our literal SQL... It appears that SQL::Translator::Diff takes a "case_insensitive" option. That it passes it to the SQL::Translator::Schema::* objects... SQL::Translator::Schema::Table->get_field($field, "case_insensitive"); However, that appears to only treat field names as case_insensitive... which probably makes sense. So yeah... guess we'll be uppercasing our default_value if it's literal SQL, if we want to create SQL diffs using DBIx::Class::Schema.
So I've solved the issue of the timestamp, the varchar being turned into text... I think I know why the "DROP INDEXES" are in there... but I'm not sure how to fix that except by giving those tables proper primary keys (which aren't foreign keys to other tables). What else... Oh! I tried dropping a foreign key just to see what would happen, and it got nasty. 1) It dropped the foreign key. Ok. 2) It dropped the index for the foreign key. Ok. 3) It tacked on an additional "Alter table" to the SQL statement rendering the entire statement invalid. NOT OK :(. This can be mitigated by using the "no_batch_alters" option, as it will separate those 3 things into separate statements. So the foreign key gets dropped correctly. The index gets dropped correctly. The final alter table... is just completely ineffectual. So that works... but it's sub-optimal... -- By the way, the whole dropping of an index where the primary key is also a foreign key... will generate errors because no index exists for it in the database...
So I finally am running some diffs between clean installs from the "web installer" and the "dbic deploy". There are a lot of differences... ( DBRev 3.17.00.055 && a couple tweaks to DBIC files) 1) 'ON UPDATE CURRENT_TIMESTAMP' is missing from many timestamps I think this can be provided as an "extra" in a DBIC Result class though. AFFECTED: `accountlines` `action_logs` `aqbudgets` `aqorders` `aqorders_items` `biblio` `biblioitems` `borrower_debarments` `borrower_modifications` `course_items` `course_reserves` `courses` `creator_batches` `currency` `deletedbiblio` `deletedbiblioitems` `deleteditems` `issues` `items` `old_issues` `old_reserves` `patroncards` `reserveconstraints` `reserves` `suggestions` `virtualshelfcontents` `virtualshelves` --- 2) All int(egers) default to int(11), all tinyint to tinyint(4), all smallint to smallint(6). DBIx::Class::Schema::Loader doesn't appear to load the "width" or "range" for integers. So while kohastructure.sql and the web installer dump say `notify_level` int(2), a mysql dump of a DBIC deploy says `notify_level` int(11). This doesn't seem solvable... except by changing all exciting integers to have a width of 11 and enforcing width/size limits through code. http://cpansearch.perl.org/src/ILMARI/SQL-Translator-0.11020/lib/SQL/Translator/Parser/MySQL.pm sub normalize_field() AFFECTED: `accountlines` int(11) `aqbasketgroups` tinyint(4) `aqbudgetperiods` tinyint(4) x2 `aqbudgets` int(11) `aqbudgets_planning` tinyint(4) `aqcontacts` tinyint(4) x4 `aqorders` tinyint(4) `auth_subfield_structure` tiny(4) x4 `biblio` tinyint(4) `biblioitems` int(11) `borrower_attribute_types` tinyint(4) x6 `borrower_message_preferences` tinyint(4) `borrower_modifications` tinyint(4) x2 `borrower_sync` tinyint(4) `borrowers` tinyint(4) x2 `branch_borrower_circ_rules` int(11) `branch_item_rules` tinyint(4) `branch_transfer_limits` int(11) `branchcategories` tinyint(4) `categories` tinyint(4) x5 `columns_settings` int(11) x2 `creator_batches` int(11) `creator_images` int(11) `creator_layouts` int(11) x5 `creator_templates` int(11) x4 `currency` tinyint(4) `default_borrower_circ_rules` int(11) `default_branch_circ_rules` int(11), tinyint(4) `default_branch_item_rules` tinyint(4) `default_circ_rules` int(11) `deletedbiblio` tinyint(4) `deletedbiblioitems` int(11) `deletedborrowers` tinyint(4) x2 `deleteditems` tinyint(4) x6 `import_biblios` tinyint(4) `issues` tinyint(4), int(11) `issuingrules` tinyint(4) x2, int(11) x4 `items` tinyint(4) x6 `letter` tinyint(4) `marc_modification_template_actions` int(11), tinyint(4) `marc_subfield_structure` tinyint(4) x3, int(11) `matchpoint_components` int(11) x2 `message_attributes` tinyint(4) `message_transports` tinyint(4) `notifys` int(11) `old_issues` int(11), tinyint(4) `old_reserves` tinyint(4) x2 `overduerules`int(11) x4 `overduerules_transport_types` int(11) `printers_profile` int(11) x2 `ratings` tinyint(4) `reserves` int(4) x2 `saved_sql` tinyint(4) `special_holidays` smallint(6) `subscription` tinyint(4) x3, int(11) `suggestions` int(11), smallint(6) `tags_all` int(11) `tags_approval` int(11) x2 `tags_index` int(11) `transport_cost` tinyint(4) `virtualshelves` tinyint(4) x3 --- 3) Indexes are missing For `accountlines`, the indexes for `borrowernumber` and `itemnumber` appear, but only because they are also foreign keys. There is no index for `timestamp` when using DBIC, but there is when using the Web Installer. AFFECTED: `accountlines` `action_logs` `alert` `aqorders` `auth_header` `auth_subfield_structure` `authorised_values` `biblio` `biblioitems` `borrower_attribute_types` `borrower_modifications` `borrowers` `branchcategories` `categories` `class_sort_rules` `class_sources` `creator_templates` `deletedbiblio` `deletedbiblioitems` `deletedborrowers` `deleteditems` `import_auths` `import_batches` `import_biblios` `import_items` `import_records` `issues` `issuingrules` `item_circulation_alert_preferences` (although the web installer index looks really weird anyway) `items` `itemtypes` `language_descriptions` `language_rfc4646_to_iso639` `language_script_bidi` `language_script_mapping` `language_subtag_registry` `linktracker` `marc_matchers` `marc_subfield_structure` `misc_files` `old_issues` `old_reserves` `reports_dictionary` `reserves` `saved_sql` `search_history` `statistics` `subscriptionhistory` `suggestions` `zebraqueue` --- 4) Extra indexes appear in DBIC Because foreign keys are always indexed by default, there are sometimes extra indexes when using DBIC. AFFECTED: `aqbasketusers` `aqbudgetborrowers` `aqorders_transfers` `auth_tag_structure` `borrower_message_transport_preferences` `branch_borrower_circ_rules` `branch_item_rules` `course_instructors` `course_items` `default_borrower_circ_rules` `default_branch_circ_rules` `default_branch_item_rules` `hold_fill_targets` `oai_sets_biblios` `patronimage` `ratings` `serialitems` `subscriptionroutinglist` `tags_index` `transport_cost` --- 5) Index names && foreign key names are different _Indices_ Web installer: `acctsborridx` DBIC: `accountlines_idx_borrowernumber` _Foreign keys_ Web Installer: `accountlines_ibfk_1` DBIC: `accoutlines_fk_borrowernumber` Honestly, I prefer the DBIC style, but I think this will cause problems. Not sure if this is solvable... AFFECTED: `accountlines` `aqbasket` `aqbasketgroups` `aqbasketusers` `aqbooksellers` `aqbudgetborrowers` `aqbudgets_planning` `aqcontacts` `aqcontract` `aqinvoices` `aqorders` `aqorders_items` `aqorders_transfers` `auth_tag_structure` `authorised_values_branches` `biblioimages` `biblioitems` `borrower_attribute_types_branches` `borrower_attributes` `borrower_debarments` `borrower_files` `borrower_message_preferences` `borrower_message_transport_preferences` `borrower_sync` `borrowers` `branch_borrower_circ_rules` `branch_item_rules` `branchrelations` `branchtransfers` `categories_branches` `class_sources` `collections` `course_instructors` `course_items` `course_reserves` `creator_batches` `default_borrower_circ_rules` `default_branch_circ_rules` `default_branch_item_rules` `hold_fill_targets` `import_auths` `import_biblios` `import_items` `import_record_matches` `import_records` `issues` `letter` `marc_modification_template_actions` `matchchecks` `matcher_matchpoints` `matchpoint_component_norms` `matchpoint_components` `matchpoints` `message_transports` `oai_sets_biblios` `oai_sets_descriptions` `oai_sets_mappings` `old_issues` `old_reserves` `opac_news` `overduerules_transport_types` `patron_list_patrons` `patron_lists` `patroncards` `patronimage` `permissions` `ratings` `reserves` `reviews` `serialitems` `subscription` `subscriptionroutinglist` `tags_all` `tags_approval` `tags_index` `transport_cost` `user_permissions` `virtualshelfcontents` `virtualshelfshares` `virtualshelves` --- 6) DBIC doesn't include COMMENTs Honestly, I don't see this mattering at all... AFFECTED: `collections_tracking` `export_format` `search_history` --- 7) (UTF8) CHARACTER SET and COLLATE do not appear in DBIC AFFECTED: `creator_layouts` `marc_subfield_structure` --- 8) DBIC has extra foreign keys AFFECTED: `issues` `items` `biblio` --- 9) DBIC has extra tables that aren't in kohastructure.sql... AFFECTED: _DBIC_ `closure` `closure_rrule` --- 10) DBIC wasn't able to create tables DBD::mysql::db do failed: Can't create table 'kohadev38.items_search_fields' (errno: 150) at /usr/lib/perl5/site_perl/5.16.2/DBIx/Class/Storage/DBI.pm line 3053. DBD::mysql::db do failed: Incorrect table definition; there can be only one auto column and it must be defined as a key at /usr/lib/perl5/site_perl/5.16.2/DBIx/Class/Storage/DBI.pm line 3053 AFFECTED: `items_search_fields` `message_queue` --- 11) Default values... Missing a Default statement in DBIC... AFFECTED: `itemtypes` --- 12) Table type incorrect in DBIC AFFECTED: `pending_offline_operations` (should be MyISAM not InnoDB) ------ **) Double Precision Of course, I had already tampered with the cases of 'double' being written as 'double precision' in DBIC... since it wouldn't even parse 'double precision', but I'll mention the affected tables as we'll need to generate/write these correctly the first time... AFFECTED: `itemtypes` `statistics`
(In reply to David Cook from comment #8) > 10) DBIC wasn't able to create tables > > DBD::mysql::db do failed: Can't create table 'kohadev38.items_search_fields' > (errno: 150) at /usr/lib/perl5/site_perl/5.16.2/DBIx/Class/Storage/DBI.pm > line 3053. > DBD::mysql::db do failed: Incorrect table definition; there can be only one > auto column and it must be defined as a key at > /usr/lib/perl5/site_perl/5.16.2/DBIx/Class/Storage/DBI.pm line 3053 > > AFFECTED: > `items_search_fields` > `message_queue` > Muahahaha! Solved! 1) `items_search_fields` Foreign keys must reference indexed columns. In kohastructure.sql, `authorised_values`.`category` has an index. However, that index is missing when using DBIC, so we get an errno 150, when we try to add `items_search_fields` because the constraint can't be generated. The fix? Add an index to `authorised_values`.`category`. DONE! (http://dev.mysql.com/doc/refman/5.5/en/innodb-foreign-key-constraints.html) 2) `message_queue` This error is also because of a missing key. In kohastructure.sql, `message_queue`.`message_id` has an index. In theory, it should be a primary key, but it's not. It's just a regular key. Anyway, it's enough not to generate errors. However, with DBIC, that index is missing, so we get an error when trying to add `message_queue`. --- In summary, add indexes for `authorised_values`.`category` and `message_queue`.`message_id`, and both tables will be created properly.
I've summarized all the problems I've encountered on the wiki: http://wiki.koha-community.org/wiki/DB_schema_bugs#Koha_DBIx::Class_Problems I'll probably still post to here, but I find the wiki will be easier for keeping track of these issues on a more on-going basis...
Updated the wiki a lot... http://wiki.koha-community.org/wiki/DB_schema_bugs#Koha_DBIx::Class_Problems Found some solutions, but I think we need to stop using DBIx::Class::Schema::Loader as soon as we can... as it's never going to accurately load the MySQL database.
Hi there! Can you share the code you have been working on here as commits? I am not asking for anything "git bz apply":able, just something to read your thoughts from.
If you want to reproduce the issues I was having exactly, I suggest checking out commit 85c25c619f8684f99c80fe6cf6a5c4885e903b5a (Bug 9165: (Followup) Tidied code slightly), but it'll probably work on master too.
Created attachment 42503 [details] [review] Wind this back... these are experiments for using DBIC schema control
Created attachment 42504 [details] [review] Latest work
Closing this due to age and as it's unlikely it will ever happen