[Bug 18651] New: Move of checkouts is still not correctly handled
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 Bug ID: 18651 Summary: Move of checkouts is still not correctly handled Change sponsored?: --- Product: Koha Version: unspecified Hardware: All OS: All Status: ASSIGNED Severity: major Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: jonathan.druart@bugs.koha-community.org Reporter: jonathan.druart@bugs.koha-community.org QA Contact: testopia@bugs.koha-community.org Depends on: 18242 Bug 18242 fixes a major: The move of a checkout from the issues table to the old_issues table was not done in a transaction, that leads to data lost. The issues row was deleted but not inserted to the old_issues table. Now that we have 18242, the move is done in a transaction but there are still several problems: 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. 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 3. accountlines.issue_id are not updated! Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18242 [Bug 18242] Move of checkouts to old_issues is not handle correctly -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #1 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 63629 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=63629&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #2 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 63630 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=63630&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #3 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 63631 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=63631&action=edit Bug 18651: Update accountlines.issue_id is the issue_id has been changed during the move -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #4 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 63632 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=63632&action=edit DEBUG patches: I'd like feedback Check what the debug statement returns: there are 4 accountlines entries, is that correct? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #63629|0 |1 is obsolete| | Attachment #63630|0 |1 is obsolete| | Attachment #63631|0 |1 is obsolete| | Attachment #63632|0 |1 is obsolete| | --- Comment #5 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 63633 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=63633&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #63633|0 |1 is obsolete| | --- Comment #6 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 63634 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=63634&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #7 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 63635 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=63635&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #8 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 63636 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=63636&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #9 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 63637 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=63637&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version|unspecified |master CC| |tomascohen@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 Barton Chittenden <barton@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |barton@bywatersolutions.com --- Comment #10 from Barton Chittenden <barton@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 ### -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 Mason James <mtj@kohaaloha.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mtj@kohaaloha.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 Colin Campbell <colin.campbell@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |colin.campbell@ptfs-europe. | |com --- Comment #11 from Colin Campbell <colin.campbell@ptfs-europe.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com --- Comment #12 from Kyle M Hall <kyle@bywatersolutions.com> --- 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
-- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #13 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 63654 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=63654&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #14 from Kyle M Hall <kyle@bywatersolutions.com> --- 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] Bug 18651: Copy the row before modify the id
-- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 Barton Chittenden <barton@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=18242 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #15 from Kyle M Hall <kyle@bywatersolutions.com> --- Comment on attachment 63654 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=63654 Bug 18651: Copy the row before modify the id Review of attachment 63654: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=18651&attachment=63654) ----------------------------------------------------------------- ::: 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #63654|0 |1 is obsolete| | --- Comment #16 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 63659 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=63659&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #17 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- I have messed up the example. I reattached the patch amending the commit message. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #18 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 63706 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=63706&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|major |critical --- Comment #19 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Upping severity, please let move this forward quickly! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 dilan@calyx.net.au changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dilan@calyx.net.au --- Comment #20 from dilan@calyx.net.au --- Is there a test plan for this? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |veron@veron.ch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #21 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #63634|0 |1 is obsolete| | --- Comment #22 from Chris Cormack <chris@bigballofwax.co.nz> --- Created attachment 64352 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=64352&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #63635|0 |1 is obsolete| | --- Comment #23 from Chris Cormack <chris@bigballofwax.co.nz> --- Created attachment 64353 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=64353&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #63636|0 |1 is obsolete| | --- Comment #24 from Chris Cormack <chris@bigballofwax.co.nz> --- Created attachment 64354 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=64354&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #63637|0 |1 is obsolete| | --- Comment #25 from Chris Cormack <chris@bigballofwax.co.nz> --- Created attachment 64355 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=64355&action=edit Bug 18651 Updating tests Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #63659|0 |1 is obsolete| | --- Comment #26 from Chris Cormack <chris@bigballofwax.co.nz> --- Created attachment 64356 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=64356&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #63706|0 |1 is obsolete| | --- Comment #27 from Chris Cormack <chris@bigballofwax.co.nz> --- Created attachment 64357 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=64357&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #28 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- QA: Looking here now -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |m.de.rooy@rijksmuseum.nl |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #29 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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 ? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #64352|0 |1 is obsolete| | Attachment #64353|0 |1 is obsolete| | Attachment #64354|0 |1 is obsolete| | Attachment #64355|0 |1 is obsolete| | Attachment #64356|0 |1 is obsolete| | Attachment #64357|0 |1 is obsolete| | --- Comment #30 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 64394 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=64394&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #31 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 64395 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=64395&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #32 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 64396 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=64396&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #33 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 64397 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=64397&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #34 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 64398 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=64398&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #35 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 64399 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=64399&action=edit Bug 18651: Limit the life span of the LOCK We only need the table to be locked for the fetch, increment, store sequence -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #36 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|critical |blocker CC| |alex.sassmannshausen@ptfs-e | |urope.com, | |julian.maurice@biblibre.com | |, | |martin.renvoize@ptfs-europe | |.com --- Comment #37 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- This is now blocker, new stable releases have to include it! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #38 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Will have another look today -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #39 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Marcel de Rooy from comment #38)
Will have another look today
QA: Resuming here now -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |BLOCKED --- Comment #40 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Still found something. Working on it. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|BLOCKED |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #64394|0 |1 is obsolete| | --- Comment #41 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 64448 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=64448&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #64395|0 |1 is obsolete| | --- Comment #42 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 64449 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=64449&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #64396|0 |1 is obsolete| | --- Comment #43 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 64450 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=64450&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #64397|0 |1 is obsolete| | --- Comment #44 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 64451 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=64451&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #64398|0 |1 is obsolete| | --- Comment #45 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 64452 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=64452&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #64399|0 |1 is obsolete| | --- Comment #46 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 64453 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=64453&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #47 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 64454 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=64454&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #48 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 64455 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=64455&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master --- Comment #49 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Pushed to master for 17.11, thanks to everybody involved! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #50 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #51 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 64489 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=64489&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #52 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Last patch pushed to master. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 DEVINIM <kohadevinim@devinim.com.tr> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #64455|0 |1 is obsolete| | --- Comment #53 from DEVINIM <kohadevinim@devinim.com.tr> --- Created attachment 64490 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=64490&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #54 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to DEVINIM from comment #53)
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>
What's up, DEVINIM ? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 DEVINIM <kohadevinim@devinim.com.tr> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kohadevinim@devinim.com.tr --- Comment #55 from DEVINIM <kohadevinim@devinim.com.tr> --- (In reply to Marcel de Rooy from comment #54)
(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 ?
Hi Marcel, So sorry. How can I delete this attachment, I don't know. Can you tell me? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #56 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 64495 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=64495&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #64455|1 |0 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #64490|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #57 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Jonathan Druart from comment #56)
Created attachment 64495 [details] [review] Bug 18651: Do no LOCK/UNLOCK the table
Pushed to master! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #58 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m, katrin.fischer@bsz-bw.de --- Comment #59 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- @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). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 Fridolin SOMERS <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Medium patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 Fridolin SOMERS <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |Pushed to Stable --- Comment #60 from Fridolin SOMERS <fridolin.somers@biblibre.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #61 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #62 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Also: unable to check in. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 David Kuhn <techservspec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |techservspec@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #63 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #64 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #65 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=18931 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #66 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #67 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #68 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #69 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #70 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 65134 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=65134&action=edit Screenshot of failed checkins -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #71 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #72 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 jmbroust <jean-manuel.broust@univ-lyon2.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jean-manuel.broust@univ-lyo | |n2.fr -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #73 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #74 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #75 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Hm, but that would mean that you can't check-in the book and it remains on the patron account...? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #76 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |18966 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18966 [Bug 18966] Move of checkouts - Deal with duplicate IDs at DBMS level -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #77 from Barton Chittenden <barton@bywatersolutions.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #78 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 --- Comment #79 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18651 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |FIXED Status|Pushed to Stable |RESOLVED -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org