Description
Kyle M Hall (khall)
2015-03-04 09:08:35 UTC
Created attachment 36441 [details] [review] Bug 13790 - Add unique id issue_id to issues and old_issues tables Test Plan: 1) Use a database with existing issues and old issues 2) Apply this patch 3) Run updatedatabase 4) Inspect the database a) old_issues should now have a column issue_id starting with 1 b) issues should now have a column issue_id starting with the number of rows in the old_issues table plus one Created attachment 36442 [details] [review] Bug 13790 - Update Koha Schema Hi Kyle, Olli has another patch for this on bug 13743, also this doesn't seem to take into account the changes we made after testing on bug 9303. I am wondering about the auto_increment on old_issues and the renumbering of the old entries especially. Could you try and figure this out together? Created attachment 37108 [details] [review] Bug 13790 - Add unique id issue_id to issues and old_issues tables Test Plan: 1) Use a database with existing issues and old issues 2) Apply this patch 3) Run updatedatabase 4) Inspect the database a) old_issues should now have a column issue_id starting with 1 b) issues should now have a column issue_id starting with the number of rows in the old_issues table plus one 5) Perform a checkout, note it is written to the database 6) Check in the checked out item, note it is moved to the old_issues table Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 37109 [details] [review] Bug 13790 - Update Koha Schema Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> *** Bug 13743 has been marked as a duplicate of this bug. *** Created attachment 37709 [details] [review] [SIGNED OFF] Bug 13790 - Add unique id issue_id to issues and old_issues tables Test Plan: 1) Use a database with existing issues and old issues 2) Apply this patch 3) Run updatedatabase 4) Inspect the database a) old_issues should now have a column issue_id starting with 1 b) issues should now have a column issue_id starting with the number of rows in the old_issues table plus one 5) Perform a checkout, note it is written to the database 6) Check in the checked out item, note it is moved to the old_issues table Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Nick Clemens <nick@quecheelibrary.org> Created attachment 37710 [details] [review] [SIGNED OFF] Bug 13790 - Update Koha Schema Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Nick Clemens <nick@quecheelibrary.org> I cleared a simple conflict in updatedatabase (or attempted at least) and tested and all worked as expected. Hope it was okay to clear and sign off in one step. QA: Looking here now.. Created attachment 38073 [details] [review] Bug 13790 - Update Koha Schema Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Nick Clemens <nick@quecheelibrary.org> That was just a trivial rebase. QA Comment: Being somewhat strict now :) I have some hesitations about your db revision: It makes Koha::Database appear now in mysql/updatedatabase.pl. Well, it must be introduced one day. That is true, but maybe it needs a little bit more thought? The DBIx project is not moving so much these days.. The $schema should not be in a far-away if statement btw. Secondly, you combine the old way with $dbh from C4 with some DBIx statements. That does not look good to me. And what about these lines: + my $max_issue_id = $schema->resultset('Issue')->get_column('issue_id')->max(); + $max_issue_id ||= $schema->resultset('OldIssue')->get_column('issue_id')->max(); + $max_issue_id ||= 0; + $max_issue_id++; + $dbh->do(qq{ + ALTER TABLE issues AUTO_INCREMENT = $max_issue_id} That is somewhat strange. It will work, but you should just pick max from issues. If there are no records, you are ready. Otherwise +1. All by all, I recommend adjusting your patch. Created attachment 38081 [details] [review] Bug 13790 [QA Followup] Created attachment 38082 [details] [review] Bug 13790 [QA Followup] Created attachment 38155 [details] [review] Bug 13790 - Add unique id issue_id to issues and old_issues tables Test Plan: 1) Use a database with existing issues and old issues 2) Apply this patch 3) Run updatedatabase 4) Inspect the database a) old_issues should now have a column issue_id starting with 1 b) issues should now have a column issue_id starting with the number of rows in the old_issues table plus one 5) Perform a checkout, note it is written to the database 6) Check in the checked out item, note it is moved to the old_issues table Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Nick Clemens <nick@quecheelibrary.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 38156 [details] [review] Bug 13790 - Update Koha Schema Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Nick Clemens <nick@quecheelibrary.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 38157 [details] [review] Bug 13790 [QA Followup] Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 38158 [details] [review] Bug 13790: [QA Follow-up] Relocation of db revision in updatedatabase The db rev lines should not be under the bug 13068 sandbox section. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Thinking about Katrin's comment on Bug 13743 regarding the AUTO_INCREMENT of old_issues. Isnt it a bit dangerous to have the old_issues.issue_id as AUTO_INCREMENT? This feature works as well without it, and prevents accidentally adding old_issues from hypothetical other sources without a issue_id known. When issues.issue_id and old_issues.issue_id get out of sync, that would cause serious and hard-to-see issues when old_issues-table is not populated on check-ins. Nobody should touch the old_issues -table from anywhere but the C4::Circulation::AddReturn(), but dropping the AUTO_INCREMENT will make it a bit harder. I don't think this issue is a blocker, but I would feel better. BTW. Separating issues and old_issues is really awkward, but I guess we need to just live with it. Thinking from a statistical point of view. In the kohastructure.sql the old_issues is not auto increment (first patch), but in the database update it seems to be (could only look at the patches, not apply them). Can someone confirm? I am also in favor of not auto_incrementing old_issues and being consistent! Kyle: Please adjust ! (In reply to Katrin Fischer from comment #21) > In the kohastructure.sql the old_issues is not auto increment (first patch), > but in the database update it seems to be (could only look at the patches, > not apply them). Can someone confirm? > > I am also in favor of not auto_incrementing old_issues and being consistent! Agreed, old_issues should no autoincrement! I'll take a look! Created attachment 38182 [details] [review] Bug 13790 [QA Followup] - Disable auto increment on old_issues after pk values are created Created attachment 38184 [details] [review] Bug 13790 [QA Followup] - Disable auto increment on old_issues after pk values are created Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 38349 [details] [review] Bug 13790 - Add unique id issue_id to issues and old_issues tables Test Plan: 1) Use a database with existing issues and old issues 2) Apply this patch 3) Run updatedatabase 4) Inspect the database a) old_issues should now have a column issue_id starting with 1 b) issues should now have a column issue_id starting with the number of rows in the old_issues table plus one 5) Perform a checkout, note it is written to the database 6) Check in the checked out item, note it is moved to the old_issues table Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Nick Clemens <nick@quecheelibrary.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 38350 [details] [review] Bug 13790 - Add unique id issue_id to issues and old_issues tables Test Plan: 1) Use a database with existing issues and old issues 2) Apply this patch 3) Run updatedatabase 4) Inspect the database a) old_issues should now have a column issue_id starting with 1 b) issues should now have a column issue_id starting with the number of rows in the old_issues table plus one 5) Perform a checkout, note it is written to the database 6) Check in the checked out item, note it is moved to the old_issues table Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Nick Clemens <nick@quecheelibrary.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 38351 [details] [review] Bug 13790 - Update Koha Schema Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Nick Clemens <nick@quecheelibrary.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 38352 [details] [review] Bug 13790 [QA Followup] Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 38353 [details] [review] Bug 13790: [QA Follow-up] Relocation of db revision in updatedatabase The db rev lines should not be under the bug 13068 sandbox section. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 38354 [details] [review] Bug 13790 [QA Followup] - Disable auto increment on old_issues after pk values are created Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 38429 [details] [review] The script we use to update the schema didn't like the checksum of the provided patch. So I obsolete it and regenerate properly. Patches pushed to master. Thanks Kyle! I have problem with this patch. When applied to our database it generates following errors: [Mon Apr 27 12:33:40 2015] updatedatabase.pl: DBD::mysql::db do failed: Multiple primary key defined at /srv/koha/installer/data/mysql/updatedatabase.pl line 10166. [Mon Apr 27 12:33:40 2015] updatedatabase.pl: DBD::mysql::db do failed: Unknown column 'issue_id' in 'order clause' at /srv/koha/installer/data/mysql/updatedatabase.pl line 10170. [Mon Apr 27 12:33:40 2015] updatedatabase.pl: DBD::mysql::st execute failed: Unknown column 'me.issue_id' in 'field list' at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1481. DBIx::Class::ResultSetColumn::max(): Unknown column 'me.issue_id' in 'field list' at /srv/koha/installer/data/mysql/updatedatabase.pl line 10174 It seems that ALTER TABLE issues ADD issue_id INT( 11 ) NOT NULL AUTO_INCREMENT PRIMARY KEY FIRST doesn't work because we already have itemnumber defined as primary key. Should we drop it before this alter table? (In reply to Dobrica Pavlinusic from comment #34) > doesn't work because we already have itemnumber defined as primary key. > Should we drop it before this alter table? Dobrica, you mean you altered the original DB definition? Same problem here. It seems that primary key was introduced by Bug 4002 (version 3.05.00.009 - d0c9592592c0bd6c529915a9fbc04476f9b52851), and it was removed *only from kohastructure.sql* by Bug 6716 (version 3.05.00.011 - 91b5a09c6cef398d9ad39ca1ced7d6569a87456b) So the problem will happen to those who started with Koha 3.4 or less. To make the updatedatabase process to finish correctly I had to run the following query: ALTER TABLE issues DROP PRIMARY KEY Hi Julian, thx for investigating. I think we need to fix this prior to release. I have filed a separate 'blocker' bug 14069. (In reply to Katrin Fischer from comment #39) > I have filed a separate 'blocker' bug 14069. Great job! |