Bug 10455

Summary: remove redundant 'biblioitems.marc' field
Product: Koha Reporter: Mason James <mtj>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact:
Severity: normal    
Priority: P5 - low CC: dcook, f.demians, jonathan.druart, josef.moravec, katrin.fischer, kyle, m.de.rooy, magnus, nick, pasi.kallinen
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11084
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 15342, 17196, 17731, 20126    
Attachments: Bug 10455: Remove biblioitems.marc - DB changes
Bug 10455: Remove biblioitems.marc
Bug 10455: Remove biblioitems.marc - rm MARC21_parse_test.pl
Bug 10455: Remove biblioitems.marc - update delete_records_via_leader.pl
Bug 10455: (follow-up) Remove biblioitems.marc
Bug 10455: (follow-up) Remove biblioitems.marc
Bug 10455: (follow-up 2) Remove biblioitems.marc
Bug 10455: (follow-up 2) Remove biblioitems.marc
Bug 10455: (follow-up 2) Remove biblioitems.marc
Bug 10455: (follow-up 2) Remove biblioitems.marc
Bug 10455: (follow-up 3) Remove biblioitems.marc
Bug 10455: (follow-up 3) Remove biblioitems.marc
Bug 10455: Remove MARC21_utf8_flag_fix.pl
Bug 10455: Fix tests
Bug 10455: Fix tests
Bug 10455: Remove biblioitems.marc - DB changes
Bug 10455: Remove biblioitems.marc
Bug 10455: Remove biblioitems.marc - rm MARC21_parse_test.pl
Bug 10455: Remove biblioitems.marc - update delete_records_via_leader.pl
Bug 10455: (follow-up) Remove biblioitems.marc
Bug 10455: (follow-up 2) Remove biblioitems.marc
Bug 10455: (follow-up 3) Remove biblioitems.marc
Bug 10455: Remove MARC21_utf8_flag_fix.pl
Bug 10455: Fix tests
Bug 10455: Remove biblioitems.marc - DB changes
Bug 10455: Remove biblioitems.marc
Bug 10455: Remove biblioitems.marc - rm MARC21_parse_test.pl
Bug 10455: Remove biblioitems.marc - update delete_records_via_leader.pl
Bug 10455: (follow-up) Remove biblioitems.marc
Bug 10455: (follow-up 2) Remove biblioitems.marc
Bug 10455: (follow-up 3) Remove biblioitems.marc
Bug 10455: Remove MARC21_utf8_flag_fix.pl
Bug 10455: Fix tests
Bug 10455 (QA Followup)

Description Mason James 2013-06-13 03:11:43 UTC
in Koha, for each bib, we have identical MARC data stored in *two* different columns in the 'biblioitems' table (biblioitems.marcxml and biblioitems.marc)

as far as i know, we only need to store this data in one field, not two

this patch will attempt to remove code referencing the 'biblioitems.marc' field, and replace it with 'biblioitems.marcxml'
Comment 1 Magnus Enger 2013-06-24 09:23:02 UTC
The data might be the same, but the format is different, because biblioitems.marc is iso2709, right? 

I seem to remember someone saying that we should keep the iso2709 in biblioitems.marc, because if you want to export e.g. 500k records in iso2709 format, transforming all those records from marcxml on the fly would be *really* heavy. But I might be wrong...
Comment 2 paxed 2014-02-20 11:48:58 UTC
(In reply to Magnus Enger from comment #1)
> 
> I seem to remember someone saying that we should keep the iso2709 in
> biblioitems.marc, because if you want to export e.g. 500k records in iso2709
> format, transforming all those records from marcxml on the fly would be
> *really* heavy. But I might be wrong...

I'd think not having to take care of twice the data would weigh more than possibly having a hypothetical situation of exporting 500k iso2709 records...
Comment 3 Galen Charlton 2015-06-19 13:41:23 UTC
(In reply to Magnus Enger from comment #1)
> I seem to remember someone saying that we should keep the iso2709 in
> biblioitems.marc, because if you want to export e.g. 500k records in iso2709
> format, transforming all those records from marcxml on the fly would be
> *really* heavy. But I might be wrong...

Sure, if you want ISO2709 and are starting from MARCXML, converting to ISO2709 for an export will consume a bit more CPU than dumping pre-existing ISO2709 directly... but (a) the cost of conversion is not that much, (b) exports are relatively infrequent operations, (c) and as such, better to pay the cost during exports than to (admittedly slightly) slow down the operation of saving or modifying a bib record.

I'm taking of removing the "In Discussion" status in the hopes that somebody takes up writing a patch.
Comment 4 Jonathan Druart 2015-06-19 15:04:13 UTC
biblioitems.marc is used by misc/cronjobs/MARC21_parse_test.pl. Does this script still used?
Comment 5 David Cook 2015-06-22 03:44:08 UTC
(In reply to Jonathan Druart from comment #4)
> biblioitems.marc is used by misc/cronjobs/MARC21_parse_test.pl. Does this
> script still used?

I've never even heard of this script before, so I don't think that we use it. However, I suppose it's possible that someone uses it?

Could we just remove the references to biblioitems.marc in this one script?
Comment 6 Jonathan Druart 2015-06-22 08:29:22 UTC
(In reply to David Cook from comment #5)
> (In reply to Jonathan Druart from comment #4)
> > biblioitems.marc is used by misc/cronjobs/MARC21_parse_test.pl. Does this
> > script still used?
> 
> I've never even heard of this script before, so I don't think that we use
> it. However, I suppose it's possible that someone uses it?
> 
> Could we just remove the references to biblioitems.marc in this one script?

Yes we could, if nobody uses the --fix option:

243    -f --fix       Replace biblioitems.marcxml from data in marc field, default OFF
Comment 7 Magnus Enger 2015-11-25 10:17:00 UTC
(In reply to Jonathan Druart from comment #6)
> Yes we could, if nobody uses the --fix option:
> 
> 243    -f --fix       Replace biblioitems.marcxml from data in marc field,
> default OFF

The script was added in 2009 and not touched since:
http://git.koha-community.org/gitweb/?p=koha.git;a=commit;h=84ff3747b80d42914a8d570b6b4ff5667eab663b

From the commit message: 

"When marcxml cannot be parsed into a MARC::Record object, the biblio is
undisplayable and it obviously breaks many features in Koha.  This script
can test to parse every marcxml, and alert on failures.  Optionally, the
marcxml can be replaced from the marc field."

Didn't we at some point switch from treating marc (ISO2709) as the authoritative format, to treating marcxml as the authoritative format? I'd say loosing the ability to recreate marcxml from marc is no big deal.
Comment 8 Katrin Fischer 2015-11-25 11:30:23 UTC
I tend to agree with Magnus - I am not sure that this script makes that much sense.
Comment 9 David Cook 2016-01-11 00:59:49 UTC
Just noticed that http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11084 is using biblioitems.marc as a string... that would block this bug...
Comment 10 Jonathan Druart 2016-01-13 14:43:51 UTC
(In reply to Jonathan Druart from comment #6)
> (In reply to David Cook from comment #5)
> > (In reply to Jonathan Druart from comment #4)
> > > biblioitems.marc is used by misc/cronjobs/MARC21_parse_test.pl. Does this
> > > script still used?
> > 
> > I've never even heard of this script before, so I don't think that we use
> > it. However, I suppose it's possible that someone uses it?
> > 
> > Could we just remove the references to biblioitems.marc in this one script?
> 
> Yes we could, if nobody uses the --fix option:
> 
> 243    -f --fix       Replace biblioitems.marcxml from data in marc field,
> default OFF

I have just used it right now.
For testing purpose I have removed the marcxml field.
Using --fix, I was able to repopulate it from the marc.
Comment 11 David Cook 2016-01-24 23:30:24 UTC
(In reply to Jonathan Druart from comment #10)
> I have just used it right now.
> For testing purpose I have removed the marcxml field.
> Using --fix, I was able to repopulate it from the marc.

But why did you remove the marcxml field at all? :p
Comment 12 Jonathan Druart 2016-01-25 09:44:16 UTC
(In reply to David Cook from comment #11)
> (In reply to Jonathan Druart from comment #10)
> > I have just used it right now.
> > For testing purpose I have removed the marcxml field.
> > Using --fix, I was able to repopulate it from the marc.
> 
> But why did you remove the marcxml field at all? :p

I have no idea, really :D
Comment 13 Jonathan Druart 2016-08-25 09:00:46 UTC
As of a topic on the ML (http://lists.koha-community.org/pipermail/koha-devel/2016-July/042821.html) removing this column would bring performance gains.
Comment 14 Jonathan Druart 2016-08-25 09:30:20 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2016-08-25 09:30:24 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2016-08-25 09:30:28 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2016-08-25 09:30:35 UTC Comment hidden (obsolete)
Comment 18 Jonathan Druart 2016-08-25 10:27:20 UTC Comment hidden (obsolete)
Comment 19 Jonathan Druart 2016-08-25 10:30:31 UTC Comment hidden (obsolete)
Comment 20 Jonathan Druart 2016-08-25 12:31:53 UTC Comment hidden (obsolete)
Comment 21 Jonathan Druart 2016-08-25 12:33:09 UTC Comment hidden (obsolete)
Comment 22 Jonathan Druart 2016-08-25 12:44:02 UTC Comment hidden (obsolete)
Comment 23 Jonathan Druart 2016-08-25 12:45:12 UTC Comment hidden (obsolete)
Comment 24 Jonathan Druart 2016-08-25 12:45:16 UTC Comment hidden (obsolete)
Comment 25 Jonathan Druart 2016-08-25 12:48:17 UTC Comment hidden (obsolete)
Comment 26 Jonathan Druart 2016-08-25 13:55:14 UTC Comment hidden (obsolete)
Comment 27 Jonathan Druart 2016-08-25 15:18:18 UTC Comment hidden (obsolete)
Comment 28 Mason James 2016-09-09 07:53:24 UTC Comment hidden (obsolete)
Comment 29 Mason James 2016-09-09 08:20:38 UTC
Created attachment 55402 [details] [review]
Bug 10455: Remove biblioitems.marc - DB changes

Signed-off-by: Mason James <mtj@kohaaloha.com>
Comment 30 Mason James 2016-09-09 08:20:47 UTC
Created attachment 55403 [details] [review]
Bug 10455: Remove biblioitems.marc

Any discussions about biblioitems.marc bring to the same conclusion:
This field is useless and should be removed.
We are storing MARC data into 2 different fields, that does not make
sense.

Test plan:
Add / update / export / import /delete records
should work as before

Signed-off-by: Mason James <mtj@kohaaloha.com>
Comment 31 Mason James 2016-09-09 08:20:59 UTC
Created attachment 55404 [details] [review]
Bug 10455: Remove biblioitems.marc - rm MARC21_parse_test.pl

This script seems to be unused and it won't be of any usefulness after
the removal of biblioitems.marc

Signed-off-by: Mason James <mtj@kohaaloha.com>
Comment 32 Mason James 2016-09-09 08:21:05 UTC
Created attachment 55405 [details] [review]
Bug 10455: Remove biblioitems.marc - update delete_records_via_leader.pl

Recently added, delete_records_via_leader.pl reads biblioitems.marc as a
text field and search for record to delete regarding the leader 5.
This can be acchieve doing the same thing on biblioitems.marcxml (will
certainly be slower) waiting for a patch on bug 15537.

Test plan:
Confirm that this script works as before, to do so the easiest way would
be to dump your DB before executing the update DB entry, execute the
script to delete records, reinsert the DB, execute the udpate DB entry
(remove biblioitems.marc), execute the script to delete records.
You should get the same number of records deleted.

Signed-off-by: Mason James <mtj@kohaaloha.com>
Comment 33 Mason James 2016-09-09 08:21:10 UTC
Created attachment 55406 [details] [review]
Bug 10455: (follow-up) Remove biblioitems.marc

Signed-off-by: Mason James <mtj@kohaaloha.com>
Comment 34 Mason James 2016-09-09 08:21:15 UTC
Created attachment 55407 [details] [review]
Bug 10455: (follow-up 2) Remove biblioitems.marc

Signed-off-by: Mason James <mtj@kohaaloha.com>
Comment 35 Mason James 2016-09-09 08:21:20 UTC
Created attachment 55408 [details] [review]
Bug 10455: (follow-up 3) Remove biblioitems.marc

Signed-off-by: Mason James <mtj@kohaaloha.com>
Comment 36 Mason James 2016-09-09 08:21:26 UTC
Created attachment 55409 [details] [review]
Bug 10455: Remove MARC21_utf8_flag_fix.pl

Is this script still in use?
It uses the biblioitems.marc field so if it's still useful it will need
to be rewritten.

Signed-off-by: Mason James <mtj@kohaaloha.com>
Comment 37 Mason James 2016-09-09 08:21:31 UTC
Created attachment 55410 [details] [review]
Bug 10455: Fix tests

Signed-off-by: Mason James <mtj@kohaaloha.com>
Comment 38 Mason James 2016-09-09 10:01:04 UTC
I've tested Jonathan's 9 patches. 

Everything seems to work as expected, and the patches all look good to me

I used the misc/devel/update_dbix_class_files.pl script to build the Koha/Scheme/Results/* files, before testing delete_records_via_leader.pl

thanks Jonathan! :0)
Comment 39 Nick Clemens 2016-09-27 16:05:48 UTC
In:
misc/cronjobs/delete_records_via_leader.pl

I think you need:
$schema->resultset('Biblioitem')->search( { marcxml => { LIKE => '%<leader>_____d%' } } );

Instead of:
$schema->resultset('Biblioitem')->search( { marcxml => { LIKE => '<leader>_____d%' } } );

Otherwise seems to be working well
Comment 40 Nick Clemens 2016-09-27 17:46:00 UTC
Created attachment 55844 [details] [review]
Bug 10455: Remove biblioitems.marc - DB changes

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 41 Nick Clemens 2016-09-27 17:46:06 UTC
Created attachment 55845 [details] [review]
Bug 10455: Remove biblioitems.marc

Any discussions about biblioitems.marc bring to the same conclusion:
This field is useless and should be removed.
We are storing MARC data into 2 different fields, that does not make
sense.

Test plan:
Add / update / export / import /delete records
should work as before

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 42 Nick Clemens 2016-09-27 17:46:12 UTC
Created attachment 55846 [details] [review]
Bug 10455: Remove biblioitems.marc - rm MARC21_parse_test.pl

This script seems to be unused and it won't be of any usefulness after
the removal of biblioitems.marc

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 43 Nick Clemens 2016-09-27 17:46:17 UTC
Created attachment 55847 [details] [review]
Bug 10455: Remove biblioitems.marc - update delete_records_via_leader.pl

Recently added, delete_records_via_leader.pl reads biblioitems.marc as a
text field and search for record to delete regarding the leader 5.
This can be acchieve doing the same thing on biblioitems.marcxml (will
certainly be slower) waiting for a patch on bug 15537.

Test plan:
Confirm that this script works as before, to do so the easiest way would
be to dump your DB before executing the update DB entry, execute the
script to delete records, reinsert the DB, execute the udpate DB entry
(remove biblioitems.marc), execute the script to delete records.
You should get the same number of records deleted.

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 44 Nick Clemens 2016-09-27 17:46:23 UTC
Created attachment 55848 [details] [review]
Bug 10455: (follow-up) Remove biblioitems.marc

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 45 Nick Clemens 2016-09-27 17:46:28 UTC
Created attachment 55849 [details] [review]
Bug 10455: (follow-up 2) Remove biblioitems.marc

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 46 Nick Clemens 2016-09-27 17:46:34 UTC
Created attachment 55850 [details] [review]
Bug 10455: (follow-up 3) Remove biblioitems.marc

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 47 Nick Clemens 2016-09-27 17:46:38 UTC
Created attachment 55851 [details] [review]
Bug 10455: Remove MARC21_utf8_flag_fix.pl

Is this script still in use?
It uses the biblioitems.marc field so if it's still useful it will need
to be rewritten.

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 48 Nick Clemens 2016-09-27 17:46:53 UTC
Created attachment 55852 [details] [review]
Bug 10455: Fix tests

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 49 Nick Clemens 2016-09-27 17:47:06 UTC
Created attachment 55853 [details] [review]
Bug 10455 (QA Followup)

Fix record matching in misc/cronjobs/delete_records_via_leader.pl

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 50 David Cook 2016-10-11 06:23:03 UTC
Woo team!
Comment 51 Kyle M Hall 2016-10-21 14:48:17 UTC
Pushed to master for 16.11, thanks Jonathan!
Comment 52 Frédéric Demians 2016-10-22 13:04:01 UTC
Great. Rather an 'enhancement' than a 'normal' bug for me.
Comment 53 Katrin Fischer 2016-11-30 07:02:04 UTC
16.11 is stable now, but this was pushed pre-release and won't be backported.
Comment 54 Marcel de Rooy 2018-02-02 10:26:38 UTC
This change has interesting side-effects !

-    $sth = $dbh->prepare("UPDATE biblioitems SET marc=?,marcxml=? WHERE biblionumber=?");
-    $sth->execute( $record->as_usmarc(), $record->as_xml_record($encoding), $biblionumber );
+    $sth = $dbh->prepare("UPDATE biblioitems SET marcxml=? WHERE biblionumber=?");
+    $sth->execute( $record->as_xml_record($encoding), $biblionumber );

It seems that since we do no longer call the sub as_usmarc, the automatic field lengths in the marc are not calculated anymore.
This needs our attention. Will open a new report now!
Comment 55 Marcel de Rooy 2018-02-02 11:08:40 UTC
(In reply to Marcel de Rooy from comment #54)
> This change has interesting side-effects !
> 
> -    $sth = $dbh->prepare("UPDATE biblioitems SET marc=?,marcxml=? WHERE
> biblionumber=?");
> -    $sth->execute( $record->as_usmarc(), $record->as_xml_record($encoding),
> $biblionumber );
> +    $sth = $dbh->prepare("UPDATE biblioitems SET marcxml=? WHERE
> biblionumber=?");
> +    $sth->execute( $record->as_xml_record($encoding), $biblionumber );
> 
> It seems that since we do no longer call the sub as_usmarc, the automatic
> field lengths in the marc are not calculated anymore.
> This needs our attention. Will open a new report now!

Trivial fix submitted. Please have a look and sign off.