In bug 34656, it came to light that, if a real-time holds queue update and the holds queue builder cron try to update the same bib at the same time, it can cause the cron to fail. Specifically, if a real-time update adds a particular item to hold_fill_targets, and the holds queue builder tries to add the same item afterwards, the cron will fail with an error like: C4::HoldsQueue::AddToHoldTargetMap(): DBI Exception: DBD::mysql::st execute failed: Duplicate entry '464273' for key 'PRIMARY' at /usr/share/koha/lib/C4/HoldsQueue.pm line 1015 The biggest issue here is that any remaining holds that the cron has not yet processed will be left out of the holds queue until something else triggers them to update. (in our system, the issue in bug 34656 was occurring regularly, and typically left over 1,000 holds out of the holds queue!) If the holds queue builder can be forced to run while RealTimeHoldsQueue is enabled, it needs a way to avoid crashing if a real-time update happens to add a bib back into the queue before the queue builder gets to it.
Created attachment 155594 [details] [review] Bug 34678: Unit test
Created attachment 155595 [details] [review] Bug 34678: Allow new entries to overwrite hold_fill_targets When using background jobs, there is a possibility of a race condition where two jobs will be updating the holds queue for the same biblio. We should try to minimize those cases (see bug 34596) In the meantime though, we should prevent jobs possibly dying, and allow the most recent update to succeed. There is a possibility two updates wil assign different items to the same reserve, and that a reserve could end up in the queue twice, however, whichever one is filled first will delete both entries. as filling the hold deletes by reserve id (see bug 24359) To test: 1 - prove -v t/db_dependent/Reserves.t 2 - It fails 3 - Apply patch 4 - t/db_dependent/Reserves.t 5 - It succeeds!
Created attachment 155628 [details] [review] Bug 34678: Unit test Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Created attachment 155629 [details] [review] Bug 34678: Allow new entries to overwrite hold_fill_targets When using background jobs, there is a possibility of a race condition where two jobs will be updating the holds queue for the same biblio. We should try to minimize those cases (see bug 34596) In the meantime though, we should prevent jobs possibly dying, and allow the most recent update to succeed. There is a possibility two updates wil assign different items to the same reserve, and that a reserve could end up in the queue twice, however, whichever one is filled first will delete both entries. as filling the hold deletes by reserve id (see bug 24359) To test: 1 - prove -v t/db_dependent/Reserves.t 2 - It fails 3 - Apply patch 4 - t/db_dependent/Reserves.t 5 - It succeeds! Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Should we use a MySQL extension like REPLACE ?
(In reply to Marcel de Rooy from comment #5) > Should we use a MySQL extension like REPLACE ? It appears that MERGE is the closest SQL standard keyword with the same functionality. However, MySQL/MariaDB doesn't support it. There is also no DBIC alternative to use as universal fix. The alternative would be to run a delete query with the insert as a transaction. That feels rather clunky and inefficient to simply avoid a specific use of this keyword.
Maybe it is time to discuss (again) if we're gonna take advantage of the supported DB engine features. This discussion should take flame somewhere else I think.
Well, the consensus is/was to avoid MySQLisms...
(In reply to Jonathan Druart from comment #8) > Well, the consensus is/was to avoid MySQLisms... I that case I think the atomic delete + commit is the best course of action!
(In reply to Kyle M Hall from comment #9) > (In reply to Jonathan Druart from comment #8) > > Well, the consensus is/was to avoid MySQLisms... > > I that case I think the atomic delete + commit is the best course of action! OK, go ahead. Discussion over?
Created attachment 157913 [details] [review] Bug 34678: Unit test
Created attachment 157914 [details] [review] Bug 34678: Allow new entries to overwrite hold_fill_targets When using background jobs, there is a possibility of a race condition where two jobs will be updating the holds queue for the same biblio. We should try to minimize those cases (see bug 34596) In the meantime though, we should prevent jobs possibly dying, and allow the most recent update to succeed. There is a possibility two updates wil assign different items to the same reserve, and that a reserve could end up in the queue twice, however, whichever one is filled first will delete both entries. as filling the hold deletes by reserve id (see bug 24359) This patch adds a transaction to delete and then inset the new row To test: 1 - prove -v t/db_dependent/Reserves.t 2 - It fails 3 - Apply patch 4 - t/db_dependent/Reserves.t 5 - It succeeds!
Created attachment 157950 [details] [review] Bug 34678: Unit test Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Created attachment 157951 [details] [review] Bug 34678: Allow new entries to overwrite hold_fill_targets When using background jobs, there is a possibility of a race condition where two jobs will be updating the holds queue for the same biblio. We should try to minimize those cases (see bug 34596) In the meantime though, we should prevent jobs possibly dying, and allow the most recent update to succeed. There is a possibility two updates wil assign different items to the same reserve, and that a reserve could end up in the queue twice, however, whichever one is filled first will delete both entries. as filling the hold deletes by reserve id (see bug 24359) This patch adds a transaction to delete and then inset the new row To test: 1 - prove -v t/db_dependent/Reserves.t 2 - It fails 3 - Apply patch 4 - t/db_dependent/Reserves.t 5 - It succeeds! Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
QA: Looking here
=head2 AddToHoldTargetMap =cut Hmm. Great documentation :)
Created attachment 157970 [details] [review] Bug 34678: Unit test Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 157971 [details] [review] Bug 34678: Allow new entries to overwrite hold_fill_targets When using background jobs, there is a possibility of a race condition where two jobs will be updating the holds queue for the same biblio. We should try to minimize those cases (see bug 34596) In the meantime though, we should prevent jobs possibly dying, and allow the most recent update to succeed. There is a possibility two updates wil assign different items to the same reserve, and that a reserve could end up in the queue twice, however, whichever one is filled first will delete both entries. as filling the hold deletes by reserve id (see bug 24359) This patch adds a transaction to delete and then inset the new row To test: 1 - prove -v t/db_dependent/Reserves.t 2 - It fails 3 - Apply patch 4 - t/db_dependent/Reserves.t 5 - It succeeds! Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Pushed to master for 23.11. Nice work everyone, thanks!
Pushed to 23.05.x for 23.05.06
Nice work everyone! Pushed to oldstable for 22.11.x