Bug 35438

Summary: Importing records can create too large transactions
Product: Koha Reporter: Nick Clemens (kidclamp) <nick>
Component: ToolsAssignee: Kyle M Hall (khall) <kyle>
Status: RESOLVED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: normal    
Priority: P5 - low CC: dcook, fridolin.somers, jonathan.druart, kyle, lucas, m.de.rooy, mspinney, tomascohen, wainuiwitikapark
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35439
GIT URL: Change sponsored?: ---
Patch complexity: Small patch Documentation contact:
Documentation submission: Text to go in the release notes:
Version(s) released in:
24.05.00,23.11.02,23.05.09,22.11.28
Circulation function:
Attachments: Bug 35438: Transact each record import separately
Bug 35438: Transact each record import separately
Bug 35438: Transact each record import separately
Bug 35438: Remove skip_intermediate_commit parameter
Bug 35438: Transact each record import separately
Bug 35438: Remove skip_intermediate_commit parameter

Description Nick Clemens (kidclamp) 2023-11-29 18:05:37 UTC
When importing a staged file we commit only every 50 records

For an authority import we are also merging, which can affect many more biblios, and these all end up in the transaction.

This can cause tables locks and issues across Koha

Why do we not commit each record as we go?
Comment 1 David Cook 2023-11-29 22:21:26 UTC
(In reply to Nick Clemens from comment #0)
> When importing a staged file we commit only every 50 records
> 
> For an authority import we are also merging, which can affect many more
> biblios, and these all end up in the transaction.
> 
> This can cause tables locks and issues across Koha
> 
> Why do we not commit each record as we go?

Historically, it was probably for speed reasons, but since we're using background jobs I think committing each record as we go should be fine in terms of performance. Back in the day, we didn't use transactions either, so they wouldn't have been an issue back then.

I work on another system which is quite busy, and I've had to commit more frequently to avoid deadlocks and other transaction related issues. It is much slower but it's much more reliable.
Comment 2 Kyle M Hall (khall) 2023-11-30 17:11:58 UTC
Created attachment 159413 [details] [review]
Bug 35438: Transact each record import separately

When importing a staged file we commit every 50 records
For an authority import we are also merging, which can affect many more biblios, and these all end up in the transaction.
This can cause tables locks and issues across Koha

Test Plan:
1) Apply this patch
2) prove t/db_dependent/ImportBatch.t
Comment 3 David Nind 2023-11-30 17:38:11 UTC
Created attachment 159415 [details] [review]
Bug 35438: Transact each record import separately

When importing a staged file we commit every 50 records
For an authority import we are also merging, which can affect many more biblios, and these all end up in the transaction.
This can cause tables locks and issues across Koha

Test Plan:
1) Apply this patch
2) prove t/db_dependent/ImportBatch.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 4 Jonathan Druart 2023-12-01 06:51:40 UTC
The idea of surrounding the whole job with a transaction was to be able to "retry" it in case it fails. If we process only a subset it won't be possible (unless parsing the "report").
Comment 5 Kyle M Hall (khall) 2023-12-01 11:49:19 UTC
(In reply to Jonathan Druart from comment #4)
> The idea of surrounding the whole job with a transaction was to be able to
> "retry" it in case it fails. If we process only a subset it won't be
> possible (unless parsing the "report").

I think it was a great theory and and bad practice. Especially since we are transacting 50 at a time and not entire batches ( perish the thought! ).
Comment 6 Kyle M Hall (khall) 2023-12-01 11:53:42 UTC
Created attachment 159442 [details] [review]
Bug 35438: Transact each record import separately

When importing a staged file we commit every 50 records
For an authority import we are also merging, which can affect many more biblios, and these all end up in the transaction.
This can cause tables locks and issues across Koha

Test Plan:
1) Apply this patch
2) prove t/db_dependent/ImportBatch.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 7 Kyle M Hall (khall) 2023-12-01 11:53:47 UTC
Created attachment 159443 [details] [review]
Bug 35438: Remove skip_intermediate_commit parameter

It no longer does anything.
Comment 8 Marcel de Rooy 2024-01-12 10:07:32 UTC
Looking here
Comment 9 Marcel de Rooy 2024-01-12 10:19:47 UTC
Created attachment 160917 [details] [review]
Bug 35438: Transact each record import separately

When importing a staged file we commit every 50 records
For an authority import we are also merging, which can affect many more biblios, and these all end up in the transaction.
This can cause tables locks and issues across Koha

Test Plan:
1) Apply this patch
2) prove t/db_dependent/ImportBatch.t

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Marcel de Rooy 2024-01-12 10:19:49 UTC
Created attachment 160918 [details] [review]
Bug 35438: Remove skip_intermediate_commit parameter

It no longer does anything.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Katrin Fischer 2024-01-16 11:09:22 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 12 Fridolin Somers 2024-01-17 09:30:22 UTC
Pushed to 23.11.x for 23.11.02
Comment 13 Lucas Gass (lukeg) 2024-02-02 17:49:11 UTC
Backported to 23.05.x for upcoming 23.05.09
Comment 14 Wainui Witika-Park 2025-06-23 03:16:24 UTC
Applied to 22.11 for 22.11.28