We recently saw a site perform a batch deletion of ~18k items (and subsequently 2k bibs) While running the jobs we suddenly started seeing all sorts of actions fail, most notably placing reserves, and errors in the logs like: DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Lock wait timeout exceeded; try restarting transaction at /usr/share/koha/lib/Koha/Object.pm line 170 I believe part of the problem is that the entire batch deletion is a single transaction. While we should make parts of this atomic, I don't think the entire job should succeed or fail on an error. I think we should commit each item, then each bib, then continue.
*** Bug 26077 has been marked as a duplicate of this bug. ***
Created attachment 161219 [details] [review] Bug 35165: Do not delete items in a transaction We are testing the deletion with safe_delete before removing, beyond that I think the intent was to delete all of the items, deleting some is better than not deleting any. To test: 1 - Apply patch, restart all 2 - Submit a batch deletion 3 - Confirm items are deleted
Created attachment 161220 [details] [review] Bug 35165: Tidy
(In reply to Nick Clemens from comment #0) > While we should make parts of this atomic, I don't think the entire job > should succeed or fail on an error. I think we should commit each item, then > each bib, then continue. Each item and bib are singular actions - I don't actually think we need any transactions at this level at all.
In "Details of job" after the deletion we have "Progress: 0/4" instead of "Progress: 4/4" without patch. In the same page, in "Report" we have "No items deleted." instead of "4 item(s) deleted." without patch.
The bug appears just if we select "Delete records if no items remain."
(In reply to Loïc from comment #5) > In "Details of job" after the deletion we have "Progress: 0/4" instead of > "Progress: 4/4" without patch. > > In the same page, in "Report" we have "No items deleted." instead of "4 > item(s) deleted." without patch. (In reply to Loïc from comment #6) > The bug appears just if we select "Delete records if no items remain." I do not recreate this - did you restart all after applying the patch?
> While we should make parts of this atomic, I don't think the entire job should succeed or fail on an error. I think it should. We should discuss that with other people. If something unexpected happened we should not commit.
(In reply to Jonathan Druart from comment #8) > > While we should make parts of this atomic, I don't think the entire job should succeed or fail on an error. > > I think it should. We should discuss that with other people. If something > unexpected happened we should not commit. The current situation places an unknown limit on the number that can be deleted - too many and you lock your system, can't circulate, can't catalog - this is very bad
(In reply to Jonathan Druart from comment #8) > I think it should. Can you elaborate? Nick presents a clear impact on the end-user and what appears to be a reasonable solution.
Created attachment 161368 [details] [review] Bug 35165: Do not delete items in a transaction We are testing the deletion with safe_delete before removing, beyond that I think the intent was to delete all of the items, deleting some is better than not deleting any. To test: 1 - Apply patch, restart all 2 - Submit a batch deletion 3 - Confirm items are deleted Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Created attachment 161369 [details] [review] Bug 35165: Tidy Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
(In reply to Jonathan Druart from comment #8) > If something > unexpected happened we should not commit. There's no expectation from the user that the entire batch of items will or will not delete as a group. So long as the user can reliably see which records were successfully deleted and which were not, the success of the job as a whole should not require every item succeed.
Created attachment 161370 [details] [review] Bug 35165: Tidy Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
This is how I implemented the background job at the beginning. If we decide to change how it should behave I am fine with that, but then we should have the same behaviour everywhere and make it clear (for end-users and devs).
The problem is mainly with the "retry". Say you have a "delete bib from 1 to 10". The first job deletes 1 to 5 but fails for 6 to 10. If you retry later after you fixed the deletion problem for 6-10, the second job (retry) will report failures for deletion 1 to 5.
Created attachment 161480 [details] [review] Bug 35165: Do not delete items in a transaction We are testing the deletion with safe_delete before removing, beyond that I think the intent was to delete all of the items, deleting some is better than not deleting any. To test: 1 - Apply patch, restart all 2 - Submit a batch deletion 3 - Confirm items are deleted Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
We should avoid separate tidy patches.
Apart from discussion, I dont see unit tests.
(In reply to Marcel de Rooy from comment #18) > We should avoid separate tidy patches. When we make a change that reindents a whole section, keeping the tidy patch separate makes rebasing far easier. Why should we avoid them?
(In reply to Nick Clemens from comment #20) > (In reply to Marcel de Rooy from comment #18) > > We should avoid separate tidy patches. > > When we make a change that reindents a whole section, keeping the tidy patch > separate makes rebasing far easier. Why should we avoid them? General rules always have exceptions :)