Created attachment 55113 [details] [review]
Bug 17234 - updatedatabase.pl's ALTER IGNORE break with mysql 5.7.4+

The doc says: "As of MySQL 5.7.4, the IGNORE clause for ALTER TABLE is removed and its use produces an error."
This fix replaces ALTER IGNORE with ALTER in updatedatabase.pl

To TEST, try an upgrade from 3.18 to 3.22 after installing the latest mysql (at least 5.7.4, by the doc).
Some will fail with error

DBD::mysql::db do failed: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'IGNORE TABLE aqbasket
            ADD KEY authorisedby (authorisedby)' at line 1 [for Statement "
        ALTER IGNORE TABLE aqbasket
            ADD KEY authorisedby (authorisedby)
    "] at ./installer/data/mysql/updatedatabase.pl line 10563.

0) Find a database on 3.18, save it.
1) Set your code base to 3.22(or master)
2) run updatedatabase.pl
3) See the errors.
4) Apply the patch
5) Reload the 3.18 database
6) succeed with updatedatabase.pl

Comment on attachment 55113 [details] [review]
Bug 17234 - updatedatabase.pl's ALTER IGNORE break with mysql 5.7.4+

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

I'll test this, and believe it will pass just fine.

Comment on attachment 55144 [details] [review]
Bug 17234 - updatedatabase.pl fails on invalid entry in ENUM column

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

::: installer/data/mysql/updatedatabase.pl
@@ +12280,4 @@
>               `id` int(11) NOT NULL AUTO_INCREMENT, 
>               `name` varchar(255) NOT NULL COMMENT 'the name of the field as it will be stored in the search engine',
>               `label` varchar(255) NOT NULL COMMENT 'the human readable name of the field, for display', 
> +             `type` ENUM('', 'string', 'date', 'number', 'boolean', 'sum') NOT NULL COMMENT 'what type of data this holds, relevant when storing it in the search engine',

I haven't looked yet, but this seems like a bad solution. Invalid entries in the ENUM column are likely caused by applying patches that didn't get into master. I may get around to looking further, but this patch portion doesn't seem right to me.

Is this the right solution? What happens if the DB structure is already changed?

(In reply to Tomás Cohen Arazi from comment #5)
> Is this the right solution? What happens if the DB structure is already
> changed?

It also doesn't make sense, because just a few lines above: DROP IF EXISTS.

Could someone explain me why I don't get the same error:

mysql> set sql_mode="STRICT_TRANS_TABLES";
Query OK, 0 rows affected (0.00 sec)

mysql> ALTER IGNORE TABLE suggestions ADD KEY biblionumber (biblionumber);
ERROR 1061 (42000): Duplicate key name 'biblionumber'

And so I don't understand the IGNORE keyword :)

Server version: 5.6.30-1 (Debian) # And not 5.7

(In reply to Jonathan Druart from comment #7)
> Could someone explain me why I don't get the same error:
> 
> mysql> set sql_mode="STRICT_TRANS_TABLES";
> Query OK, 0 rows affected (0.00 sec)
> 
> mysql> ALTER IGNORE TABLE suggestions ADD KEY biblionumber (biblionumber);
> ERROR 1061 (42000): Duplicate key name 'biblionumber'
> 
> And so I don't understand the IGNORE keyword :)
> 
> Server version: 5.6.30-1 (Debian) # And not 5.7

"As of MySQL 5.7.4, the IGNORE clause for ALTER TABLE is removed and its use produces an error."
(https://dev.mysql.com/doc/refman/5.7/en/alter-table.html)
I'd leave out the ignore.

This affects Ubuntu 16.04 LTS currently, and will affect Debian in the future. This is a pre-emptive patch.

(In reply to M. Tompsett from comment #4)
> > +             `type` ENUM('', 'string', 'date', 'number', 'boolean', 'sum') NOT NULL COMMENT 'what type of data this holds, relevant when storing it in the search engine',
> 
> I haven't looked yet, but this seems like a bad solution. Invalid entries in
> the ENUM column are likely caused by applying patches that didn't get into
> master. I may get around to looking further, but this patch portion doesn't
> seem right to me.

Maybe I shouldn't have mixed the two patches.  But both issues prevent me from completing updatedatabase.pl with mysql 5.7, related to its stringent requirements.

That said, the entries in admin/searchengine/elasticsearch/mappings.yaml specify '' as type.  So this to fix updatedatabase, there are two options:
1) correct a few (hundreds?) entries in mappings.yaml, knowing what you're doing with elastic search.
2) allow for empty types.


NOTE: there are more blockers.  More small patches coming.

`type` ENUM('', 'string', 'date', 'number', 'boolean', 'sum') NOT NULL COMMENT 'what type of data this holds, relevant when storing it in the search engine',

(In reply to Blou from comment #9)
> That said, the entries in admin/searchengine/elasticsearch/mappings.yaml
> specify '' as type.

$ grep "\stype:" admin/searchengine/elasticsearch/mappings.yaml | sort -u
Confirms what you have said.

But I can't help but wonder if perhaps 58 tweaks to the mappings.yaml file is more correct.

Perhaps you may wish to split this off to its own bug, obsolete it here, and then the ALTER IGNORE can more forward?

Comment on attachment 55144 [details] [review]
Bug 17234 - updatedatabase.pl fails on invalid entry in ENUM column

I'll remove the second patch.  I'll submit it in a new bug, along with the other similar fixes.  Along the way, I'll comment how it would be maybe preferable to fix the .yml rather than allowing ''.

At least the first patch will move ahead.

Blou: https://wiki.koha-community.org/wiki/Developers_Handbook#Database

On a positive note, this runs on Debian 8, mysql 5.5.
I need to set up Ubuntu and test on that.

(In reply to M. Tompsett from comment #13)
> On a positive note, this runs on Debian 8, mysql 5.5.
> I need to set up Ubuntu and test on that.

Almost. Just have issues with C4::Installer::load_sql to resolve. Particularly actually successfully installing 3.18.x

Created attachment 55634 [details] [review]
Bug 17234: [DO NOT PUSH] Make 3.18.x install at least.

This patch hobbles past installing a 3.18.x on Ubuntu 16.04 LTS.
2 tweaks to kohastructure and ignoring the security warning about
the command line password makes it succeed enough for a DB structure
to attempt to upgrade to master.

TEST PLAN
---------
1) git checkout -b my_3.18.x origin/3.18.x
2) drop database koha_library; create database koha_library;
3) apply this hacky patch
4) web install
   -- libraries may need to be sudo apt-get install'd
      in order to get it to run.
5) hobble through
6) ignore the two errors on installed just the mandatory things.

7) git checkout -b bug_17234 origin/master
8) apply the patch to test
9) web install
10) ignore all the date barfing, because that is not what was
    fixed.
11) run koha qa test tools on this.

Created attachment 55635 [details] [review]
Bug 17234 - updatedatabase.pl's ALTER IGNORE break with mysql 5.7.4+

The doc says: "As of MySQL 5.7.4, the IGNORE clause for ALTER TABLE is removed and its use produces an error."
This fix replaces ALTER IGNORE with ALTER in updatedatabase.pl

To TEST, try an upgrade from 3.18 to 3.22 after installing the latest mysql (at least 5.7.4, by the doc).
Some will fail with error

DBD::mysql::db do failed: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'IGNORE TABLE aqbasket
            ADD KEY authorisedby (authorisedby)' at line 1 [for Statement "
        ALTER IGNORE TABLE aqbasket
            ADD KEY authorisedby (authorisedby)
    "] at ./installer/data/mysql/updatedatabase.pl line 10563.

0) Find a database on 3.18, save it.
1) Set your code base to 3.22(or master)
2) run updatedatabase.pl
3) See the errors.
4) Apply the patch
5) Reload the 3.18 database
6) succeed with updatedatabase.pl

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

$ git grep "ALTER IGNORE"
DOH! Bug 16276 pushed 6 days ago.
Please patch for that.

Created attachment 55704 [details] [review]
Bug 17234: Follow up to handle new problems

Bug 16276 added two more ALTER IGNORES. This removes them.

FTR: I still don't think it is a good measure to just remove the IGNORE keywords. We need to deal with those errors in a better way for end users.

Right now Koha will show really scary messages about wrong DB structure when updatedatabase.pl is ran. I think it is far better to just add a warning about 5.7 with a pointer to 'how to disable mysql 5.7 strictness' than scaring end users.

(In reply to Tomás Cohen Arazi from comment #19)
> FTR: I still don't think it is a good measure to just remove the IGNORE
> keywords. We need to deal with those errors in a better way for end users.
> 
> Right now Koha will show really scary messages about wrong DB structure when
> updatedatabase.pl is ran. I think it is far better to just add a warning
> about 5.7 with a pointer to 'how to disable mysql 5.7 strictness' than
> scaring end users.

I completely agree with that.
We need to replace these occurrences with a call to a new subroutine which will check if the key already exists or not.

Something like:

unless ( constraint_exist($table, $constraint_name ) ) {
    # Execute the alter table
}

Right now, updatedatabase.pl does not work for mysql5.7.
And IGNORE will never work for mysql5.7+
How long do you intend to postpone this change?

Removing strictness seems way more dangerous than accepting that certain transactions might fail (which they don't right now from my experience).

And a warning won't help a user whose db just got wrecked by a failed upgrade.

(In reply to Blou from comment #21)
> Right now, updatedatabase.pl does not work for mysql5.7.
> And IGNORE will never work for mysql5.7+
> How long do you intend to postpone this change?
> 
> Removing strictness seems way more dangerous than accepting that certain
> transactions might fail (which they don't right now from my experience).
> 
> And a warning won't help a user whose db just got wrecked by a failed
> upgrade.

Koha is not usable with strict mode enabled anyway...

(In reply to Jonathan Druart from comment #20)
> unless ( constraint_exist($table, $constraint_name ) ) {
>     # Execute the alter table
> }

I will write this patch.

Created attachment 55722 [details] [review]
Bug 17234: Add constraint_exists and column_exists to updatedatabase.pl

These 2 subroutines will help us deal with the absense of ALTER IGNORE
TABLE

(In reply to Blou from comment #21)
> Right now, updatedatabase.pl does not work for mysql5.7.
> And IGNORE will never work for mysql5.7+
> How long do you intend to postpone this change?

Why would we want to postpone a bugfix?

> Removing strictness seems way more dangerous than accepting that certain
> transactions might fail (which they don't right now from my experience).

Why? If you enable it, Koha won't work.

> And a warning won't help a user whose db just got wrecked by a failed
> upgrade.

I don't think it's pleasant to get lot of warnings when you upgrade your install.
It's our job to deal with these warnings.
I think the patch I have just submitted will help us to deal with existing columns/constraints.

NOTE that I have *not* tested this patch

Oooo.... JD, I'm looking forward to testing this later.

Created attachment 55729 [details] [review]
Bug 17234: Add constraint_exists and column_exists to updatedatabase.pl

These 2 subroutines will help us deal with the absense of ALTER IGNORE
TABLE

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

I followed test plan on bug 17324 to test this.

Just wondering, I don't think the subs where they are are usable by say, perl atomicupdates. (or, please tell how to use them because even though I should know or it's obvious, I don't)

Also it's pushing one more thing onto the RM, if you don't allow people to use the perl atomicupdates to do their dev and such.

The solution is lovely, otherwise.

(In reply to Liz Rea from comment #29)
> Just wondering, I don't think the subs where they are are usable by say,
> perl atomicupdates. (or, please tell how to use them because even though I
> should know or it's obvious, I don't)

Good point, Liz. Perhaps those to functions are better suited to Koha::Database?

Created attachment 55833 [details] [review]
Bug 17234: Move new subroutines to C4::Installer

I have added a skeleton.perl on bug 17356.

So, signoff?

(In reply to Jonathan Druart from comment #33)
> So, signoff?

Item number three on my TO DO list. Sorry. It does eyeball well.

(In reply to Blou from comment #21)
> How long do you intend to postpone this change?

Regarding the delay to get a signoff, it seems that it was not very important anyway...

(In reply to Jonathan Druart from comment #35)
> (In reply to Blou from comment #21)
> > How long do you intend to postpone this change?
> 
> Regarding the delay to get a signoff, it seems that it was not very
> important anyway...

Thanks for the comment. I lost track of it.

Created attachment 56820 [details] [review]
Bug 17234 [DO NOT PUSH] Make 3.14.x "install"

Tweak fields, so primary key is workable.
Ignore command line mysql error security warning

Created attachment 56821 [details] [review]
Bug 17234 [DO NOT PUSH] Make v3.00.00 "install"

Tweak fields, so primary key is workable.
Ignore command line mysql error security warning

This is beautiful!

TEST PLAN
---------
On an Ubuntu 16.04 LTS git install:
git checkout -b bug_17234 origin/3.18.x
git bz apply 17234
i
-- apply the DO NOT PUSH (yes it says 3.14.x, but it works for 3.18.x)
-- in a MySQL client drop and recreate your koha_library DB
-- run a web install and select all data (just 3 issues for Marc21, but not really the point of this patch)
git reset --hard origin/master
./installer/data/mysql/updatedatabase.pl
-- there will be ALTER TABLE messages in the upgrade output.
git reset --hard origin/3.18.x
git bz apply 17234
i
-- apply the DO NOT PUSH
-- in a MySQL client drop and recreate your koha_library DB
-- run a web install and select all data (just 3 issues for Marc21, but not really the point of this patch)
git reset --hard origin/master
git bz apply 17234
i
apply the other patches
./installer/data/mysql/updatedatabase.pl
-- there will be no ALTER TABLE messages in the upgrade output.
In fact, just the 0000-00-00 messages (see bug 17320).

(In reply to M. Tompsett from comment #39)
> In fact, just the 0000-00-00 messages (see bug 17320).

Oops and one other unrelated.
Upgrade to 3.23.00.049 done (Bug 12267 - Allow password option in Patron Attribute non functional)
DBD::mysql::st execute failed: Data truncated for column 'type' at row 1 [for Statement "INSERT INTO `search_field` ( `label`, `name`, `type`) VALUES ( ?, ?, ? )" with ParamValues: 0='pl', 1='pl', 2=''] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1834.
DBIx::Class::Storage::DBI::_dbh_execute(): Data truncated for column 'type' at row 1 at /home/mtompset/kohaclone/Koha/Objects.pm line 101

Created attachment 56822 [details] [review]
Bug 17234 - updatedatabase.pl's ALTER IGNORE break with mysql 5.7.4+

The doc says: "As of MySQL 5.7.4, the IGNORE clause for ALTER TABLE is removed and its use produces an error."
This fix replaces ALTER IGNORE with ALTER in updatedatabase.pl

To TEST, try an upgrade from 3.18 to 3.22 after installing the latest mysql (at least 5.7.4, by the doc).
Some will fail with error

DBD::mysql::db do failed: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'IGNORE TABLE aqbasket
            ADD KEY authorisedby (authorisedby)' at line 1 [for Statement "
        ALTER IGNORE TABLE aqbasket
            ADD KEY authorisedby (authorisedby)
    "] at ./installer/data/mysql/updatedatabase.pl line 10563.

0) Find a database on 3.18, save it.
1) Set your code base to 3.22(or master)
2) run updatedatabase.pl
3) See the errors.
4) Apply the patch
5) Reload the 3.18 database
6) succeed with updatedatabase.pl

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

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

Created attachment 56823 [details] [review]
Bug 17234: Follow up to handle new problems

Bug 16276 added two more ALTER IGNORES. This removes them.

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

Created attachment 56824 [details] [review]
Bug 17234: Add constraint_exists and column_exists to updatedatabase.pl

These 2 subroutines will help us deal with the absense of ALTER IGNORE
TABLE

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

Created attachment 56825 [details] [review]
Bug 17234: Move new subroutines to C4::Installer

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

I followed my test plan in comment #39. I've not set it to signed off, because perhaps another sign off might be a good idea, as some of the code is mine. I approve of Jonathan Druart's constraint and column code wholeheartedly!

Okay, maybe I'm too cautious, and that will leave this forever stuck. Signed off it is.

@QA: We need the new constraint_exists and column_exists subroutine ASAP in the master branch. That way we will be able to use them.
Otherwise I will split these patches and move the 2 subroutines to another bug report to make it move forward quickly.

Could we get some basic unit tests for the new subroutines?

Created attachment 58548 [details] [review]
Bug 17234: Two new functions lack tests

This adds two tests to t/db_dependent/Installer.t

TEST PLAN
---------
1) Apply patch
2) prove -v t/db_dependent/Installer.t
   -- column and constraint tests were added.
3) run koha qa test tools

As per Kyle Hall's request in comment #48, added missing tests. Set to needs sign off. Though really, everything should be good. :)

Created attachment 58551 [details] [review]
Bug 17234: Two new functions lack tests

This adds two tests to t/db_dependent/Installer.t

TEST PLAN
---------
1) Apply patch
2) prove -v t/db_dependent/Installer.t
   -- column and constraint tests were added.
3) run koha qa test tools

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 58552 [details] [review]
Bug 17234: Test the column and constraint non-existence

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Skipping SO for the 2 last patches.

Pushed to master for 17.05, thanks Philippe, Mark, and Jonathan!

There is something wrong here, I will open a new bug report soon.

Created attachment 58865 [details] [review]
Bug 17234: Need to separate KEY and FOREIGN KEY checks

In the previous patch we use the constraint_exists subroutine to verify
if an index or a foreign key exists.
But the `SHOW INDEX` query does not return foreign keys (as its name
suggests!).
We need another subroutine foreign_key_exists to check the FK existence.

I have found that because t/db_dependent/TestBuilder.t fails on
oai_sets_biblios, because oai_sets_biblios_ibfk_1 has not been removed.

Test plan:
0/ Do not apply this patch
1/ Use a 3.20 DB
2/ update the DB
3/ SHOW CREATE TABLE oai_sets_biblios
will display oai_sets_biblios_ibfk_1

Apply the patch and repeat 1, 2, 3
=> Will not display oai_sets_biblios_ibfk_1
It has been removed as expected.

(In reply to Jonathan Druart from comment #55)
> There is something wrong here, I will open a new bug report soon.

I finally did not, we really cannot wait 3 months to get this followup in...

Followup "Bug 17234: Need to separate KEY and FOREIGN KEY checks" has been pushed to master!

These patches have been pushed to 16.11.x and will be in 16.11.02.
.

Created attachment 59003 [details] [review]
[16.05.x] Bug 17234: Add foreign_key_exists index_exists column_exists to C4::Installer

This patch is an adapted squashed patch of the following commits for the 16.05.x (and lower) branches:

(In reply to Jonathan Druart from comment #60)
> Created attachment 59003 [details] [review] [review]
> [16.05.x] Bug 17234: Add foreign_key_exists index_exists column_exists to
> C4::Installer
> 
> This patch is an adapted squashed patch of the following commits for the
> 16.05.x (and lower) branches:

Just in case, do not care about it for now.

Pushed to 3.22.x for 3.22.15

Pushed to 16.05.x, for 16.05.08 release

*** Bug 22159 has been marked as a duplicate of this bug. ***