Bug 35438 - Importing records can create too large transactions
Summary: Importing records can create too large transactions
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-11-29 18:05 UTC by Nick Clemens (kidclamp)
Modified: 2024-02-06 14:34 UTC (History)
9 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:
24.05.00,23.11.02,23.05.09


Attachments
Bug 35438: Transact each record import separately (2.34 KB, patch)
2023-11-30 17:11 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35438: Transact each record import separately (2.39 KB, patch)
2023-11-30 17:38 UTC, David Nind
Details | Diff | Splinter Review
Bug 35438: Transact each record import separately (2.45 KB, patch)
2023-12-01 11:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35438: Remove skip_intermediate_commit parameter (1.94 KB, patch)
2023-12-01 11:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35438: Transact each record import separately (2.54 KB, patch)
2024-01-12 10:19 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35438: Remove skip_intermediate_commit parameter (2.03 KB, patch)
2024-01-12 10:19 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 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 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 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 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 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 2024-02-02 17:49:11 UTC
Backported to 23.05.x for upcoming 23.05.09