Bug 20086 - AddRenewal is not executed as a transaction and can results in partial success and doubled fines
Summary: AddRenewal is not executed as a transaction and can results in partial succes...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Nick Clemens
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on:
Blocks: 26457
  Show dependency treegraph
 
Reported: 2018-01-24 13:50 UTC by Nick Clemens
Modified: 2021-06-14 21:29 UTC (History)
13 users (show)

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


Attachments
Bug 20086 - AddRenewal is not executed as a transaction and can results in partial success and doubled fines (5.56 KB, patch)
2018-01-24 14:24 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 20086: Use txn_do (9.98 KB, patch)
2019-10-17 14:31 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 20086: Use txn_do (10.04 KB, patch)
2019-10-17 16:32 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20086: (follow-up) No need to call ->new on Koha::Database (1009 bytes, patch)
2019-10-17 16:32 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20086: Execute AddRenewal in a transaction to avoid partial success and doubled fines (10.72 KB, patch)
2019-10-21 07:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20086: (follow-up) No need to call ->new on Koha::Database (1.06 KB, patch)
2019-10-21 07:08 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2018-01-24 13:50:11 UTC
I can't recreate on demand, however, I can confirm several reports in the wild.

On a large/busy system, grep for "Deadlock" in the plack error logs. On several sites we are seeing occasional:
DBD::mysql::st execute failed: Deadlock found when trying to get lock; try restarting transaction [for Statement "UPDATE issues SET date_due = ?, renewals = ?, lastreneweddate = ?
                            WHERE borrowernumber=? 
                            AND itemnumber=?" with ParamValues: 0='2099-01-01 23:59', 1=2, 2='2099-01-01', 3="#####", 4="#####"] at /usr/share/koha/lib/C4/Circulation.pm line 2895. 

(dates and numbers altered for privacy)


In this case the fines are closed, but when the issue update fails the librarian repeats the renewal and fines are then recreated (koha finds no open fine, so calculates a new one)

Ideally the whole sub would be a transaction
Comment 1 Nick Clemens 2018-01-24 14:24:07 UTC
Created attachment 70879 [details] [review]
Bug 20086 - AddRenewal is not executed as a transaction and can results in partial success and doubled fines

This patch starts a transaction and only commits if renewal and fines
updates and charges are successful (partial in any cna be problematic)

There is no feedback (as currently there is none either) but if part
fails, all fails.

I didn't include stats and notifications in the transaction, but we
could.

To test:
1 - Apply patch
2 - prove t/db_dependent/Circulation.t
3 - Attempt circs and renewals should be no difference
4 - If possible make part of transaction fail and ensure all fails
Comment 2 Marcel de Rooy 2018-01-24 16:03:27 UTC
What if we are already in a transaction? Several unit tests include a call to AddRenewal..
Comment 3 Marcel de Rooy 2018-01-25 08:15:49 UTC
(In reply to Marcel de Rooy from comment #2)
> What if we are already in a transaction? Several unit tests include a call
> to AddRenewal..

Should not be a problem for DBIx. Only the outer transaction will commit. Use of txn_do is recommended.
Comment 4 Nick Clemens 2019-10-17 14:31:54 UTC
Created attachment 94374 [details] [review]
Bug 20086: Use txn_do
Comment 5 Martin Renvoize 2019-10-17 16:32:00 UTC
Created attachment 94377 [details] [review]
Bug 20086: Use txn_do

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2019-10-17 16:32:03 UTC
Created attachment 94378 [details] [review]
Bug 20086: (follow-up) No need to call ->new on Koha::Database

We should use the existing pattern of calling schema directly on
Koha::Database rather than creating a new object.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Jonathan Druart 2019-10-21 07:08:00 UTC
Created attachment 94421 [details] [review]
Bug 20086: Execute AddRenewal in a transaction to avoid partial success and doubled fines

This patch starts a transaction and only commits if renewal and fines
updates and charges are successful (partial in any cna be problematic)

There is no feedback (as currently there is none either) but if part
fails, all fails.

I didn't include stats and notifications in the transaction, but we
could. (Edit JD: not true, they are included)

To test:
1 - Apply patch
2 - prove t/db_dependent/Circulation.t
3 - Attempt circs and renewals should be no difference
4 - If possible make part of transaction fail and ensure all fails

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Amended: commit title and one indendation (return statement)
Comment 8 Jonathan Druart 2019-10-21 07:08:04 UTC
Created attachment 94422 [details] [review]
Bug 20086: (follow-up) No need to call ->new on Koha::Database

We should use the existing pattern of calling schema directly on
Koha::Database rather than creating a new object.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 9 Martin Renvoize 2019-10-21 08:46:05 UTC
Nice work!

Pushed to master for 19.11.00
Comment 10 Fridolin Somers 2019-11-08 15:43:41 UTC
There is a conflict for 19.05.x.
Please provide dedicated patches.
Comment 11 David Cook 2020-09-15 01:11:35 UTC
According to Bug 26457, the same deadlocks are still occurring. 

Based on Didier's report, this whole sub transaction probably made it worse?

Interesting that the problem happened before adding the (too) large transaction though. 

In Didier's report, the lock contention seems to be due to the "WHERE borrowernumber =" causing the issuesborridx index to be locked. That shouldn't be an issue for this bug report, since there wasn't a large transaction that would cause waiting for locks...

I'm guessing that some of the fine logic is slow though, and since the renewals are processed asynchronously, doing a large volume of renewals at once could cause lock contention I suppose...

Anyway, best to continue this at Bug 26457...