Bug 29325 - commit_file.pl error 'Already in a transaction'
Summary: commit_file.pl error 'Already in a transaction'
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low critical (vote)
Assignee: Nick Clemens
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-10-26 14:43 UTC by Tomás Cohen Arazi
Modified: 2022-10-24 14:59 UTC (History)
14 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This fixes the command line script misc/commit_file.pl and manage staged MARC records tool in the staff interface so that imported records are processed. The error message from The command line script was failing with this error message "DBIx::Class::Storage::DBI::_exec_txn_begin(): DBI Exception: DBD::mysql::db begin_work failed: Already in a transaction at /kohadevbox/koha/C4/Biblio.pm line 303". In the staff interface, the processing of staged records would fail without any error messages.
Version(s) released in:
22.11.00,22.05.01,21.11.12,21.05.20


Attachments
Bug 29325: Fix commit_file.pl (3.13 KB, patch)
2021-10-26 22:30 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29325: Fix commit_file.pl (3.20 KB, patch)
2021-11-09 15:02 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29325: (QA follow-up) Remove unexisting parameters of BatchRevertRecords (966 bytes, patch)
2021-11-26 10:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29325: Fix commit_file.pl (3.37 KB, patch)
2022-06-01 20:00 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29325: (QA follow-up) Remove unexisting parameters of BatchRevertRecords (983 bytes, patch)
2022-06-01 20:00 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29325: Call progress callback one last time to confirm comppletion (1.08 KB, patch)
2022-06-01 20:00 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29325: Fix import from staff client (2.05 KB, patch)
2022-06-01 20:00 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29325: Handle the transaction in BatchCommitRecords (3.07 KB, patch)
2022-06-01 20:00 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29325: Fix commit_file.pl (3.37 KB, patch)
2022-06-21 11:18 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29325: (QA follow-up) Remove unexisting parameters of BatchRevertRecords (983 bytes, patch)
2022-06-21 11:18 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29325: Call progress callback one last time to confirm comppletion (1.22 KB, patch)
2022-06-21 11:18 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29325: Fix import from staff client (2.05 KB, patch)
2022-06-21 11:19 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29325: Handle the transaction in BatchCommitRecords (3.27 KB, patch)
2022-06-21 11:19 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29325: Fix commit_file.pl (3.43 KB, patch)
2022-06-21 15:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29325: (QA follow-up) Remove unexisting parameters of BatchRevertRecords (1.02 KB, patch)
2022-06-21 15:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29325: Call progress callback one last time to confirm comppletion (1.28 KB, patch)
2022-06-21 15:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29325: Fix import from staff client (2.12 KB, patch)
2022-06-21 15:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29325: Handle the transaction in BatchCommitRecords (3.31 KB, patch)
2022-06-21 15:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29325: Fix commit_file.pl (3.47 KB, patch)
2022-06-24 08:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29325: (QA follow-up) Remove unexisting parameters of BatchRevertRecords (1.06 KB, patch)
2022-06-24 08:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29325: Call progress callback one last time to confirm comppletion (1.19 KB, patch)
2022-06-24 08:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29325: Fix import from staff client (2.16 KB, patch)
2022-06-24 08:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29325: Handle the transaction in BatchCommitRecords (3.17 KB, patch)
2022-06-24 08:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29325: Fix commit_file.pl (7.53 KB, patch)
2022-06-24 13:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29325: (QA follow-up) Tidy up (3.02 KB, patch)
2022-06-24 13:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29325: [22.05.X] Fix commit_file.pl (9.19 KB, patch)
2022-06-24 16:36 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29325: [21.11.x] Fix commit_file.pl (9.07 KB, patch)
2022-07-01 10:28 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2021-10-26 14:43:41 UTC
To reproduce:
1. Pick a file for staging:
   $ kshell
  k$ misc/stage_file.pl --file TestDataImportKoha.mrc
=> SUCCESS: All good
2. Commit!
  k$ misc/commit_file.pl --batch-number 1
=> FAIL: You see
DBIx::Class::Storage::DBI::_exec_txn_begin(): DBI Exception: DBD::mysql::db begin_work failed: Already in a transaction at /kohadevbox/koha/C4/Biblio.pm line 303
Comment 1 Katrin Fischer 2021-10-26 14:58:12 UTC
We also see this error in 20.11 sometimes for our nightly imports.
Comment 2 Tomás Cohen Arazi 2021-10-26 22:30:11 UTC
I'm submitting what I made to have commit_file.pl work. But I'm not sure how to solve the manage-marc-import.pl which I assume my patch breaks. I found debugging the forked process impossible. Probably because I was too tired.

CCing people that could help before I wake up :-D
Comment 3 Tomás Cohen Arazi 2021-10-26 22:30:34 UTC
Created attachment 126944 [details] [review]
Bug 29325: Fix commit_file.pl

This patch fixes the broken commit_file.pl script.

It doesn't deal with commiting the import from the UI.

To test:
1. Pick a file for staging:
   $ kshell
  k$ misc/stage_file.pl --file TestDataImportKoha.mrc
=> SUCCESS: All good
2. Commit!
  k$ misc/commit_file.pl --batch-number 1
=> FAIL: You see
DBIx::Class::Storage::DBI::_exec_txn_begin(): DBI Exception: DBD::mysql::db begin_work failed: Already in a transaction at /kohadevbox/koha/C4/Biblio.pm line 303
3. Apply this patch
4. Repeat 2
=> SUCCESS: Commit succeeds
5. Sign off :-D
Comment 4 Jonathan Druart 2021-10-27 13:00:25 UTC
(In reply to Tomás Cohen Arazi from comment #2)
> I'm submitting what I made to have commit_file.pl work. But I'm not sure how
> to solve the manage-marc-import.pl which I assume my patch breaks. I found
> debugging the forked process impossible. Probably because I was too tired.

You need to remove the "close STDERR;" statement of the child.


What is broken from the UI? I managed to stage and import.
Comment 5 Jonathan Druart 2021-10-27 13:06:08 UTC
+            # report progress and commit
+            &$progress_callback($rec_num, $schema);
+            $schema->txn_begin;

Should be the reverse. Commit then report progress (commit can fail).
Comment 6 Tomás Cohen Arazi 2021-10-27 16:41:07 UTC
(In reply to Jonathan Druart from comment #5)
> +            # report progress and commit
> +            &$progress_callback($rec_num, $schema);
> +            $schema->txn_begin;
> 
> Should be the reverse. Commit then report progress (commit can fail).

The code is a bit tricky. The commit_file.pl-defined callback actually does the commit. So we are basically calling the callback (because we reached the max chunk size), which commits the chunk, and then start a new transaction for the rest of the work.

This actually works for commit_file.pl.

And it is probably why I found manage-marc-import.pl not working: because it originally didn't commit inside the callback.
Comment 7 Katrin Fischer 2021-10-27 21:00:56 UTC
We are also seeing this with the CLI scripts.
Comment 8 Tomás Cohen Arazi 2021-10-28 10:58:42 UTC
(In reply to Katrin Fischer from comment #7)
> We are also seeing this with the CLI scripts.

Did the patch work for you?
Comment 9 Katrin Fischer 2021-10-28 20:10:02 UTC
I am sorry, I could not test and it doesn't happen consistently.
Comment 10 Tomás Cohen Arazi 2021-10-29 10:54:24 UTC
Setting NSO as it worked for Jonathan on the UI, and we need more eyes here. This is broken in stable. I will try again today.
Comment 11 Martin Renvoize 2021-11-09 15:02:03 UTC
Created attachment 127489 [details] [review]
Bug 29325: Fix commit_file.pl

This patch fixes the broken commit_file.pl script.

It doesn't deal with commiting the import from the UI.

To test:
1. Pick a file for staging:
   $ kshell
  k$ misc/stage_file.pl --file TestDataImportKoha.mrc
=> SUCCESS: All good
2. Commit!
  k$ misc/commit_file.pl --batch-number 1
=> FAIL: You see
DBIx::Class::Storage::DBI::_exec_txn_begin(): DBI Exception: DBD::mysql::db begin_work failed: Already in a transaction at /kohadevbox/koha/C4/Biblio.pm line 303
3. Apply this patch
4. Repeat 2
=> SUCCESS: Commit succeeds
5. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2021-11-09 15:03:15 UTC
This also worked for me.. though I'm not really sure why when reading the code.. we seem to mix up commit calls and nest forks inside txn_do subs.. which all confuses me.


But.. it seems to work.. so signing off and leaving for bigger brains to QA ;P
Comment 13 Marcel de Rooy 2021-11-26 09:44:44 UTC
Advanced hacking?
Comment 14 Marcel de Rooy 2021-11-26 10:27:54 UTC
Looking here
Comment 15 Marcel de Rooy 2021-11-26 10:30:27 UTC
Created attachment 128033 [details] [review]
Bug 29325: (QA follow-up) Remove unexisting parameters of BatchRevertRecords

There is no interval and callback as in BatchCommitRecords.
Comment 16 Marcel de Rooy 2021-11-26 10:44:41 UTC
git grep BatchCommitRecords
C4/ImportBatch.pm:      BatchCommitRecords
C4/ImportBatch.pm:=head2 BatchCommitRecords
C4/ImportBatch.pm:        BatchCommitRecords($batch_id, $framework,
C4/ImportBatch.pm:sub BatchCommitRecords {

misc/commit_file.pl:use C4::ImportBatch qw( GetAllImportBatches GetImportBatch BatchCommitRecords BatchRevertRecords );
misc/commit_file.pl:        BatchCommitRecords($import_batch_id, $framework, 100, \&print_progress_and_commit);
misc/cronjobs/import_webservice_batch.pl:use C4::ImportBatch qw( BatchCommitRecords );
misc/cronjobs/import_webservice_batch.pl:BatchCommitRecords($_, $framework) foreach @$batch_ids;
svc/import_bib:use C4::ImportBatch qw( GetWebserviceBatchId AddBiblioToBatch AddItemsToImportBiblio BatchFindDuplicates BatchCommitRecords );
svc/import_bib:    BatchCommitRecords($batch_id, $framework) if lc($import_mode) eq 'direct';
tools/manage-marc-import.pl:use C4::ImportBatch qw( CleanBatch DeleteBatch GetImportBatch GetImportBatchOverlayAction GetImportBatchNoMatchAction GetImportBatchItemAction SetImportBatchOverlayAction SetImportBatchNoMatchAction SetImportBatchItemAction BatchFindDuplicates SetImportBatchMatcher GetItemNumbersFromImportBatch GetImportBatchRangeDesc GetNumberOfNonZ3950ImportBatches BatchCommitRecords BatchRevertRecords );
tools/manage-marc-import.pl:              = BatchCommitRecords( $import_batch_id, $framework, 50,
Comment 17 Marcel de Rooy 2021-11-26 10:50:00 UTC
We really should not add these txn begin etc statements in the ImportBatch module. Not only is it ugly, we probably break other scripts using this routine.

Before resolving this issue, we should address the question: Do we really want to do commits along the way? And import a partial batch potentially? I think we should not. So that makes life simpler. Do a commit at the end in the misc_file script. Dont touch the module.

When just commenting three lines, the script already works:
diff --git a/misc/commit_file.pl b/misc/commit_file.pl
index 543d7b06c7..e1ff4c4bd8 100755
--- a/misc/commit_file.pl
+++ b/misc/commit_file.pl
@@ -46,7 +46,7 @@ if ($list_batches) {
 C4::Context->set_userenv(0, 'batch', 0, 'batch', 'batch', 'batch', 'batch');

 my $dbh = C4::Context->dbh;
-$dbh->{AutoCommit} = 0;
+#$dbh->{AutoCommit} = 0;
 if ($batch_number =~ /^\d+$/ and $batch_number > 0) {
     my $batch = GetImportBatch($batch_number);
     die "$0: import batch $batch_number does not exist in database\n" unless defined $batch;
@@ -59,7 +59,7 @@ if ($batch_number =~ /^\d+$/ and $batch_number > 0) {
             unless $batch->{'import_status'} eq "staged" or $batch->{'import_status'} eq "reverted";
         process_batch($batch_number);
     }
-    $dbh->commit();
+#    $dbh->commit();
 } else {
     die "$0: please specify a numeric batch ID\n";
 }
@@ -132,7 +132,7 @@ _SUMMARY_
 sub print_progress_and_commit {
     my $recs = shift;
     print "... processed $recs records\n";
-    $dbh->commit();
+#    $dbh->commit();
 }

 sub print_usage {

But note that I now also commit when we fail halfway.

The name of the progress callback should be adjusted.
Comment 18 Tomás Cohen Arazi 2021-11-26 11:02:58 UTC
The whole area clearly needs a big rewrite... I didn't intend to do it as part of the bugfix because this is hurting us in stable, and backportability is important in this case.

With a big transaction, you will minimize the chances of table lock issues, but you will also need to work on refactoring the reporting.
Comment 19 Marcel de Rooy 2021-11-26 11:07:43 UTC
(In reply to Tomás Cohen Arazi from comment #18)
> With a big transaction, you will minimize the chances of table lock issues,
> but you will also need to work on refactoring the reporting.

What do you mean? Skip the commit, print a number?
Comment 20 Ulrich Kleiber 2021-12-15 14:45:49 UTC
Under Koha version 20.11 we have the following problem with this patch.

The commit_file.pl script (CLI) runs through without error message.

Example:

MARC record import report
----------------------------------------
Batch number:                    1
Number of new records added:     25315
Number of records replaced:      0
Number of records ignored:       0
Number of items added:           26973
Number of items replaced:        0
Number of items ignored:         0

In reality, however, not all bibliographic records are imported. In this example the last 14 records are missing.
On the Staff page "Manage staged MARC records" the job status is displayed as "Importing" and the last 14 titles have the status "Staged".

After a reset, another attempt with the same data resulted in the same problem, except that this time 16 records remained with the status "Staged".
Comment 21 Katrin Fischer 2021-12-23 10:34:47 UTC
Tomas or Marcel, any chance for a follow-up?
Comment 22 Ulrich Kleiber 2021-12-23 11:08:26 UTC
Without this patch the error message appears again, but all bibliographic records are imported.

(In reply to Ulrich Kleiber from comment #20)
> Under Koha version 20.11 we have the following problem with this patch.
> 
> The commit_file.pl script (CLI) runs through without error message.
> 
> Example:
> 
> MARC record import report
> ----------------------------------------
> Batch number:                    1
> Number of new records added:     25315
> Number of records replaced:      0
> Number of records ignored:       0
> Number of items added:           26973
> Number of items replaced:        0
> Number of items ignored:         0
> 
> In reality, however, not all bibliographic records are imported. In this
> example the last 14 records are missing.
> On the Staff page "Manage staged MARC records" the job status is displayed
> as "Importing" and the last 14 titles have the status "Staged".
> 
> After a reset, another attempt with the same data resulted in the same
> problem, except that this time 16 records remained with the status "Staged".
Comment 23 Katrin Fischer 2022-01-07 08:47:02 UTC
We are using this script not only in migrations, but also for our nightly updates from the union catalog. Since we updated the server from 18.11 to 20.11 we are seeing the error every night.

I'd be really glad to see this resolved.
Comment 24 Marcel de Rooy 2022-01-10 09:51:23 UTC
(In reply to Katrin Fischer from comment #21)
> Tomas or Marcel, any chance for a follow-up?

I would be very happy to review any follow-ups, Tomas :)
Comment 25 Katrin Fischer 2022-03-23 10:40:51 UTC
This is still a major issue for us as we have over 130 installations receiving nightly updates and the error makes it hard to see real issues. Any chance we could get this moving again?
Comment 26 Nick Clemens 2022-06-01 20:00:13 UTC
Created attachment 135556 [details] [review]
Bug 29325: Fix commit_file.pl

This patch fixes the broken commit_file.pl script.

It doesn't deal with commiting the import from the UI.

To test:
1. Pick a file for staging:
   $ kshell
  k$ misc/stage_file.pl --file TestDataImportKoha.mrc
=> SUCCESS: All good
2. Commit!
  k$ misc/commit_file.pl --batch-number 1
=> FAIL: You see
DBIx::Class::Storage::DBI::_exec_txn_begin(): DBI Exception: DBD::mysql::db begin_work failed: Already in a transaction at /kohadevbox/koha/C4/Biblio.pm line 303
3. Apply this patch
4. Repeat 2
=> SUCCESS: Commit succeeds
5. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 27 Nick Clemens 2022-06-01 20:00:18 UTC
Created attachment 135557 [details] [review]
Bug 29325: (QA follow-up) Remove unexisting parameters of BatchRevertRecords

There is no interval and callback as in BatchCommitRecords.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 28 Nick Clemens 2022-06-01 20:00:22 UTC
Created attachment 135558 [details] [review]
Bug 29325: Call progress callback one last time to confirm comppletion

Previously after finishing the loop we were still in a transaction that never completed - we should report progress when done
one final time to commit the last records

To test:
1 - Stage a file with > 100 records
2 - Commit file
3 - Confirm batch is imported and no records left as staged

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 29 Nick Clemens 2022-06-01 20:00:26 UTC
Created attachment 135559 [details] [review]
Bug 29325: Fix import from staff client

same test as before, but via the staff client

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 30 Nick Clemens 2022-06-01 20:00:31 UTC
Created attachment 135560 [details] [review]
Bug 29325: Handle the transaction in BatchCommitRecords

Requiring the callback to commit was breaking reversion, and likely elsewhere

Let's simplify and say that the routine iteself will handle the txn and commit

TO test:
1 - Stage a file
2 - Import a file
3 - Revert a file
4 - Test staff client and command line

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 31 Nick Clemens 2022-06-01 20:01:11 UTC
Probably we should squash all this back into one patch
Comment 32 David Nind 2022-06-19 21:36:36 UTC
Patch no longer applies 8-(..

git bz apply 29325

Bug 29325 - commit_file.pl error 'Already in a transaction'

135556 - Bug 29325: Fix commit_file.pl
135557 - Bug 29325: (QA follow-up) Remove unexisting parameters of BatchRevertRecords
135558 - Bug 29325: Call progress callback one last time to confirm comppletion
135559 - Bug 29325: Fix import from staff client
135560 - Bug 29325: Handle the transaction in BatchCommitRecords

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 29325: Fix commit_file.pl
.git/rebase-apply/patch:16: trailing whitespace.
    # optional callback to monitor status 
error: sha1 information is lacking or useless (C4/ImportBatch.pm).
error: could not build fake ancestor
Patch failed at 0001 Bug 29325: Fix commit_file.pl
Comment 33 Johanna Räisä 2022-06-20 08:53:52 UTC
I found out that this is quite critical since it blocks the biblio import. The batch is marked as imported but the biblios aren't imported to biblio tables.
Comment 34 Katrin Fischer 2022-06-20 09:44:51 UTC
(In reply to Johanna Räisä from comment #33)
> I found out that this is quite critical since it blocks the biblio import.
> The batch is marked as imported but the biblios aren't imported to biblio
> tables.

I was assured it could be safely ignored :( 

How did you test Johanna? The new/unmatched records were missing?
Comment 35 Johanna Räisä 2022-06-20 10:28:12 UTC
(In reply to Katrin Fischer from comment #34)
> (In reply to Johanna Räisä from comment #33)
> > I found out that this is quite critical since it blocks the biblio import.
> > The batch is marked as imported but the biblios aren't imported to biblio
> > tables.
> 
> I was assured it could be safely ignored :( 
> 
> How did you test Johanna? The new/unmatched records were missing?

I ran commit_file.pl to a one new batch and the batch status change into "imported" and record statuses where "imported" but the new biblios didn't come to biblio_metadata, biblio and biblioitems. I guess the error prevents those to be saved.
Comment 36 Katrin Fischer 2022-06-20 11:06:08 UTC
(In reply to Johanna Räisä from comment #35)
> (In reply to Katrin Fischer from comment #34)
> > (In reply to Johanna Räisä from comment #33)
> > > I found out that this is quite critical since it blocks the biblio import.
> > > The batch is marked as imported but the biblios aren't imported to biblio
> > > tables.
> > 
> > I was assured it could be safely ignored :( 
> > 
> > How did you test Johanna? The new/unmatched records were missing?
> 
> I ran commit_file.pl to a one new batch and the batch status change into
> "imported" and record statuses where "imported" but the new biblios didn't
> come to biblio_metadata, biblio and biblioitems. I guess the error prevents
> those to be saved.

That's pretty horrible :( Thx, Johanna. Hope we can get a rebase soon and have updated severity.
Comment 37 Martin Renvoize 2022-06-20 16:39:37 UTC
I'm not sure this goes far enough either..

I'm tempted to have a go from scratch on a new bug.. I feel like we should be using 'https://metacpan.org/dist/DBIx-Class/view/lib/DBIx/Class/Manual/Cookbook.pod#Nested-transactions-and-auto-savepoints' here.
Comment 38 Katrin Fischer 2022-06-20 17:00:03 UTC
This is broken down to 20.11... would be great to have a fix that poses not a too big issue for backport.
Comment 39 Nick Clemens 2022-06-21 11:18:49 UTC
Created attachment 136390 [details] [review]
Bug 29325: Fix commit_file.pl

This patch fixes the broken commit_file.pl script.

It doesn't deal with commiting the import from the UI.

To test:
1. Pick a file for staging:
   $ kshell
  k$ misc/stage_file.pl --file TestDataImportKoha.mrc
=> SUCCESS: All good
2. Commit!
  k$ misc/commit_file.pl --batch-number 1
=> FAIL: You see
DBIx::Class::Storage::DBI::_exec_txn_begin(): DBI Exception: DBD::mysql::db begin_work failed: Already in a transaction at /kohadevbox/koha/C4/Biblio.pm line 303
3. Apply this patch
4. Repeat 2
=> SUCCESS: Commit succeeds
5. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 40 Nick Clemens 2022-06-21 11:18:54 UTC
Created attachment 136391 [details] [review]
Bug 29325: (QA follow-up) Remove unexisting parameters of BatchRevertRecords

There is no interval and callback as in BatchCommitRecords.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 41 Nick Clemens 2022-06-21 11:18:59 UTC
Created attachment 136392 [details] [review]
Bug 29325: Call progress callback one last time to confirm comppletion

Previously after finishing the loop we were still in a transaction that never completed - we should report progress when done
one final time to commit the last records

To test:
1 - Stage a file with > 100 records
2 - Commit file
3 - Confirm batch is imported and no records left as staged

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 42 Nick Clemens 2022-06-21 11:19:03 UTC
Created attachment 136393 [details] [review]
Bug 29325: Fix import from staff client

same test as before, but via the staff client

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 43 Nick Clemens 2022-06-21 11:19:08 UTC
Created attachment 136394 [details] [review]
Bug 29325: Handle the transaction in BatchCommitRecords

Requiring the callback to commit was breaking reversion, and likely elsewhere

Let's simplify and say that the routine iteself will handle the txn and commit

TO test:
1 - Stage a file
2 - Import a file
3 - Revert a file
4 - Test staff client and command line

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 44 Martin Renvoize 2022-06-21 15:28:49 UTC
Created attachment 136398 [details] [review]
Bug 29325: Fix commit_file.pl

This patch fixes the broken commit_file.pl script.

It doesn't deal with commiting the import from the UI.

To test:
1. Pick a file for staging:
   $ kshell
  k$ misc/stage_file.pl --file TestDataImportKoha.mrc
=> SUCCESS: All good
2. Commit!
  k$ misc/commit_file.pl --batch-number 1
=> FAIL: You see
DBIx::Class::Storage::DBI::_exec_txn_begin(): DBI Exception: DBD::mysql::db begin_work failed: Already in a transaction at /kohadevbox/koha/C4/Biblio.pm line 303
3. Apply this patch
4. Repeat 2
=> SUCCESS: Commit succeeds
5. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 45 Martin Renvoize 2022-06-21 15:28:54 UTC
Created attachment 136399 [details] [review]
Bug 29325: (QA follow-up) Remove unexisting parameters of BatchRevertRecords

There is no interval and callback as in BatchCommitRecords.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 46 Martin Renvoize 2022-06-21 15:28:58 UTC
Created attachment 136400 [details] [review]
Bug 29325: Call progress callback one last time to confirm comppletion

Previously after finishing the loop we were still in a transaction that never completed - we should report progress when done
one final time to commit the last records

To test:
1 - Stage a file with > 100 records
2 - Commit file
3 - Confirm batch is imported and no records left as staged

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 47 Martin Renvoize 2022-06-21 15:29:03 UTC
Created attachment 136401 [details] [review]
Bug 29325: Fix import from staff client

same test as before, but via the staff client

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 48 Martin Renvoize 2022-06-21 15:29:08 UTC
Created attachment 136402 [details] [review]
Bug 29325: Handle the transaction in BatchCommitRecords

Requiring the callback to commit was breaking reversion, and likely elsewhere

Let's simplify and say that the routine iteself will handle the txn and commit

TO test:
1 - Stage a file
2 - Import a file
3 - Revert a file
4 - Test staff client and command line

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 49 Martin Renvoize 2022-06-21 15:31:43 UTC
The code reads a fair bit better now, though there still some peculiarities.. it would be really nice to clean this up whilst moving it to the more modern background task queue implementation.

In my testing this appears to all work.. but I'd love to see some more tests to make sure I'm not imagining positives.. 

We need to confirm the command line version works, the staff client without pushing to background and the staff client with pushing to background all work still.

Whilst reading the code, it also felt like we could perhaps drop the following line.. but it's 'interesting'...

$dbh->{InactiveDestroy}  = 1;
Comment 50 Marcel de Rooy 2022-06-24 08:53:06 UTC
QA: Having a look here
Comment 51 Marcel de Rooy 2022-06-24 08:54:46 UTC
Created attachment 136478 [details] [review]
Bug 29325: Fix commit_file.pl

This patch fixes the broken commit_file.pl script.

It doesn't deal with commiting the import from the UI.

To test:
1. Pick a file for staging:
   $ kshell
  k$ misc/stage_file.pl --file TestDataImportKoha.mrc
=> SUCCESS: All good
2. Commit!
  k$ misc/commit_file.pl --batch-number 1
=> FAIL: You see
DBIx::Class::Storage::DBI::_exec_txn_begin(): DBI Exception: DBD::mysql::db begin_work failed: Already in a transaction at /kohadevbox/koha/C4/Biblio.pm line 303
3. Apply this patch
4. Repeat 2
=> SUCCESS: Commit succeeds
5. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 52 Marcel de Rooy 2022-06-24 08:54:51 UTC
Created attachment 136479 [details] [review]
Bug 29325: (QA follow-up) Remove unexisting parameters of BatchRevertRecords

There is no interval and callback as in BatchCommitRecords.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 53 Marcel de Rooy 2022-06-24 08:54:56 UTC
Created attachment 136480 [details] [review]
Bug 29325: Call progress callback one last time to confirm comppletion

Previously after finishing the loop we were still in a transaction that never completed - we should report progress when done
one final time to commit the last records

To test:
1 - Stage a file with > 100 records
2 - Commit file
3 - Confirm batch is imported and no records left as staged

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 54 Marcel de Rooy 2022-06-24 08:55:00 UTC
Created attachment 136481 [details] [review]
Bug 29325: Fix import from staff client

same test as before, but via the staff client

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 55 Marcel de Rooy 2022-06-24 08:55:05 UTC
Created attachment 136482 [details] [review]
Bug 29325: Handle the transaction in BatchCommitRecords

Requiring the callback to commit was breaking reversion, and likely elsewhere

Let's simplify and say that the routine iteself will handle the txn and commit

TO test:
1 - Stage a file
2 - Import a file
3 - Revert a file
4 - Test staff client and command line

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 56 Marcel de Rooy 2022-06-24 08:55:28 UTC
Starting with a trivial rebase
Comment 57 Marcel de Rooy 2022-06-24 09:12:27 UTC
(In reply to Nick Clemens from comment #31)
> Probably we should squash all this back into one patch

Yes, I think so. It is pretty confusing now.
Comment 58 Marcel de Rooy 2022-06-24 09:51:33 UTC
There may be some issues here still
Comment 59 Marcel de Rooy 2022-06-24 09:55:56 UTC
Funny btw that kind of thing still is in the script:

Number of items added:           $num_items_deleted

Will fix it.
Comment 60 Marcel de Rooy 2022-06-24 10:00:33 UTC
(In reply to Marcel de Rooy from comment #58)
> There may be some issues here still

The main thing I still need to check if why BatchRevert seems to work with the commit_file script but not via tools in interface.
Will finish Monday.
Comment 61 Marcel de Rooy 2022-06-24 13:36:05 UTC
Created attachment 136496 [details] [review]
Bug 29325: Fix commit_file.pl

This patch fixes the broken commit_file.pl script.

It doesn't deal with commiting the import from the UI.

To test:
1. Pick a file for staging:
   $ kshell
  k$ misc/stage_file.pl --file TestDataImportKoha.mrc
=> SUCCESS: All good
2. Commit!
  k$ misc/commit_file.pl --batch-number 1
=> FAIL: You see
DBIx::Class::Storage::DBI::_exec_txn_begin(): DBI Exception: DBD::mysql::db begin_work failed: Already in a transaction at /kohadevbox/koha/C4/Biblio.pm line 303
3. Apply this patch
4. Repeat 2
=> SUCCESS: Commit succeeds
5. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Bug 29325: (QA follow-up) Remove unexisting parameters of BatchRevertRecords

There is no interval and callback as in BatchCommitRecords.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Bug 29325: Call progress callback one last time to confirm comppletion

Previously after finishing the loop we were still in a transaction that never completed - we should report progress when done
one final time to commit the last records

To test:
1 - Stage a file with > 100 records
2 - Commit file
3 - Confirm batch is imported and no records left as staged

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Bug 29325: Fix import from staff client

same test as before, but via the staff client

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Bug 29325: Handle the transaction in BatchCommitRecords

Requiring the callback to commit was breaking reversion, and likely elsewhere

Let's simplify and say that the routine iteself will handle the txn and commit

TO test:
1 - Stage a file
2 - Import a file
3 - Revert a file
4 - Test staff client and command line

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 62 Marcel de Rooy 2022-06-24 13:36:10 UTC
Created attachment 136497 [details] [review]
Bug 29325: (QA follow-up) Tidy up

tools/manage-marc-import.pl: sub commit_batch does no longer need $schema
tools/manage-marc-import.pl: sub revert_batch: calling BatchRevertRecords which has no interval and callback
misc/commit_file.pl: sub print_progress_and_commit does no longer commit, renamed
misc/commit_file.pl: sub print_progress does no longer have a schema parameter
misc/commit_file.pl: sub revert_batch reported deleted items as added

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 63 Marcel de Rooy 2022-06-24 13:36:44 UTC
(In reply to Marcel de Rooy from comment #60)
> (In reply to Marcel de Rooy from comment #58)
> > There may be some issues here still
> 
> The main thing I still need to check if why BatchRevert seems to work with
> the commit_file script but not via tools in interface.
> Will finish Monday.

Resolved already :) Go ahead, Tomas.
Comment 64 Tomás Cohen Arazi 2022-06-24 14:38:00 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 65 Lucas Gass 2022-06-24 16:09:44 UTC
Won't apply cleanly to 22.05.x, can someone rebase this for 22.05 ASAP?
Comment 66 Nick Clemens 2022-06-24 16:36:28 UTC
Created attachment 136531 [details] [review]
Bug 29325: [22.05.X] Fix commit_file.pl

This patch fixes the broken commit_file.pl script.

It doesn't deal with commiting the import from the UI.

To test:
1. Pick a file for staging:
   $ kshell
  k$ misc/stage_file.pl --file TestDataImportKoha.mrc
=> SUCCESS: All good
2. Commit!
  k$ misc/commit_file.pl --batch-number 1
=> FAIL: You see
DBIx::Class::Storage::DBI::_exec_txn_begin(): DBI Exception: DBD::mysql::db begin_work failed: Already in a transaction at /kohadevbox/koha/C4/Biblio.pm line 303
3. Apply this patch
4. Repeat 2
=> SUCCESS: Commit succeeds
5. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Bug 29325: (QA follow-up) Remove unexisting parameters of BatchRevertRecords

There is no interval and callback as in BatchCommitRecords.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Bug 29325: Call progress callback one last time to confirm comppletion

Previously after finishing the loop we were still in a transaction that never completed - we should report progress when done
one final time to commit the last records

To test:
1 - Stage a file with > 100 records
2 - Commit file
3 - Confirm batch is imported and no records left as staged

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Bug 29325: Fix import from staff client

same test as before, but via the staff client

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Bug 29325: Handle the transaction in BatchCommitRecords

Requiring the callback to commit was breaking reversion, and likely elsewhere

Let's simplify and say that the routine iteself will handle the txn and commit

TO test:
1 - Stage a file
2 - Import a file
3 - Revert a file
4 - Test staff client and command line

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Bug 29325: (QA follow-up) Tidy up

tools/manage-marc-import.pl: sub commit_batch does no longer need $schema
tools/manage-marc-import.pl: sub revert_batch: calling BatchRevertRecords which has no interval and callback
misc/commit_file.pl: sub print_progress_and_commit does no longer commit, renamed
misc/commit_file.pl: sub print_progress does no longer have a schema parameter
misc/commit_file.pl: sub revert_batch reported deleted items as added

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 67 Lucas Gass 2022-06-24 16:46:04 UTC
Thanks for the speedy rebase Nick! 

Backported to 22.05.x for 22.05.01
Comment 68 Katrin Fischer 2022-06-27 11:18:45 UTC
Hi Arthur and all, please back-port, this issue is present in 20.11 and pretty bad.
Comment 69 Magnus Enger 2022-06-29 09:48:35 UTC
I can confirm the problem on 21.11.05. I applied the patch for 22.05 with one minor confict, and it seems to have fixed the problem.
Comment 70 Katrin Fischer 2022-06-29 09:54:01 UTC
(In reply to Magnus Enger from comment #69)
> I can confirm the problem on 21.11.05. I applied the patch for 22.05 with
> one minor confict, and it seems to have fixed the problem.

I've run a first test on 20.11 (also with 2 minor conflicts) and the import worked fine and created no errors. But hope we can do more extensive testing for our daily imports soon.
Comment 71 Nick Clemens 2022-07-01 10:28:43 UTC
Created attachment 136803 [details] [review]
Bug 29325: [21.11.x] Fix commit_file.pl

This patch fixes the broken commit_file.pl script.

It doesn't deal with commiting the import from the UI.

To test:
1. Pick a file for staging:
   $ kshell
  k$ misc/stage_file.pl --file TestDataImportKoha.mrc
=> SUCCESS: All good
2. Commit!
  k$ misc/commit_file.pl --batch-number 1
=> FAIL: You see
DBIx::Class::Storage::DBI::_exec_txn_begin(): DBI Exception: DBD::mysql::db begin_work failed: Already in a transaction at /kohadevbox/koha/C4/Biblio.pm line 303
3. Apply this patch
4. Repeat 2
=> SUCCESS: Commit succeeds
5. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Bug 29325: (QA follow-up) Remove unexisting parameters of BatchRevertRecords

There is no interval and callback as in BatchCommitRecords.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Bug 29325: Call progress callback one last time to confirm comppletion

Previously after finishing the loop we were still in a transaction that never completed - we should report progress when done
one final time to commit the last records

To test:
1 - Stage a file with > 100 records
2 - Commit file
3 - Confirm batch is imported and no records left as staged

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Bug 29325: Fix import from staff client

same test as before, but via the staff client

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Bug 29325: Handle the transaction in BatchCommitRecords

Requiring the callback to commit was breaking reversion, and likely elsewhere

Let's simplify and say that the routine iteself will handle the txn and commit

TO test:
1 - Stage a file
2 - Import a file
3 - Revert a file
4 - Test staff client and command line

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Bug 29325: (QA follow-up) Tidy up

tools/manage-marc-import.pl: sub commit_batch does no longer need $schema
tools/manage-marc-import.pl: sub revert_batch: calling BatchRevertRecords which has no interval and callback
misc/commit_file.pl: sub print_progress_and_commit does no longer commit, renamed
misc/commit_file.pl: sub print_progress does no longer have a schema parameter
misc/commit_file.pl: sub revert_batch reported deleted items as added

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 72 Martin Renvoize 2022-09-29 08:27:34 UTC
This one really is vitally important for 21.11's.. I've added the rel_21_11_candidate keyword to try and grab the attention of the rmaint (and left a later on irc too)
Comment 73 Marcel de Rooy 2022-09-29 08:36:58 UTC
Good catch, Martin.

Whats going wrong here? Why is a patch marked critical not going down the chain to the next RMaint?
Do we need to revisit the workflow ?
Comment 74 Jonathan Druart 2022-10-14 07:06:12 UTC
This reintroduced code removed by bug 28617.

+BEGIN {
+    # find Koha's Perl modules
+    # test carefully before changing this
+    use FindBin ();
+    eval { require "$FindBin::Bin/kohalib.pl" };
+}
Comment 75 Jonathan Druart 2022-10-14 14:00:22 UTC
(In reply to Martin Renvoize from comment #72)
> This one really is vitally important for 21.11's.. I've added the
> rel_21_11_candidate keyword to try and grab the attention of the rmaint (and
> left a later on irc too)

ping Arthur, did you get that?
Comment 76 Arthur Suzuki 2022-10-14 20:45:47 UTC
yup. done.
Comment 77 Victor Grousset/tuxayo 2022-10-17 00:44:42 UTC
Test plan is missing something:

> 3. Apply this patch
> 4. Repeat 2
> => SUCCESS: Commit succeeds
> 5. Sign off :-D

step 1 should be repeated as well. Otherwise there is nothing to commit and no error. And when repeating step two, change --batch-number from 1 to 2
Comment 78 Victor Grousset/tuxayo 2022-10-17 00:45:25 UTC
Backported: Pushed to 21.05.x branch for the upcoming release (21.05.20)

Nothing to document.
Comment 79 Arthur Suzuki 2022-10-22 22:07:34 UTC
Thanks!

Pushed to 21.11 for 21.11.12