Bug 11096 - Koha cannot retrieve big records from Zebra
Summary: Koha cannot retrieve big records from Zebra
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Target Milestone: ---
Assignee: Galen Charlton
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 11499
Blocks: 9234 11940 12347
  Show dependency treegraph
 
Reported: 2013-10-21 19:16 UTC by Tomás Cohen Arazi
Modified: 2017-04-21 16:44 UTC (History)
13 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Bot Control: Ignore this bug
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 11096: Remove use of MARC::File::USMARC from AuthoritiesMarc.pm (3.56 KB, patch)
2013-10-22 17:44 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11096: force XML syntax for Zconn (18.40 KB, patch)
2013-10-22 17:44 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11096: Take advantage of MARC::Field->as_string feature (2.06 KB, patch)
2013-11-08 14:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11096: Take advantage of MARC::Field->as_string feature (2.46 KB, patch)
2013-11-08 17:58 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Sample record (lots of items) (433.09 KB, text/plain)
2013-11-08 18:13 UTC, Tomás Cohen Arazi
Details
Bug 11096: force XML syntax for Zconn (18.45 KB, patch)
2013-11-18 14:44 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Squashed patch. (22.25 KB, patch)
2013-11-19 13:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11096: wrap MARC::Record->new_from_xml call with eval (1.66 KB, patch)
2013-11-19 13:58 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[PASSED QA] Bug 11096: force XML syntax for Zconn and remove MARC::File::USMARC everywhere. (22.35 KB, patch)
2013-12-02 14:14 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 11096: wrap MARC::Record->new_from_xml call with eval (2.25 KB, patch)
2013-12-02 14:14 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED OFF] Bug 11096: force XML syntax for Zconn and remove MARC::File::USMARC everywhere. (22.35 KB, patch)
2013-12-02 14:16 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED OFF] Bug 11096: wrap MARC::Record->new_from_xml call with eval (2.25 KB, patch)
2013-12-02 14:16 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 11096: force XML syntax for Zconn and remove MARC::File::USMARC everywhere. (22.45 KB, patch)
2013-12-06 12:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11096: wrap MARC::Record->new_from_xml call with eval (2.30 KB, patch)
2013-12-06 12:26 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11096: (follow-up) add direct regression tests (294.11 KB, patch)
2013-12-10 00:24 UTC, Galen Charlton
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 11096: (follow-up) add direct regression tests (293.99 KB, patch)
2014-01-08 17:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11096 - Wrap each test set configuration into a subtest (1.55 KB, patch)
2014-01-08 17:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[SIGNED OFF] Bug 11096: force XML syntax for Zconn and remove MARC::File::USMARC everywhere. (22.46 KB, patch)
2014-01-08 18:52 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[SIGNED OFF] Bug 11096: wrap MARC::Record->new_from_xml call with eval (2.31 KB, patch)
2014-01-08 18:53 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11096: Make use of MARCXML conditional of DOM being set (13.96 KB, patch)
2014-01-08 18:53 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11096: force XML syntax for Zebra in DOM (23.79 KB, patch)
2014-01-10 18:34 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11096: (follow-up) add direct regression tests (294.03 KB, patch)
2014-01-11 03:15 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 11096 - Wrap each test set configuration into a subtest (1.60 KB, patch)
2014-01-11 03:15 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 11096: force XML syntax for Zebra in DOM (23.84 KB, patch)
2014-01-11 03:15 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 11096: (follow-up) add direct regression tests (294.08 KB, patch)
2014-01-17 13:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11096 - Wrap each test set configuration into a subtest (1.66 KB, patch)
2014-01-17 13:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11096: force XML syntax for Zebra in DOM (23.96 KB, patch)
2014-01-17 13:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11096: (qa followup) facets count and UNIMARC record syntax (1.83 KB, patch)
2014-01-20 14:44 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11096: (qa followup) facets count and UNIMARC record syntax (1.89 KB, patch)
2014-01-22 08:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11096: If zebra_bib_index_mode is not defined, assume it is grs1. (1.41 KB, patch)
2014-01-22 08:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11096: (qa followup) facets count and UNIMARC record syntax (1.83 KB, patch)
2014-01-22 13:08 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11096: fallback to the correct defaults if config entries missing (1.56 KB, patch)
2014-01-22 13:13 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11096: fallback to the correct defaults if config entries missing (1.84 KB, patch)
2014-01-22 13:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11096: (follow-up) add direct regression tests (4.44 KB, patch)
2014-01-25 14:27 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11096 - Wrap each test set configuration into a subtest (1.66 KB, patch)
2014-01-25 14:30 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11096: (follow-up) add direct regression tests (294.32 KB, patch)
2014-01-29 21:15 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[SIGNED OFF] Bug 11096: add direct regression tests (294.32 KB, patch)
2014-01-30 16:38 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[SIGNED OFF] Bug 11096: Wrap each test set configuration into a subtest (3.45 KB, patch)
2014-01-30 16:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11096: Koha<->Zebra should use the XML syntax with DOM (24.70 KB, patch)
2014-01-30 16:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11096: Koha<->Zebra should use the XML syntax with DOM (30.68 KB, patch)
2014-01-30 18:10 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11096: Koha<->Zebra should use the XML syntax with DOM (30.75 KB, patch)
2014-02-14 16:04 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 11096: add direct regression tests (294.39 KB, patch)
2014-02-26 10:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
[PASSED QA] Bug 11096: Wrap each test set configuration into a subtest (3.51 KB, patch)
2014-02-26 10:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
[PASSED QA] Bug 11096: Koha<->Zebra should use the XML syntax with DOM (30.82 KB, patch)
2014-02-26 10:53 UTC, Martin Renvoize
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 2013-10-21 19:16:09 UTC
When we switched from ISO2709 to MARCXML as the default indexing format we got rid of the errors regarding big record indexing; notably serials with lots of items.

The problem is that the current ZOOM connection in Koha is set to retreive records in 'usmarc'. This makes retreiving search results silently fail for big records, and even when those records are correctly indexed, the user would fail to reach them.

Steps to reproduce:
- Create a new simple record with 100$a = Arquitectura viva
- Add it several big note fields
- Add several items to it
- Reindex, search for Arquitectura viva
- Record doesn't show
- In a terminal, use yaz-client to do a manual query (adjust to your setup):
$ yaz-client -k 100000 unix:/home/tomas/koha-dev/var/run/zebradb/bibliosocket
> base biblios
> f @attr 1=4 @attr 4=1 @attr 6=3 "Arquitectura viva"
> s
It should fail because of the size of the record.
- Rerun like this:
> format xml
> elem marcxml
> f @attr 1=4 @attr 4=1 @attr 6=3 "Arquitectura viva"
> s
There you go :-D
Comment 1 Tomás Cohen Arazi 2013-10-22 17:44:13 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2013-10-22 17:44:48 UTC Comment hidden (obsolete)
Comment 3 David Cook 2013-10-22 22:55:04 UTC
I've applied the patches, but I'm still unable to retrieve the big record from Zebra.

I created a new bib record with an excessive number of 5xx fields and added many items, and it has created problems with the MARC record leader.

This is the leader for a small record:
<leader>03322cam a2200469 a 4500</leader>

Here is the (broken) leader for a big record (without items) in MySQL:
<leader>143530nama22006133  45000</leader>

Here is the (broken) leader for a big record (with items) in Zebra (accessed via YAZ):
<leader>584260nama22019693  4500</leader>

As you can see, both of the broken leaders are quite broken. The MySQL version is 25 characters rather than 24. In Zebra, it has 24 positions, but much of the data are in the wrong place because of that 6 character record length.

--

When I try to access the record in Yaz-client, I receive the following error:

<!-- Directory offset 276: Data out of bounds 59725 >= 58426 -->
</record>

As Tomas pointed out, 584260 has been shortened to 58426 as Yaz is expecting 5 characters rather than 6 characters for the record length.

--

So...at the moment...it seems that indexing and retrieving oversized records doesn't work.

If you try to bring up a detail page manually, you can, but the XSLTs won't work correctly as it can't rely on the leader positions correctly. A "book" might show up as a "set".

In one case, some of the XSLT display isn't even showing because the data that is mixed around is totally unusable.

--

Admittedly, the most likely causes of big records are notes and items, so perhaps it would make sense to separate out the item records from the bib records when indexing. 

As for the notes...I'm not sure how we would handle that. While there is a 9999 character limit on a note field, you could have a hundred note fields that break your record.

--

I'm not really sure what the answer is, but here is the problem.
Comment 4 David Cook 2013-10-22 23:14:30 UTC
Martin reminded me that we thought about changing the leader on the fly...we could check for the length of the record and if it is too long, then we could replace it with xxxxxx or spaces or low numbers or something like that.

We would need to take this into account anytime we were ingesting or disseminating MARC records though...
Comment 5 David Cook 2013-10-23 00:11:28 UTC
By the coordinated efforts of Galen, Jared, Thomas, Tomas, and myself, we've found that the problems that I was experiencing were due to using version 2.0.3-1 of MARC::Record and the GRS-1 indexing filter.

Newer versions of MARC::Record will set the record length of the leader to 99999 if it exceeds 99999. Here is the commit that changed MARC::Record:

https://sourceforge.net/p/marcpm/code/ci/3efc45777f8e4bec1cd09beb6ab2968e5fb0e44f/

So, upgrading to a newer version of MARC::Record will fix this problem when using DOM.

However, these problems will persist when using GRS-1 (as it seems that GRS-1 relies on the ISO2709 spec in some way internally). 

As Jared mentioned on IRC, the best bet is probably to push this patch, even though it will only work for DOM indexing.

GRS-1 indexing will just work the same as always, so there is no negative impact for GRS-1 users for using this patch.
Comment 6 Katrin Fischer 2013-10-27 02:36:29 UTC
David, can you sign off on this?
Comment 7 Tomás Cohen Arazi 2013-11-08 14:04:06 UTC Comment hidden (obsolete)
Comment 8 Tomás Cohen Arazi 2013-11-08 17:58:42 UTC Comment hidden (obsolete)
Comment 9 Tomás Cohen Arazi 2013-11-08 18:13:35 UTC
Created attachment 22846 [details]
Sample record (lots of items)

This is a sample record with lots of items and note fields. To use download and do:

- Import the record:
 $ misc/migration_tools/bulkmarcimport.pl -v -b -m=marcxml -file=sample_record.marcxml
 $ misc/migration_tools/rebuild_zebra.pl -b -a -z -v -x

- Have zebrasrv running, for example like this:

 $ zebrasrv -f ~/koha-dev/etc/koha-conf.xml -k 10240000 -v "fatal,warn,log"

- Do a search for "Arquitectura viva"
  You get no results
- Apply the patch series
- Re-do the search
  Voila
Comment 10 Chris Cormack 2013-11-17 07:21:55 UTC
Tomás, the second patch conflicts here

<<<<<<< HEAD
        my $marcrecord = eval { MARC::File::USMARC::decode( $marcresults->[$i] ); };
        if ( $@ ) {
            warn "ERROR DECODING RECORD - $@: " . $marcresults->[$i];
            next;
        }

=======
        my $marcrecord = MARC::Record->new_from_xml(
            $marcresults->[$i],
            'UTF-8'
        );
>>>>>>> Bug 11096: force XML syntax for Zconn

I think because the eval was recently introduced, should we be evaling for new_from_xml or does it handle errors better than MARC::File::USMARC::decode does?
Comment 11 Tomás Cohen Arazi 2013-11-18 14:44:11 UTC Comment hidden (obsolete)
Comment 12 Tomás Cohen Arazi 2013-11-18 14:46:16 UTC
(In reply to Chris Cormack from comment #10)
> Tomás, the second patch conflicts here
> 
> <<<<<<< HEAD
>         my $marcrecord = eval { MARC::File::USMARC::decode(
> $marcresults->[$i] ); };
>         if ( $@ ) {
>             warn "ERROR DECODING RECORD - $@: " . $marcresults->[$i];
>             next;
>         }
> 
> =======
>         my $marcrecord = MARC::Record->new_from_xml(
>             $marcresults->[$i],
>             'UTF-8'
>         );
> >>>>>>> Bug 11096: force XML syntax for Zconn
> 
> I think because the eval was recently introduced, should we be evaling for
> new_from_xml or does it handle errors better than MARC::File::USMARC::decode
> does?

MARC::File::XML->new_from_xml implicitly calls its own decode method, which also croaks if problems arise decoding. So I attached a rebased patch that preserves that introduced behaviour (I mean the eval).
Comment 13 Tomás Cohen Arazi 2013-11-19 13:16:50 UTC Comment hidden (obsolete)
Comment 14 Tomás Cohen Arazi 2013-11-19 13:58:07 UTC Comment hidden (obsolete)
Comment 15 Katrin Fischer 2013-12-02 14:14:20 UTC Comment hidden (obsolete)
Comment 16 Katrin Fischer 2013-12-02 14:14:42 UTC Comment hidden (obsolete)
Comment 17 Katrin Fischer 2013-12-02 14:16:37 UTC Comment hidden (obsolete)
Comment 18 Katrin Fischer 2013-12-02 14:16:49 UTC Comment hidden (obsolete)
Comment 19 Kyle M Hall 2013-12-06 12:25:43 UTC Comment hidden (obsolete)
Comment 20 Kyle M Hall 2013-12-06 12:26:06 UTC Comment hidden (obsolete)
Comment 21 Galen Charlton 2013-12-10 00:23:11 UTC
This has run into a snag.  While reviewing this, I decided to write a direct regression test, which can be found here:

http://git.librarypolice.com/?p=koha-galen.git;a=commit;h=cffafe30284f65ef21fc315117b5d7abb39483dc

This patch applies on both the current master as well as master with the patches for bug 11096 applied.

On current master, the new tests *work* for MARC21 if the indexing mode is GRS-1, but, as expected, fail if the indexing mode is DOM.

On top of the patches for this bug, the new tests work if the indexing mode is DOM.  Yay!

Here's the catch: they fail, and fail with a crash, if the indexing mode is GRS-1.  This is because Zebra itself returns a Z39.50 error 14 (failure while presenting record) in line 493 of Search.pm:

  $record = $results[ $i - 1 ]->record($j)->raw();

Because Zebra refuses to render an invalid ISO2709 blob MARC XML, ->record($j) returns undef, and the crash occurs when it tries to run undef->raw().

Unfortunately, it's not a matter of just wrapping an eval() around it and carrying on.  Here's the situation today:

Over-large record in Zebra with GRS-1: can retrieve it
... with DOM: can't retrieve it

Note that it works for GRS-1 mostly because (a) Zebra doesn't try to do any conversions; it think it's storing iso2709, and if you want iso2709 back, it just sends it and (b) MARC::Record is pretty forgiving.

Here's the situation with the current proposed patches:
... with GRS-1: can't retrieve, and it crashes the search
... with DOM: it works

To sum up: this patch would cause a regression for GRS-1 users that happen to have over-large records in their database.

I see couple ways to solve this.  One would be to simply immediately desupport GRS-1 -- but I don't think our UNIAMRC users are quite there yet.

The other way would be to make the code adjust how it retrieves records, and whether to use the USMARC or the XML decoder, based on the values of the zebra_bib_index_mode and zebra_auth_index_mode settings.
Comment 22 Galen Charlton 2013-12-10 00:24:44 UTC Comment hidden (obsolete)
Comment 23 Galen Charlton 2013-12-10 00:25:33 UTC
(In reply to Galen Charlton from comment #21)
> This has run into a snag.  While reviewing this, I decided to write a direct
> regression test, which can be found here:
> 
> http://git.librarypolice.com/?p=koha-galen.git;a=commit;
> h=cffafe30284f65ef21fc315117b5d7abb39483dc
> 
> This patch applies on both the current master as well as master with the
> patches for bug 11096 applied.

I've also attached the regression test patch to this bug.
Comment 24 Tomás Cohen Arazi 2014-01-08 17:22:10 UTC Comment hidden (obsolete)
Comment 25 Tomás Cohen Arazi 2014-01-08 17:22:26 UTC Comment hidden (obsolete)
Comment 26 Tomás Cohen Arazi 2014-01-08 18:52:48 UTC Comment hidden (obsolete)
Comment 27 Tomás Cohen Arazi 2014-01-08 18:53:08 UTC Comment hidden (obsolete)
Comment 28 Tomás Cohen Arazi 2014-01-08 18:53:23 UTC Comment hidden (obsolete)
Comment 29 Tomás Cohen Arazi 2014-01-10 16:10:16 UTC
Made the use of MARCXML for serialization dependent on having DOM set as Galen proposed on comment #21.

Tests now pass.
Comment 30 Tomás Cohen Arazi 2014-01-10 16:12:30 UTC
Please recall that I made the bug depend on bug 11499 (fixes correctly setting indexing mode in the test scripts) so apply the patches in order, on top of the patches for that bug.
Comment 31 Tomás Cohen Arazi 2014-01-10 18:34:22 UTC Comment hidden (obsolete)
Comment 32 Chris Cormack 2014-01-11 03:15:03 UTC Comment hidden (obsolete)
Comment 33 Chris Cormack 2014-01-11 03:15:24 UTC Comment hidden (obsolete)
Comment 34 Chris Cormack 2014-01-11 03:15:42 UTC Comment hidden (obsolete)
Comment 35 Kyle M Hall 2014-01-17 13:03:25 UTC Comment hidden (obsolete)
Comment 36 Kyle M Hall 2014-01-17 13:03:33 UTC Comment hidden (obsolete)
Comment 37 Kyle M Hall 2014-01-17 13:03:38 UTC Comment hidden (obsolete)
Comment 38 Kyle M Hall 2014-01-17 13:09:04 UTC
Works great!

On a side note, search results are crazy slow for the example record Tomás uploaded. This is not an issue with this patch but does expose how slowly Koha process all items on a search results. I've seen this before with records that have hundreds of items that don't surpass the marc record length. Records that are overly large due to bib level notes fields and such will not trigger this kind of slowness. It may behoove us to find any efficiencies we can for item processing for search results.
Comment 39 Katrin Fischer 2014-01-19 11:41:22 UTC
To add another set of eyes to this I have done some tests today and I found a problem in the rewrite of the facetting code.

Without the patches the item type and other item related facets show the number of records related to them, but with the patch, the number of occurrences of the tag/items is shown. I think this is an unwanted change in behaviour that can be confusing to patrons. We shouldn't mix number of items and number of records in the facets. I think it's related to this change:

                                     unless ( $data ~~ @used_datas ) {
-                                        $facets_counter->{ $facet->{idx} }
-                                          ->{$data}++;
                                         push @used_datas, $data;
                                     }
-                                }    # fields
+
+                                    $facets_counter->{ $facet->{idx} }->{$data}++;
+
+                                } # fields

I will do more tests, but failing QA to get this fixed.
Comment 40 Katrin Fischer 2014-01-19 12:49:08 UTC
TESTING NOTES

MARC21 - b:dom - a:dom

1- Reindexing
     OK Full bib reindex -b -x -r -v
     OK Full auth reindex -a -x -r -v
     OK Update record
     OK Delete record
2- Catalog search
     OK Searching
     OK big record can be found and displayed
3- Facets
     OK limiting on several facets
     !!! facet counts
4- Authority search
     OK Searching
     OK Update record
     OK Delete record
5- prove t/db_dependent/Search.t
     OK All tests pass

Problems found:
  Item related facets show the "wrong" count as what you expect would
  be a record related count, but what you see is the item related count
  Example: Itemtype: Books (15) can be only 5 records
  
  Typo in documentation serhaps/perhaps introduced by third
  patch.

I am going to try and repeat the tests on a UNIMARC bib:grs1, auth:dom installation next.
Comment 41 Katrin Fischer 2014-01-19 14:50:49 UTC
UNIMARC - b:grs1 - a:dom

The patches break search in my UNIMARC installation. A normal search for the keyword "der" that gives results on my master branch with freshly reindexed records, fails on the branch with patches applied, reindex and restart of the zebra server:

Error: Can't call method "raw" on an undefined value at /home/katrin/kohacloneU/C4/Search.pm line 503. 

Authority search seems to work ok.
Comment 42 Tomás Cohen Arazi 2014-01-20 14:44:13 UTC Comment hidden (obsolete)
Comment 43 Jonathan Druart 2014-01-21 16:01:38 UTC
Sorry Tomás, bad news.

I get the 'Can't call method "data" on an undefined value' error with and without the last patch.

Marked as Failed QA.
Comment 44 Tomás Cohen Arazi 2014-01-21 21:41:31 UTC
Jonathan, can u tell me how is your setup? Specifically in koha-conf.xml do you have:

     <retrievalinfo>
       <retrieval syntax="unimarc" name="F"/>
? or
     <retrievalinfo>
       <retrieval syntax="usmarc" name="F"/>

I assume you have this in your config section:

 <zebra_bib_index_mode>grs1</zebra_bib_index_mode>
 <zebra_auth_index_mode>dom</zebra_auth_index_mode>
Comment 45 Jonathan Druart 2014-01-22 08:34:42 UTC
(In reply to Tomás Cohen Arazi from comment #44)
> Jonathan, can u tell me how is your setup? Specifically in koha-conf.xml do
> you have:
> 
>      <retrievalinfo>
>        <retrieval syntax="unimarc" name="F"/>
> ? or
>      <retrievalinfo>
>        <retrieval syntax="usmarc" name="F"/>
> 
> I assume you have this in your config section:
> 
>  <zebra_bib_index_mode>grs1</zebra_bib_index_mode>
>  <zebra_auth_index_mode>dom</zebra_auth_index_mode>

Hum, I did not have the zebra_bib_index_mode defined.
In your patch, in C4::Search the test is done on zebra_bib_index_mode eq 'dom' but in C4::Context it is done on 'grs1'.
I think it should be done every where on 'dom'. That way, if the value does not exist, we assume it is a dom instance.
Tomás, please signoff if you agree.
Comment 46 Jonathan Druart 2014-01-22 08:35:39 UTC Comment hidden (obsolete)
Comment 47 Jonathan Druart 2014-01-22 08:35:46 UTC Comment hidden (obsolete)
Comment 48 Tomás Cohen Arazi 2014-01-22 13:08:01 UTC Comment hidden (obsolete)
Comment 49 Tomás Cohen Arazi 2014-01-22 13:13:20 UTC Comment hidden (obsolete)
Comment 50 Jonathan Druart 2014-01-22 13:46:40 UTC Comment hidden (obsolete)
Comment 51 Katrin Fischer 2014-01-25 09:37:51 UTC
Tomas, I am sorry, but I have problem to resolve the conflict in Search.t in the first patch and have the following patches apply. Can you please take a look?
Comment 52 Tomás Cohen Arazi 2014-01-25 14:27:41 UTC Comment hidden (obsolete)
Comment 53 Tomás Cohen Arazi 2014-01-25 14:30:48 UTC Comment hidden (obsolete)
Comment 54 Katrin Fischer 2014-01-25 14:56:14 UTC
Hm, the tests are failing for me:
...
t/db_dependent/Search.t .. 4/4 # Looks like you failed 2 tests of 4.
t/db_dependent/Search.t .. Dubious, test returned 2 (wstat 512, 0x200)
Failed 2/4 subtests 

Test Summary Report
-------------------
t/db_dependent/Search.t (Wstat: 512 Tests: 4 Failed: 2)
  Failed tests:  1-2
  Non-zero exit status: 2
Comment 55 Tomás Cohen Arazi 2014-01-25 15:00:59 UTC
(In reply to Katrin Fischer from comment #54)
> Hm, the tests are failing for me:
> ...
> t/db_dependent/Search.t .. 4/4 # Looks like you failed 2 tests of 4.
> t/db_dependent/Search.t .. Dubious, test returned 2 (wstat 512, 0x200)
> Failed 2/4 subtests 
> 
> Test Summary Report
> -------------------
> t/db_dependent/Search.t (Wstat: 512 Tests: 4 Failed: 2)
>   Failed tests:  1-2
>   Non-zero exit status: 2

Are you sure you're on latest master with all the patches applied? It is passing for me http://snag.gy/fHAsw.jpg
Comment 56 Katrin Fischer 2014-01-25 15:04:44 UTC
Yes, sure of both. Hm, so we need to find out where the difference is.
Comment 57 Tomás Cohen Arazi 2014-01-25 15:07:09 UTC
(In reply to Katrin Fischer from comment #56)
> Yes, sure of both. Hm, so we need to find out where the difference is.

MARC::Record version 2.0.6 is needed
Comment 58 Katrin Fischer 2014-01-25 15:08:40 UTC
Is it packaged yet?
Comment 59 Tomás Cohen Arazi 2014-01-25 15:12:50 UTC
(In reply to Katrin Fischer from comment #58)
> Is it packaged yet?

In my repo, it is :-D 

http://ubuntu.unc.edu.ar/koha/
Comment 60 Katrin Fischer 2014-01-25 15:23:50 UTC
Installed it from CPAN - tests still failing.
Comment 61 Katrin Fischer 2014-01-25 15:25:14 UTC
not ok 48 - getRecords on _ALLRECORDS PQF returned all records
not ok 53 - Weighted query returns best match first # TODO Query weighting does not behave exactly the same in DOM vs. GRS
not ok 100 - can do a search that retrieves an over-large bib record (bug 11096)
not ok 101 - able to render over-large bib record (bug 11096)
not ok 102 - able to render over-large bib record (bug 11096)
not ok 103 - able to render over-large bib record (bug 11096)
Comment 62 Katrin Fischer 2014-01-25 15:39:38 UTC
The first 2 tests also exist on the master branch and pass there.
Comment 63 Tomás Cohen Arazi 2014-01-25 15:45:26 UTC
(In reply to Katrin Fischer from comment #62)
> The first 2 tests also exist on the master branch and pass there.

Can you email me your dev setup? I cannot reproduce the problems you refer to, and hence cannot provide a workaround.



tomas@zoe:koha-community-src(clean_master)$ git fetch
tomas@zoe:koha-community-src(clean_master)$ git reset --hard origin/master 
HEAD is now at 27c312b Bug 11533: fix authority searching with no sorting when QueryParser is enabled
tomas@zoe:koha-community-src(clean_master)$ git bz apply 11096
Bug 11096 - Koha cannot retrieve big records from Zebra

24492 - Bug 11096: force XML syntax for Zebra in DOM
24602 - Bug 11096: (qa followup) facets count and UNIMARC record syntax
24607 - Bug 11096: fallback to the correct defaults if config entries missing
24731 - Bug 11096: (follow-up) add direct regression tests
24732 - Bug 11096 - Wrap each test set configuration into a subtest

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 11096: force XML syntax for Zebra in DOM
Applying: Bug 11096: (qa followup) facets count and UNIMARC record syntax
Applying: Bug 11096: fallback to the correct defaults if config entries missing
Applying: Bug 11096: (follow-up) add direct regression tests
Applying: Bug 11096 - Wrap each test set configuration into a subtest
tomas@zoe:koha-community-src(clean_master)$ prove t/db_dependent/Search.t 2> /dev/null
t/db_dependent/Search.t .. ok   
All tests successful.
Files=1, Tests=4, 29 wallclock secs ( 0.09 usr  0.01 sys + 20.84 cusr  2.72 csys = 23.66 CPU)
Result: PASS
tomas@zoe:koha-community-src(clean_master)$
Comment 64 Katrin Fischer 2014-01-25 15:58:38 UTC
What exactly do you need?
Comment 65 Tomás Cohen Arazi 2014-01-29 21:15:14 UTC Comment hidden (obsolete)
Comment 66 Tomás Cohen Arazi 2014-01-30 16:38:51 UTC Comment hidden (obsolete)
Comment 67 Tomás Cohen Arazi 2014-01-30 16:39:15 UTC Comment hidden (obsolete)
Comment 68 Tomás Cohen Arazi 2014-01-30 16:41:50 UTC Comment hidden (obsolete)
Comment 69 Tomás Cohen Arazi 2014-01-30 18:10:31 UTC Comment hidden (obsolete)
Comment 70 Kyle M Hall 2014-02-14 16:04:40 UTC Comment hidden (obsolete)
Comment 71 Martin Renvoize 2014-02-26 10:52:54 UTC
Created attachment 25634 [details] [review]
[PASSED QA] Bug 11096: add direct regression tests

This adds direct tests of indexing and retrieving
bib records that are larger than the ISO2709 limit
of 99,999 octets.

To test: verify that prove -v t/db_dependent/Search.t passes.

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 72 Martin Renvoize 2014-02-26 10:53:10 UTC
Created attachment 25635 [details] [review]
[PASSED QA] Bug 11096: Wrap each test set configuration into a subtest

This makes it easier to read. Also fixes the test count, and is rebased
against 11499 which is needed to correctly test the patches for this bug
(i.e. indexing mode needs to be correctly set in koha-conf.xml)

Regards
To+

Sponsored-by: Universidad Nacional de Cordoba
Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 73 Martin Renvoize 2014-02-26 10:53:17 UTC
Created attachment 25636 [details] [review]
[PASSED QA] Bug 11096: Koha<->Zebra should use the XML syntax with DOM

This patch makes Koha <-> Zebra use MARCXML for the serialization when
using DOM, and USMARC for GRS-1.

* The following functions are modified to set the Zebra record syntax according
to the current sysprefs and configuration:

- C4::Context->Zconn
- C4::Context-_new_Zconn

* A new function 'new_record_from_zebra' is introduced, which checks the context
we are in, and creates the MARC::Record object using the right constructor.

The following packages get touched to make use of the new function:
- C4::Search
- C4::AuthoritiesMarc

and the same happens to the UI scripts that make use of them (both in the OPAC and
STAFF interfaces).

* Calls to the unsafe ZOOM::Record->render()[1] method are removed.
Due to this last change the code for building facets was rewritten. And for
performance on the facets creation I pushed higher version dependencies for
MARC::File::XML and MARC::Record (we rely on MARC::Field->as_string).

* Calls to MARC::Record->new_from_xml and MARC::Record->new_from_usmarc are wrapped with
eval for catching problems [2].

* As of bug 3087, UNIMARC uses the 'unimarc' record syntax. this case is correctly handled.

* As of bug 7818 misc/migration_tools/rebuild_zebra.pl behaves like:

- bib_index_mode (defaults to 'grs1' if not specified)
- auth_index_mode (defaults to 'dom')

here we do exactly the same.

To test:
 - prove t/db_dependent/Search.t should pass.
 - Searching should remain functional.
 - Indexing and searching for a big record should work (that's what the unit tests do).
 - Test an index scan search (on the staff interface):
    Search > More options > Check "Scan indexes".
 - Enable 'itemBarcodeFallbackSearch' and try to circulate any word, it shouldn't break.
 - Searching for a biblio in a new subscription shouldn't break.
 - Running bulkmarcimport.pl shouldn't break.
 - And so on... for the rest of the .pl files.

Regards
To+

[1] http://search.cpan.org/~mirk/Net-Z3950-ZOOM/lib/ZOOM.pod#render()
[2] a record that cannot be parsed by MARC::Record is simply skipped (bug 10684)

Sponsored-by: Universidad Nacional de Cordoba
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 74 Martin Renvoize 2014-02-26 11:00:56 UTC
Setting Passed QA:

1. Ran through the test plan without any failures using marc21 setup.
 - Tested authority searching still works as expected.
 - Tested bilbiographic searching still works as expected.
  - Simple search, also truncated search terms
  - Advanced search, also itemtype and pubyear limits
 - Tested dontmerge preference, editing a linked authority and
   confirming the change is made to the linked records as well
2. Ran db_dependant tests. Success all round
3. Ran through QA script. Success all round

I don't have a working UNIMARC setup, so I've not tested this again unimarc, however, with Katrin's earlier sign-off climing unimarc was ok, I thought this OK to pass anyway.
Comment 75 Galen Charlton 2014-02-28 19:57:48 UTC
Pushed to master.  Thanks, Tomás!
Comment 76 Tomás Cohen Arazi 2014-03-02 01:01:18 UTC
Yay!
Comment 77 Marcel de Rooy 2014-03-24 13:39:18 UTC
I hope this patch will not cause a lot of trouble for non-package users. At least I am struggling now with getting dom indexing to work in master. In 3.14.4 [without this patch], it worked right away.

I would not recommend to push this to stable too quickly.
Comment 78 Katrin Fischer 2014-03-24 13:52:12 UTC
Hi Marcel, all my tests were done on a dev installation and it seemed to work ok there, but I didn't test the very last iteration I think. Can you give some more details about the problems you are facing?
Comment 79 Marcel de Rooy 2014-03-26 13:36:37 UTC
(In reply to Katrin Fischer from comment #78)
> Hi Marcel, all my tests were done on a dev installation and it seemed to
> work ok there, but I didn't test the very last iteration I think. Can you
> give some more details about the problems you are facing?

OK. It seems to work fine. I was struggling with marcxml as element set name. But in a clean master it must work. I do not think that marcxml is an element set name (as we did use F for full in the old situation, which is more of an element set name, although we did not actually differentiate it from B [brief] at all). Just omitting the element set name here, could be even better. (I will submit some code in this direction on bug 9612 soon. May include it..)

So no reasons for me anymore to postpone pushing this to stable.
Comment 80 Frédéric Demians 2014-05-23 11:02:07 UTC
What is the impact of this patch on search performances? My tests show that OPAC result page seems about 2 times slower with this patch when compared without.
Comment 81 Fridolin SOMERS 2014-05-23 15:07:02 UTC
(In reply to Frédéric Demians from comment #80)
> What is the impact of this patch on search performances? My tests show that
> OPAC result page seems about 2 times slower with this patch when compared
> without.

I noticed it also.
I bet it comes from the change in facets code :
"This patch also removes calls to the unsafe ZOOM::Record->render()"
I agree this code is ugly but it was more performant.
Comment 82 Frédéric Demians 2014-05-24 05:37:23 UTC
(In reply to Fridolin SOMERS from comment #81)

> I noticed it also.  I bet it comes from the change in facets code : "This
> patch also removes calls to the unsafe ZOOM::Record->render()" I agree this
> code is ugly but it was more performant.

Yes, the facets building code is the culprit. It can be seen easily by playing
with maxRecordsForFacets syspref.
Comment 83 David Cook 2014-05-26 07:06:27 UTC
I think that we're having an issue with this in 3.14.5...

We're still getting out of bounds errors for large records.

Then, when we try to shorten those records, Zebra doesn't appear to be updating, even when doing a full re-index.

Using MARCXML switch, DOM indexing, non-package install.
Comment 84 Tomás Cohen Arazi 2014-05-26 18:58:01 UTC
(In reply to David Cook from comment #83)
> I think that we're having an issue with this in 3.14.5...
> 
> We're still getting out of bounds errors for large records.
> 
> Then, when we try to shorten those records, Zebra doesn't appear to be
> updating, even when doing a full re-index.
> 
> Using MARCXML switch, DOM indexing, non-package install.

David, I don't think this has been pushed to the stable branches.

Fridolin and Marcel, I'm filling a new bug for the performance hit. I think I have a patch to fix it.
Comment 85 David Cook 2014-05-26 23:32:00 UTC
(In reply to Tomás Cohen Arazi from comment #84)
> (In reply to David Cook from comment #83)
> > I think that we're having an issue with this in 3.14.5...
> > 
> > We're still getting out of bounds errors for large records.
> > 
> > Then, when we try to shorten those records, Zebra doesn't appear to be
> > updating, even when doing a full re-index.
> > 
> > Using MARCXML switch, DOM indexing, non-package install.
> 
> David, I don't think this has been pushed to the stable branches.
> 
> Fridolin and Marcel, I'm filling a new bug for the performance hit. I think
> I have a patch to fix it.

Ah, good one, Tomás. I was mixing up my dates. I thought "pushed to master" was for 3.14 but it was for 3.16.

My bad!
Comment 86 Pongtawat 2014-08-16 10:04:06 UTC
Just a note that this should also solve a problem with long tag. For example 505 with long UTF-8 text will easily exceed ISO2709 standard (of 9,999 chars) and cause problem with Zebra index and display.

Before this patch, for records with long tag, some index might work and some might not. Search display of these records may either missing (due to invalid UTF-8 sequence error), show up with no items or show up with broken link.

(I haven't test that this patch solve the long tag problem. But theoritically, it should).
Comment 87 Wolfram Schneider 2016-01-25 11:11:45 UTC
If marcxml + XSLT is to slow, then you might try to use turbomarc instead. It's a smaller XML for MARC and much faster to work with in XSLT.
Comment 88 David Cook 2016-01-26 23:27:14 UTC
(In reply to Wolfram Schneider from comment #87)
> If marcxml + XSLT is to slow, then you might try to use turbomarc instead.
> It's a smaller XML for MARC and much faster to work with in XSLT.

http://www.indexdata.com/news/2014/06/turbomarc-faster-xml-marc-records

I can't imagine us using Turbomarc for anything but indexing, but then we'd lose time converting from MARCXML to/from Turbomarc as well...

Personally, I like the idea of using our own intermediary format which can be exported as MARCXML (or Turbomarc or whatever), although I suppose the problem with that is our intermediary format could grow and then not be perfectly exportable as MARCXML, which means lost data for users, although perhaps not a big problem if you provide easy ways of customizing exports, so people could export into fields like 9AA (see http://www.loc.gov/standards/marcxml/schema/MARC21slim.xsd for valid tag ranges) and then massage from there...

The intermediary format could also help us move away from XML and Zebra anyway, so that we could use any indexing engine like Solr, ElasticSearch, or whatever.

Really, we only need MARC(XML) for interchange.

Consider this:

"2.1. The MARC 21 formats are communication formats, primarily designed to provide specifications for the exchange of bibliographic and related information between systems. They are widely used in a variety of exchange and processing environments. As communication formats, they do not mandate internal storage or display formats to be used by individual systems."
(https://www.loc.gov/marc/96principl.html)

I suppose my argument could be used to support turbomarc as well... although I don't know how well turbomarc would work itself as a format (besides the performance improvement for XSL transformations).

I suppose another argument against changing from MARC(XML) could be the need to provide sufficient mechanisms for using AACR2 and RDA. MARC pretty much already has that built in, but then it also has limitations. 

Anyway, that's all rather tangential.

I don't know about turbomarc. It seems like it would be a bit painful to work with :/.