Bug 17196

Summary: Move marcxml out of the biblioitems table
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: bob, hagud, josef.moravec, katrin.fischer, kyle, m.de.rooy, martin.renvoize, mtj, nick, olli-antti.kivilahti, pablo.bianchi, ztajoli
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17088
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19332
Change sponsored?: --- Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
This development moves marcxml out of the biblioitems and deletedbiblioitems tables and moves it to two new tables: biblio_metadata and deletedbiblio_metadata. SQL queries using the biblioitems table but not the marcxml column will get a performance boost. Storing the marcxml in a specific table will allow us to store several metadata formats (USMARC, MARCXML, MIJ, etc.). ATTENTION: all reports that use ExtractValue to retrieve MARC tags from biblioitems.marcxml need to be updated/rewritten.
Version(s) released in:
Bug Depends on: 10455, 17193, 17197    
Bug Blocks: 18265, 17898, 18284, 19332, 19583, 19638, 19724, 21908    
Attachments: Bug 17196: Move marcxml out of the biblioitems table
Bug 17196: Fix query builder for item search
Bug 17196: Update misc/maintenance/sanitize_records.pl
Bug 17196: Update switch_marc21_series_info.pl
Bug 17196: DB Changes
Bug 17196: DB Changes
Bug 17196: Move marcxml out of the biblioitems table
Bug 17196: Fix query builder for item search
Bug 17196: Update misc/maintenance/sanitize_records.pl
Bug 17196: Update switch_marc21_series_info.pl
Bug 17196: DB Changes
Bug 17196: Move marcxml out of the biblioitems table
Bug 17196: Fix query builder for item search
Bug 17196: Update misc/maintenance/sanitize_records.pl
Bug 17196: Update switch_marc21_series_info.pl
Bug 17196: DB Changes
Bug 17196: Simplify tests for the different marc flavours
Bug 17196: Fix - Update metadata on update
Bug 17196: Simplify tests for the different marc flavours
Bug 17196: Fix - Update metadata on update
Bug 17196: follow-up for bug 17088
Bug 17196: Move marcxml out of the biblioitems table
Bug 17196: Fix query builder for item search
Bug 17196: Update misc/maintenance/sanitize_records.pl
Bug 17196: Update switch_marc21_series_info.pl
Bug 17196: DB Changes
Bug 17196: Simplify tests for the different marc flavours
Bug 17196: Fix - Update metadata on update
Bug 17196: follow-up for bug 17088
Bug 17196: follow-up for bug 17088
Bug 17196: Fix build_oai_sets.pl
Bug 17196: Update POD of sanitise_records.pl
[SIGNED-OFF] Bug 17196: Move marcxml out of the biblioitems table
[SIGNED-OFF] Bug 17196: Fix query builder for item search
[SIGNED-OFF] Bug 17196: Update misc/maintenance/sanitize_records.pl
[SIGNED-OFF] Bug 17196: Update switch_marc21_series_info.pl
[SIGNED-OFF] Bug 17196: DB Changes
[SIGNED-OFF] Bug 17196: Simplify tests for the different marc flavours
[SIGNED-OFF] Bug 17196: Fix - Update metadata on update
[SIGNED-OFF] Bug 17196: follow-up for bug 17088
[SIGNED-OFF] Bug 17196: Fix build_oai_sets.pl
[SIGNED-OFF] Bug 17196: Update POD of sanitise_records.pl
Move marcxml out of the biblioitems table
Fix query builder for item search
[SIGNED-OFF] Bug 17196: Move marcxml out of the biblioitems table
[SIGNED-OFF] Bug 17196: Fix query builder for item search
[SIGNED-OFF] Bug 17196: Update misc/maintenance/sanitize_records.pl
[SIGNED-OFF] Bug 17196: Update switch_marc21_series_info.pl
[SIGNED-OFF] Bug 17196: DB Changes
[SIGNED-OFF] Bug 17196: Simplify tests for the different marc flavours
[SIGNED-OFF] Bug 17196: Fix - Update metadata on update
[SIGNED-OFF] Bug 17196: follow-up for bug 17088
[SIGNED-OFF] Bug 17196: Fix build_oai_sets.pl
[SIGNED-OFF] Bug 17196: Update POD of sanitise_records.pl
Bug 17196: Fix delete_records_via_leader.pl
Bug 17196: Move marcxml out of the biblioitems table
Bug 17196: Fix query builder for item search
Bug 17196: Update misc/maintenance/sanitize_records.pl
Bug 17196: Update switch_marc21_series_info.pl
Bug 17196: DB Changes
Bug 17196: Simplify tests for the different marc flavours
Bug 17196: Fix - Update metadata on update
Bug 17196: follow-up for bug 17088
Bug 17196: Fix build_oai_sets.pl
Bug 17196: Update POD of sanitise_records.pl
Bug 17196: Fix delete_records_via_leader.pl
Bug 17196 - (QA Followup) Add items() to Koha::Biblioitems
Bug 17196: Move marcxml out of the biblioitems table
Bug 17196: Fix query builder for item search
Bug 17196: Update misc/maintenance/sanitize_records.pl
Bug 17196: Update switch_marc21_series_info.pl
Bug 17196: DB Changes
Bug 17196: Simplify tests for the different marc flavours
Bug 17196: Fix - Update metadata on update
Bug 17196: follow-up for bug 17088
Bug 17196: Fix build_oai_sets.pl
Bug 17196: Update POD of sanitise_records.pl
Bug 17196: Fix delete_records_via_leader.pl
Bug 17196 - (QA Followup) Use items from Koha::Biblio
Bug 17196: [QA Follow-up] Correct POD in ILSDI/Services.pm
Bug 17196: [QA Follow-up] DBIx schema changes
Bug 17196: [QA Follow-up] Wrong module in Record.t
Bug 17196: [QA Follow-up] Missing unique key in kohastructure
Bug 17196: Remove occurrence of marcxml in columns.def
Bug 17196: Remove occurrence of marcxml in columns.def
Bug 17196: [QA Follow-up] Adjust some text on marcxml
Bug 17196: [QA Follow-up] Additional fix on acqui/basketgroup

Description Jonathan Druart 2016-08-25 13:43:45 UTC

    
Comment 1 Jonathan Druart 2016-08-26 09:33:13 UTC
Created attachment 54910 [details] [review]
Bug 17196: Move marcxml out of the biblioitems table

Two discussions on koha-devel lead to the same conclusion:
biblioitems.marcxml should be moved out this table
- biblio and biblioitems
http://lists.koha-community.org/pipermail/koha-devel/2013-April/039239.html
- biblioitems.marcxml & biblioitems.marc / HUGE performance issue !
http://lists.koha-community.org/pipermail/koha-devel/2016-July/042821.html

There are several goals to do it:
- Performance
As Paul Poulain wrote, a simple query like
  SELECT publicationyear, count(publicationyear) FROM biblioitems GROUP BY publicationyear;
takes more than 10min on a DB with more than 1M bibliographic records
but only 3sec (!) on the same DB without the biblioitems.marcxml field
Note that priori to this patch set, the biblioitems.marcxml was not
retrieved systematically, but was, at least, in
C4::Acquisition::GetOrdersByBiblionumber and C4::Acquisition::GetOrders
- Flexibility
Storing the marcxml in a specific table would allow use to store several
kind of metadata (USMARC, MARCXML, MIJ, etc.) and different formats (marcflavour)
- Clean code
It would be a first step toward Koha::MetadataRecord for bibliographic
records (not done in this patch set).

Test plan:
- Update the DBIC Schema
- Add / Edit / Delete / Import / Export bibliographic records
- Add items
- Reindex records using ES
- Confirm that the following scripts still work:
    * misc/cronjobs/delete_records_via_leader.pl
    * misc/migration_tools/build_oai_sets.pl
- Look at the reading history at the OPAC (opac-readingrecord.pl)
- At the OPAC, click on a tag, you must see the result

Note: Changes in Koha/OAI/Server/ListRecords.pm is planned on bug 15108.
Comment 2 Jonathan Druart 2016-08-26 09:33:16 UTC
Created attachment 54911 [details] [review]
Bug 17196: Fix query builder for item search

The item search needs to join on biblio_metadata to allow search on
marcxml field

Test plan:
Launch complex item searches (using marc fields).
Comment 3 Jonathan Druart 2016-08-26 09:33:22 UTC
Created attachment 54912 [details] [review]
Bug 17196: Update misc/maintenance/sanitize_records.pl

Test plan:
Use the sanitize_records.pl maintenance script with the --auto-search
option
Comment 4 Jonathan Druart 2016-08-26 09:33:25 UTC
Created attachment 54913 [details] [review]
Bug 17196: Update switch_marc21_series_info.pl

Not sure if this script is still used, could someone confirm?

Test plan:
If you know how to test it, please do
Comment 5 Jonathan Druart 2016-08-26 09:33:36 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2016-08-26 10:06:51 UTC
Created attachment 54918 [details] [review]
Bug 17196: DB Changes

Remove biblioitems.marcxml and deletedbiblioitems.marcxml
and adds 2 table biblio_metadata and deletedbiblio_metadata
Comment 7 Mason James 2016-09-26 01:53:07 UTC
Hi Jonathan

I attempted to test this patchset, but I had a big problem trying to 'git bz apply' the  1st patch, on master (0254fd7) 

0254fd7 Bug 17135 - DBRev 16.06.00.028

would you take another look at the patchset please?

---------------

mason@xen1:~/g/k/Koha$ git bz apply 17196
Bug 17196 - Move marcxml out of the biblioitems table

54910 - Bug 17196: Move marcxml out of the biblioitems table
54911 - Bug 17196: Fix query builder for item search
54912 - Bug 17196: Update misc/maintenance/sanitize_records.pl
54913 - Bug 17196: Update switch_marc21_series_info.pl
54918 - Bug 17196: DB Changes

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 17196: Move marcxml out of the biblioitems table
fatal: sha1 information is lacking or useless (C4/Biblio.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 17196: Move marcxml out of the biblioitems table
The copy of the patch that failed is found in:
   /home/mason/g/k/Koha/.git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-17196-Move-marcxml-out-of-the-biblioitems-tabl-jCnYTS.patch
Comment 8 Jonathan Druart 2016-09-27 09:27:36 UTC
(In reply to Mason James from comment #7)
> Hi Jonathan
> 
> I attempted to test this patchset, but I had a big problem trying to 'git bz
> apply' the  1st patch, on master (0254fd7) 
> 
> 0254fd7 Bug 17135 - DBRev 16.06.00.028
> 
> would you take another look at the patchset please?

Everything is applying ok for me, please check the dependencies.
Comment 9 Owen Leonard 2016-09-27 13:47:52 UTC
> > I attempted to test this patchset, but I had a big problem trying to 'git bz
> > apply' the  1st patch, on master (0254fd7) 

git bz didn't apply this successfully when using the auto-inclusion of dependent patches, but I don't know why. Applying the patches manually works without error.
Comment 10 Mason James 2016-09-28 00:06:26 UTC
(In reply to Jonathan Druart from comment #8)
> (In reply to Mason James from comment #7)
> > Hi Jonathan
> > 
> > I attempted to test this patchset, but I had a big problem trying to 'git bz
> > apply' the  1st patch, on master (0254fd7) 
> > 
> > 0254fd7 Bug 17135 - DBRev 16.06.00.028
> > 
> > would you take another look at the patchset please?
> 
> Everything is applying ok for me, please check the dependencies.

afaik (In reply to Jonathan Druart from comment #8)
> (In reply to Mason James from comment #7)
> > Hi Jonathan
> > 
> > I attempted to test this patchset, but I had a big problem trying to 'git bz
> > apply' the  1st patch, on master (0254fd7) 
> > 
> > 0254fd7 Bug 17135 - DBRev 16.06.00.028
> > 
> > would you take another look at the patchset please?
> 
> Everything is applying ok for me, please check the dependencies.

i have all listed dependencies applied on commit 56da7be (10455 17193 17197)

the patch still fails for me
Comment 11 Mason James 2016-09-28 00:07:43 UTC
(In reply to Mason James from comment #10)
> (In reply to Jonathan Druart from comment #8)
> > (In reply to Mason James from comment #7)
> > > Hi Jonathan
> > > 
> > > I attempted to test this patchset, but I had a big problem trying to 'git bz
> > > apply' the  1st patch, on master (0254fd7) 
> > > 
> > > 0254fd7 Bug 17135 - DBRev 16.06.00.028
> > > 
> > > would you take another look at the patchset please?
> > 
> > Everything is applying ok for me, please check the dependencies.
> 
> afaik (In reply to Jonathan Druart from comment #8)
> > (In reply to Mason James from comment #7)
> > > Hi Jonathan
> > > 
> > > I attempted to test this patchset, but I had a big problem trying to 'git bz
> > > apply' the  1st patch, on master (0254fd7) 
> > > 
> > > 0254fd7 Bug 17135 - DBRev 16.06.00.028
> > > 
> > > would you take another look at the patchset please?
> > 
> > Everything is applying ok for me, please check the dependencies.
> 
> i have all listed dependencies applied on commit 56da7be (10455 17193 17197)
> 
> the patch still fails for me

here's the transcript...

mason@xen1:~/g/k/Koha$ git log --oneline'
56da7be Bug 17352 - Patron search type is hard coded to 'contain' in circ/circulation.pl
5eb4b06 Bug 16273 - DBRev 16.06.00.031
3d4dc58 Bug 16273: Add the ability not to display the login details for newly registered patrons
8d0b412 Bug 16273: Add the new pref PatronSelfRegistrationPrefillForm
d85e80b Bug 17301: Follow-up - Standardize headings

mason@xen1:~/g/k/Koha$ git bz apply 10455
Bug 10455 - remove redundant 'biblioitems.marc' field
55844 - Bug 10455: Remove biblioitems.marc - DB changes
55845 - Bug 10455: Remove biblioitems.marc
55846 - Bug 10455: Remove biblioitems.marc - rm MARC21_parse_test.pl
55847 - Bug 10455: Remove biblioitems.marc - update delete_records_via_leader.pl
55848 - Bug 10455: (follow-up) Remove biblioitems.marc
55849 - Bug 10455: (follow-up 2) Remove biblioitems.marc
55850 - Bug 10455: (follow-up 3) Remove biblioitems.marc
55851 - Bug 10455: Remove MARC21_utf8_flag_fix.pl
55852 - Bug 10455: Fix tests
55853 - Bug 10455 (QA Followup)

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 10455: Remove biblioitems.marc - DB changes
Applying: Bug 10455: Remove biblioitems.marc
Applying: Bug 10455: Remove biblioitems.marc - rm MARC21_parse_test.pl
Applying: Bug 10455: Remove biblioitems.marc - update delete_records_via_leader.pl
Applying: Bug 10455: (follow-up) Remove biblioitems.marc
Applying: Bug 10455: (follow-up 2) Remove biblioitems.marc
Applying: Bug 10455: (follow-up 3) Remove biblioitems.marc
Applying: Bug 10455: Remove MARC21_utf8_flag_fix.pl
Applying: Bug 10455: Fix tests
Applying: Bug 10455 (QA Followup)
mason@xen1:~/g/k/Koha$
mason@xen1:~/g/k/Koha$ git bz apply 17193
Bug 17193 - C4::Search::SearchAcquisitions is not used

54962 - [PASSED QA] Bug 17193: Remove C4::Search::SearchAcquisitions

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 17193: Remove C4::Search::SearchAcquisitions
Using index info to reconstruct a base tree...
M       C4/Search.pm
Falling back to patching base and 3-way merge...
No changes -- Patch already applied.
mason@xen1:~/g/k/Koha$ git bz apply 17197
Bug 17197 - misc/batchupdateISBNs.pl is certainly no longer in use

55478 - [PASSED QA] Bug 17197: Remove misc/batchupdateISBNs.pl

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 17197: Remove misc/batchupdateISBNs.pl
Using index info to reconstruct a base tree...
A       misc/batchupdateISBNs.pl
Falling back to patching base and 3-way merge...
No changes -- Patch already applied.
mason@xen1:~/g/k/Koha$

mason@xen1:~/g/k/Koha$ git log --oneline'
4f6b1ee Bug 10455 (QA Followup)
98af430 Bug 10455: Fix tests
a5e6f98 Bug 10455: Remove MARC21_utf8_flag_fix.pl
fbb280d Bug 10455: (follow-up 3) Remove biblioitems.marc
81a0fb2 Bug 10455: (follow-up 2) Remove biblioitems.marc
4e3520d Bug 10455: (follow-up) Remove biblioitems.marc
339d2bc Bug 10455: Remove biblioitems.marc - update delete_records_via_leader.pl
d572063 Bug 10455: Remove biblioitems.marc - rm MARC21_parse_test.pl
dccc79a Bug 10455: Remove biblioitems.marc
558f6b1 Bug 10455: Remove biblioitems.marc - DB changes
56da7be Bug 17352 - Patron search type is hard coded to 'contain' in circ/circulation.pl
5eb4b06 Bug 16273 - DBRev 16.06.00.031

mason@xen1:~/g/k/Koha$ git bz apply 17196
Bug 17196 - Move marcxml out of the biblioitems table

54910 - Bug 17196: Move marcxml out of the biblioitems table
54911 - Bug 17196: Fix query builder for item search
54912 - Bug 17196: Update misc/maintenance/sanitize_records.pl
54913 - Bug 17196: Update switch_marc21_series_info.pl
54918 - Bug 17196: DB Changes

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 17196: Move marcxml out of the biblioitems table
fatal: sha1 information is lacking or useless (C4/Biblio.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 17196: Move marcxml out of the biblioitems table
The copy of the patch that failed is found in:
   /home/mason/g/k/Koha/.git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-17196-Move-marcxml-out-of-the-biblioitems-tabl-HBdRv1.patch

mason@xen1:~/g/k/Koha$ git apply /tmp/Bug-17196-Move-marcxml-out-of-the-biblioitems-tabl-HBdRv1.patch
error: patch failed: C4/Biblio.pm:42
error: C4/Biblio.pm: patch does not apply
error: patch failed: C4/ILSDI/Services.pm:406
error: C4/ILSDI/Services.pm: patch does not apply
error: patch failed: misc/cronjobs/delete_records_via_leader.pl:68
error: misc/cronjobs/delete_records_via_leader.pl: patch does not apply
Comment 12 Mason James 2016-09-28 00:15:50 UTC
(In reply to Owen Leonard from comment #9)
> > > I attempted to test this patchset, but I had a big problem trying to 'git bz
> > > apply' the  1st patch, on master (0254fd7) 
> 
> git bz didn't apply this successfully when using the auto-inclusion of
> dependent patches, but I don't know why. Applying the patches manually works
> without error.

hi Owen, would you paste your command workflow to the ticket, please?
Comment 13 Jonathan Druart 2016-09-28 07:40:29 UTC
Created attachment 55856 [details] [review]
Bug 17196: Move marcxml out of the biblioitems table

Two discussions on koha-devel lead to the same conclusion:
biblioitems.marcxml should be moved out this table
- biblio and biblioitems
http://lists.koha-community.org/pipermail/koha-devel/2013-April/039239.html
- biblioitems.marcxml & biblioitems.marc / HUGE performance issue !
http://lists.koha-community.org/pipermail/koha-devel/2016-July/042821.html

There are several goals to do it:
- Performance
As Paul Poulain wrote, a simple query like
  SELECT publicationyear, count(publicationyear) FROM biblioitems GROUP BY publicationyear;
takes more than 10min on a DB with more than 1M bibliographic records
but only 3sec (!) on the same DB without the biblioitems.marcxml field
Note that priori to this patch set, the biblioitems.marcxml was not
retrieved systematically, but was, at least, in
C4::Acquisition::GetOrdersByBiblionumber and C4::Acquisition::GetOrders
- Flexibility
Storing the marcxml in a specific table would allow use to store several
kind of metadata (USMARC, MARCXML, MIJ, etc.) and different formats (marcflavour)
- Clean code
It would be a first step toward Koha::MetadataRecord for bibliographic
records (not done in this patch set).

Test plan:
- Update the DBIC Schema
- Add / Edit / Delete / Import / Export bibliographic records
- Add items
- Reindex records using ES
- Confirm that the following scripts still work:
    * misc/cronjobs/delete_records_via_leader.pl
    * misc/migration_tools/build_oai_sets.pl
- Look at the reading history at the OPAC (opac-readingrecord.pl)
- At the OPAC, click on a tag, you must see the result

Note: Changes in Koha/OAI/Server/ListRecords.pm is planned on bug 15108.
Comment 14 Jonathan Druart 2016-09-28 07:40:32 UTC
Created attachment 55857 [details] [review]
Bug 17196: Fix query builder for item search

The item search needs to join on biblio_metadata to allow search on
marcxml field

Test plan:
Launch complex item searches (using marc fields).
Comment 15 Jonathan Druart 2016-09-28 07:40:36 UTC
Created attachment 55858 [details] [review]
Bug 17196: Update misc/maintenance/sanitize_records.pl

Test plan:
Use the sanitize_records.pl maintenance script with the --auto-search
option
Comment 16 Jonathan Druart 2016-09-28 07:40:39 UTC
Created attachment 55859 [details] [review]
Bug 17196: Update switch_marc21_series_info.pl

Not sure if this script is still used, could someone confirm?

Test plan:
If you know how to test it, please do
Comment 17 Jonathan Druart 2016-09-28 07:40:43 UTC
Created attachment 55860 [details] [review]
Bug 17196: DB Changes

Remove biblioitems.marcxml and deletedbiblioitems.marcxml
and adds 2 table biblio_metadata and deletedbiblio_metadata
Comment 18 Mason James 2016-09-29 07:58:58 UTC
Created attachment 55904 [details] [review]
Bug 17196: Move marcxml out of the biblioitems table

Two discussions on koha-devel lead to the same conclusion:
biblioitems.marcxml should be moved out this table
- biblio and biblioitems
http://lists.koha-community.org/pipermail/koha-devel/2013-April/039239.html
- biblioitems.marcxml & biblioitems.marc / HUGE performance issue !
http://lists.koha-community.org/pipermail/koha-devel/2016-July/042821.html

There are several goals to do it:
- Performance
As Paul Poulain wrote, a simple query like
  SELECT publicationyear, count(publicationyear) FROM biblioitems GROUP BY publicationyear;
takes more than 10min on a DB with more than 1M bibliographic records
but only 3sec (!) on the same DB without the biblioitems.marcxml field
Note that priori to this patch set, the biblioitems.marcxml was not
retrieved systematically, but was, at least, in
C4::Acquisition::GetOrdersByBiblionumber and C4::Acquisition::GetOrders
- Flexibility
Storing the marcxml in a specific table would allow use to store several
kind of metadata (USMARC, MARCXML, MIJ, etc.) and different formats (marcflavour)
- Clean code
It would be a first step toward Koha::MetadataRecord for bibliographic
records (not done in this patch set).

Test plan:
- Update the DBIC Schema
- Add / Edit / Delete / Import / Export bibliographic records
- Add items
- Reindex records using ES
- Confirm that the following scripts still work:
    * misc/cronjobs/delete_records_via_leader.pl
    * misc/migration_tools/build_oai_sets.pl
- Look at the reading history at the OPAC (opac-readingrecord.pl)
- At the OPAC, click on a tag, you must see the result

Note: Changes in Koha/OAI/Server/ListRecords.pm is planned on bug 15108.

Signed-off-by: Mason James <mtj@kohaaloha.com>
Comment 19 Mason James 2016-09-29 07:59:07 UTC
Created attachment 55905 [details] [review]
Bug 17196: Fix query builder for item search

The item search needs to join on biblio_metadata to allow search on
marcxml field

Test plan:
Launch complex item searches (using marc fields).

Signed-off-by: Mason James <mtj@kohaaloha.com>
Comment 20 Mason James 2016-09-29 07:59:13 UTC
Created attachment 55906 [details] [review]
Bug 17196: Update misc/maintenance/sanitize_records.pl

Test plan:
Use the sanitize_records.pl maintenance script with the --auto-search
option

Signed-off-by: Mason James <mtj@kohaaloha.com>
Comment 21 Mason James 2016-09-29 07:59:19 UTC
Created attachment 55907 [details] [review]
Bug 17196: Update switch_marc21_series_info.pl

Not sure if this script is still used, could someone confirm?

Test plan:
If you know how to test it, please do

Signed-off-by: Mason James <mtj@kohaaloha.com>
Comment 22 Mason James 2016-09-29 07:59:24 UTC
Created attachment 55908 [details] [review]
Bug 17196: DB Changes

Remove biblioitems.marcxml and deletedbiblioitems.marcxml
and adds 2 table biblio_metadata and deletedbiblio_metadata

Signed-off-by: Mason James <mtj@kohaaloha.com>
Comment 23 Mason James 2016-09-29 10:16:59 UTC
Hi Folks
I have made some good progress testing this patch-set

I did hit a problem, when attempting to save a bib record via the cataloguing editor
I added a "DBI->trace(2,'/tmp/some.log')" to the start of the ModBiblio() sub

When saving a bib, I noticed that there is no SQL in the log to UPDATE the 'biblio_metadata' table

fyi: before testing, i ran 'update_dbix_class_files.pl' to update the schemas
Comment 24 Mason James 2016-09-29 10:26:35 UTC
(In reply to Mason James from comment #23)
> Hi Folks
> I have made some good progress testing this patch-set
> 
> I did hit a problem, when attempting to save a bib record via the
> cataloguing editor
> I added a "DBI->trace(2,'/tmp/some.log')" to the start of the ModBiblio() sub
> 
> When saving a bib, I noticed that there is no SQL in the log to UPDATE the
> 'biblio_metadata' table

a quick grep thru the dbi.log confirms this (i think?)

$ grep UPDATE /tmp/some.log
    -> prepare for DBD::mysql::db (DBI::db=HASH(0x55203c8)~0x552f888 'UPDATE biblio SET frameworkcode=? WHERE biblionumber=?') thr#1a20010
        -> dbd_st_prepare MYSQL_VERSION_ID 50550, SQL statement: UPDATE biblio SET frameworkcode=? WHERE biblionumber=?
>count_params statement UPDATE biblio SET frameworkcode=? WHERE biblionumber=?
>parse_params statement UPDATE biblio SET frameworkcode=? WHERE biblionumber=?
Binding parameters: UPDATE biblio SET frameworkcode='BKS' WHERE biblionumber='3449'
        UPDATE biblio
        UPDATE biblio
        UPDATE biblio
        UPDATE biblio
Binding parameters: UPDATE biblio
    -> prepare for DBD::mysql::db (DBI::db=HASH(0x55203c8)~0x552f888 'UPDATE biblioitems
        -> dbd_st_prepare MYSQL_VERSION_ID 50550, SQL statement: UPDATE biblioitems
>count_params statement UPDATE biblioitems
>parse_params statement UPDATE biblioitems
Binding parameters: UPDATE biblioitems
Comment 25 Mason James 2016-09-29 10:28:57 UTC
(In reply to Mason James from comment #23)
> Hi Folks
> I have made some good progress testing this patch-set
> 
> I did hit a problem, when attempting to save a bib record via the
> cataloguing editor

could someone else confirm this bug, please?
Comment 26 Jonathan Druart 2016-09-29 12:38:28 UTC
Created attachment 55921 [details] [review]
Bug 17196: Simplify tests for the different marc flavours

centralize the way MARC field are set.
Comment 27 Jonathan Druart 2016-09-29 12:38:32 UTC
Created attachment 55922 [details] [review]
Bug 17196: Fix - Update metadata on update
Comment 28 Jonathan Druart 2016-09-29 12:39:04 UTC
(In reply to Mason James from comment #25)
> (In reply to Mason James from comment #23)
> > Hi Folks
> > I have made some good progress testing this patch-set
> > 
> > I did hit a problem, when attempting to save a bib record via the
> > cataloguing editor
> 
> could someone else confirm this bug, please?

Confirmed!
Sorry about that, I should have written these tests before...
Comment 29 Mason James 2016-09-30 00:25:31 UTC
Created attachment 55950 [details] [review]
Bug 17196: Simplify tests for the different marc flavours

centralize the way MARC field are set.

Signed-off-by: Mason James <mtj@kohaaloha.com>
Comment 30 Mason James 2016-09-30 00:25:58 UTC
Created attachment 55951 [details] [review]
Bug 17196: Fix - Update metadata on update

Signed-off-by: Mason James <mtj@kohaaloha.com>
Comment 31 Mason James 2016-09-30 00:29:35 UTC
(In reply to Jonathan Druart from comment #28)
> (In reply to Mason James from comment #25)
> > (In reply to Mason James from comment #23)
> > > Hi Folks
> > > I have made some good progress testing this patch-set
> > > 
> > > I did hit a problem, when attempting to save a bib record via the
> > > cataloguing editor
> > 
> > could someone else confirm this bug, please?
> 
> Confirmed!
> Sorry about that, I should have written these tests before...

Hi Jonathan, your last 2 patches fixed the 'save' problem. I signed-off on them

I realise that there is still some ES testing, for the patch-set. 
I'll try to do that
Comment 32 Jonathan Druart 2016-10-04 14:20:06 UTC
Created attachment 56038 [details] [review]
Bug 17196: follow-up for bug 17088

If bug 17088 is pushed first, we will need this patch.
Comment 33 Martin Renvoize 2016-10-20 11:12:14 UTC
So.. I'm in favour of this in general..

But.. I'm wondering about the wisdom of the schema a bit.

I would settle on a serialisation format (JSON, XML) and abstract the inflation/deflation of it into the dbic class using InflateColumn and friends.

This would allow us to take advantage of either postgres's jsonb store or mysql's in built xml handling consistently allowing those tools to be fully utilised in reports. Without doing so, would mean reports would end up much more limited.

So, I would ditch the 'format' field, fix the serialisation format in the metadata field and use dbic to inflate into perl data-structures consistently.  We could still happily bind to the 'marcflavour' field to build objects of different types from the inflated perl structures (though I would change marcflavour to just flavour in case we want to support non-marc later ;)

Just my two pence.
Comment 34 Hugo Agud 2016-11-30 16:19:55 UTC
CONFLICT (content): Merge conflict in koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-detail.tt
Comment 35 Hugo Agud 2016-11-30 16:46:17 UTC
sorry, I published the conflict in the wrong bug
Comment 36 Jonathan Druart 2016-11-30 16:55:01 UTC
Created attachment 57835 [details] [review]
Bug 17196: Move marcxml out of the biblioitems table

Two discussions on koha-devel lead to the same conclusion:
biblioitems.marcxml should be moved out this table
- biblio and biblioitems
http://lists.koha-community.org/pipermail/koha-devel/2013-April/039239.html
- biblioitems.marcxml & biblioitems.marc / HUGE performance issue !
http://lists.koha-community.org/pipermail/koha-devel/2016-July/042821.html

There are several goals to do it:
- Performance
As Paul Poulain wrote, a simple query like
  SELECT publicationyear, count(publicationyear) FROM biblioitems GROUP BY publicationyear;
takes more than 10min on a DB with more than 1M bibliographic records
but only 3sec (!) on the same DB without the biblioitems.marcxml field
Note that priori to this patch set, the biblioitems.marcxml was not
retrieved systematically, but was, at least, in
C4::Acquisition::GetOrdersByBiblionumber and C4::Acquisition::GetOrders
- Flexibility
Storing the marcxml in a specific table would allow use to store several
kind of metadata (USMARC, MARCXML, MIJ, etc.) and different formats (marcflavour)
- Clean code
It would be a first step toward Koha::MetadataRecord for bibliographic
records (not done in this patch set).

Test plan:
- Update the DBIC Schema
- Add / Edit / Delete / Import / Export bibliographic records
- Add items
- Reindex records using ES
- Confirm that the following scripts still work:
    * misc/cronjobs/delete_records_via_leader.pl
    * misc/migration_tools/build_oai_sets.pl
- Look at the reading history at the OPAC (opac-readingrecord.pl)
- At the OPAC, click on a tag, you must see the result

Note: Changes in Koha/OAI/Server/ListRecords.pm is planned on bug 15108.

Signed-off-by: Mason James <mtj@kohaaloha.com>
Comment 37 Jonathan Druart 2016-11-30 16:55:06 UTC
Created attachment 57836 [details] [review]
Bug 17196: Fix query builder for item search

The item search needs to join on biblio_metadata to allow search on
marcxml field

Test plan:
Launch complex item searches (using marc fields).

Signed-off-by: Mason James <mtj@kohaaloha.com>
Comment 38 Jonathan Druart 2016-11-30 16:55:11 UTC Comment hidden (obsolete)
Comment 39 Jonathan Druart 2016-11-30 16:55:16 UTC Comment hidden (obsolete)
Comment 40 Jonathan Druart 2016-11-30 16:55:19 UTC Comment hidden (obsolete)
Comment 41 Jonathan Druart 2016-11-30 16:55:24 UTC Comment hidden (obsolete)
Comment 42 Jonathan Druart 2016-11-30 16:55:28 UTC Comment hidden (obsolete)
Comment 43 Jonathan Druart 2016-11-30 16:55:31 UTC Comment hidden (obsolete)
Comment 44 Jonathan Druart 2016-11-30 16:56:45 UTC
(In reply to Hugo Agud from comment #35)
> sorry, I published the conflict in the wrong bug

But they needed a rebase anyway :)
Comment 45 Zeno Tajoli 2016-12-06 11:42:25 UTC
Hi Johathan,
I'm testig the patches in this bug.
I think there is a problem in "Bug 17196: follow-up for bug 17088".
Now is mandatory, bug 17088 is into the source.
The result of "prove Record.t" is:

Record.t .. DBIx::Class::Row::store_column(): No such column 'format' on Koha::Schema::Result::Biblioitem at /production/koha/install/bug17196/Koha/Object.pm line 71
# Looks like your test exited with 255 before it could output anything.
Record.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 4/4 subtests

In fact a'format' column is not present in table biblioitems.

Bye
Comment 46 Jonathan Druart 2016-12-06 14:06:07 UTC Comment hidden (obsolete)
Comment 47 Jonathan Druart 2016-12-06 14:06:33 UTC
Hi Zeno,
Yes, thanks for reporting.
Last patch should fix the tests.
Comment 48 Josef Moravec 2016-12-06 21:12:52 UTC
So, I started to test this, it looks good so far

only few thing:

1. build_oai_sets.pl script does return this error on every biblio:
(biblio 3) Error while creating record from marcxml: Empty String at /usr/share/perl5/MARC/File/XML.pm line 448.

2. When I click on tag in opac, I get ISE, the log says:
Can't locate object method "subfield" via package "<?xml version="1.0" encoding="UTF-8"?><record .. whole xml record" at C4/Search.pm line 1925

3. The usage text for --auto-search paramater in sanitize_records.pl still says: "Automatically search records containing "&amp;" in  biblioitems.marcxml or in the specified fields."
Comment 49 Martin Renvoize 2016-12-07 07:02:00 UTC
(In reply to Martin Renvoize from comment #33)

No response to this :(

Personally, I'd use JSON as both Postgres (https://www.postgresql.org/docs/current/static/datatype-json.html) and MySQL (https://dev.mysql.com/doc/refman/5.7/en/json.html) have pretty extensive support for marked performance gains using JSON typed fields.
Comment 50 Jonathan Druart 2016-12-07 07:51:48 UTC
(In reply to Martin Renvoize from comment #49)
> (In reply to Martin Renvoize from comment #33)
> 
> No response to this :(
> 
> Personally, I'd use JSON as both Postgres
> (https://www.postgresql.org/docs/current/static/datatype-json.html) and
> MySQL (https://dev.mysql.com/doc/refman/5.7/en/json.html) have pretty
> extensive support for marked performance gains using JSON typed fields.

The first goal of these patches is to move the data out of this table to bring speed improvement.
Afterwards we will think about what we can do with this new table :)
As you know, we are not ready yet to support another DBMS.
Comment 51 Jonathan Druart 2016-12-07 08:38:16 UTC
Created attachment 58007 [details] [review]
Bug 17196: Fix build_oai_sets.pl

The wrong value was retrieved.
Comment 52 Jonathan Druart 2016-12-07 08:38:21 UTC
Created attachment 58008 [details] [review]
Bug 17196: Update POD of sanitise_records.pl
Comment 53 Jonathan Druart 2016-12-07 08:40:46 UTC
(In reply to Josef Moravec from comment #48)
> So, I started to test this, it looks good so far
> 
> only few thing:
> 
> 1. build_oai_sets.pl script does return this error on every biblio:
> (biblio 3) Error while creating record from marcxml: Empty String at
> /usr/share/perl5/MARC/File/XML.pm line 448.

Will be fixed by
  Bug 17196: Fix build_oai_sets.pl

> 2. When I click on tag in opac, I get ISE, the log says:
> Can't locate object method "subfield" via package "<?xml version="1.0"
> encoding="UTF-8"?><record .. whole xml record" at C4/Search.pm line 1925

It's a bug from master, please open a new bug report.
Note that it only happens when SearchEngine=Elastic

> 3. The usage text for --auto-search paramater in sanitize_records.pl still
> says: "Automatically search records containing "&amp;" in 
> biblioitems.marcxml or in the specified fields."

Will be fixed by
  Bug 17196: Update POD of sanitise_records.pl
Comment 54 Zeno Tajoli 2016-12-07 12:38:15 UTC
Hi Jonathan,

I think there problems on "Fix query builder for item search".
I try to do a search with a personalized index based on 245$c.
The search doesn't use the index based on 245$c.
But it use the standard indexes (based on table 'items').

No errors in intranet logs

Bye
Comment 55 Jonathan Druart 2016-12-07 13:55:06 UTC
(In reply to Zeno Tajoli from comment #54)
> Hi Jonathan,
> 
> I think there problems on "Fix query builder for item search".
> I try to do a search with a personalized index based on 245$c.
> The search doesn't use the index based on 245$c.
> But it use the standard indexes (based on table 'items').
> 
> No errors in intranet logs
> 
> Bye

Hi Zeno, I am not sure to understand correctly what you mean. Which index did you create?
Did you confirm that what you describe is not the same behaviour as in master?
Comment 56 Zeno Tajoli 2016-12-07 14:06:21 UTC
Hi,

I go here:Home › Administration › Item search fields and I add a index based on tag 245$c.
But I check the default here:
http://kohastaffsandbox.ptfs-europe.co.uk and also on master it doesn't work.
So it is an other bug.

Bye
Comment 57 Jonathan Druart 2016-12-07 14:39:49 UTC
(In reply to Zeno Tajoli from comment #56)
> I go here:Home › Administration › Item search fields and I add a index based
> on tag 245$c.
> But I check the default here:
> http://kohastaffsandbox.ptfs-europe.co.uk and also on master it doesn't work.
> So it is an other bug.

Ok thanks, so please report it on another bug report.
Comment 58 Josef Moravec 2016-12-07 21:23:17 UTC
(In reply to Jonathan Druart from comment #53)
> (In reply to Josef Moravec from comment #48)
>
> > 2. When I click on tag in opac, I get ISE, the log says:
> > Can't locate object method "subfield" via package "<?xml version="1.0"
> > encoding="UTF-8"?><record .. whole xml record" at C4/Search.pm line 1925
> 
> It's a bug from master, please open a new bug report.
> Note that it only happens when SearchEngine=Elastic

Done in bug 17739 as you have seen ;)
Comment 59 Josef Moravec 2016-12-07 21:24:49 UTC Comment hidden (obsolete)
Comment 60 Josef Moravec 2016-12-07 21:24:57 UTC Comment hidden (obsolete)
Comment 61 Josef Moravec 2016-12-07 21:25:04 UTC Comment hidden (obsolete)
Comment 62 Josef Moravec 2016-12-07 21:25:11 UTC Comment hidden (obsolete)
Comment 63 Josef Moravec 2016-12-07 21:25:18 UTC Comment hidden (obsolete)
Comment 64 Josef Moravec 2016-12-07 21:25:26 UTC Comment hidden (obsolete)
Comment 65 Josef Moravec 2016-12-07 21:25:33 UTC Comment hidden (obsolete)
Comment 66 Josef Moravec 2016-12-07 21:25:41 UTC Comment hidden (obsolete)
Comment 67 Josef Moravec 2016-12-07 21:25:56 UTC Comment hidden (obsolete)
Comment 68 Josef Moravec 2016-12-07 21:26:59 UTC Comment hidden (obsolete)
Comment 69 Zeno Tajoli 2016-12-08 09:55:36 UTC Comment hidden (obsolete)
Comment 70 Zeno Tajoli 2016-12-08 09:56:18 UTC Comment hidden (obsolete)
Comment 71 Zeno Tajoli 2016-12-08 09:59:50 UTC Comment hidden (obsolete)
Comment 72 Zeno Tajoli 2016-12-08 10:00:41 UTC Comment hidden (obsolete)
Comment 73 Zeno Tajoli 2016-12-08 10:01:15 UTC Comment hidden (obsolete)
Comment 74 Zeno Tajoli 2016-12-08 10:02:02 UTC Comment hidden (obsolete)
Comment 75 Zeno Tajoli 2016-12-08 10:02:43 UTC Comment hidden (obsolete)
Comment 76 Zeno Tajoli 2016-12-08 10:03:15 UTC Comment hidden (obsolete)
Comment 77 Zeno Tajoli 2016-12-08 10:03:52 UTC Comment hidden (obsolete)
Comment 78 Zeno Tajoli 2016-12-08 10:04:27 UTC Comment hidden (obsolete)
Comment 79 Zeno Tajoli 2016-12-08 10:04:57 UTC Comment hidden (obsolete)
Comment 80 Zeno Tajoli 2016-12-08 10:05:26 UTC Comment hidden (obsolete)
Comment 81 Zeno Tajoli 2016-12-08 10:24:14 UTC
I have done a second sign-off and written notes about this develop.
Comment 82 Nick Clemens 2016-12-29 17:07:47 UTC
Testing this I found one things:

When trying delete_records_via_leader.pl:
kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ perl misc/cronjobs/delete_records_via_leader.pl -v -c -i
RECORD: 288
Can't use string ("Koha::Biblioitem") as a HASH ref while "strict refs" in use at /home/vagrant/kohaclone/Koha/Object.pm line 219.
Comment 83 Jonathan Druart 2016-12-29 17:39:36 UTC
Created attachment 58504 [details] [review]
Bug 17196: Fix delete_records_via_leader.pl
Comment 84 Nick Clemens 2016-12-29 18:20:00 UTC
Created attachment 58505 [details] [review]
Bug 17196: Move marcxml out of the biblioitems table

Two discussions on koha-devel lead to the same conclusion:
biblioitems.marcxml should be moved out this table
- biblio and biblioitems
http://lists.koha-community.org/pipermail/koha-devel/2013-April/039239.html
- biblioitems.marcxml & biblioitems.marc / HUGE performance issue !
http://lists.koha-community.org/pipermail/koha-devel/2016-July/042821.html

There are several goals to do it:
- Performance
As Paul Poulain wrote, a simple query like
  SELECT publicationyear, count(publicationyear) FROM biblioitems GROUP BY publicationyear;
takes more than 10min on a DB with more than 1M bibliographic records
but only 3sec (!) on the same DB without the biblioitems.marcxml field
Note that priori to this patch set, the biblioitems.marcxml was not
retrieved systematically, but was, at least, in
C4::Acquisition::GetOrdersByBiblionumber and C4::Acquisition::GetOrders
- Flexibility
Storing the marcxml in a specific table would allow use to store several
kind of metadata (USMARC, MARCXML, MIJ, etc.) and different formats (marcflavour)
- Clean code
It would be a first step toward Koha::MetadataRecord for bibliographic
records (not done in this patch set).

Test plan:
- Update the DBIC Schema
- Add / Edit / Delete / Import / Export bibliographic records
- Add items
- Reindex records using ES
- Confirm that the following scripts still work:
    * misc/cronjobs/delete_records_via_leader.pl
    * misc/migration_tools/build_oai_sets.pl
- Look at the reading history at the OPAC (opac-readingrecord.pl)
- At the OPAC, click on a tag, you must see the result

Note: Changes in Koha/OAI/Server/ListRecords.pm is planned on bug 15108.

Signed-off-by: Mason James <mtj@kohaaloha.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Zeno Tajoli <z.tajoli@cineca.it>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 85 Nick Clemens 2016-12-29 18:20:10 UTC
Created attachment 58506 [details] [review]
Bug 17196: Fix query builder for item search

The item search needs to join on biblio_metadata to allow search on
marcxml field

Test plan:
Launch complex item searches (using marc fields).

Signed-off-by: Mason James <mtj@kohaaloha.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Zeno Tajoli <z.tajoli@cineca.it>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 86 Nick Clemens 2016-12-29 18:20:34 UTC
Created attachment 58507 [details] [review]
Bug 17196: Update misc/maintenance/sanitize_records.pl

Test plan:
Use the sanitize_records.pl maintenance script with the --auto-search
option

Signed-off-by: Mason James <mtj@kohaaloha.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Zeno Tajoli <z.tajoli@cineca.it>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 87 Nick Clemens 2016-12-29 18:20:42 UTC
Created attachment 58508 [details] [review]
Bug 17196: Update switch_marc21_series_info.pl

Not sure if this script is still used, could someone confirm?

Test plan:
If you know how to test it, please do

Signed-off-by: Mason James <mtj@kohaaloha.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Zeno Tajoli <z.tajoli@cineca.it>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 88 Nick Clemens 2016-12-29 18:20:57 UTC
Created attachment 58509 [details] [review]
Bug 17196: DB Changes

Remove biblioitems.marcxml and deletedbiblioitems.marcxml
and adds 2 table biblio_metadata and deletedbiblio_metadata

Signed-off-by: Mason James <mtj@kohaaloha.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Zeno Tajoli <z.tajoli@cineca.it>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 89 Nick Clemens 2016-12-29 18:21:18 UTC
Created attachment 58510 [details] [review]
Bug 17196: Simplify tests for the different marc flavours

centralize the way MARC field are set.

Signed-off-by: Mason James <mtj@kohaaloha.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Zeno Tajoli <z.tajoli@cineca.it>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 90 Nick Clemens 2016-12-29 18:21:28 UTC
Created attachment 58511 [details] [review]
Bug 17196: Fix - Update metadata on update

Signed-off-by: Mason James <mtj@kohaaloha.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Zeno Tajoli <z.tajoli@cineca.it>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 91 Nick Clemens 2016-12-29 18:21:38 UTC
Created attachment 58512 [details] [review]
Bug 17196: follow-up for bug 17088

If bug 17088 is pushed first, we will need this patch.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Zeno Tajoli <z.tajoli@cineca.it>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 92 Nick Clemens 2016-12-29 18:21:55 UTC
Created attachment 58513 [details] [review]
Bug 17196: Fix build_oai_sets.pl

The wrong value was retrieved.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Zeno Tajoli <z.tajoli@cineca.it>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 93 Nick Clemens 2016-12-29 18:22:15 UTC
Created attachment 58514 [details] [review]
Bug 17196: Update POD of sanitise_records.pl

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Zeno Tajoli <z.tajoli@cineca.it>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 94 Nick Clemens 2016-12-29 18:23:37 UTC
Created attachment 58516 [details] [review]
Bug 17196: Fix delete_records_via_leader.pl

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 95 Nick Clemens 2016-12-29 18:23:45 UTC
Created attachment 58517 [details] [review]
Bug 17196 - (QA Followup) Add items() to Koha::Biblioitems

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 96 Nick Clemens 2016-12-29 18:24:16 UTC
Tested and provided a small update
Comment 97 Marcel de Rooy 2017-01-11 13:36:06 UTC
Still applies. If it does on early Friday morning, I will have a look.
Comment 98 Marcel de Rooy 2017-01-13 07:18:37 UTC
QA: Working on this one now.
Comment 99 Marcel de Rooy 2017-01-13 09:40:49 UTC
Created attachment 58923 [details] [review]
Bug 17196: Move marcxml out of the biblioitems table

Two discussions on koha-devel lead to the same conclusion:
biblioitems.marcxml should be moved out this table
- biblio and biblioitems
http://lists.koha-community.org/pipermail/koha-devel/2013-April/039239.html
- biblioitems.marcxml & biblioitems.marc / HUGE performance issue !
http://lists.koha-community.org/pipermail/koha-devel/2016-July/042821.html

There are several goals to do it:
- Performance
As Paul Poulain wrote, a simple query like
  SELECT publicationyear, count(publicationyear) FROM biblioitems GROUP BY publicationyear;
takes more than 10min on a DB with more than 1M bibliographic records
but only 3sec (!) on the same DB without the biblioitems.marcxml field
Note that priori to this patch set, the biblioitems.marcxml was not
retrieved systematically, but was, at least, in
C4::Acquisition::GetOrdersByBiblionumber and C4::Acquisition::GetOrders
- Flexibility
Storing the marcxml in a specific table would allow use to store several
kind of metadata (USMARC, MARCXML, MIJ, etc.) and different formats (marcflavour)
- Clean code
It would be a first step toward Koha::MetadataRecord for bibliographic
records (not done in this patch set).

Test plan:
- Update the DBIC Schema
- Add / Edit / Delete / Import / Export bibliographic records
- Add items
- Reindex records using ES
- Confirm that the following scripts still work:
    * misc/cronjobs/delete_records_via_leader.pl
    * misc/migration_tools/build_oai_sets.pl
- Look at the reading history at the OPAC (opac-readingrecord.pl)
- At the OPAC, click on a tag, you must see the result

Note: Changes in Koha/OAI/Server/ListRecords.pm is planned on bug 15108.

Signed-off-by: Mason James <mtj@kohaaloha.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Zeno Tajoli <z.tajoli@cineca.it>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 100 Marcel de Rooy 2017-01-13 09:40:54 UTC
Created attachment 58924 [details] [review]
Bug 17196: Fix query builder for item search

The item search needs to join on biblio_metadata to allow search on
marcxml field

Test plan:
Launch complex item searches (using marc fields).

Signed-off-by: Mason James <mtj@kohaaloha.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Zeno Tajoli <z.tajoli@cineca.it>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 101 Marcel de Rooy 2017-01-13 09:41:00 UTC
Created attachment 58925 [details] [review]
Bug 17196: Update misc/maintenance/sanitize_records.pl

Test plan:
Use the sanitize_records.pl maintenance script with the --auto-search
option

Signed-off-by: Mason James <mtj@kohaaloha.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Zeno Tajoli <z.tajoli@cineca.it>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 102 Marcel de Rooy 2017-01-13 09:41:06 UTC
Created attachment 58926 [details] [review]
Bug 17196: Update switch_marc21_series_info.pl

Not sure if this script is still used, could someone confirm?

Test plan:
If you know how to test it, please do

Signed-off-by: Mason James <mtj@kohaaloha.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Zeno Tajoli <z.tajoli@cineca.it>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 103 Marcel de Rooy 2017-01-13 09:41:12 UTC
Created attachment 58927 [details] [review]
Bug 17196: DB Changes

Remove biblioitems.marcxml and deletedbiblioitems.marcxml
and adds 2 table biblio_metadata and deletedbiblio_metadata

Signed-off-by: Mason James <mtj@kohaaloha.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Zeno Tajoli <z.tajoli@cineca.it>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 104 Marcel de Rooy 2017-01-13 09:41:18 UTC
Created attachment 58928 [details] [review]
Bug 17196: Simplify tests for the different marc flavours

centralize the way MARC field are set.

Signed-off-by: Mason James <mtj@kohaaloha.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Zeno Tajoli <z.tajoli@cineca.it>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 105 Marcel de Rooy 2017-01-13 09:41:24 UTC
Created attachment 58929 [details] [review]
Bug 17196: Fix - Update metadata on update

Signed-off-by: Mason James <mtj@kohaaloha.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Zeno Tajoli <z.tajoli@cineca.it>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 106 Marcel de Rooy 2017-01-13 09:41:29 UTC
Created attachment 58930 [details] [review]
Bug 17196: follow-up for bug 17088

If bug 17088 is pushed first, we will need this patch.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Zeno Tajoli <z.tajoli@cineca.it>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 107 Marcel de Rooy 2017-01-13 09:41:35 UTC
Created attachment 58931 [details] [review]
Bug 17196: Fix build_oai_sets.pl

The wrong value was retrieved.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Zeno Tajoli <z.tajoli@cineca.it>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 108 Marcel de Rooy 2017-01-13 09:41:40 UTC
Created attachment 58932 [details] [review]
Bug 17196: Update POD of sanitise_records.pl

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Zeno Tajoli <z.tajoli@cineca.it>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 109 Marcel de Rooy 2017-01-13 09:41:46 UTC
Created attachment 58933 [details] [review]
Bug 17196: Fix delete_records_via_leader.pl

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 110 Marcel de Rooy 2017-01-13 09:41:52 UTC
Created attachment 58934 [details] [review]
Bug 17196 - (QA Followup) Use items from Koha::Biblio

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended: Using items from Koha::Biblio seems better :)
Comment 111 Marcel de Rooy 2017-01-13 09:41:59 UTC
Created attachment 58935 [details] [review]
Bug 17196: [QA Follow-up] Correct POD in ILSDI/Services.pm

QA tools complains about:
Apparent command =cut not preceded by blank line.

Trivial fix.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 112 Marcel de Rooy 2017-01-13 09:42:04 UTC
Created attachment 58936 [details] [review]
Bug 17196: [QA Follow-up] DBIx schema changes

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 113 Marcel de Rooy 2017-01-13 09:42:10 UTC
Created attachment 58937 [details] [review]
Bug 17196: [QA Follow-up] Wrong module in Record.t

Should be the singular version instead of the plural one.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 114 Marcel de Rooy 2017-01-13 09:42:16 UTC
Created attachment 58938 [details] [review]
Bug 17196: [QA Follow-up] Missing unique key in kohastructure

See the atomic update.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested on command line.
Comment 115 Marcel de Rooy 2017-01-13 09:43:16 UTC
Still working on it little bit, but looks good. Please wait..
Comment 116 Marcel de Rooy 2017-01-13 09:45:04 UTC
Some preliminary findings:

opac/opac-readingrecord.pl: Calling GetXmlBiblio seems useless here. Probably should be GetMarcBiblio.
opac-search.pl: map { (C4::Context->config('zebra_bib_index_mode') eq 'dom')? $_->{marcxml}: $_->{marc}; }  This marc reference should not have been there anymore?
C4/Items.pm: changes aimed for catalogue/itemsearch.pl, but I am not sure if they are needed. Does item search actually looks into the xml? Seems not. New report??
Comment 117 Jonathan Druart 2017-01-13 09:56:23 UTC
(In reply to Marcel de Rooy from comment #116)
> Some preliminary findings:
> 
> opac/opac-readingrecord.pl: Calling GetXmlBiblio seems useless here.
> Probably should be GetMarcBiblio.

GetMarcBiblio does much more stuffs than GetXmlBiblio. Since we just want the metadata it's better like that.

> opac-search.pl: map { (C4::Context->config('zebra_bib_index_mode') eq
> 'dom')? $_->{marcxml}: $_->{marc}; }  This marc reference should not have
> been there anymore?

Nope, the column does no longer exist.

> C4/Items.pm: changes aimed for catalogue/itemsearch.pl, but I am not sure if
> they are needed. Does item search actually looks into the xml? Seems not.
> New report??

I don't think so, it uses it.
Comment 118 Jonathan Druart 2017-01-13 09:58:27 UTC
(In reply to Jonathan Druart from comment #117)
> > C4/Items.pm: changes aimed for catalogue/itemsearch.pl, but I am not sure if
> > they are needed. Does item search actually looks into the xml? Seems not.
> > New report??
> 
> I don't think so, it uses it.

See C4::Items::_SearchItems_build_where_fragment
Comment 119 Marcel de Rooy 2017-01-13 10:04:45 UTC
> C4/Items.pm: changes aimed for catalogue/itemsearch.pl, but I am not sure if
> they are needed. Does item search actually looks into the xml? Seems not.
> New report??

So that is wrong. You can do it via dmin/items_search_fields.pl.
Works fine.
Comment 120 Marcel de Rooy 2017-01-13 10:05:27 UTC
dmin -> admin
Comment 121 Jonathan Druart 2017-01-13 10:38:22 UTC
Created attachment 58939 [details] [review]
Bug 17196: Remove occurrence of marcxml in columns.def

Also remove a wrong ref to the marc column
Comment 122 Marcel de Rooy 2017-01-13 10:59:26 UTC
Created attachment 58943 [details] [review]
Bug 17196: Remove occurrence of marcxml in columns.def

Also remove a wrong ref to the marc column

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 123 Marcel de Rooy 2017-01-13 11:15:31 UTC
Created attachment 58946 [details] [review]
Bug 17196: [QA Follow-up] Adjust some text on marcxml

No code changes here, just text.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 124 Marcel de Rooy 2017-01-13 11:15:36 UTC
Created attachment 58947 [details] [review]
Bug 17196: [QA Follow-up] Additional fix on acqui/basketgroup

acqui/basketgroup calls GetOrders and expects marcxml in the results.
Fixing it by an additional call of GetXmlBiblio.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 125 Marcel de Rooy 2017-01-13 11:17:59 UTC
Last comments:

> Changes in Koha/OAI/Server/ListRecords.pm is planned on bug 15108.
Failed QA several weeks ago. The author did not respond until now.

[OK] t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t
[OK] t/db_dependent/Holds/HoldFulfillmentPolicy.t
[OK] t/db_dependent/Holds/HoldItemtypeLimit.t
[OK} t/db_dependent/HoldsQueue.t
[OK] t/db_dependent/UsageStats.t  
[OK] t/db_dependent/Members/GetAllIssues.t
[OK] t/db_dependent/Items.t
[OK] t/ImportBatch.t
[OK] t/db_dependent/OAI/Server.t
[OK] t/db_dependent/Record/Record.t
[OK] t/db_dependent/Record.t
[OK] t/db_dependent/Search.t
[OK] t/db_dependent/Acquisition/GetOrdersByBiblionumber.t
[OK] t/db_dependent/Acquisition/TransferOrder.t

misc/migration_tools/koha-svc.pl: Did not verify it this still works.
koha-svc.pl http://koha-dev:8080/cgi-bin/koha/svc svc-user svc-password $biblionumber [bib-42.xml]
Did not check REST API.

Well finally, we know about Reports. Should we still warn somewhere about that ?
Comment 126 Josef Moravec 2017-01-13 11:28:06 UTC
(In reply to Marcel de Rooy from comment #125)
> Last comments:
> 
> > Changes in Koha/OAI/Server/ListRecords.pm is planned on bug 15108.
> Failed QA several weeks ago. The author did not respond until now.
> 

Ere Maijala is primerly working on VuFind development, so probably we could try to contact them by e-mail directly to ask if he is plannig to work a bit on this OAI enhancement.
Comment 127 Marcel de Rooy 2017-01-13 12:00:57 UTC
(In reply to Josef Moravec from comment #126)
> (In reply to Marcel de Rooy from comment #125)
> > Last comments:
> > 
> > > Changes in Koha/OAI/Server/ListRecords.pm is planned on bug 15108.
> > Failed QA several weeks ago. The author did not respond until now.
> > 
> 
> Ere Maijala is primerly working on VuFind development, so probably we could
> try to contact them by e-mail directly to ask if he is plannig to work a bit
> on this OAI enhancement.

Josef,
If you know him, could you please ask him for rebase and feedback.
Thanks
Comment 128 Kyle M Hall 2017-01-13 14:39:58 UTC
Pushed to master for 17.05, thanks Jonathan, Nick, Marcel!
Comment 129 Katrin Fischer 2017-01-15 19:00:27 UTC
This won't get ported back to 16.11.x as it is an enhancement.
Comment 130 David Cook 2017-01-16 00:14:38 UTC
Comment on attachment 58927 [details] [review]
Bug 17196: DB Changes

Review of attachment 58927 [details] [review]:
-----------------------------------------------------------------

::: installer/data/mysql/kohastructure.sql
@@ +3926,5 @@
> +
> +CREATE TABLE biblio_metadata (
> +    `id` INT(11) NOT NULL AUTO_INCREMENT,
> +    `biblionumber` INT(11) NOT NULL,
> +    `format` VARCHAR(16) NOT NULL,

I know this has already been pushed, but maybe the RM could add some comments to this SQL?

In the comments, I think format was "USMARC, MARCXML, MiJ"? I'd maybe add a comment saying something like "ISO2709, XML, JSON", although maybe I'm misunderstanding how the field will actually be used...

@@ +3927,5 @@
> +CREATE TABLE biblio_metadata (
> +    `id` INT(11) NOT NULL AUTO_INCREMENT,
> +    `biblionumber` INT(11) NOT NULL,
> +    `format` VARCHAR(16) NOT NULL,
> +    `marcflavour` VARCHAR(16) NOT NULL,

It seems unfortunate that we're using marcflavour here after divorcing this table from MARC in every other way. 

Although I don't know how else we'd do this. Both UNIMARC and USMARC share the same MARCXML schema, and if we can have non-XML data, we couldn't use "schema" instad of "marcflavour". Although if it were Dublin Core, you'd want to be able to specify that schema I reckon. 

Maybe a person could cheat and use a format of XML and a schema of UNIMARC or USMARC, and you'd use the actual MARCXML schema for both but you'd know you're dealing with one standard or the other. 

I don't know. I'm guessing all the work's actually be done on this one anyway, so I'm probably too late with these comments.
Comment 131 Zeno Tajoli 2017-01-16 07:07:43 UTC
Hi,

> +CREATE TABLE biblio_metadata (
> +    `id` INT(11) NOT NULL AUTO_INCREMENT,
> +    `biblionumber` INT(11) NOT NULL,
> +    `format` VARCHAR(16) NOT NULL,
> +    `marcflavour` VARCHAR(16) NOT NULL,

in fact I think it is a case of not perfect selection of names.
'format' = 'physical_format' [ISO2709, XML, JSON, etc.]
'marcflavour' = 'logical_format' [MARC21, UNIMARC, Dublin Core, etc.]
Comment 132 Jonathan Druart 2017-01-16 08:38:35 UTC
I am willing to change the names of these fields if you have a good suggestion.
It's the beginning of the end of MARC, so I think I can spend 10 min to rename the fields.

(In reply to David Cook from comment #130)
> I don't know. I'm guessing all the work's actually be done on this one
> anyway, so I'm probably too late with these comments.

Indeed, I asked for feedbacks on these patches for the last 6 months...
But better late than never, isn't it?
Comment 133 Katrin Fischer 2017-01-22 16:45:44 UTC
This won't get ported back to 16.11.x as it is an enhancement.
Comment 134 Olli-Antti Kivilahti 2017-03-14 14:37:59 UTC
I don't want to beat a dead horse, but can't resist:

> There are several goals to do it:
> - Performance
> As Paul Poulain wrote, a simple query like
>   SELECT publicationyear, count(publicationyear) FROM biblioitems GROUP BY
> publicationyear;
> takes more than 10min on a DB with more than 1M bibliographic records
> but only 3sec (!) on the same DB without the biblioitems.marcxml field

All our test environments:
MariaDB [koha]> SELECT COUNT(*) FROM biblioitems;
+----------+
| COUNT(*) |
+----------+
|  1424537 |
+----------+
1 row in set (0.34 sec)


On Ubuntu14.04 MariaDB using a normal 10k HDD with Intel Xeon 3GHz:
innodb_buffer_pool_size = 128MB

MariaDB [koha]> SELECT publicationyear, count(publicationyear) FROM biblioitems GROUP BY publicationyear;
+-----------------+------------------------+
| publicationyear | count(publicationyear) |
+-----------------+------------------------+
| NULL            |                      0 |
+-----------------+------------------------+
1 row in set (39.64 sec)

The search is arguably slow.
I wonder how the performance inspiration server (10 min search time) is configured?


On Ubuntu14.04 MariaDB using RAID-1 SSD with Intel Xeon 3GHz 32GB RAM
innodb_buffer_pool_size = 24G (Percona autoconfigurer)

MariaDB [koha]> SELECT publicationyear, count(publicationyear) FROM biblioitems GROUP BY publicationyear;
+-----------------+------------------------+
| publicationyear | count(publicationyear) |
+-----------------+------------------------+
| NULL            |                      0 |
+-----------------+------------------------+
1 row in set (5.56 sec)




RAM and SSDs are really cheap nowadays.

See also Bug 18265
Comment 135 Jonathan Druart 2017-03-14 15:11:51 UTC
(In reply to Olli-Antti Kivilahti from comment #134)
> I don't want to beat a dead horse, but can't resist:

Try to be around when we ask for feedbacks and tests, *before* things are pushed. I am sure it would be more useful for everybody.
I cannot count how many times I have asked for help on this one.

> The search is arguably slow.
> I wonder how the performance inspiration server (10 min search time) is
> configured?

"This is a test DB, not optimized, but we were surprised by the results."
http://lists.koha-community.org/pipermail/koha-devel/2016-July/042821.html
Comment 136 Olli-Antti Kivilahti 2017-03-14 16:56:33 UTC
Just noticed while doing DB changes, that biblio_metadata is linked to the biblio table instead of the biblioitems-table!

This will be fixed in the followup.


>Try to be around when we ask for feedbacks and tests, *before* things are pushed.
>I am sure it would be more useful for everybody.
>I cannot count how many times I have asked for help on this one.

Unfortunately that is not possible at the moment. I can only bark later.
Comment 137 Olli-Antti Kivilahti 2017-03-15 10:34:48 UTC
Followup Bug 18265 is ready to be SO'd.

Regardless of the bugs found, I think this change is welcome.

I hope we get the fixes in asap.

http://quoteinvestigator.com/2014/12/16/no-mistakes/
Comment 138 Jonathan Druart 2017-03-16 18:24:21 UTC
(In reply to Olli-Antti Kivilahti from comment #137)
> Followup Bug 18265 is ready to be SO'd.


I have opened and submitted a patch on bug 18284 to deal with the move of the biblio metadata and the data lost.
Comment 139 Katrin Fischer 2019-02-10 02:18:35 UTC
*** Bug 15192 has been marked as a duplicate of this bug. ***