Bug 34678 - Concurrent changes to the holds can fail due to primary key constraints
Summary: Concurrent changes to the holds can fail due to primary key constraints
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens (kidclamp)
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-08-31 16:27 UTC by Emily Lamancusa
Modified: 2023-11-13 15:23 UTC (History)
11 users (show)

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


Attachments
Bug 34678: Unit test (1.74 KB, patch)
2023-09-13 17:10 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 34678: Allow new entries to overwrite hold_fill_targets (1.59 KB, patch)
2023-09-13 17:10 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 34678: Unit test (1.81 KB, patch)
2023-09-14 18:47 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 34678: Allow new entries to overwrite hold_fill_targets (1.66 KB, patch)
2023-09-14 18:47 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 34678: Unit test (2.30 KB, patch)
2023-10-26 11:55 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 34678: Allow new entries to overwrite hold_fill_targets (2.65 KB, patch)
2023-10-26 11:55 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 34678: Unit test (2.37 KB, patch)
2023-10-26 19:51 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 34678: Allow new entries to overwrite hold_fill_targets (2.72 KB, patch)
2023-10-26 19:51 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 34678: Unit test (2.46 KB, patch)
2023-10-27 06:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 34678: Allow new entries to overwrite hold_fill_targets (2.82 KB, patch)
2023-10-27 06:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Emily Lamancusa 2023-08-31 16:27:23 UTC
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.
Comment 1 Nick Clemens (kidclamp) 2023-09-13 17:10:38 UTC
Created attachment 155594 [details] [review]
Bug 34678: Unit test
Comment 2 Nick Clemens (kidclamp) 2023-09-13 17:10:40 UTC
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!
Comment 3 Emily Lamancusa 2023-09-14 18:47:19 UTC
Created attachment 155628 [details] [review]
Bug 34678: Unit test

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 4 Emily Lamancusa 2023-09-14 18:47:22 UTC
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>
Comment 5 Marcel de Rooy 2023-09-15 06:46:25 UTC
Should we use a MySQL extension like REPLACE ?
Comment 6 Kyle M Hall 2023-09-15 12:17:08 UTC
(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.
Comment 7 Tomás Cohen Arazi 2023-09-15 12:50:27 UTC
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.
Comment 8 Jonathan Druart 2023-09-18 20:27:00 UTC
Well, the consensus is/was to avoid MySQLisms...
Comment 9 Kyle M Hall 2023-09-19 10:37:35 UTC
(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!
Comment 10 Marcel de Rooy 2023-09-22 09:21:41 UTC
(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?
Comment 11 Nick Clemens (kidclamp) 2023-10-26 11:55:23 UTC
Created attachment 157913 [details] [review]
Bug 34678: Unit test
Comment 12 Nick Clemens (kidclamp) 2023-10-26 11:55:25 UTC
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!
Comment 13 Emily Lamancusa 2023-10-26 19:51:37 UTC
Created attachment 157950 [details] [review]
Bug 34678: Unit test

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 14 Emily Lamancusa 2023-10-26 19:51:40 UTC
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>
Comment 15 Marcel de Rooy 2023-10-27 06:25:39 UTC
QA: Looking here
Comment 16 Marcel de Rooy 2023-10-27 06:26:00 UTC
=head2 AddToHoldTargetMap

=cut

Hmm. Great documentation :)
Comment 17 Marcel de Rooy 2023-10-27 06:45:16 UTC
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>
Comment 18 Marcel de Rooy 2023-10-27 06:45:19 UTC
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>
Comment 19 Tomás Cohen Arazi 2023-10-27 19:52:18 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 20 Fridolin Somers 2023-11-08 07:37:26 UTC
Pushed to 23.05.x for 23.05.06
Comment 21 Matt Blenkinsop 2023-11-13 15:23:12 UTC
Nice work everyone!

Pushed to oldstable for 22.11.x