Bug 17676

Summary: Default COLLATE for marc_subfield_structure is not set
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: critical    
Priority: P5 - low CC: katrin.fischer, kyle, m.de.rooy, mirko, mtompset
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17721
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19670
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 11944, 17216    
Bug Blocks:    
Attachments: Bug 17676: Update default COLLATE of marc_subfield_structure
[Patch for versions prior to 16.11] Bug 17676: Update default COLLATE of marc_subfield_structure
Bug 17676: Update default COLLATE of marc_subfield_structure
Bug 17676: Update default COLLATE of marc_subfield_structure
Bug 17676: [Pre-16.11 patch] Update default COLLATE of marc_subfield_structure
Bug 17676: Update default COLLATE of marc_subfield_structure

Description Jonathan Druart 2016-11-24 13:43:59 UTC
(This issue has been raised on bug 17216 comments 93-103)
On bug 11944 (3.19.00.006) we updated the default COLLATE for all our tables to utf8_unicode_ci but not the marc_subfield_structure table.
Indeed we want to keep tagsubfield a utf8_bin (to allow lowercase and uppercase of the same letter for subfields).
We should have set the default collate to utf8_unicode_ci for further changes.
Comment 1 Jonathan Druart 2016-11-24 13:52:58 UTC
Created attachment 57749 [details] [review]
Bug 17676: Update default COLLATE of marc_subfield_structure

(This issue has been raised on bug 17216 comments 93-103)
On bug 11944 (3.19.00.006) we updated the default COLLATE for all our tables to
utf8_unicode_ci but not the marc_subfield_structure table.
Indeed we want to keep tagsubfield a utf8_bin (to allow lowercase and uppercase
of the same letter for subfields).
We should have set the default collate to utf8_unicode_ci for further changes.

This patch updates the DB entry 3.19.00.006 to set the default COLLATE to this
table (for people upgrading from prior to 3.19.00.006) and set this
default COLLATE on 16.06.00.033 (for people upgrading from after 3.19.00.006).

The error is:
DBD::mysql::db do failed: Can't create table `koha_kohadev`.`#sql-306_9f9` (errno: 150 "Foreign key constraint is incorrectly formed") [for Statement "
            ALTER TABLE marc_subfield_structure
            MODIFY COLUMN authorised_value VARCHAR(32) DEFAULT NULL,
            ADD CONSTRAINT marc_subfield_structure_ibfk_1 FOREIGN KEY (authorised_value) REFERENCES authorised_value_categories (category_name) ON UPDATE CASCADE ON DELETE SET NULL;
            "] at installer/data/mysql/updatedatabase.pl line 13175.
Upgrade to 16.06.00.033 done (Bug 17216 - Add a new table to store authorized value categories)

Test plan:
1/ git checkout v3.18.00
2/ Do an install
3/ git checkout master;
4/ perl installer/data/mysql/updatedatabase.pl
=> Without this patch, you get the error
=> With this patch applied you will not get it and the default COLLATE for
marc_subfield_structure will be correctly set. Make sure tagsubfield is
still utf8_bin
Comment 2 Jonathan Druart 2016-11-24 13:56:36 UTC
Created attachment 57750 [details] [review]
[Patch for versions prior to 16.11] Bug 17676: Update default COLLATE of marc_subfield_structure

Patch for branches < 16.11
See bug 17676 for more information
Comment 3 Mark Tompsett 2016-11-24 19:12:58 UTC
So, we aren't planning on fixing the 3.18.x fresh install, upgrade to 3.22.x, 16.11 released but only upgrade to 16.05.x? We will wait until they hit 16.11 to fix those cases? Yes, they will get fixed when people pass through 16.11, which is why I still signed off.
Comment 4 Mark Tompsett 2016-11-24 19:14:35 UTC
Created attachment 57760 [details] [review]
Bug 17676: Update default COLLATE of marc_subfield_structure

Patch for branches < 16.11
See bug 17676 for more information

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 5 Mark Tompsett 2016-11-24 19:15:30 UTC
Created attachment 57761 [details] [review]
Bug 17676: Update default COLLATE of marc_subfield_structure

(This issue has been raised on bug 17216 comments 93-103)
On bug 11944 (3.19.00.006) we updated the default COLLATE for all our tables to
utf8_unicode_ci but not the marc_subfield_structure table.
Indeed we want to keep tagsubfield a utf8_bin (to allow lowercase and uppercase
of the same letter for subfields).
We should have set the default collate to utf8_unicode_ci for further changes.

This patch updates the DB entry 3.19.00.006 to set the default COLLATE to this
table (for people upgrading from prior to 3.19.00.006) and set this
default COLLATE on 16.06.00.033 (for people upgrading from after 3.19.00.006).

The error is:
DBD::mysql::db do failed: Can't create table `koha_kohadev`.`#sql-306_9f9` (errno: 150 "Foreign key constraint is incorrectly formed") [for Statement "
            ALTER TABLE marc_subfield_structure
            MODIFY COLUMN authorised_value VARCHAR(32) DEFAULT NULL,
            ADD CONSTRAINT marc_subfield_structure_ibfk_1 FOREIGN KEY (authorised_value) REFERENCES authorised_value_categories (category_name) ON UPDATE CASCADE ON DELETE SET NULL;
            "] at installer/data/mysql/updatedatabase.pl line 13175.
Upgrade to 16.06.00.033 done (Bug 17216 - Add a new table to store authorized value categories)

Test plan:
1/ git checkout v3.18.00
2/ Do an install
3/ git checkout master;
4/ perl installer/data/mysql/updatedatabase.pl
=> Without this patch, you get the error
=> With this patch applied you will not get it and the default COLLATE for
marc_subfield_structure will be correctly set. Make sure tagsubfield is
still utf8_bin

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 6 Mark Tompsett 2016-11-24 19:16:13 UTC
Created attachment 57762 [details] [review]
Bug 17676: [Pre-16.11 patch] Update default COLLATE of marc_subfield_structure

Patch for branches < 16.11
See bug 17676 for more information

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 7 Mark Tompsett 2016-11-24 19:16:56 UTC
See comment #3.
Comment 8 Jonathan Druart 2016-11-25 07:50:57 UTC
(In reply to M. Tompsett from comment #3)
> So, we aren't planning on fixing the 3.18.x fresh install, upgrade to
> 3.22.x, 16.11 released but only upgrade to 16.05.x? We will wait until they
> hit 16.11 to fix those cases? Yes, they will get fixed when people pass
> through 16.11, which is why I still signed off.

3.18 is no longer maintained and does not have the problem (problem appears in 3.19.00.006).
The upgrade was wrong so it would make sense to fix it. But this table has not been modified since bug 17216, so we should not encounter any problems for pre-16.11 versions.
But yes, ideally the modification of the default collate could be done on all DB.
Comment 9 Marcel de Rooy 2016-11-25 09:32:43 UTC
QA: Looking at this one now.
Comment 10 Marcel de Rooy 2016-11-25 10:56:08 UTC
Created attachment 57769 [details] [review]
Bug 17676: Update default COLLATE of marc_subfield_structure

(This issue has been raised on bug 17216 comments 93-103)
On bug 11944 (3.19.00.006) we updated the default COLLATE for all our tables to
utf8_unicode_ci but not the marc_subfield_structure table.
Indeed we want to keep tagsubfield a utf8_bin (to allow lowercase and uppercase
of the same letter for subfields).
We should have set the default collate to utf8_unicode_ci for further changes.

This patch updates the DB entry 3.19.00.006 to set the default COLLATE to this
table (for people upgrading from prior to 3.19.00.006) and set this
default COLLATE on 16.06.00.033 (for people upgrading from after 3.19.00.006).

The error is:
DBD::mysql::db do failed: Can't create table `koha_kohadev`.`#sql-306_9f9` (errno: 150 "Foreign key constraint is incorrectly formed") [for Statement "
            ALTER TABLE marc_subfield_structure
            MODIFY COLUMN authorised_value VARCHAR(32) DEFAULT NULL,
            ADD CONSTRAINT marc_subfield_structure_ibfk_1 FOREIGN KEY (authorised_value) REFERENCES authorised_value_categories (category_name) ON UPDATE CASCADE ON DELETE SET NULL;
            "] at installer/data/mysql/updatedatabase.pl line 13175.
Upgrade to 16.06.00.033 done (Bug 17216 - Add a new table to store authorized value categories)

Test plan:
1/ git checkout v3.18.00
2/ Do an install
3/ git checkout master;
4/ perl installer/data/mysql/updatedatabase.pl
=> Without this patch, you get the error
=> With this patch applied you will not get it and the default COLLATE for
marc_subfield_structure will be correctly set. Make sure tagsubfield is
still utf8_bin

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Will put some notes on the Bugzilla report.
Comment 11 Marcel de Rooy 2016-11-25 11:14:17 UTC
QA Comment:
Passed QA. Seems to fix the issue mentioned.
Still some concerns.

> Indeed we want to keep tagsubfield a utf8_bin (to allow lowercase and uppercase of the same letter for subfields).
Could you provide an example where we want to do that? Is that MARC conform?
And if so(!), why would you need utf8_bin to do so? You can still insert them, only finding the right a or A would be harder.
It seems to me that we should remove this strange exception, and make sure that all tagsubfields are saved lowercase. Should not be too hard.
Can MARC::Record handle subfields a and A btw?
But this discussion should be on a new report.

> if ( $table[1] !~ /COLLATE=utf8_unicode_ci/ and $table[1] !~ /COLLATE=utf8mb4_unicode_ci/ ) { #catches utf8mb4 collated tables
This is only 99,9% safe (as you are probably aware of).
If you find one column in unicode_ci, the table might still be something else (theoretically). Perhaps someone added a custom column with collation?
Since you only change the default here, why not always do it? Replacing X with X will not be a problem..
Comment 12 Katrin Fischer 2016-11-26 13:50:43 UTC
(In reply to Marcel de Rooy from comment #11)
> QA Comment:
> Passed QA. Seems to fix the issue mentioned.
> Still some concerns.
> 
> > Indeed we want to keep tagsubfield a utf8_bin (to allow lowercase and uppercase of the same letter for subfields).
> Could you provide an example where we want to do that? Is that MARC conform?
> And if so(!), why would you need utf8_bin to do so? You can still insert
> them, only finding the right a or A would be harder.
> It seems to me that we should remove this strange exception, and make sure
> that all tagsubfields are saved lowercase. Should not be too hard.
> Can MARC::Record handle subfields a and A btw?
> But this discussion should be on a new report.
> 
> > if ( $table[1] !~ /COLLATE=utf8_unicode_ci/ and $table[1] !~ /COLLATE=utf8mb4_unicode_ci/ ) { #catches utf8mb4 collated tables
> This is only 99,9% safe (as you are probably aware of).
> If you find one column in unicode_ci, the table might still be something
> else (theoretically). Perhaps someone added a custom column with collation?
> Since you only change the default here, why not always do it? Replacing X
> with X will not be a problem..

It's not strictly MARC conform, but we use this a lot and I have talked to others using it as well. Take the 952 field as the example - all subfield codes are taken. Using upper case letters works fantastically now and allows you to store and index data that we got no other sensible spot for. We got it all working perfectly, why break this feature without need?
Comment 13 Katrin Fischer 2016-11-26 13:54:53 UTC
Ah, and I think I have encountered upper case in German MARC - just can't find a documentation right now.
Comment 14 Mirko Tietgen 2016-11-28 11:26:27 UTC
I built 16.11 + the last patch on 17216 + this and I get

Upgrade to 3.23.00.004 done (Bug 9819 - stopwords related code should be removed)
Upgrade to 3.23.00.005 done (Bug 11569 - Typo in userpermissions.sql)
Can't call method "numberpattern" on unblessed reference at /usr/share/koha/intranet/cgi-bin/installer/data/mysql/updatedatabase.pl line 11483.
dpkg: Fehler beim Bearbeiten des Paketes koha-common (--configure):
 Unterprozess installiertes post-installation-Skript gab den Fehlerwert 255 zurück
Comment 15 Mirko Tietgen 2016-11-28 11:32:40 UTC
Jonathan:

1. should I use the last patch on 17216 or not?
2. I used the second patch here because it is for 16.11, right?
Comment 16 Jonathan Druart 2016-11-28 11:48:46 UTC
(In reply to Mirko Tietgen from comment #14)
> I built 16.11 + the last patch on 17216 + this and I get
> 
> Upgrade to 3.23.00.004 done (Bug 9819 - stopwords related code should be
> removed)
> Upgrade to 3.23.00.005 done (Bug 11569 - Typo in userpermissions.sql)
> Can't call method "numberpattern" on unblessed reference at
> /usr/share/koha/intranet/cgi-bin/installer/data/mysql/updatedatabase.pl line
> 11483.
> dpkg: Fehler beim Bearbeiten des Paketes koha-common (--configure):
>  Unterprozess installiertes post-installation-Skript gab den Fehlerwert 255
> zurück

This comes from somewhere else:
  commit f11f07a703eca7da2d32c105f8c9f4f46f940b6f
    Bug 17292 - Use of DBIx in updatedatabase.pl broke upgrade  


(In reply to Mirko Tietgen from comment #15)
> Jonathan:
> 
> 1. should I use the last patch on 17216 or not?

Yes

> 2. I used the second patch here because it is for 16.11, right?

Yes
Comment 17 Mirko Tietgen 2016-11-28 12:38:01 UTC
I confirm everything works now that I alsp got 17292. Thanks!
Comment 18 Kyle M Hall 2016-11-29 17:42:06 UTC
Pushed to master! Thanks Jonathan!
Comment 19 Katrin Fischer 2016-11-29 22:41:46 UTC
This patch has been pushed to 16.11.x, will be in 16.11.01!