Description
Tomás Cohen Arazi (tcohen)
2021-10-26 14:43:41 UTC
We also see this error in 20.11 sometimes for our nightly imports. 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 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 (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. + # report progress and commit + &$progress_callback($rec_num, $schema); + $schema->txn_begin; Should be the reverse. Commit then report progress (commit can fail). (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. We are also seeing this with the CLI scripts. (In reply to Katrin Fischer from comment #7) > We are also seeing this with the CLI scripts. Did the patch work for you? I am sorry, I could not test and it doesn't happen consistently. 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. 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> 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 Advanced hacking? Looking here Created attachment 128033 [details] [review] Bug 29325: (QA follow-up) Remove unexisting parameters of BatchRevertRecords There is no interval and callback as in BatchCommitRecords. 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, 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. 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. (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? 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". Tomas or Marcel, any chance for a follow-up? 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". 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. (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 :) 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? 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> 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> 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> 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> 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> Probably we should squash all this back into one patch 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 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. (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? (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. (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. 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. This is broken down to 20.11... would be great to have a fix that poses not a too big issue for backport. 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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; QA: Having a look here 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> 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> 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> 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> 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> Starting with a trivial rebase (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. There may be some issues here still Funny btw that kind of thing still is in the script: Number of items added: $num_items_deleted Will fix it. (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. 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> 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> (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. Pushed to master for 22.11. Nice work everyone, thanks! Won't apply cleanly to 22.05.x, can someone rebase this for 22.05 ASAP? 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> Thanks for the speedy rebase Nick! Backported to 22.05.x for 22.05.01 Hi Arthur and all, please back-port, this issue is present in 20.11 and pretty bad. 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. (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. 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> 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) 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 ? 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" }; +} (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? yup. done. 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
Backported: Pushed to 21.05.x branch for the upcoming release (21.05.20) Nothing to document. Thanks! Pushed to 21.11 for 21.11.12 |