Bug 18651 - Move of checkouts is still not correctly handled
Summary: Move of checkouts is still not correctly handled
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low blocker (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on: 18242
Blocks: 18966
  Show dependency treegraph
 
Reported: 2017-05-22 17:38 UTC by Jonathan Druart
Modified: 2017-07-28 00:13 UTC (History)
17 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 18651: Update issue_id in AddReturn (2.48 KB, patch)
2017-05-22 17:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18651: Do not charge if the checkin failed (4.65 KB, patch)
2017-05-22 17:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18651: Update accountlines.issue_id is the issue_id has been changed during the move (2.77 KB, patch)
2017-05-22 17:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
DEBUG patches: I'd like feedback (1.41 KB, patch)
2017-05-22 17:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18651: Update issue_id in AddReturn (2.52 KB, patch)
2017-05-22 18:04 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18651: Update issue_id in AddReturn (2.52 KB, patch)
2017-05-22 18:06 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18651: Do not charge if the checkin failed (4.69 KB, patch)
2017-05-22 18:07 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18651: Update accountlines.issue_id is the issue_id has been changed during the move (2.81 KB, patch)
2017-05-22 18:07 UTC, Kyle M Hall
Details | Diff | Splinter Review
DEBUG patches: I'd like feedback (1.46 KB, patch)
2017-05-22 18:07 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18651: Copy the row before modify the id (3.45 KB, patch)
2017-05-23 13:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18651: Copy the row before modify the id (3.49 KB, patch)
2017-05-23 15:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18651: Use a READ and WRITE LOCK (1.44 KB, patch)
2017-05-24 15:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18651: Update issue_id in AddReturn (2.58 KB, patch)
2017-06-15 20:59 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 18651: Do not charge if the checkin failed (4.76 KB, patch)
2017-06-15 20:59 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 18651: Update accountlines.issue_id is the issue_id has been changed during the move (2.87 KB, patch)
2017-06-15 21:00 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 18651 Updating tests (1.36 KB, patch)
2017-06-15 21:00 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 18651: Copy the row before modify the id (3.54 KB, patch)
2017-06-15 21:01 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 18651: Use a READ and WRITE LOCK (1.50 KB, patch)
2017-06-15 21:01 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 18651: Update issue_id in AddReturn (2.57 KB, patch)
2017-06-16 15:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18651: Do not charge if the checkin failed (4.75 KB, patch)
2017-06-16 15:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18651: Update accountlines.issue_id is the issue_id has been changed during the move (2.86 KB, patch)
2017-06-16 15:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18651: Copy the row before modify the id (3.53 KB, patch)
2017-06-16 15:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18651: Use a READ and WRITE LOCK (1.49 KB, patch)
2017-06-16 15:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18651: Limit the life span of the LOCK (1.75 KB, patch)
2017-06-16 15:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18651: Update issue_id in AddReturn (2.67 KB, patch)
2017-06-20 13:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18651: Do not charge if the checkin failed (4.84 KB, patch)
2017-06-20 13:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18651: Update accountlines.issue_id is the issue_id has been changed during the move (2.96 KB, patch)
2017-06-20 13:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18651: Copy the row before modify the id (3.63 KB, patch)
2017-06-20 13:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18651: Use a READ and WRITE LOCK (1.58 KB, patch)
2017-06-20 13:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18651: Limit the life span of the LOCK (1.84 KB, patch)
2017-06-20 13:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18651: [QA Follow-up] Remove unused variable (1017 bytes, patch)
2017-06-20 13:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18651: [QA Follow-up] Fix the MAX(issue_id) calculation (1.61 KB, patch)
2017-06-20 13:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18651: Fix tests if no circ rule exist (1.40 KB, patch)
2017-06-21 14:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18651: [QA Follow-up] Fix the MAX(issue_id) calculation (1.57 KB, patch)
2017-06-21 14:42 UTC, DEVINIM
Details | Diff | Splinter Review
Bug 18651: Do no LOCK/UNLOCK the table (2.16 KB, patch)
2017-06-21 17:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Screenshot of failed checkins (117.61 KB, image/png)
2017-07-20 05:50 UTC, Katrin Fischer
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2017-05-22 17:38:25 UTC
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!
Comment 1 Jonathan Druart 2017-05-22 17:44:23 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.
Comment 2 Jonathan Druart 2017-05-22 17:44:27 UTC
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
Comment 3 Jonathan Druart 2017-05-22 17:44:31 UTC
Created attachment 63631 [details] [review]
Bug 18651: Update accountlines.issue_id is the issue_id has been changed during the move
Comment 4 Jonathan Druart 2017-05-22 17:44:36 UTC
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?
Comment 5 Kyle M Hall 2017-05-22 18:04:52 UTC
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>
Comment 6 Kyle M Hall 2017-05-22 18:06:40 UTC
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>
Comment 7 Kyle M Hall 2017-05-22 18:07:35 UTC
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>
Comment 8 Kyle M Hall 2017-05-22 18:07:38 UTC
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>
Comment 9 Kyle M Hall 2017-05-22 18:07:41 UTC
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>
Comment 10 Barton Chittenden 2017-05-22 22:52:03 UTC
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 ###
Comment 11 Colin Campbell 2017-05-23 08:39:37 UTC
(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
Comment 12 Kyle M Hall 2017-05-23 13:44:45 UTC
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
Comment 13 Jonathan Druart 2017-05-23 13:48:45 UTC
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
Comment 14 Kyle M Hall 2017-05-23 14:01:07 UTC
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 15 Kyle M Hall 2017-05-23 15:24:47 UTC
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.
Comment 16 Jonathan Druart 2017-05-23 15:25:24 UTC
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
Comment 17 Jonathan Druart 2017-05-23 15:26:28 UTC
I have messed up the example. I reattached the patch amending the commit message.
Comment 18 Jonathan Druart 2017-05-24 15:25:51 UTC
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
Comment 19 Jonathan Druart 2017-06-14 02:43:42 UTC
Upping severity, please let move this forward quickly!
Comment 20 Dilan Johnpullé 2017-06-14 06:07:58 UTC
Is there a test plan for this?
Comment 21 Jonathan Druart 2017-06-14 12:11:07 UTC
(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.
Comment 22 Chris Cormack 2017-06-15 20:59:33 UTC
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>
Comment 23 Chris Cormack 2017-06-15 20:59:56 UTC
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>
Comment 24 Chris Cormack 2017-06-15 21:00:15 UTC
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>
Comment 25 Chris Cormack 2017-06-15 21:00:40 UTC
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>
Comment 26 Chris Cormack 2017-06-15 21:01:00 UTC
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>
Comment 27 Chris Cormack 2017-06-15 21:01:35 UTC
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>
Comment 28 Marcel de Rooy 2017-06-16 06:28:51 UTC
QA: Looking here now
Comment 29 Marcel de Rooy 2017-06-16 07:20:24 UTC
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 ?
Comment 30 Jonathan Druart 2017-06-16 15:16:53 UTC
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>
Comment 31 Jonathan Druart 2017-06-16 15:17:53 UTC
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>
Comment 32 Jonathan Druart 2017-06-16 15:18:01 UTC
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>
Comment 33 Jonathan Druart 2017-06-16 15:18:07 UTC
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>
Comment 34 Jonathan Druart 2017-06-16 15:18:13 UTC
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>
Comment 35 Jonathan Druart 2017-06-16 15:18:21 UTC
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
Comment 36 Jonathan Druart 2017-06-16 15:19:28 UTC
(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.
Comment 37 Jonathan Druart 2017-06-19 20:14:10 UTC
This is now blocker, new stable releases have to include it!
Comment 38 Marcel de Rooy 2017-06-20 06:00:30 UTC
Will have another look today
Comment 39 Marcel de Rooy 2017-06-20 12:06:54 UTC
(In reply to Marcel de Rooy from comment #38)
> Will have another look today

QA: Resuming here now
Comment 40 Marcel de Rooy 2017-06-20 12:46:04 UTC
Still found something. Working on it.
Comment 41 Marcel de Rooy 2017-06-20 13:15:20 UTC
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>
Comment 42 Marcel de Rooy 2017-06-20 13:15:25 UTC
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>
Comment 43 Marcel de Rooy 2017-06-20 13:15:30 UTC
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>
Comment 44 Marcel de Rooy 2017-06-20 13:15:35 UTC
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>
Comment 45 Marcel de Rooy 2017-06-20 13:15:41 UTC
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>
Comment 46 Marcel de Rooy 2017-06-20 13:15:46 UTC
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>
Comment 47 Marcel de Rooy 2017-06-20 13:15:51 UTC
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>
Comment 48 Marcel de Rooy 2017-06-20 13:15:56 UTC
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>
Comment 49 Jonathan Druart 2017-06-20 17:40:55 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 50 Jonathan Druart 2017-06-21 14:04:25 UTC
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.
Comment 51 Jonathan Druart 2017-06-21 14:07:18 UTC
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
Comment 52 Jonathan Druart 2017-06-21 14:09:07 UTC
Last patch pushed to master.
Comment 53 DEVINIM 2017-06-21 14:42:43 UTC
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>
Comment 54 Marcel de Rooy 2017-06-21 14:50:59 UTC
(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 ?
Comment 55 DEVINIM 2017-06-21 15:31:38 UTC
(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?
Comment 56 Jonathan Druart 2017-06-21 17:16:52 UTC
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.
Comment 57 Jonathan Druart 2017-06-21 17:23:51 UTC
(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!
Comment 58 Jonathan Druart 2017-06-21 17:24:48 UTC
(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.
Comment 59 Jonathan Druart 2017-06-21 21:24:51 UTC
@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).
Comment 60 Fridolin SOMERS 2017-06-22 07:53:47 UTC
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.
Comment 61 Katrin Fischer 2017-06-22 09:00:33 UTC
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?
Comment 62 Katrin Fischer 2017-06-22 09:01:40 UTC
Also: unable to check in.
Comment 63 Jonathan Druart 2017-07-05 13:34:09 UTC
(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
Comment 64 Katrin Fischer 2017-07-05 13:57:15 UTC
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.
Comment 65 Jonathan Druart 2017-07-12 18:05:40 UTC
(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
Comment 66 Katrin Fischer 2017-07-19 22:22:49 UTC
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
Comment 67 Jonathan Druart 2017-07-19 23:04:21 UTC
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.
Comment 68 Katrin Fischer 2017-07-19 23:22:15 UTC
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.
Comment 69 Katrin Fischer 2017-07-20 05:47:17 UTC
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.
Comment 70 Katrin Fischer 2017-07-20 05:50:57 UTC
Created attachment 65134 [details]
Screenshot of failed checkins
Comment 71 Katrin Fischer 2017-07-20 06:41:34 UTC
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.
Comment 72 Jonathan Druart 2017-07-20 13:07:46 UTC
(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.
Comment 73 Katrin Fischer 2017-07-20 13:47:33 UTC
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!
Comment 74 Jonathan Druart 2017-07-20 14:25:29 UTC
(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.
Comment 75 Katrin Fischer 2017-07-20 14:30:48 UTC
Hm, but that would mean that you can't check-in the book and it remains on the patron account...?
Comment 76 Jonathan Druart 2017-07-20 14:58:55 UTC
(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.
Comment 77 Barton Chittenden 2017-07-23 22:53:15 UTC
(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.
Comment 78 Katrin Fischer 2017-07-23 23:11:19 UTC
(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.
Comment 79 Jonathan Druart 2017-07-24 14:58:04 UTC
(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.