Bug 25539 - Remove AddBiblio "defer_marc_save" option
Summary: Remove AddBiblio "defer_marc_save" option
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P3 normal (vote)
Assignee: David Gustafsson
QA Contact: Testopia
URL:
Keywords:
Depends on: 14957
Blocks:
  Show dependency treegraph
 
Reported: 2020-05-19 09:50 UTC by David Gustafsson
Modified: 2021-05-06 08:27 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 25539: Remove AddBiblio option "defer_marc_save" (7.42 KB, patch)
2020-05-19 10:05 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 25539: Remove AddBiblio option "defer_marc_save" (6.97 KB, patch)
2020-05-19 10:08 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 25539: Remove AddBiblio option "defer_marc_save" (7.04 KB, patch)
2020-09-09 10:59 UTC, Martin Renvoize
Details | Diff | Splinter Review
Strip items when adding new biblio to preserve previous behaviour of the script (1.75 KB, patch)
2020-09-17 12:11 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 25539: Strip items when adding new biblio to preserve previous behaviour of the script (1.71 KB, patch)
2020-09-17 12:13 UTC, David Gustafsson
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Gustafsson 2020-05-19 09:50:09 UTC
In bulkmarcimoprt.pl AddBiblio is called with defer_marc_save => 1. This perhaps made sense sometime when items where saved embedded with the MARCXML, but since that is no longer the case(?) there is no need for saving the MARC-data once more after processing items in bulkmarcimport.pl. Thus this option can/should be removed. The current behavior actually blocks bug 14957, we recently discovered, since calling 
ModBiblioMarc after the record has been saved bypasses marc merge rules on updates (which are applied in ModBiblio) and that save will ignore the possible rules that has been setup.

I also discovered some other odd things that will not address in this bug. One of the being that since ModBiblio runs _strip_item_fields no items will ever be imported though bulkmarcimport on updates (but will be in inserts). I'm pretty sure this is the case (have tested and verified, but seems odd this kind of bug has survived undetected for so long).
Comment 1 David Gustafsson 2020-05-19 10:05:34 UTC
Created attachment 105065 [details] [review]
Bug 25539: Remove AddBiblio option "defer_marc_save"

Items are no longer embedded in the MARCXML and because of
this the MARC data does not need to be saved once more
after changing record items data. The "defer_marc_save"
is no longer needed since bulkmarcimport.pl was the only
place this option was utilized in order to resave MARC data
after possibly changing items data.
Comment 2 David Gustafsson 2020-05-19 10:08:34 UTC
Created attachment 105066 [details] [review]
Bug 25539: Remove AddBiblio option "defer_marc_save"

Items are no longer embedded in the MARCXML and because of
this the MARC data does not need to be saved once more
after changing record items data. The "defer_marc_save"
is no longer needed since bulkmarcimport.pl was the only
place this option was utilized in order to resave MARC data
after possibly changing items data.
Comment 3 Victor Grousset/tuxayo 2020-05-31 21:20:03 UTC
I would like to try to test this. But I need a test plan as I don't know well enough this part of Koha to improvise one.

It can looks like this:
1. Step to prepare some stuff
2. Another step
3. Do something else that exposes the current issue
4. Apply the patch
5. Redo some of the above steps
6. See that the issue is gone
Comment 4 David Gustafsson 2020-06-22 08:48:27 UTC
Yes, I usually include a test-plan, this fix will not produce any change of behaviour with the current code, but causes a bug together with bug 14957. So in that case the test-plan would have to include applying that patch.
Comment 5 David Gustafsson 2020-06-22 08:49:51 UTC
Bad working that, this patch does not cause a bug together with bug 14957, but rather it resolves a bug with the current code base.
Comment 6 Katrin Fischer 2020-06-22 21:09:11 UTC
(In reply to David Gustafsson from comment #4)
> Yes, I usually include a test-plan, this fix will not produce any change of
> behaviour with the current code, but causes a bug together with bug 14957.
> So in that case the test-plan would have to include applying that patch.

No change in behaviour can still be tested with and without the patch. I think it would really help to get this moving and unlock bug 14957. But looking at the patch I get an idea of why this is hard. Some example uses might still be good to note and test.
Comment 7 Martin Renvoize 2020-09-09 10:59:56 UTC
Created attachment 109789 [details] [review]
Bug 25539: Remove AddBiblio option "defer_marc_save"

Items are no longer embedded in the MARCXML and because of
this the MARC data does not need to be saved once more
after changing record items data. The "defer_marc_save"
is no longer needed since bulkmarcimport.pl was the only
place this option was utilized in order to resave MARC data
after possibly changing items data.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2020-09-09 11:02:49 UTC
It took a little getting my head around, but I believe this is all correct.

For reference, the commit adding the defer_marc_save option had the message:

====
item rework: replaced AddBiblioAndItems

Replace C4::Biblio::AddBiblioAndItems with two
things:

* An option to C4::Biblio::AddBiblio to defer writing
  biblioitems.marc and biblioitems.marcxml.  This
  option was created to give a significant
  speed boost to bulkmarcimport.pl, but is *not*
  recommended for general use.
* C4::Items::AddItemBatchFromMarc

This refactoring removes the need to have functions
in C4::Biblio and C4::Items that call each other's
private functions.

Signed-off-by: Chris Cormack <crc@liblime.com>
Signed-off-by: Joshua Ferraro <jmf@liblime.com>
====

I believe it is no defunk as David suggests. I tried a few runs of bulkmarcimport with and without the patch and found no really noticeable slowdown in performance (though performance of this script really isn't very good these days :( )

Signing off.
Comment 9 Nick Clemens 2020-09-11 13:09:15 UTC
Small things:
Please put change for ModBiblio in its own patch, appreciated, but not entirely related :-)

You have some stray whitespace changes

Blocker:
Before this patch the items are stripped before saving the marc. After this patch they are not.

To test:
1 - Export a record with items from Koha into a file: test.mrc
2 - perl misc/migration_tools/bulkmarcimport.pl -b -file test.mrc
3 - You will get error on adding items, but not important for this test
4 - Check the biblio_metadata table:
    SELECT metadata FROM biblio_metadata ORDER BY biblionumber DESC LIMIT 1\G
5 - Note no item fields
6 - Apply patch
7 - perl misc/migration_tools/bulkmarcimport.pl -b -file test.mrc
8 - Check the biblio_metadata table as above
9 - Item fields are stored in the db
Comment 10 David Gustafsson 2020-09-16 12:12:57 UTC
I had a look at this, and I think this is a bit of a mess to sort out. You are calling bulkmarcimport.pl without -update or -insert set, which seems to default to -insert ($insert = 1, $update = undef), and without -match with a matchpoint.

bulkmarcimport will correctly get original id through:

$originalid = GetRecordId( $record, $tagid, $subfieldid );

But will not use $originalid as a match ($biblionumber will have no value on line 453 where record is either inserted or updated). Because of this and $update = undef the block inside of if ($insert) {... on line 470 will run inserting a new record with defer_marc_save => 1. After this block:

eval { ( $itemnumbers_ref, $errors_ref ) = AddItemBatchFromMarc( $record, $biblionumber, $biblioitemnumber, '' ); };

Will be run with a $biblionumber set to a biblio without any marc data yet saved.

Inside of AddItemBatchFromMarc

my $item_object = Koha::Item->new($item)->store;

will be run.

Inside Koha/Item.pm Koha::Item::store this will get executed:

C4::Biblio::ModZebra( $self->biblionumber, "specialUpdate", "biblioserver" )
            unless $params->{skip_modzebra_update}

ModZebra will attempt to load the Biblio Marc data:

$record = GetMarcBiblio({
    biblionumber => $biblionumber,
    embed_items  => 1 });

But this will return an undefined record since the marc-data has not yet been saved.

This will cause the import in later processing of this undefined $record to crash which will not propagate to bulkmarcimport.pl since run within an eval.

I get this when running bulkmarcimport without the patch. The new record marc data is later saved in the following block which comes shortly after:

   my $clone_record = $record->clone();
   C4::Biblio::_strip_item_fields($clone_record, '');
   # This sets the marc fields if there was an error, and also calls
   # defer_marc_save.
   ModBiblioMarc( $clone_record, $biblionumber, $framework );

which I removed in the patch (was not aware that the marc data was loaded as a side effect of AddItemBarchFromMarc).

This code needs to be removed somehow though since it will bypass the marc merge rules in bug 14957.

I think by removing this block a new bug is exposed which is that AddBiblio will not strip items before saving. Perhaps this could be addressed by stripping items before calling AddBiblio in bulkmarcimport.pl. I will look further into this.
Comment 11 David Gustafsson 2020-09-17 12:11:31 UTC
Created attachment 110260 [details] [review]
Strip items when adding new biblio to preserve previous behaviour of the script
Comment 12 David Gustafsson 2020-09-17 12:13:09 UTC
Created attachment 110261 [details] [review]
Bug 25539: Strip items when adding new biblio to preserve previous behaviour of the script
Comment 13 David Gustafsson 2020-09-17 12:38:14 UTC
I have now modified the patch so that items will be stripped also when adding new biblios. You will still get an error message when adding an exported biblio with existing items since the item ids will cause a duplicate primary key constraint error.

If understanding this correctly this should be considered "better" or in line with the current behavior of the script since the unmodified script will bail out even earlier with a different error (triggered by not saving the marc when attempting to load it). Even if the current script would get past that (which it would if removing the "defer_marc_save" option) you would still get the exact same error since AddItemBatchFromMarc would be called on the $record with updated internal bibio ids but with the same item-ids.
Comment 14 David Gustafsson 2020-09-17 13:26:20 UTC
About the whitespace in the patch, I removed superfluous white space in some places, but sure, to beconsistent that should perhaps be another issue since there is also some other instances of tabs instead of spaces and other incorrect formatting that could warrant another issue. I can remove this from the patch in this case.
Comment 15 Martin Renvoize 2020-10-12 09:39:28 UTC
Er.. not sure what's being 'Discussed' here at the moment.. is this ready for testing again?
Comment 16 Martin Renvoize 2021-03-29 12:22:47 UTC
Removing bug 14957 as a dependant. We have PQA'd on that patchset and it doesn't appear to actually require this as a dependency.

I'd still be interested to see where this one is going, I'm not sure what discussion is in progress?
Comment 17 Martin Renvoize 2021-03-29 12:36:11 UTC
I commented out the ability to use overlay rules with the bulkmarcimport route.. that unblocks that bug so it can be pushed and allows us to continue work here to re-enable that part of the feature.
Comment 18 Victor Grousset/tuxayo 2021-03-29 16:30:24 UTC
> Removing bug 14957 as a dependant

Actually it was re-added ^^
Comment 19 Martin Renvoize 2021-03-29 17:04:06 UTC
(In reply to Victor Grousset/tuxayo from comment #18)
> > Removing bug 14957 as a dependant
> 
> Actually it was re-added ^^

Actually, it was a swap ;).. this was blocking the other bug, now it depends upon it ;)
Comment 20 David Gustafsson 2021-03-29 17:20:59 UTC
(In reply to Martin Renvoize from comment #16)
> Removing bug 14957 as a dependant. We have PQA'd on that patchset and it
> doesn't appear to actually require this as a dependency.
> 
> I'd still be interested to see where this one is going, I'm not sure what
> discussion is in progress?

Perhaps you are aware of this since commented out the bulkmarcimport source, but it does require it as a dependency for bulkmarcimport. We use a fork of bulkmarcimport.pl which has been largely rewritten and decided not to try getting this into Koha because of the huge task of splitting this into minor patches which probably was a mistake. But there was such a huge overhead of doing so at the time. Would like to contribute those changes since fixed a large number of issues and bugs, but the authorities part is largely untested (since we don't import those) and therefore I would guess largely broken. I'm quite quite puzzled how bulkmarcimport.pl works for anyone else since discovered many major issues/bugs which had to be addressed in our use case, but obviously it does. This is the current patch for bulkmarcimport.pl and it would probably be a nightmare to review: https://github.com/ub-digit/Koha/commit/4615cf59575bb32bf9aeb8eaa716fd926201327d
Comment 21 David Gustafsson 2021-03-29 17:25:48 UTC
I completely forgot to add why I brought the patch up. The reason being that it's hard to fix the issue without exposing other bugs as mentioned in comment #13. It would probably be better to perform a more thorough refactor, but then there is the issue that we already did that but does not have a clean patch-set to contribute. Perhaps I will have to take the time to go through our current patch and verify that the current options are still supported and the script behaves in more or less the same way, so no or minimal API breakage.
Comment 22 Martin Renvoize 2021-05-06 08:27:00 UTC
(In reply to David Gustafsson from comment #21)
> I completely forgot to add why I brought the patch up. The reason being that
> it's hard to fix the issue without exposing other bugs as mentioned in
> comment #13. It would probably be better to perform a more thorough
> refactor, but then there is the issue that we already did that but does not
> have a clean patch-set to contribute. Perhaps I will have to take the time
> to go through our current patch and verify that the current options are
> still supported and the script behaves in more or less the same way, so no
> or minimal API breakage.

I'm more than happy to take a look at a bulkmarcimport patch.. even if it is initially large and unwieldy. We use that script a lot at PTFS-E and have been contemplating working on it to improve performance for some time.. though I'm not aware of any real bugs.. I'll ask the migrations team as they're the one's that utilise it.. perhaps they're using it in a different way from yourselves.