Description
Jonathan Druart
2017-05-22 17:38:25 UTC
Created attachment 63629 [details] [review] Bug 18651: Update issue_id in AddReturn 1. AddReturn returns a $issue hashref with the old issue_id value => At first glance it does not affect anything, but would be good to fix it for future uses. Created attachment 63630 [details] [review] Bug 18651: Do not charge if the checkin failed 2. If the move fails for whatever reason (see https://lists.katipo.co.nz/pipermail/koha/2017-May/048045.html for an example), fines can be charged. It should not Created attachment 63631 [details] [review] Bug 18651: Update accountlines.issue_id is the issue_id has been changed during the move Created attachment 63632 [details] [review] DEBUG patches: I'd like feedback Check what the debug statement returns: there are 4 accountlines entries, is that correct? Created attachment 63633 [details] [review] Bug 18651: Update issue_id in AddReturn 1. AddReturn returns a $issue hashref with the old issue_id value => At first glance it does not affect anything, but would be good to fix it for future uses. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 63634 [details] [review] Bug 18651: Update issue_id in AddReturn 1. AddReturn returns a $issue hashref with the old issue_id value => At first glance it does not affect anything, but would be good to fix it for future uses. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 63635 [details] [review] Bug 18651: Do not charge if the checkin failed 2. If the move fails for whatever reason (see https://lists.katipo.co.nz/pipermail/koha/2017-May/048045.html for an example), fines can be charged. It should not Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 63636 [details] [review] Bug 18651: Update accountlines.issue_id is the issue_id has been changed during the move Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 63637 [details] [review] DEBUG patches: I'd like feedback Check what the debug statement returns: there are 4 accountlines entries, is that correct? https://bugs.koha-community.org/show_bug.cgi?id=18651 Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> We have a problem with the auto_increment on issues.issue_id -- the auto_increment is not kept if the database is stopped; it is simply calculated by mysql when the database starts. That caused problems if we re-start the database after we've moved issues to the old_issues table but before we've checked out enough items to increase auto_increment past max(old_issues.issue_id) + 1. As a concrete example, let's say that we check out 3 items. items.issue_id is now 3, and the auto_increment is set to 4. We then check in the items with issue_id = 2 and issue_id = 3. max(old_issues.issue_id) is now 3 max(issues.issue_id) is now 1. We restart mysql at this point, mysql reads max(issues.issue_id) and sets auto_increment to 2. We try to check in the item with issue_id=2. We can't insert into old_issues due to the duplicate key constraint. I suspect that the right thing to do might be to use the mysql --init-file option to run something like ALTER TABLE issues AUTO_INCREMENT= ### ( find the max of issues.issue_id and old_issues.issue_id ) + 1 ### (In reply to Barton Chittenden from comment #10) > We have a problem with the auto_increment on issues.issue_id -- the > auto_increment is not kept if the database is stopped; it is simply > calculated by mysql when the database starts. > > That caused problems if we re-start the database after we've moved issues to > the old_issues table but before we've checked out enough items to increase > auto_increment past max(old_issues.issue_id) + 1. > > As a concrete example, let's say that we check out 3 items. items.issue_id > is now 3, and the auto_increment is set to 4. > > We then check in the items with issue_id = 2 and issue_id = 3. > > max(old_issues.issue_id) is now 3 > max(issues.issue_id) is now 1. > > We restart mysql at this point, mysql reads max(issues.issue_id) and sets > auto_increment to 2. We try to check in the item with issue_id=2. We can't > insert into old_issues due to the duplicate key constraint. > > I suspect that the right thing to do might be to use the mysql --init-file > option to run something like > > ALTER TABLE issues AUTO_INCREMENT= ### ( find the max of issues.issue_id and > old_issues.issue_id ) + 1 ### old_issues should not be auto_increment there is one id sequence and that comes from issues. If you try and maintain 2 sequences you have a race condition No, but if the last return is the last issue, then max issue_id in old_issues will be higher than the max id in issues. Thus, the current auto_increment issue_id will match an existing old_issues issue_id and thus everything blows up.
Barton is proposing that on startup, we set the auto_increment for issues.issue_id to GREATEST( MAX(issues.issue_id), MAX(old_issues.issue_id) ) + 1
> old_issues should not be auto_increment there is one id sequence and that
> comes from issues. If you try and maintain 2 sequences you have a race
> condition
Created attachment 63654 [details] [review] Bug 18651: Copy the row before modify the id If the "max(issue_id) from old_issue + 1" already exists in issues, the move fails. For instance we have 1, 2, 3, 4 in issues checkin 4 1, 2, 3 in issues (AI=5) 4 in old_issues Restart mysql => AI is reset to MAX(issue_id) => 4 checkout a new one 1, 2, 3, 4 in issues (AI=5) 4 in old_issues checkin 4 (will get id 5 in old_issues) 1, 2, 3 in issues (AI=5) 4, 5 in old_issues => This works with and without this patch Now we have 1, 2, 3 in issues (AI=5) 4, 5 in old_issues Restart mysql => AI is reset to MAX(issue_id) => 4 checkout a 2 new ones 1, 2, 3, 4, 5 in issues (AI=6) 4 in old_issues checkin 4 1, 2, 3, 5 in issues (AI=6) 4, 5 in old_issues => This did not work without with patch The update of the issue_id was made before the move (so in the issues table), the PK did not allow it I like the solution. Correct me if I'm wrong, but we still aren't correcting the auto_increment value for issue_id. This effectively *does* create a race condition if two checkins happen simultaneously where the issue_id's are already in old_issues. The only solution that comes to mind at the moment is lines 18651 through 2192 in a loop that ends when the query on line 2192 succeeds. Is that reasonable? Or do we just accept the small risk that an error condition will be triggered? As long as the failure doesn't introduce bad and inconsistent data I think it's worth the tiny risk of a librarian getting an error on checkin once in a blue moon, and have to check the item in a second time. (In reply to Jonathan Druart from comment #13) > Created attachment 63654 [details] [review] [review] > Bug 18651: Copy the row before modify the id Comment on attachment 63654 [details] [review] Bug 18651: Copy the row before modify the id Review of attachment 63654 [details] [review]: ----------------------------------------------------------------- ::: C4/Circulation.pm @@ +2180,4 @@ > > + # Create the old_issues entry > + my $old_checkout_data = $issue->unblessed; > + Start the loop here @@ +2193,3 @@ > > + # Update the fines > + $dbh->do(q|UPDATE accountlines SET issue_id = ? WHERE issue_id = ?|, undef, $old_checkout->issue_id, $issue->issue_id); If the dbh update files, go back to "Start the loop here", otherwise continue. Created attachment 63659 [details] [review] Bug 18651: Copy the row before modify the id If the "max(issue_id) from old_issue + 1" already exists in issues, the move fails. For instance we have 1, 2, 3, 4 in issues checkin 4 1, 2, 3 in issues (AI=5) 4 in old_issues Restart mysql => AI is reset to MAX(issue_id) => 4 checkout a new one 1, 2, 3, 4 in issues (AI=5) 4 in old_issues checkin 4 (will get id 5 in old_issues) 1, 2, 3 in issues (AI=5) 4, 5 in old_issues => This works with and without this patch Now we have 1, 2, 3 in issues (AI=5) 4, 5 in old_issues Restart mysql => AI is reset to MAX(issue_id) => 4 checkout 2 new ones 1, 2, 3, 4, 5 in issues (AI=7) 4, 5 in old_issues checkin 4 (4 becomes 6 in old_issues) 1, 2, 3, 5 in issues (AI=6) 4, 5, 6 in old_issues => This did not work without with patch The update of the issue_id was made before the move (so in the issues table), the PK did not allow it I have messed up the example. I reattached the patch amending the commit message. Created attachment 63706 [details] [review] Bug 18651: Use a READ and WRITE LOCK For more info, see: commit be156d9ad9e5bcfadab34d44f90e04fd61e256ad Bug 15854: Use a READ and WRITE LOCK on message_queue and commit b40456f7dd4b8a988f9c6a5718452936101cb8ff Bug 18364: Do not LOCK/UNLOCK tables from tests Upping severity, please let move this forward quickly! Is there a test plan for this? (In reply to Dilan Johnpullé from comment #20) > Is there a test plan for this? No, there is no test plan to follow-up step-by-step. You will need to understand the problem to test and confirm the patch works. Created attachment 64352 [details] [review] Bug 18651: Update issue_id in AddReturn 1. AddReturn returns a $issue hashref with the old issue_id value => At first glance it does not affect anything, but would be good to fix it for future uses. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Created attachment 64353 [details] [review] Bug 18651: Do not charge if the checkin failed 2. If the move fails for whatever reason (see https://lists.katipo.co.nz/pipermail/koha/2017-May/048045.html for an example), fines can be charged. It should not Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Created attachment 64354 [details] [review] Bug 18651: Update accountlines.issue_id is the issue_id has been changed during the move Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Created attachment 64355 [details] [review] Bug 18651 Updating tests Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Created attachment 64356 [details] [review] Bug 18651: Copy the row before modify the id If the "max(issue_id) from old_issue + 1" already exists in issues, the move fails. For instance we have 1, 2, 3, 4 in issues checkin 4 1, 2, 3 in issues (AI=5) 4 in old_issues Restart mysql => AI is reset to MAX(issue_id) => 4 checkout a new one 1, 2, 3, 4 in issues (AI=5) 4 in old_issues checkin 4 (will get id 5 in old_issues) 1, 2, 3 in issues (AI=5) 4, 5 in old_issues => This works with and without this patch Now we have 1, 2, 3 in issues (AI=5) 4, 5 in old_issues Restart mysql => AI is reset to MAX(issue_id) => 4 checkout 2 new ones 1, 2, 3, 4, 5 in issues (AI=7) 4, 5 in old_issues checkin 4 (4 becomes 6 in old_issues) 1, 2, 3, 5 in issues (AI=6) 4, 5, 6 in old_issues => This did not work without with patch The update of the issue_id was made before the move (so in the issues table), the PK did not allow it Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Created attachment 64357 [details] [review] Bug 18651: Use a READ and WRITE LOCK For more info, see: commit be156d9ad9e5bcfadab34d44f90e04fd61e256ad Bug 15854: Use a READ and WRITE LOCK on message_queue and commit b40456f7dd4b8a988f9c6a5718452936101cb8ff Bug 18364: Do not LOCK/UNLOCK tables from tests Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> QA: Looking here now Generally looks good. But still some things: Patch 4 seems to be just for debugging purposes (Data::Printer). The search is saved in account_lines but it is not used any more? Patch 6 adds locking. You lock the message queue. But you should lock old issues, I guess. Furthermore, how long do you really need the lock? Your lock now extends to the very end of MarkIssueReturned, but isn't the lock actually needed only when calculating max+1 and storing it in old issues ? Created attachment 64394 [details] [review] Bug 18651: Update issue_id in AddReturn 1. AddReturn returns a $issue hashref with the old issue_id value => At first glance it does not affect anything, but would be good to fix it for future uses. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Created attachment 64395 [details] [review] Bug 18651: Do not charge if the checkin failed 2. If the move fails for whatever reason (see https://lists.katipo.co.nz/pipermail/koha/2017-May/048045.html for an example), fines can be charged. It should not Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Created attachment 64396 [details] [review] Bug 18651: Update accountlines.issue_id is the issue_id has been changed during the move Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Created attachment 64397 [details] [review] Bug 18651: Copy the row before modify the id If the "max(issue_id) from old_issue + 1" already exists in issues, the move fails. For instance we have 1, 2, 3, 4 in issues checkin 4 1, 2, 3 in issues (AI=5) 4 in old_issues Restart mysql => AI is reset to MAX(issue_id) => 4 checkout a new one 1, 2, 3, 4 in issues (AI=5) 4 in old_issues checkin 4 (will get id 5 in old_issues) 1, 2, 3 in issues (AI=5) 4, 5 in old_issues => This works with and without this patch Now we have 1, 2, 3 in issues (AI=5) 4, 5 in old_issues Restart mysql => AI is reset to MAX(issue_id) => 4 checkout 2 new ones 1, 2, 3, 4, 5 in issues (AI=7) 4, 5 in old_issues checkin 4 (4 becomes 6 in old_issues) 1, 2, 3, 5 in issues (AI=6) 4, 5, 6 in old_issues => This did not work without with patch The update of the issue_id was made before the move (so in the issues table), the PK did not allow it Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Created attachment 64398 [details] [review] Bug 18651: Use a READ and WRITE LOCK For more info, see: commit be156d9ad9e5bcfadab34d44f90e04fd61e256ad Bug 15854: Use a READ and WRITE LOCK on message_queue and commit b40456f7dd4b8a988f9c6a5718452936101cb8ff Bug 18364: Do not LOCK/UNLOCK tables from tests Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Created attachment 64399 [details] [review] Bug 18651: Limit the life span of the LOCK We only need the table to be locked for the fetch, increment, store sequence (In reply to Marcel de Rooy from comment #29) > Generally looks good. But still some things: > > Patch 4 seems to be just for debugging purposes (Data::Printer). The search > is saved in account_lines but it is not used any more? Sounds like I have messed my patches when I squashed them. I removed it. > Patch 6 adds locking. You lock the message queue. But you should lock old > issues, I guess. Outch :-/ > Furthermore, how long do you really need the lock? Your lock now extends to > the very end of MarkIssueReturned, but isn't the lock actually needed only > when calculating max+1 and storing it in old issues ? Yes agreed. This is now blocker, new stable releases have to include it! Will have another look today (In reply to Marcel de Rooy from comment #38) > Will have another look today QA: Resuming here now Still found something. Working on it. Created attachment 64448 [details] [review] Bug 18651: Update issue_id in AddReturn 1. AddReturn returns a $issue hashref with the old issue_id value => At first glance it does not affect anything, but would be good to fix it for future uses. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 64449 [details] [review] Bug 18651: Do not charge if the checkin failed 2. If the move fails for whatever reason (see https://lists.katipo.co.nz/pipermail/koha/2017-May/048045.html for an example), fines can be charged. It should not Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 64450 [details] [review] Bug 18651: Update accountlines.issue_id is the issue_id has been changed during the move Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 64451 [details] [review] Bug 18651: Copy the row before modify the id If the "max(issue_id) from old_issue + 1" already exists in issues, the move fails. For instance we have 1, 2, 3, 4 in issues checkin 4 1, 2, 3 in issues (AI=5) 4 in old_issues Restart mysql => AI is reset to MAX(issue_id) => 4 checkout a new one 1, 2, 3, 4 in issues (AI=5) 4 in old_issues checkin 4 (will get id 5 in old_issues) 1, 2, 3 in issues (AI=5) 4, 5 in old_issues => This works with and without this patch Now we have 1, 2, 3 in issues (AI=5) 4, 5 in old_issues Restart mysql => AI is reset to MAX(issue_id) => 4 checkout 2 new ones 1, 2, 3, 4, 5 in issues (AI=7) 4, 5 in old_issues checkin 4 (4 becomes 6 in old_issues) 1, 2, 3, 5 in issues (AI=6) 4, 5, 6 in old_issues => This did not work without with patch The update of the issue_id was made before the move (so in the issues table), the PK did not allow it Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 64452 [details] [review] Bug 18651: Use a READ and WRITE LOCK For more info, see: commit be156d9ad9e5bcfadab34d44f90e04fd61e256ad Bug 15854: Use a READ and WRITE LOCK on message_queue and commit b40456f7dd4b8a988f9c6a5718452936101cb8ff Bug 18364: Do not LOCK/UNLOCK tables from tests Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 64453 [details] [review] Bug 18651: Limit the life span of the LOCK We only need the table to be locked for the fetch, increment, store sequence Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 64454 [details] [review] Bug 18651: [QA Follow-up] Remove unused variable Variable $original_issue_id is not used. The id is retrieved later from $issue when updating accountlines. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 64455 [details] [review] Bug 18651: [QA Follow-up] Fix the MAX(issue_id) calculation Found this by inserting the same issue_id in old_issues before checkin: The call to ->search( )->get_column is in scalar context and will return the number of results, i.e. always 1. If you have an issue_id 2 in old_issues, it will crash: DBIx::Class::Storage::DBI::_dbh_execute(): Duplicate entry '2' for key 'PRIMARY' The fix is fairly simple: Put get_column in list context and pick the first array entry. NOTE: Using DBIx's get_column()->max here might look simpler here. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Pushed to master for 17.11, thanks to everybody involved! The following test is failing on Jenkins: # Subtest: Handle ids duplication 1..4 ok 1 - No account lines should exist on old issue_id not ok 2 - Two account lines should exist on new issue_id ok 3 - AddReturn should return the issue with the new issue_id ok 4 - If an item is checked-in, it should be moved to old_issues even if the issue_id already existed in the table not ok 4 - Handle ids duplication I guess it is because there is no issuing rules defined. Created attachment 64489 [details] [review] Bug 18651: Fix tests if no circ rule exist The following test is failing on Jenkins: # Subtest: Handle ids duplication 1..4 ok 1 - No account lines should exist on old issue_id not ok 2 - Two account lines should exist on new issue_id ok 3 - AddReturn should return the issue with the new issue_id ok 4 - If an item is checked-in, it should be moved to old_issues even if the issue_id already existed in the table not ok 4 - Handle ids duplication When no circ rule exist Last patch pushed to master. Created attachment 64490 [details] [review] Bug 18651: [QA Follow-up] Fix the MAX(issue_id) calculation Found this by inserting the same issue_id in old_issues before checkin: The call to ->search( )->get_column is in scalar context and will return the number of results, i.e. always 1. If you have an issue_id 2 in old_issues, it will crash: DBIx::Class::Storage::DBI::_dbh_execute(): Duplicate entry '2' for key 'PRIMARY' The fix is fairly simple: Put get_column in list context and pick the first array entry. NOTE: Using DBIx's get_column()->max here might look simpler here. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> (In reply to DEVINIM from comment #53) > Created attachment 64490 [details] [review] [review] > Bug 18651: [QA Follow-up] Fix the MAX(issue_id) calculation > > Found this by inserting the same issue_id in old_issues before checkin: > The call to ->search( )->get_column is in scalar context and will > return the number of results, i.e. always 1. > If you have an issue_id 2 in old_issues, it will crash: > DBIx::Class::Storage::DBI::_dbh_execute(): Duplicate entry '2' for key > 'PRIMARY' > > The fix is fairly simple: Put get_column in list context and pick the first > array entry. > NOTE: Using DBIx's get_column()->max here might look simpler here. > > Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> What's up, DEVINIM ? (In reply to Marcel de Rooy from comment #54) > (In reply to DEVINIM from comment #53) > > Created attachment 64490 [details] [review] [review] [review] > > Bug 18651: [QA Follow-up] Fix the MAX(issue_id) calculation > > > > Found this by inserting the same issue_id in old_issues before checkin: > > The call to ->search( )->get_column is in scalar context and will > > return the number of results, i.e. always 1. > > If you have an issue_id 2 in old_issues, it will crash: > > DBIx::Class::Storage::DBI::_dbh_execute(): Duplicate entry '2' for key > > 'PRIMARY' > > > > The fix is fairly simple: Put get_column in list context and pick the first > > array entry. > > NOTE: Using DBIx's get_column()->max here might look simpler here. > > > > Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> > > What's up, DEVINIM ? Hi Marcel, So sorry. How can I delete this attachment, I don't know. Can you tell me? Created attachment 64495 [details] [review] Bug 18651: Do no LOCK/UNLOCK the table We cannot LOCK the old_issues table here, other tables are accessed and DBIx::Class rename it with "me": DBD::mysql::st execute failed: Table 'me' was not locked with LOCK TABLES [for Statement "SELECT `me`.`issue_id`, `me`.`borrowernumber`, `me`.`itemnumber`, `me`.`date_due`, `me`.`branchcode`, `me`.`returndate`, `me`.`lastreneweddate`, `me`.`renewals`, `me`.`auto_renew`, `me`.`auto_renew_error`, `me`.`timestamp`, `me`.`issuedate`, `me`.`onsite_checkout`, `me`.`note`, `me`.`notedate` FROM `old_issues` `me` WHERE ( `me`.`issue_id` = ? )" with ParamValues: 0='2'] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1832. Consequence: We could have a checkin refused if there is a race, but this is the simplest and safest way to fix it. (In reply to Jonathan Druart from comment #56) > Created attachment 64495 [details] [review] [review] > Bug 18651: Do no LOCK/UNLOCK the table Pushed to master! (In reply to DEVINIM from comment #55) > (In reply to Marcel de Rooy from comment #54) > > (In reply to DEVINIM from comment #53) > > > Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> > > > > What's up, DEVINIM ? > > Hi Marcel, > So sorry. How can I delete this attachment, I don't know. > Can you tell me? To obsolete/unobsolete patch you need to Edit > Details > obsolete I did it. @Rmaints, this should be pushed everywhere bug 18242 is present (read all stable maintained releases: 16.05, 16.11, 17.05). Please test it before (at least checkin and make sure the tests pass). Pushed to 17.05.x, will be in 17.05.01 UT passes, return works from check outs table and return page. Checkout history works on patron and record. Looks fine. I have massive problems making this work with 16.11.x - starting with the Return.t tests. And now an error in Circulation.pm. I think I am missing dependencies for the new Koha::Object work again. Can someone help? Also: unable to check in. (In reply to Katrin Fischer from comment #61) > I have massive problems making this work with 16.11.x - starting with the > Return.t tests. And now an error in Circulation.pm. I think I am missing > dependencies for the new Koha::Object work again. Can someone help? I have rebased and add a commit to adapt the code to 16.11.x Everything is pushed to the bug_18651_16.11.x branch on my github repo - https://github.com/joubu/Koha/commits/bug_18651_16.11.x Thx a lot, I will try to test ASAP. If someone wants to help and test against 16.11.x too, that would be much appreciated. Hope we can fix this for good. (In reply to Barton Chittenden from comment #10) > I suspect that the right thing to do might be to use the mysql --init-file > option to run something like > > ALTER TABLE issues AUTO_INCREMENT= ### ( find the max of issues.issue_id and > old_issues.issue_id ) + 1 ### See https://wiki.koha-community.org/wiki/DBMS_auto_increment_fix I've tested the patches for 16.11.x, but I think this is still pretty problematic: 1) Unable to check-in more than one problematic issue at once - Check out some items - Check them in again - Restart MySql - Check out some items again - Check them in all at once using the checkboxes in circulation => Only the first item will be returned, the others report: Unable to check in 2) Once out of sync, PKs stay out of sync - Check out some items - Check them in again - Restart MySQL - Check item out => issue_id will be in old_issues already, say it's 8 - Check the item in => issue_id in old_issues is changed, say 9 - Check out next item => issues.issue_id is 9 - Check in item => old_issues.issue_id is changed to 10 ... So I think we will keep changing numbers once the bug manifested, as the PK is never fixed. Not sure if I might have a thinking error there? Could we use something like this to fix the PK when we notice that the issue_id has to be changed in order to be able to move it? ALTER TABLE issues AUTO_INCREMENT = (bigger max(issue_id) from old_issues and issues) +1 I am not sure I will be able to provide something better before your next release. What you describe in 1) is the note I let on the last patch: "Consequence: We could have a checkin refused if there is a race, but this is the simplest and safest way to fix it." What is the problem with 2)? The only thing I can suggest is to validate the sql server-side fix, then remove all this ugly code and explode with an error (i.e. display a friendly warning) if the fix is not set up correctly. Rewriting the issue_ids seems like a dangerous thing - it fixes the problem, but it only works because we are not heavily relying on it yet. I'd prefer to keep it to a minimum. Is there a reason not to use ALTER TABLE in the code to reset the PK if a problem has been identified? In my testing the problem with 1) seems to appear more often for the problematic issue_ids, which would be another reason to limit their occurrence as much as we can. But I can't really prove that yet. The race condition with the lock worries me. It happened pretty consistently and often in my testing yesterday checking in with the checkboxes from the patron account. Another area that I could imagine this happens is doing batch checkins via SIP and I am not sure how well the self checks would handle this error. Do you know if this has been tested? I am not too friendly with SIP. Maybe a warning and pushing people to fix this on db level would be better, but it might be hard to do for some of them that haven't set up Koha themselves and don't have someone IT-savvy in-house. Created attachment 65134 [details]
Screenshot of failed checkins
At this moment, I don't feel confident in this patch set to be the right fix. - We will keep the state of constant renumbering. Once a MySQL restart has caused the issue, you will never fully recover from it. - We introduce new issues with the lock/race condition as it keeps check ins from happening in my tests pretty constantly. At this point, I am wondering if we shouldn't go as far as reverting bug 18242 and push massively for the database based fix. If your DB has the doubled up id issue, it means that when returning those items, the entry will get lost, but the problem will end there. I have a feeling, that the current fix causes issues like https://lists.katipo.co.nz/pipermail/koha/2017-July/048437.html But I am not fully grasping how it happens yet. Also we have seen weird issues in circulation ourselves testing 16.11.x. with problematic issue_ids. (In reply to Katrin Fischer from comment #71) > At this point, I am wondering if we shouldn't go as far as reverting bug > 18242 and push massively for the database based fix. If your DB has the > doubled up id issue, it means that when returning those items, the entry > will get lost, but the problem will end there. The revert will remove too many things, we must keep the transaction (to avoid data lost) and display a friendly message if this problem happen (linking to the wiki solution). Have you seen bug 18931? I think it should be a good one to have. But if we keep the transaction, we keep the renumbering... I'd really feel better if we could figure out what happens in reports like 18963 and https://lists.katipo.co.nz/pipermail/koha/2017-July/048437.html. And we need to fix the PK if we renumber! (In reply to Katrin Fischer from comment #73) > But if we keep the transaction, we keep the renumbering... No, we just prevent datalost. If the move does not work, we stop the process. Hm, but that would mean that you can't check-in the book and it remains on the patron account...? (In reply to Katrin Fischer from comment #75) > Hm, but that would mean that you can't check-in the book and it remains on > the patron account...? Yes of course. (In reply to Jonathan Druart from comment #76) > (In reply to Katrin Fischer from comment #75) > > Hm, but that would mean that you can't check-in the book and it remains on > > the patron account...? > > Yes of course. Jonathan -- that means that the item will *never* be able to be checked in, and will stay on the patron's account permanently, because the issue_id in oldissues will will never change. That's not an acceptable solution from a library's perspective. At Bywater, we've been fixing these by hand by updating issues.issue_id by hand as they come up (this works OK as long as we remember to update the issue_id in the accountlines table)... but it's a time-waster, to be sure. (In reply to Barton Chittenden from comment #77) > (In reply to Jonathan Druart from comment #76) > > (In reply to Katrin Fischer from comment #75) > > > Hm, but that would mean that you can't check-in the book and it remains on > > > the patron account...? > > > > Yes of course. > > Jonathan -- that means that the item will *never* be able to be checked in, > and will stay on the patron's account permanently, because the issue_id in > oldissues will will never change. That's not an acceptable solution from a > library's perspective. > > At Bywater, we've been fixing these by hand by updating issues.issue_id by > hand as they come up (this works OK as long as we remember to update the > issue_id in the accountlines table)... but it's a time-waster, to be sure. Hm, at the moment this should not be necessary, as we renumber in stable releases and before that they were silently lost? But yes, without an overwrite option or similar they would stay on the account until a clean-up script that has to be written still can be run. (In reply to Barton Chittenden from comment #77) > (In reply to Jonathan Druart from comment #76) > > (In reply to Katrin Fischer from comment #75) > > > Hm, but that would mean that you can't check-in the book and it remains on > > > the patron account...? > > > > Yes of course. > > Jonathan -- that means that the item will *never* be able to be checked in, > and will stay on the patron's account permanently, because the issue_id in > oldissues will will never change. That's not an acceptable solution from a > library's perspective. What is not acceptable is the current situation we have in all stable releases (no checkin possible). Please continue the discussion on bug 18966. |