Bug 33987 - Combine multiple db updates in C4::ImportBatch::BatchCommitRecords for efficiency/avoiding possible deadlocks
Summary: Combine multiple db updates in C4::ImportBatch::BatchCommitRecords for effici...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Kyle M Hall (khall)
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-06-12 17:39 UTC by Kyle M Hall (khall)
Modified: 2023-12-28 20:47 UTC (History)
1 user (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.02,22.11.08
Circulation function:


Attachments
Bug 33987 - Combine multiple db updates one in BatchCommitRecords (2.34 KB, patch)
2023-06-12 18:34 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 33987 - Combine multiple db updates one in BatchCommitRecords (2.39 KB, patch)
2023-06-13 14:35 UTC, Sam Lau
Details | Diff | Splinter Review
Bug 33987 - Combine multiple db updates one in BatchCommitRecords (2.48 KB, patch)
2023-06-16 07:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33987 - Combine multiple db updates one in BatchCommitRecords (2.45 KB, patch)
2023-06-16 11:40 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 33987: Combine multiple db updates one in BatchCommitRecords (2.50 KB, patch)
2023-06-16 12:22 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall (khall) 2023-06-12 17:39:25 UTC
When replacing existing records BatchCommitRecords will the table import_records will be updated three times for three different fields by three different queries. Not only is this inefficient ( especially for large batches ), it seems that this is causing the dreaded "Lock wait timeout exceeded; try restarting transaction" error on some mysql/mariadb configurations.
Comment 1 Kyle M Hall (khall) 2023-06-12 18:34:00 UTC
Created attachment 152282 [details] [review]
Bug 33987 - Combine multiple db updates one in BatchCommitRecords

When replacing existing records BatchCommitRecords will the table import_records will be updated three times for three different fields by three different queries. Not only is this inefficient ( especially for large batches ), it seems that this is causing the dreaded "Lock wait timeout exceeded; try restarting transaction" error on some mysql/mariadb configurations.

1) Test plan
2) Download a marc record from Koha
3) Modify the title of that same bib in Koha
4) Stage the downloaded record and overlay the existing record
5) Verify the title has reverted to the original title from the
   downloaded record!
Comment 2 Sam Lau 2023-06-13 14:35:23 UTC
Created attachment 152297 [details] [review]
Bug 33987 - Combine multiple db updates one in BatchCommitRecords

When replacing existing records BatchCommitRecords will the table import_records will be updated three times for three different fields by three different queries. Not only is this inefficient ( especially for large batches ), it seems that this is causing the dreaded "Lock wait timeout exceeded; try restarting transaction" error on some mysql/mariadb configurations.

1) Test plan
2) Download a marc record from Koha
3) Modify the title of that same bib in Koha
4) Stage the downloaded record and overlay the existing record
5) Verify the title has reverted to the original title from the
   downloaded record!

Signed-off-by: Sam Lau <samalau@gmail.com>
Comment 3 David Cook 2023-06-16 06:37:53 UTC
Comment on attachment 152297 [details] [review]
Bug 33987 - Combine multiple db updates one in BatchCommitRecords

Review of attachment 152297 [details] [review]:
-----------------------------------------------------------------

::: C4/ImportBatch.pm
@@ +682,5 @@
>                  $query = "UPDATE import_auths SET matched_authid = ? WHERE import_record_id = ?";
>              }
> +            # Combine xml update, SetImportRecordOverlayStatus, and SetImportRecordStatus updates into a single update for efficiency, especially in a transaction
> +            my $sth = $dbh->prepare_cached("UPDATE import_records SET marcxml_old = ?, status = ?, overlay_status = ? WHERE import_record_id = ?");
> +            $sth->execute($oldxml, 'imported', 'match_applied',, $rowref->{'import_record_id'});

Is there a typo on this line? I see two commas in a row?
Comment 4 Marcel de Rooy 2023-06-16 07:10:44 UTC
Created attachment 152399 [details] [review]
Bug 33987 - Combine multiple db updates one in BatchCommitRecords

When replacing existing records BatchCommitRecords will the table import_records will be updated three times for three different fields by three different queries. Not only is this inefficient ( especially for large batches ), it seems that this is causing the dreaded "Lock wait timeout exceeded; try restarting transaction" error on some mysql/mariadb configurations.

1) Test plan
2) Download a marc record from Koha
3) Modify the title of that same bib in Koha
4) Stage the downloaded record and overlay the existing record
5) Verify the title has reverted to the original title from the
   downloaded record!

Signed-off-by: Sam Lau <samalau@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 5 Marcel de Rooy 2023-06-16 07:11:43 UTC
(In reply to David Cook from comment #3)
> Comment on attachment 152297 [details] [review] [review]
> Bug 33987 - Combine multiple db updates one in BatchCommitRecords
> 
> Review of attachment 152297 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: C4/ImportBatch.pm
> @@ +682,5 @@
> >                  $query = "UPDATE import_auths SET matched_authid = ? WHERE import_record_id = ?";
> >              }
> > +            # Combine xml update, SetImportRecordOverlayStatus, and SetImportRecordStatus updates into a single update for efficiency, especially in a transaction
> > +            my $sth = $dbh->prepare_cached("UPDATE import_records SET marcxml_old = ?, status = ?, overlay_status = ? WHERE import_record_id = ?");
> > +            $sth->execute($oldxml, 'imported', 'match_applied',, $rowref->{'import_record_id'});
> 
> Is there a typo on this line? I see two commas in a row?

I removed the second comma. It didnt matter but it should not be there
Comment 6 Marcel de Rooy 2023-06-16 07:12:48 UTC
(In reply to Kyle M Hall from comment #0)
> When replacing existing records BatchCommitRecords will the table
> import_records will be updated three times for three different fields by
> three different queries. Not only is this inefficient ( especially for large
> batches ), it seems that this is causing the dreaded "Lock wait timeout
> exceeded; try restarting transaction" error on some mysql/mariadb
> configurations.

Patch is fine.
Would be nice if this would prevent a deadlock. But I dont think that this is the ultimate solution. We need to dig a bit deeper still probably.
Continuing on 33972
Comment 7 Tomás Cohen Arazi (tcohen) 2023-06-16 11:40:35 UTC
Created attachment 152403 [details] [review]
Bug 33987 - Combine multiple db updates one in BatchCommitRecords

When replacing existing records BatchCommitRecords will the table import_records will be updated three times for three different fields by three different queries. Not only is this inefficient ( especially for large batches ), it seems that this is causing the dreaded "Lock wait timeout exceeded; try restarting transaction" error on some mysql/mariadb configurations.

1) Test plan
2) Download a marc record from Koha
3) Modify the title of that same bib in Koha
4) Stage the downloaded record and overlay the existing record
5) Verify the title has reverted to the original title from the
   downloaded record!

Signed-off-by: Sam Lau <samalau@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Katrin Fischer 2023-06-16 12:22:58 UTC
Created attachment 152406 [details] [review]
Bug 33987: Combine multiple db updates one in BatchCommitRecords

When replacing existing records BatchCommitRecords will the table import_records will be updated three times for three different fields by three different queries. Not only is this inefficient ( especially for large batches ), it seems that this is causing the dreaded "Lock wait timeout exceeded; try restarting transaction" error on some mysql/mariadb configurations.

1) Test plan
2) Download a marc record from Koha
3) Modify the title of that same bib in Koha
4) Stage the downloaded record and overlay the existing record
5) Verify the title has reverted to the original title from the
   downloaded record!

Signed-off-by: Sam Lau <samalau@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 9 Katrin Fischer 2023-06-16 12:23:34 UTC
Fixed the commit message ;)
Comment 10 Tomás Cohen Arazi (tcohen) 2023-06-16 13:55:58 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 11 Martin Renvoize (ashimema) 2023-07-17 12:22:04 UTC
Thanks for all the hard work!

Pushed to 23.05.x for the next release
Comment 12 Pedro Amorim 2023-07-18 10:50:27 UTC
Nice work everyone!

Pushed to 22.11.x for next release