Bug 18966 - Move of checkouts - Deal with duplicate IDs at DBMS level
Summary: Move of checkouts - Deal with duplicate IDs at DBMS level
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low blocker (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 18651 18931
Blocks: 18996 19016 19317
  Show dependency treegraph
 
Reported: 2017-07-20 17:15 UTC by Jonathan Druart
Modified: 2019-06-27 09:24 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 18966: Do not deal with duplicate issue_id on checkin (10.79 KB, patch)
2017-07-20 17:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
[17.05.x] Bug 18966: Do not deal with duplicate issue_id on checkin (10.87 KB, patch)
2017-07-20 17:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
[16.11.x] Bug 18966: Do not deal with duplicate issue_id on checkin (10.84 KB, patch)
2017-07-20 17:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
[16.05.x] Bug 18966: Do not deal with duplicate issue_id on checkin (10.91 KB, patch)
2017-07-20 17:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18966: Do not deal with duplicate issue_id on checkin (10.82 KB, patch)
2017-07-24 17:48 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
(17.05.x) Bug 18966: Do not deal with duplicate issue_id on checkin (10.90 KB, patch)
2017-07-24 17:55 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
(16.11.x) Bug 18966: Do not deal with duplicate issue_id on checkin (10.86 KB, patch)
2017-07-24 18:12 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[16.11.x] Bug 18966: Do not deal with duplicate issue_id on checkin (10.92 KB, patch)
2017-07-24 18:17 UTC, Katrin Fischer
Details | Diff | Splinter Review
(16.05.x) Bug 18966: Do not deal with duplicate issue_id on checkin (10.93 KB, patch)
2017-07-24 18:24 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 18966: Do not deal with duplicate issue_id on checkin (10.93 KB, patch)
2017-07-26 09:53 UTC, Julian Maurice
Details | Diff | Splinter Review
[16.05.x] Bug 18966: fix unneeded INSERT in AddReturn() (1007 bytes, patch)
2017-07-27 13:34 UTC, Mason James
Details | Diff | Splinter Review
[16.11.x] Bug 18966: fix unneeded INSERT in AddReturn() (1007 bytes, patch)
2017-07-27 13:39 UTC, Mason James
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2017-07-20 17:15:02 UTC
Koha suffers of big bugs due to its history: When data are deleted, they are moved to another tables.
For instance issues and old_issues: when a checkin is done, it is moved to the old_issues table.
That leads to a main problem that is described on https://wiki.koha-community.org/wiki/DBMS_auto_increment_fix

However we tried first to fix the problem (for issues/old_issues) at code level on bug 18242.
The goal was to prevent data lost.
Data lost may happens in this case:
Check an item out (issue_id = 1)
Check an item in (issue_id = 1)
Restart MySQL (reset auto increment for issue_id to 1)
Check an item out (issue_id = 1)
Check an item in => BOOM, the issue_id is a PK in old_issues and the move fails.
Before bug 18242 the data were lost, we inserted the value into old_issues, which fails silently (because of RaiseError set to 0 in Koha::Database), then delete the row from issues.
That has been fixed using a transaction.

This patch introduced a regression we tried to fix on bug 18651 comment 0, the patron was charged even if the checkin was rejected.
A good way to fix that would have been to LOCK the tables:
1- Start a transaction
2- LOCK the table to make sure nobody will read id and avoid race conditions
3- Move the content from one table to the other, dealing with ids
4- UNLOCK the table
5- Commit the transaction
But there were problems using LOCK and DBIx::Class (See commit 905572910b3a - Do no LOCK/UNLOCK the table).

Finally the solution implemented is not acceptable for several reasons:
- batch checkins may fail
- issue_id will always stay out of sync (between issues and old_issues)
See 18651 comment 66.

Since the next stable releases are very soon, and we absolutely need to fix this problem, I am suggesting to:
1- Execute the move in a transaction to avoid data lost and reject the checkin if we face IDs dup
=> It will only reject 1 checkin (max is 1 * MySQL restart), no need to deal with race conditions, 
2- Display a warning on the checkin page and link to a solution/explanation
3- Communicate as much as we can on the proper fix: Update auto increment values when the DBMS is restarted - https://wiki.koha-community.org/wiki/DBMS_auto_increment_fix
4- Display a warning on the about page for corrupted data (see bug 18931)
5- Write and make available a maintenance script to fix corrupted data (TODO LATER)
Comment 1 Jonathan Druart 2017-07-20 17:19:34 UTC
Created attachment 65151 [details] [review]
Bug 18966: Do not deal with duplicate issue_id on checkin

Koha suffers of big bugs due to its history: When data are deleted, they are moved to another tables.
For instance issues and old_issues: when a checkin is done, it is moved to the old_issues table.
That leads to a main problem that is described on https://wiki.koha-community.org/wiki/DBMS_auto_increment_fix

However we tried first to fix the problem (for issues/old_issues) at code level on bug 18242.
The goal was to prevent data lost.
Data lost may happens in this case:
Check an item out (issue_id = 1)
Check an item in (issue_id = 1)
Restart MySQL (reset auto increment for issue_id to 1)
Check an item out (issue_id = 1)
Check an item in => BOOM, the issue_id is a PK in old_issues and the move fails.
Before bug 18242 the data were lost, we inserted the value into old_issues, which fails silently (because of RaiseError set to 0 in Koha::Database), then delete the row from issues.
That has been fixed using a transaction.

This patch introduced a regression we tried to fix on bug 18651 comment 0, the patron was charged even if the checkin was rejected.
A good way to fix that would have been to LOCK the tables:
1- Start a transaction
2- LOCK the table to make sure nobody will read id and avoid race conditions
3- Move the content from one table to the other, dealing with ids
4- UNLOCK the table
5- Commit the transaction
But there were problems using LOCK and DBIx::Class (See commit 905572910b3a - Do no LOCK/UNLOCK the table).

Finally the solution implemented is not acceptable for several reasons:
- batch checkins may fail
- issue_id will always stay out of sync (between issues and old_issues)
See 18651 comment 66.

Since the next stable releases are very soon, and we absolutely need to fix this problem, I am suggesting to:
1- Execute the move in a transaction to avoid data lost and reject the checkin if we face IDs dup
=> It will only reject 1 checkin (max is 1 * MySQL restart), no need to deal with race conditions,
2- Display a warning on the checkin page and link to a solution/explanation
3- Communicate as much as we can on the proper fix: Update auto increment values when the DBMS is restarted - https://wiki.koha-community.org/wiki/DBMS_auto_increment_fix
4- Display a warning on the about page for corrupted data (see bug 18931)
5- Write and make available a maintenance script to fix corrupted data (TODO LATER)
Comment 2 Jonathan Druart 2017-07-20 17:56:40 UTC
Created attachment 65154 [details] [review]
[17.05.x] Bug 18966: Do not deal with duplicate issue_id on checkin
Comment 3 Jonathan Druart 2017-07-20 17:57:13 UTC
Created attachment 65155 [details] [review]
[16.11.x] Bug 18966: Do not deal with duplicate issue_id on checkin
Comment 4 Jonathan Druart 2017-07-20 17:57:53 UTC
Created attachment 65156 [details] [review]
[16.05.x] Bug 18966: Do not deal with duplicate issue_id on checkin
Comment 5 David Cook 2017-07-21 01:04:11 UTC
Do you have a test plan for testers to follow for this one?
Comment 6 Katrin Fischer 2017-07-21 06:11:00 UTC
(In reply to David Cook from comment #5)
> Do you have a test plan for testers to follow for this one?

Check an item out (issue_id = 1)
Check an item in (issue_id = 1)
Restart MySQL (reset auto increment for issue_id to 1)
Check an item out (issue_id = 1)
Check an item in => BOOM, the issue_id is a PK in old_issues and the move fails.

For starters, instead of BOOM we should see the new warning ;) Please note the dependencies.

> => It will only reject 1 checkin (max is 1 * MySQL restart), no need to deal with race conditions,

Hm, I think it can be more than one, depending on how many items you check out and return immediately before the restart (check-out 3, check-in 3, restart mysql, PK is behind by 3)
Comment 7 Marcel de Rooy 2017-07-21 10:11:48 UTC
For another approach, a few unpolished patches on 18970 based on reverting 18242 and not keeping the renumbering (taking some possible data loss on older issues for granted).
Comment 8 Jonathan Druart 2017-07-24 14:55:51 UTC
(In reply to David Cook from comment #5)
> Do you have a test plan for testers to follow for this one?

Nope, you should only test this if you understand the problem and followed the discussions on the previous bug reports.

(In reply to Katrin Fischer from comment #6)
> > => It will only reject 1 checkin (max is 1 * MySQL restart), no need to deal with race conditions,
> 
> Hm, I think it can be more than one, depending on how many items you check
> out and return immediately before the restart (check-out 3, check-in 3,
> restart mysql, PK is behind by 3)

Yes indeed!

(In reply to Marcel de Rooy from comment #7)
> For another approach, a few unpolished patches on 18970 based on reverting
> 18242 and not keeping the renumbering (taking some possible data loss on
> older issues for granted).

The goal of this patch is to have a simple and safe fix for stable releases. I would like to avoid black magic and make sure we are going to propose something riskless.
Comment 9 Tomás Cohen Arazi 2017-07-24 17:48:07 UTC
Created attachment 65203 [details] [review]
Bug 18966: Do not deal with duplicate issue_id on checkin

Koha suffers of big bugs due to its history: When data are deleted, they are moved to another tables.
For instance issues and old_issues: when a checkin is done, it is moved to the old_issues table.
That leads to a main problem that is described on https://wiki.koha-community.org/wiki/DBMS_auto_increment_fix

However we tried first to fix the problem (for issues/old_issues) at code level on bug 18242.
The goal was to prevent data lost.
Data lost may happens in this case:
Check an item out (issue_id = 1)
Check an item in (issue_id = 1)
Restart MySQL (reset auto increment for issue_id to 1)
Check an item out (issue_id = 1)
Check an item in => BOOM, the issue_id is a PK in old_issues and the move fails.
Before bug 18242 the data were lost, we inserted the value into old_issues, which fails silently (because of RaiseError set to 0 in Koha::Database), then delete the row from issues.
That has been fixed using a transaction.

This patch introduced a regression we tried to fix on bug 18651 comment 0, the patron was charged even if the checkin was rejected.
A good way to fix that would have been to LOCK the tables:
1- Start a transaction
2- LOCK the table to make sure nobody will read id and avoid race conditions
3- Move the content from one table to the other, dealing with ids
4- UNLOCK the table
5- Commit the transaction
But there were problems using LOCK and DBIx::Class (See commit 905572910b3a - Do no LOCK/UNLOCK the table).

Finally the solution implemented is not acceptable for several reasons:
- batch checkins may fail
- issue_id will always stay out of sync (between issues and old_issues)
See 18651 comment 66.

Since the next stable releases are very soon, and we absolutely need to fix this problem, I am suggesting to:
1- Execute the move in a transaction to avoid data lost and reject the checkin if we face IDs dup
=> It will only reject 1 checkin (max is 1 * MySQL restart), no need to deal with race conditions,
2- Display a warning on the checkin page and link to a solution/explanation
3- Communicate as much as we can on the proper fix: Update auto increment values when the DBMS is restarted - https://wiki.koha-community.org/wiki/DBMS_auto_increment_fix
4- Display a warning on the about page for corrupted data (see bug 18931)
5- Write and make available a maintenance script to fix corrupted data (TODO LATER)

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 10 Tomás Cohen Arazi 2017-07-24 17:55:24 UTC
Created attachment 65204 [details] [review]
(17.05.x) Bug 18966: Do not deal with duplicate issue_id on checkin

Koha suffers of big bugs due to its history: When data are deleted, they are moved to another tables.
For instance issues and old_issues: when a checkin is done, it is moved to the old_issues table.
That leads to a main problem that is described on https://wiki.koha-community.org/wiki/DBMS_auto_increment_fix

However we tried first to fix the problem (for issues/old_issues) at code level on bug 18242.
The goal was to prevent data lost.
Data lost may happens in this case:
Check an item out (issue_id = 1)
Check an item in (issue_id = 1)
Restart MySQL (reset auto increment for issue_id to 1)
Check an item out (issue_id = 1)
Check an item in => BOOM, the issue_id is a PK in old_issues and the move fails.
Before bug 18242 the data were lost, we inserted the value into old_issues, which fails silently (because of RaiseError set to 0 in Koha::Database), then delete the row from issues.
That has been fixed using a transaction.

This patch introduced a regression we tried to fix on bug 18651 comment 0, the patron was charged even if the checkin was rejected.
A good way to fix that would have been to LOCK the tables:
1- Start a transaction
2- LOCK the table to make sure nobody will read id and avoid race conditions
3- Move the content from one table to the other, dealing with ids
4- UNLOCK the table
5- Commit the transaction
But there were problems using LOCK and DBIx::Class (See commit 905572910b3a - Do no LOCK/UNLOCK the table).

Finally the solution implemented is not acceptable for several reasons:
- batch checkins may fail
- issue_id will always stay out of sync (between issues and old_issues)
See 18651 comment 66.

Since the next stable releases are very soon, and we absolutely need to fix this problem, I am suggesting to:
1- Execute the move in a transaction to avoid data lost and reject the checkin if we face IDs dup
=> It will only reject 1 checkin (max is 1 * MySQL restart), no need to deal with race conditions,
2- Display a warning on the checkin page and link to a solution/explanation
3- Communicate as much as we can on the proper fix: Update auto increment values when the DBMS is restarted - https://wiki.koha-community.org/wiki/DBMS_auto_increment_fix
4- Display a warning on the about page for corrupted data (see bug 18931)
5- Write and make available a maintenance script to fix corrupted data (TODO LATER)

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 11 Tomás Cohen Arazi 2017-07-24 18:12:16 UTC
Created attachment 65205 [details] [review]
(16.11.x) Bug 18966: Do not deal with duplicate issue_id on checkin

Koha suffers of big bugs due to its history: When data are deleted, they are moved to another tables.
For instance issues and old_issues: when a checkin is done, it is moved to the old_issues table.
That leads to a main problem that is described on https://wiki.koha-community.org/wiki/DBMS_auto_increment_fix

However we tried first to fix the problem (for issues/old_issues) at code level on bug 18242.
The goal was to prevent data lost.
Data lost may happens in this case:
Check an item out (issue_id = 1)
Check an item in (issue_id = 1)
Restart MySQL (reset auto increment for issue_id to 1)
Check an item out (issue_id = 1)
Check an item in => BOOM, the issue_id is a PK in old_issues and the move fails.
Before bug 18242 the data were lost, we inserted the value into old_issues, which fails silently (because of RaiseError set to 0 in Koha::Database), then delete the row from issues.
That has been fixed using a transaction.

This patch introduced a regression we tried to fix on bug 18651 comment 0, the patron was charged even if the checkin was rejected.
A good way to fix that would have been to LOCK the tables:
1- Start a transaction
2- LOCK the table to make sure nobody will read id and avoid race conditions
3- Move the content from one table to the other, dealing with ids
4- UNLOCK the table
5- Commit the transaction
But there were problems using LOCK and DBIx::Class (See commit 905572910b3a - Do no LOCK/UNLOCK the table).

Finally the solution implemented is not acceptable for several reasons:
- batch checkins may fail
- issue_id will always stay out of sync (between issues and old_issues)
See 18651 comment 66.

Since the next stable releases are very soon, and we absolutely need to fix this problem, I am suggesting to:
1- Execute the move in a transaction to avoid data lost and reject the checkin if we face IDs dup
=> It will only reject 1 checkin (max is 1 * MySQL restart), no need to deal with race conditions,
2- Display a warning on the checkin page and link to a solution/explanation
3- Communicate as much as we can on the proper fix: Update auto increment values when the DBMS is restarted - https://wiki.koha-community.org/wiki/DBMS_auto_increment_fix
4- Display a warning on the about page for corrupted data (see bug 18931)
5- Write and make available a maintenance script to fix corrupted data (TODO LATER)

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 12 Katrin Fischer 2017-07-24 18:17:53 UTC
Created attachment 65206 [details] [review]
[16.11.x] Bug 18966: Do not deal with duplicate issue_id on checkin

Koha suffers of big bugs due to its history: When data are deleted, they are moved to another tables.
For instance issues and old_issues: when a checkin is done, it is moved to the old_issues table.
That leads to a main problem that is described on https://wiki.koha-community.org/wiki/DBMS_auto_increment_fix

However we tried first to fix the problem (for issues/old_issues) at code level on bug 18242.
The goal was to prevent data lost.
Data lost may happens in this case:
Check an item out (issue_id = 1)
Check an item in (issue_id = 1)
Restart MySQL (reset auto increment for issue_id to 1)
Check an item out (issue_id = 1)
Check an item in => BOOM, the issue_id is a PK in old_issues and the move fails.
Before bug 18242 the data were lost, we inserted the value into old_issues, which fails silently (because of RaiseError set to 0 in Koha::Database), then delete the row from issues.
That has been fixed using a transaction.

This patch introduced a regression we tried to fix on bug 18651 comment 0, the patron was charged even if the checkin was rejected.
A good way to fix that would have been to LOCK the tables:
1- Start a transaction
2- LOCK the table to make sure nobody will read id and avoid race conditions
3- Move the content from one table to the other, dealing with ids
4- UNLOCK the table
5- Commit the transaction
But there were problems using LOCK and DBIx::Class (See commit 905572910b3a - Do no LOCK/UNLOCK the table).

Finally the solution implemented is not acceptable for several reasons:
- batch checkins may fail
- issue_id will always stay out of sync (between issues and old_issues)
See 18651 comment 66.

Since the next stable releases are very soon, and we absolutely need to fix this problem, I am suggesting to:
1- Execute the move in a transaction to avoid data lost and reject the checkin if we face IDs dup
=> It will only reject 1 checkin (max is 1 * MySQL restart), no need to deal with race conditions,
2- Display a warning on the checkin page and link to a solution/explanation
3- Communicate as much as we can on the proper fix: Update auto increment values when the DBMS is restarted - https://wiki.koha-community.org/wiki/DBMS_auto_increment_fix
4- Display a warning on the about page for corrupted data (see bug 18931)
5- Write and make available a maintenance script to fix corrupted data (TODO LATER)

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 13 Tomás Cohen Arazi 2017-07-24 18:24:43 UTC
Created attachment 65207 [details] [review]
(16.05.x) Bug 18966: Do not deal with duplicate issue_id on checkin

Koha suffers of big bugs due to its history: When data are deleted, they are moved to another tables.
For instance issues and old_issues: when a checkin is done, it is moved to the old_issues table.
That leads to a main problem that is described on https://wiki.koha-community.org/wiki/DBMS_auto_increment_fix

However we tried first to fix the problem (for issues/old_issues) at code level on bug 18242.
The goal was to prevent data lost.
Data lost may happens in this case:
Check an item out (issue_id = 1)
Check an item in (issue_id = 1)
Restart MySQL (reset auto increment for issue_id to 1)
Check an item out (issue_id = 1)
Check an item in => BOOM, the issue_id is a PK in old_issues and the move fails.
Before bug 18242 the data were lost, we inserted the value into old_issues, which fails silently (because of RaiseError set to 0 in Koha::Database), then delete the row from issues.
That has been fixed using a transaction.

This patch introduced a regression we tried to fix on bug 18651 comment 0, the patron was charged even if the checkin was rejected.
A good way to fix that would have been to LOCK the tables:
1- Start a transaction
2- LOCK the table to make sure nobody will read id and avoid race conditions
3- Move the content from one table to the other, dealing with ids
4- UNLOCK the table
5- Commit the transaction
But there were problems using LOCK and DBIx::Class (See commit 905572910b3a - Do no LOCK/UNLOCK the table).

Finally the solution implemented is not acceptable for several reasons:
- batch checkins may fail
- issue_id will always stay out of sync (between issues and old_issues)
See 18651 comment 66.

Since the next stable releases are very soon, and we absolutely need to fix this problem, I am suggesting to:
1- Execute the move in a transaction to avoid data lost and reject the checkin if we face IDs dup
=> It will only reject 1 checkin (max is 1 * MySQL restart), no need to deal with race conditions,
2- Display a warning on the checkin page and link to a solution/explanation
3- Communicate as much as we can on the proper fix: Update auto increment values when the DBMS is restarted - https://wiki.koha-community.org/wiki/DBMS_auto_increment_fix
4- Display a warning on the about page for corrupted data (see bug 18931)
5- Write and make available a maintenance script to fix corrupted data (TODO LATER)

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 14 David Cook 2017-07-25 00:26:17 UTC
(In reply to Jonathan Druart from comment #8)
> (In reply to David Cook from comment #5)
> > Do you have a test plan for testers to follow for this one?
> 
> Nope, you should only test this if you understand the problem and followed
> the discussions on the previous bug reports.
> 

I understand the problem, but super time poor, so was hoping for some easy copy/pasting. Just me being lazy I guess :p.
Comment 15 Jonathan Druart 2017-07-25 12:56:32 UTC
(In reply to David Cook from comment #14)
> (In reply to Jonathan Druart from comment #8)
> > (In reply to David Cook from comment #5)
> > > Do you have a test plan for testers to follow for this one?
> > 
> > Nope, you should only test this if you understand the problem and followed
> > the discussions on the previous bug reports.
> > 
> 
> I understand the problem, but super time poor, so was hoping for some easy
> copy/pasting. Just me being lazy I guess :p.

For these patches we need people strongly involved in the testing process. We (and especially I) lost a lot of time because the previous tries have not been tested widely enough.
If you follow a test plan that I have written it will be nearly useless to validate it (since I already tested it when I wrote the patch). You will need to think about something I forgot.
Comment 16 Marcel de Rooy 2017-07-25 16:27:51 UTC
Will spend more time on this Friday if still needed..
Comment 17 David Cook 2017-07-26 00:23:41 UTC
(In reply to Jonathan Druart from comment #15)
> For these patches we need people strongly involved in the testing process.
> We (and especially I) lost a lot of time because the previous tries have not
> been tested widely enough.
> If you follow a test plan that I have written it will be nearly useless to
> validate it (since I already tested it when I wrote the patch). You will
> need to think about something I forgot.

By that token, every test plan is useless. 

Typically, I'll follow a test plan, and during the process I may try something else and see a problem with the patch. 

I'm really invested in fixing this bug. It constantly affects us, and while we have local workarounds, it drives me crazy. It's embarrassing constantly telling people that there is a problem with Koha that we can't immediately fix. 

But I suppose if it bothers me that much... maybe I should involve myself more in the testing process. 

I have to go buy some new headphones right now, but when I return, I will look at this again, as this is perhaps my biggest pet peeve about Koha at the moment.
Comment 18 David Cook 2017-07-26 01:18:34 UTC
With a new database, I did the following:

1. Add new biblio named 'Test'
2. Add new item with barcode 'test'
3. Checkout item 'test' to borrower 'test'
4. Checkin item 'test'

5a. Can't restart the mysqld process since I'm currently running other things on the same db server
5b. Run "OPTIMIZE TABLE issues".
#NOTE: This didn't update the auto_increment on issues. We're on MariaDB 10.0.28
5c. Run "truncate issues"
#NOTE: This did reset the auto_increment to 1.

6. Checkout item 'test' to borrower 'test'
#NOTE: It has a issue_id of 1
7. Checkin item 'test' from the checkout tab and observe message: 'Unable to check in'. 
8. Try to checkin from header/banner and observe message: 'The item has not been checked in due to a configuration issue in your system. You must ask an administrator to take a look at the about page and search for the "data problems" section'.

--

Functionally, I think this does the trick. 

In terms of messages, I think they could be improved. Maybe something like "Unable to check in due to duplicate ID. Ask a system administrator for assistance."
Comment 19 Julian Maurice 2017-07-26 09:53:18 UTC
Created attachment 65272 [details] [review]
Bug 18966: Do not deal with duplicate issue_id on checkin

Koha suffers of big bugs due to its history: When data are deleted, they
are moved to another tables.
For instance issues and old_issues: when a checkin is done, it is moved
to the old_issues table.
That leads to a main problem that is described on
https://wiki.koha-community.org/wiki/DBMS_auto_increment_fix

However we tried first to fix the problem (for issues/old_issues) at
code level on bug 18242.
The goal was to prevent data lost.
Data lost may happens in this case:
Check an item out (issue_id = 1)
Check an item in (issue_id = 1)
Restart MySQL (reset auto increment for issue_id to 1)
Check an item out (issue_id = 1)
Check an item in => BOOM, the issue_id is a PK in old_issues and the
move fails.
Before bug 18242 the data were lost, we inserted the value into
old_issues, which fails silently (because of RaiseError set to 0 in
Koha::Database), then delete the row from issues.
That has been fixed using a transaction.

This patch introduced a regression we tried to fix on bug 18651 comment
0, the patron was charged even if the checkin was rejected.
A good way to fix that would have been to LOCK the tables:
1- Start a transaction
2- LOCK the table to make sure nobody will read id and avoid race
   conditions
3- Move the content from one table to the other, dealing with ids
4- UNLOCK the table
5- Commit the transaction
But there were problems using LOCK and DBIx::Class (See commit
905572910b3a - Do no LOCK/UNLOCK the table).

Finally the solution implemented is not acceptable for several reasons:
- batch checkins may fail
- issue_id will always stay out of sync (between issues and old_issues)
See 18651 comment 66.

Since the next stable releases are very soon, and we absolutely need to
fix this problem, I am suggesting to:
1- Execute the move in a transaction to avoid data lost and reject the
   checkin if we face IDs dup
=> It will only reject 1 checkin (max is 1 * MySQL restart), no need to
   deal with race conditions,
2- Display a warning on the checkin page and link to a
   solution/explanation
3- Communicate as much as we can on the proper fix: Update auto
   increment values when the DBMS is restarted -
    https://wiki.koha-community.org/wiki/DBMS_auto_increment_fix
4- Display a warning on the about page for corrupted data (see bug
   18931)
5- Write and make available a maintenance script to fix corrupted data
   (TODO LATER)

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 20 Jonathan Druart 2017-07-26 16:53:04 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 21 Fridolin Somers 2017-07-27 09:21:31 UTC
Checking for 17.05.x :

UT passes but there is a new warn "Violation of unique constraint in OldIssue".

$ prove t/db_dependent/Circulation/Returns.t
t/db_dependent/Circulation/Returns.t .. 1/4 Violation of unique constraint in OldIssue at /home/koha/src/t/lib/TestBuilder.pm line 233.
t/db_dependent/Circulation/Returns.t .. ok   
All tests successful.
Files=1, Tests=4,  1 wallclock secs ( 0.01 usr  0.00 sys +  1.22 cusr  0.06 csys =  1.29 CPU)
Result: PASS
Comment 22 Fridolin Somers 2017-07-27 09:31:29 UTC
(In reply to Fridolin SOMERS from comment #21)
> Checking for 17.05.x :
> 
> UT passes but there is a new warn "Violation of unique constraint in
> OldIssue".
Only when old_issues already contains datas, so no problem.
Comment 23 Fridolin Somers 2017-07-27 09:39:41 UTC
Pushed to 17.05.x, will be in 17.05.02.

Tested with mysql restart, it's ok.

Just, we may use this error message in SIP serveur : C4/SIP/ILS/Transaction/Checkin.pm ?
Comment 24 Jonathan Druart 2017-07-27 13:18:22 UTC
(In reply to Fridolin SOMERS from comment #23)
> Just, we may use this error message in SIP serveur :
> C4/SIP/ILS/Transaction/Checkin.pm ?

What do you mean?
I do not think we should display a specific message in this case.
Comment 25 Mason James 2017-07-27 13:34:26 UTC
Created attachment 65293 [details] [review]
[16.05.x] Bug 18966: fix unneeded INSERT in AddReturn()
Comment 26 Mason James 2017-07-27 13:39:50 UTC
Created attachment 65294 [details] [review]
[16.11.x] Bug 18966: fix unneeded INSERT in AddReturn()
Comment 27 Katrin Fischer 2017-07-27 23:14:49 UTC
These patches have been pushed to 16.11.x and will be in 16.11.10.
Comment 28 David Cook 2017-07-28 00:44:20 UTC
Quick question... are there other bug reports for the other history tables?

I typically see this issue with the reserves/old_reserves table more than any other.
Comment 29 Marcel de Rooy 2017-07-28 09:29:06 UTC
(In reply to Jonathan Druart from comment #24)
> (In reply to Fridolin SOMERS from comment #23)
> > Just, we may use this error message in SIP serveur :
> > C4/SIP/ILS/Transaction/Checkin.pm ?
> 
> What do you mean?
> I do not think we should display a specific message in this case.

It seems that we still have a problem here.
If I check in an item where the issue id is already in old issues (I inserted this id for testing purposes), AddReturn correctly gives me back me a zero status and DataCorrupted message.
However, SIP somehow ignores it. It returns a 10 with ok=1 while it should return a 10 with ok=0.
The problem is in ILS.pm line 219
    $circ->ok( ( $checked_in_ok && $item ) || ( $item && $item->{patron} ) );
In this specific case $circ->ok was already set to false in do_checkin just before that line. But now this line sets ok back to true.
I guess this is a more fundamental problem. Because if AddReturn did set the ok flag to false for another reason, the same could happen. So I would propose to solve it on another report.
I am not fully sure if we can just test if $circ->ok is still true before running this line 219, and leave it false when it is false. 
@Colin: Could you have a look too ?

Opened bug 18996 for this *issue*.
Comment 30 Marcel de Rooy 2017-07-28 09:47:11 UTC
(In reply to Marcel de Rooy from comment #29)
> Opened bug 18996 for this *issue*.

The solution on 18996 is tied in with the fix on 17680.
Comment 31 Mason James 2017-07-31 11:01:29 UTC
Pushed to 16.05.x, for 16.05.15 release
Comment 32 Jonathan Druart 2017-08-01 15:59:18 UTC
Looks like the "update the fines" part should be removed (it should not hurt, no side-effects expected);

2177         # Update the fines
2178         $dbh->do(q|UPDATE accountlines SET issue_id = ? WHERE issue_id = ?|, undef, $old_checkout->issue_id, $issue->issue_id);
Comment 33 Katrin Fischer 2017-08-01 20:49:14 UTC
(In reply to Jonathan Druart from comment #32)
> Looks like the "update the fines" part should be removed (it should not
> hurt, no side-effects expected);
> 
> 2177         # Update the fines
> 2178         $dbh->do(q|UPDATE accountlines SET issue_id = ? WHERE issue_id
> = ?|, undef, $old_checkout->issue_id, $issue->issue_id);

hm?
Comment 34 Colin Campbell 2017-09-14 09:35:01 UTC
(In reply to Jonathan Druart from comment #32)
> Looks like the "update the fines" part should be removed (it should not
> hurt, no side-effects expected);
> 
> 2177         # Update the fines
> 2178         $dbh->do(q|UPDATE accountlines SET issue_id = ? WHERE issue_id
> = ?|, undef, $old_checkout->issue_id, $issue->issue_id);

Yes its a hangover from the stage when the code was changing issue_id in issues, its now at best a waste of cpu cycles
Comment 35 Jonathan Druart 2017-09-14 13:57:33 UTC
See bug 19317 for the removal of this line.