Bug 6990

Summary: TransformKohaToMarc enhancement
Product: Koha Reporter: Frédéric Demians <f.demians>
Component: Architecture, internals, and plumbingAssignee: Frédéric Demians <f.demians>
Status: CLOSED FIXED QA Contact: Bugs List <koha-bugs>
Severity: enhancement    
Priority: P1 - high CC: chris, januszop, katrin.fischer, nengard, paul.poulain
Version: 3.6   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 7119    
Attachments: Proposed patch
Proposed patch
TransformKohaToMarc enhancement

Description Frédéric Demians 2011-10-08 15:13:57 UTC
TransformKohaToMarc function is called for each biblio and item that has
to be build. This function execute a DB statement for each Koha field
that has to be mapped to a MARC tag/letter. This impact deeply
performances for script like rebuild_zebra, especially since items are
not anymore in bilio records and have to be rebuild on the fly. 

I'm proposing a patch which read Koha field to MARC field mapping just
one time and cache it. My test show a 30% execution time improvement on 
rebuild_zebra.pl script.
Comment 1 Frédéric Demians 2011-10-08 15:19:56 UTC Comment hidden (obsolete)
Comment 2 Frédéric Demians 2011-10-09 08:01:04 UTC Comment hidden (obsolete)
Comment 3 Paul Poulain 2011-10-24 11:38:16 UTC
Updating Version : This ENH will be for Koha 3.8
Comment 4 Paul Poulain 2011-10-25 15:05:51 UTC
Bug versionned for master. entries will be made against rel_3_8 once the patch has been applied (see thread about that on koha-devel yesterday)
Comment 5 Chris Cormack 2011-12-31 07:35:14 UTC
Created attachment 6992 [details] [review]
TransformKohaToMarc enhancement

TransformKohaToMarc function is called for each biblio and item that has
to be build. This function execute a DB statement for each Koha field
that has to be mapped to a MARC tag/letter. This impact deeply
performances for script like rebuild_zebra, especially since items are
not anymore in bilio records and have to be rebuild on the fly.

I'm proposing a patch which read Koha field to MARC field mapping just
one time and cache it. My test show a 30% execution time improvement on
rebuild_zebra.pl script. It uses already cached mapping in C4::Context.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Comment 6 Paul Poulain 2012-01-17 14:06:57 UTC
QA comment: code clear & properly written.

I made a lot of tests on UNIMARC and coudln't find any side effect (add item / add order with AcqCreateItem=ordered)
Comment 7 Nicole C. Engard 2012-01-17 19:15:48 UTC
This patch breaks searching - I had to revert it to get searching back to normal. It should be removed from master and fixed.


[13:38]  <nengard> I did a pull on my test install and now when i try to search I get this "Software error: Tag "" is not a valid tag. at /home/nengard/kohaclone/C4/Biblio.pm line 1849"  is this a bug? or something with my setup?
[13:41]  <jcamins> Sounds like something that shouldn't have been was pushed.
[13:41]  <nengard> anyone else having that issue?
[13:41]  <jcamins> I can't test now, though.
[13:46]  <oleonard-away> Searching works for me nengard 
[13:46]  <nengard> uh oh
[13:46]  <nengard> any ideas on how i might fix this little snag?
[13:46]  <nengard> or what to look at?
[13:47]  <jcamins> What's that line of code on your install?
[13:47]  <nengard> rebuilding zebra … just in case
[13:47]  <nengard> checking htat line
[13:48]  <nengard>    $record->insert_fields_ordered( MARC::Field->new(
[13:49]  <nengard> in sub TransformKohaToMarc
[13:49]  <nengard> rebuilding didn't work
[13:49]  <jcamins> That's not good.
[13:50]  <jcamins> When did you last update?
[13:53]  <nengard> jcamins i did a git pull like an hour ago
[13:53]  <collum> nengard: I was trying to load marc records into the reservoir last night, and I was getting a similar, error.
[13:53]  <collum> 3 day old installation
[13:54]  <collum> I thought it was me.
[13:54]  <nengard> hmmm
[13:54]  <cait> I can git pull and check
[13:54]  <cait> give me a sec
[14:00]  <jcamins> nengard: I meant, before that.
[14:00]  <nengard> i was updating a version of master from last week
[14:01]  <jcamins> Okay.
[14:01]  <jcamins> I'm betting the problem is related to bug 6990.
[14:01]  <huginn> Bug http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=6990 enhancement, PATCH-Sent, ---, frederic, Pushed to Master , TransformKohaToMarc enhancement
[14:01]  <jcamins> Try reverting that patch and see if it fixes it.
[14:03]  <cait> hm search working for me
[14:09]  <nengard> jcamins can you tell me what to type to revert that one?
[14:13]  <pastebot> "jcamins" at 66.108.135.39 pasted "nengard: reverting the commits for bug 6990" (3 lines) at http://paste.koha-community.org/212
[14:14]  <nengard> jcamins that worked



Nicole
Comment 8 Chris Cormack 2012-01-17 19:21:11 UTC
Searching still works for me with this patch.

Seems to be working for others also, I think this needs more testing before a revert. Remember master is unstable, we can test things a bit before pulling the revert switch. 

If this was on 3.6 of course we would revert it right away, but master is our unstable branch, and of course should not be being used in production, so we have a chance to actually test and confirm the problem (since I can't recreate others will need to) before we revert it. It may be a simple follow up is all that is needed.
Comment 9 Paul Poulain 2012-01-18 10:48:17 UTC
(In reply to comment #7)
> This patch breaks searching - I had to revert it to get searching back to
> normal. It should be removed from master and fixed.
> 
> 
> [13:38]  <nengard> I did a pull on my test install and now when i try to search
> I get this "Software error: Tag "" is not a valid tag. at
> /home/nengard/kohaclone/C4/Biblio.pm line 1849"  is this a bug? or something
> with my setup?
Investigating...

Line 1849 contains:
                $record->insert_fields_ordered( MARC::Field->new(
                    $tag, " ", " ", $letter => $value ) );

It means $tag is empty, which is bad

If I understand the code correctly, is means that there is a kohafield that is connected to an empty MARC tag (I don't mean "not connected", I mean "connected to a tag that is '   ' or something like that)

what's the result of :
select kohafield,tagfield,tagsubfield from marc_subfield_structure where kohafield > '' AND frameworkcode='';

If there is a line saying kohafield='something' and tagfield='strange thing', then, you've got it !
Comment 10 Nicole C. Engard 2012-01-18 14:10:30 UTC
mysql> select kohafield,tagfield,tagsubfield from marc_subfield_structure where
    -> kohafield > '' AND frameworkcode='';
+------------------------------+----------+-------------+
| kohafield                    | tagfield | tagsubfield |
+------------------------------+----------+-------------+
| additionalauthors.author     | 700      | a           |
| biblio.abstract              | 520      | a           |
| biblio.author                | 100      | a           |
| biblio.biblionumber          | 999      | c           |
| biblio.copyrightdate         | 260      | c           |
| biblio.notes                 | 500      | a           |
| biblio.serial                | 942      | s           |
| biblio.seriestitle           | 440      | a           |
| biblio.title                 | 245      | a           |
| biblio.unititle              | 240      | a           |
| biblioitems.biblioitemnumber | 999      | d           |
| biblioitems.cn_class         | 942      | h           |
| biblioitems.cn_item          | 942      | i           |
| biblioitems.cn_sort          | 942      | 6           |
| biblioitems.cn_source        | 942      | 2           |
| biblioitems.cn_suffix        | 942      | m           |
| biblioitems.illus            | 300      | b           |
| biblioitems.isbn             | 020      | a           |
| biblioitems.issn             | 022      | a           |
| biblioitems.itemtype         | 942      | c           |
| biblioitems.lccn             | 010      | a           |
| biblioitems.number           | 440      | n           |
| biblioitems.pages            | 300      | a           |
| biblioitems.place            | 260      | a           |
| biblioitems.publishercode    | 260      | b           |
| biblioitems.size             | 300      | c           |
| biblioitems.totalissues      | 942      | 0           |
| biblioitems.url              | 856      | u           |
| biblioitems.volume           | 440      | v           |
| biblioitems.volumedesc       | 362      | a           |
| bibliosubject.subject        | 650      | a           |
| bibliosubtitle.subtitle      | 245      | b           |
| items.barcode                | 952      | p           |
| items.booksellerid           | 952      | e           |
| items.ccode                  | 952      | 8           |
| items.cn_sort                | 952      | 6           |
| items.cn_source              | 952      | 2           |
| items.copynumber             | 952      | t           |
| items.damaged                | 952      | 4           |
| items.dateaccessioned        | 952      | d           |
| items.datelastborrowed       | 952      | s           |
| items.datelastseen           | 952      | r           |
| items.enumchron              | 952      | h           |
| items.holdingbranch          | 952      | b           |
| items.homebranch             | 952      | a           |
| items.issues                 | 952      | l           |
| items.itemcallnumber         | 952      | o           |
| items.itemlost               | 952      | 1           |
| items.itemnotes              | 952      | z           |
| items.itemnumber             | 952      | 9           |
| items.itype                  | 952      | y           |
| items.location               | 952      | c           |
| items.materials              | 952      | 3           |
| items.notforloan             | 952      | 7           |
| items.onloan                 | 952      | q           |
| items.price                  | 952      | g           |
| items.renewals               | 952      | m           |
| items.replacementprice       | 952      | v           |
| items.replacementpricedate   | 952      | w           |
| items.reserves               | 952      | n           |
| items.restricted             | 952      | 5           |
| items.stack                  | 952      | j           |
| items.uri                    | 952      | u           |
| items.wthdrawn               | 952      | 0           |
+------------------------------+----------+-------------+
64 rows in set (0.00 sec)
Comment 11 Nicole C. Engard 2012-01-18 14:20:12 UTC
Now, was I supposed to get no results? If so, what do you think made this happen because I have the frameworks installed at the time of Koha installation and my framework test works just fine.  One thing I have done that might have caused this is import/export frameworks.  Basically we need to track down what causes fields to show up without a framework cause I didn't put them in to the DB manually just for funnsies :)
Comment 12 Frédéric Demians 2012-01-18 14:36:53 UTC
Nicole, tell us what you get when you run this command on a shell:

perl -MC4::Context -MYAML -e 'print Dump(C4::Context->marcfromkohafield)'

You should have something like that:

biblioitems.biblioitemnumber:
  - 090
  - a
biblioitems.collectionissn:
  - 225
  - x
...

Each DB field is linked to a tag/letter.
Comment 13 Frédéric Demians 2012-01-18 14:39:27 UTC
In C4::Biblio, after line 1842, you can also try to add:

next unless $tag;

But I think something is wrong in your Default framework.
Comment 14 Nicole C. Engard 2012-01-18 14:47:41 UTC
I get what you said I should - a big long list of tags.  

And if something is wrong with my default framework then we need to fix the MARC Bibliographic Framework Test cause mine says everything is OK.
Comment 15 Frédéric Demians 2012-01-18 14:58:24 UTC
Have you tried adding the above line?
Comment 16 Nicole C. Engard 2012-01-18 15:17:22 UTC
I didn't want to screw with my code - I need to be able to pull from git without problems ... I did it though - and now have an uglier error:


Software error:

Global symbol "$tag" requires explicit package name at /home/nengard/kohaclone/C4/Biblio.pm line 1843.
Compilation failed in require at /home/nengard/kohaclone/C4/Reserves.pm line 27.
BEGIN failed--compilation aborted at /home/nengard/kohaclone/C4/Reserves.pm line 27.
Compilation failed in require at /home/nengard/kohaclone/C4/Circulation.pm line 26.
BEGIN failed--compilation aborted at /home/nengard/kohaclone/C4/Circulation.pm line 26.
Compilation failed in require at /home/nengard/kohaclone/C4/Overdues.pm line 26.
BEGIN failed--compilation aborted at /home/nengard/kohaclone/C4/Overdues.pm line 26.
Compilation failed in require at /home/nengard/kohaclone/C4/Members.pm line 30.
BEGIN failed--compilation aborted at /home/nengard/kohaclone/C4/Members.pm line 30.
Compilation failed in require at /home/nengard/kohaclone/C4/Auth.pm line 30.
BEGIN failed--compilation aborted at /home/nengard/kohaclone/C4/Auth.pm line 30.
Compilation failed in require at /home/nengard/kohaclone/opac/opac-search.pl line 32.
BEGIN failed--compilation aborted at /home/nengard/kohaclone/opac/opac-search.pl line 32.

For help, please send mail to the webmaster (webmaster@debian), giving this error message and the time and date of the error.
Comment 17 Nicole C. Engard 2012-01-18 15:18:46 UTC
How do i get back to a working branch now?


nengard@kohavm:~/kohaclone$ git checkout bug_6990_revert
error: You have local changes to 'C4/Biblio.pm'; cannot switch branches.
Comment 18 Frédéric Demians 2012-01-18 15:25:04 UTC
(In reply to comment #17)
> How do i get back to a working branch now?
> 
> 
> nengard@kohavm:~/kohaclone$ git checkout bug_6990_revert
> error: You have local changes to 'C4/Biblio.pm'; cannot switch branches.

To remove your local change:

git checkout C4/Biblio.pm
Comment 19 Nicole C. Engard 2012-01-20 20:22:45 UTC
Are there any other ideas on how I can search my master install?  What should I change on my frameworks?  What data to I need to update/fix/delete/change?  Pretty soon I won't be able to test and sign off on patches if I don't find a way to fix my master install.

Nicole
Comment 20 Frédéric Demians 2012-01-20 21:31:00 UTC
(In reply to comment #19)
> Are there any other ideas on how I can search my master install?  What
> should I change on my frameworks?  What data to I need to
> update/fix/delete/change?  Pretty soon I won't be able to test and
> sign off on patches if I don't find a way to fix my master install.

Could you attach a compressed dump of you SQL DB to this bug? It would
allow to reproduce the bug you're encountering.
Comment 21 Nicole C. Engard 2012-01-20 21:44:52 UTC
How do I compress it?
Comment 22 Nicole C. Engard 2012-01-20 21:48:00 UTC
I have a mysqldump and I zipped it up, but it's still to big to attach here (8mb).
Comment 23 Frédéric Demians 2012-01-20 22:04:43 UTC
(In reply to comment #22)
> I have a mysqldump and I zipped it up, but it's still to big to attach here
> (8mb).

Couldn't you upload you zipped dump on file sharing host, or on a FTP/HTTP server? If you can't, drop me a direct message, I will give you a FTP server address. Thanks.
Comment 24 Nicole C. Engard 2012-01-20 22:20:10 UTC
It's the end of the day on Friday - my brain isn't working ;)

http://www.web2learning.net/wp-content/uploads/2012/01/koha.sql_.zip
Comment 25 Frédéric Demians 2012-01-21 09:58:23 UTC
(In reply to comment #24)
> It's the end of the day on Friday - my brain isn't working ;)

> http://www.web2learning.net/wp-content/uploads/2012/01/koha.sql_.zip

It works with your data on a out-of-the-git Koha head branch:

http://nengard.tamil.fr

So I can't reproduce your bug
Comment 26 Frédéric Demians 2012-01-21 09:59:21 UTC
I would say that there is something wrong on your box. You should try reinstalling Koha.
Comment 27 Janusz Kaczmarek 2012-01-29 22:07:54 UTC
Nicole,

I was able to reproduce you error:

Software error:
Tag "" is not a valid tag. at /usr/share/koha32/lib/C4/Biblio.pm line 1849

but only when playing with mod_perl. When disabling mod_perl and returning to mod_cgi everything is fine. 

Isn't it, by chance, your case?  Aren't you running Koha under mod_perl?

Cheers,

Janusz
Comment 28 Janusz Kaczmarek 2012-01-29 23:59:48 UTC
OK, the possible reason for the error is the line 1842:

next unless my $dtm = $db_to_marc->{''}->{$name};

It seems that (at least in mod_perl environment) the expression 

$dtm = $db_to_marc->{''}->{$name}

is always true. Even if for some $name-s (such as item.timestamp or items.more_subfields_xml) the is no entry in $db_to_marc, the logical value of this assignment is true -- it seems that the result of that assignment is a reference to an empty array [] (debugged with Data::Dumper).  

[[[  Cf. the following example:

#!/usr/bin/perl
my @s = qw //;
print "Hello!\n" if my $a = \@s;

Result: "Hello!" (yes!)

If you add:

use Data::Dumper;
print Dumper $a;

you get: $VAR1 = [];   ]]]

So, for the code to behave more stable one could change the line 1842 with:

my $dtm = $db_to_marc->{''}->{$name};
next unless $dtm->[0];

It seems to be OK. 

What is your opinion?
Comment 29 Nicole C. Engard 2012-02-01 01:02:13 UTC
I am not using mod_perl and now that I pulled the latest patches down I don't get the error anymore - we might still want to investigate, but I can now search on the latest master.

Nicole
Comment 30 Janusz Kaczmarek 2012-02-02 11:10:01 UTC
I can confirm. After switching to current master the error does not occur any more when playing with mod_perl. But since Nicole was not using mod_perl and because there was no change to the code of C4::Biblio::TransformKohaToMarc (nor C4::Context::_new_marcfromkohafield on which it depends), it would mean that under *unpredictable*circumstances*, not just under mod_perl, it could happen that the result of the attribution 

$dtm = $db_to_marc->{''}->{$name} 

is something defined (like a reference to an empty table [], as it is in my case) while it would be rather expected to be undefined. (This is why Nicole, and me, got $tag being '' in line 1849, etc.)

In that context the version

my $dtm = $db_to_marc->{''}->{$name};
next unless $dtm->[0];

is more verbose, but does the same in a more robust way. 

But since nobody else experienced this error I do not know if I should send a patch...?