Bug 17234 - ALTER IGNORE TABLE is invalid in mysql 5.7. This breaks updatedatabase.pl
Summary: ALTER IGNORE TABLE is invalid in mysql 5.7. This breaks updatedatabase.pl
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Installation and upgrade (command-line installer) (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Blou
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 17258 17356 17762 17914 14187 17813
  Show dependency treegraph
 
Reported: 2016-09-01 20:47 UTC by Blou
Modified: 2017-01-27 02:49 UTC (History)
13 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 17234 - updatedatabase.pl's ALTER IGNORE break with mysql 5.7.4+ (4.39 KB, patch)
2016-09-02 12:22 UTC, Blou
Details | Diff | Splinter Review
Bug 17234 - updatedatabase.pl fails on invalid entry in ENUM column (1.34 KB, patch)
2016-09-02 19:58 UTC, Blou
Details | Diff | Splinter Review
Bug 17234: [DO NOT PUSH] Make 3.18.x install at least. (2.83 KB, patch)
2016-09-18 02:23 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 17234 - updatedatabase.pl's ALTER IGNORE break with mysql 5.7.4+ (4.44 KB, patch)
2016-09-18 02:27 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 17234: Follow up to handle new problems (1.38 KB, patch)
2016-09-19 18:28 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 17234: Add constraint_exists and column_exists to updatedatabase.pl (10.67 KB, patch)
2016-09-20 15:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17234: Add constraint_exists and column_exists to updatedatabase.pl (10.72 KB, patch)
2016-09-20 23:53 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 17234: Move new subroutines to C4::Installer (2.26 KB, patch)
2016-09-27 07:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17234 [DO NOT PUSH] Make 3.14.x "install" (2.26 KB, patch)
2016-10-25 01:09 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 17234 [DO NOT PUSH] Make v3.00.00 "install" (1.75 KB, patch)
2016-10-25 01:17 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 17234 - updatedatabase.pl's ALTER IGNORE break with mysql 5.7.4+ (4.49 KB, patch)
2016-10-25 01:35 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 17234: Follow up to handle new problems (1.43 KB, patch)
2016-10-25 01:36 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 17234: Add constraint_exists and column_exists to updatedatabase.pl (10.77 KB, patch)
2016-10-25 01:36 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 17234: Move new subroutines to C4::Installer (2.31 KB, patch)
2016-10-25 01:36 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 17234: Two new functions lack tests (3.68 KB, patch)
2016-12-30 19:10 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 17234: Two new functions lack tests (3.75 KB, patch)
2017-01-02 10:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17234: Test the column and constraint non-existence (1.37 KB, patch)
2017-01-02 10:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17234: Need to separate KEY and FOREIGN KEY checks (8.09 KB, patch)
2017-01-12 09:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
[16.05.x] Bug 17234: Add foreign_key_exists index_exists column_exists to C4::Installer (6.58 KB, patch)
2017-01-16 10:45 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Blou 2016-09-01 20:47:33 UTC
Since mysql 5.7, IGNORE is invalid in ALTER statements.

This breaks updatedatabase.pl with errors like 

BD::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 suggestions ADD KEY status (STATUS)' at line 1 [for Statement "
        ALTER IGNORE TABLE suggestions ADD KEY status (STATUS)
    "] at ./installer/data/mysql/updatedatabase.pl line 10420.
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 suggestions ADD KEY biblionumber (biblionumber)' at line 1 [for Statement "

The patch will simply remove the IGNORE statement 11 times in updatedatabase.pl
Comment 1 Blou 2016-09-02 12:22:30 UTC
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 2 Blou 2016-09-02 19:58:01 UTC Comment hidden (obsolete)
Comment 3 M. Tompsett 2016-09-05 00:07:03 UTC
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 4 M. Tompsett 2016-09-05 00:09:24 UTC
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.
Comment 5 Tomás Cohen Arazi 2016-09-05 14:22:04 UTC
Is this the right solution? What happens if the DB structure is already changed?
Comment 6 M. Tompsett 2016-09-05 17:32:45 UTC
(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.
Comment 7 Jonathan Druart 2016-09-06 08:44:36 UTC
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
Comment 8 M. Tompsett 2016-09-06 12:15:42 UTC
(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.
Comment 9 Blou 2016-09-06 12:26:41 UTC
(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.
Comment 10 M. Tompsett 2016-09-06 13:19:39 UTC
`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 11 Blou 2016-09-06 13:54:19 UTC
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.
Comment 13 M. Tompsett 2016-09-17 19:07:01 UTC
On a positive note, this runs on Debian 8, mysql 5.5.
I need to set up Ubuntu and test on that.
Comment 14 M. Tompsett 2016-09-17 22:30:24 UTC
(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
Comment 15 M. Tompsett 2016-09-18 02:23:30 UTC
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.
Comment 16 M. Tompsett 2016-09-18 02:27:37 UTC
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>
Comment 17 M. Tompsett 2016-09-19 18:25:44 UTC
$ git grep "ALTER IGNORE"
DOH! Bug 16276 pushed 6 days ago.
Please patch for that.
Comment 18 M. Tompsett 2016-09-19 18:28:19 UTC
Created attachment 55704 [details] [review]
Bug 17234: Follow up to handle new problems

Bug 16276 added two more ALTER IGNORES. This removes them.
Comment 19 Tomás Cohen Arazi 2016-09-20 13:51:53 UTC
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.
Comment 20 Jonathan Druart 2016-09-20 13:59:42 UTC
(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
}
Comment 21 Blou 2016-09-20 14:06:11 UTC
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.
Comment 22 Jonathan Druart 2016-09-20 15:11:02 UTC
(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...
Comment 23 Jonathan Druart 2016-09-20 15:17:38 UTC
(In reply to Jonathan Druart from comment #20)
> unless ( constraint_exist($table, $constraint_name ) ) {
>     # Execute the alter table
> }

I will write this patch.
Comment 24 Jonathan Druart 2016-09-20 15:57:46 UTC
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
Comment 25 Jonathan Druart 2016-09-20 16:01:12 UTC
(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
Comment 26 M. Tompsett 2016-09-20 18:27:58 UTC
Oooo.... JD, I'm looking forward to testing this later.
Comment 27 M. Tompsett 2016-09-20 23:53:48 UTC
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>
Comment 28 M. Tompsett 2016-09-20 23:54:48 UTC
I followed test plan on bug 17324 to test this.
Comment 29 Liz Rea 2016-09-21 04:11:34 UTC
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.
Comment 30 M. Tompsett 2016-09-21 11:00:25 UTC
(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?
Comment 31 Jonathan Druart 2016-09-27 07:28:45 UTC
Created attachment 55833 [details] [review]
Bug 17234: Move new subroutines to C4::Installer
Comment 32 Jonathan Druart 2016-09-27 07:34:13 UTC
I have added a skeleton.perl on bug 17356.
Comment 33 Jonathan Druart 2016-09-30 07:53:17 UTC
So, signoff?
Comment 34 M. Tompsett 2016-09-30 11:53:32 UTC
(In reply to Jonathan Druart from comment #33)
> So, signoff?

Item number three on my TO DO list. Sorry. It does eyeball well.
Comment 35 Jonathan Druart 2016-10-20 07:31:31 UTC
(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...
Comment 36 M. Tompsett 2016-10-20 10:53:58 UTC
(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.
Comment 37 M. Tompsett 2016-10-25 01:09:14 UTC
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
Comment 38 M. Tompsett 2016-10-25 01:17:48 UTC
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
Comment 39 M. Tompsett 2016-10-25 01:30:18 UTC
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).
Comment 40 M. Tompsett 2016-10-25 01:34:07 UTC
(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
Comment 41 M. Tompsett 2016-10-25 01:35:51 UTC
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>
Comment 42 M. Tompsett 2016-10-25 01:36:01 UTC
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>
Comment 43 M. Tompsett 2016-10-25 01:36:13 UTC
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>
Comment 44 M. Tompsett 2016-10-25 01:36:24 UTC
Created attachment 56825 [details] [review]
Bug 17234: Move new subroutines to C4::Installer

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 45 M. Tompsett 2016-10-25 01:39:39 UTC
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!
Comment 46 M. Tompsett 2016-11-24 19:44:32 UTC
Okay, maybe I'm too cautious, and that will leave this forever stuck. Signed off it is.
Comment 47 Jonathan Druart 2016-12-27 15:35:05 UTC
@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.
Comment 48 Kyle M Hall 2016-12-30 17:52:36 UTC
Could we get some basic unit tests for the new subroutines?
Comment 49 M. Tompsett 2016-12-30 19:10:39 UTC
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
Comment 50 M. Tompsett 2016-12-30 19:12:15 UTC
As per Kyle Hall's request in comment #48, added missing tests. Set to needs sign off. Though really, everything should be good. :)
Comment 51 Jonathan Druart 2017-01-02 10:38:10 UTC
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>
Comment 52 Jonathan Druart 2017-01-02 10:38:19 UTC
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>
Comment 53 Jonathan Druart 2017-01-02 10:39:11 UTC
Skipping SO for the 2 last patches.
Comment 54 Kyle M Hall 2017-01-11 10:53:42 UTC
Pushed to master for 17.05, thanks Philippe, Mark, and Jonathan!
Comment 55 Jonathan Druart 2017-01-12 09:40:47 UTC
There is something wrong here, I will open a new bug report soon.
Comment 56 Jonathan Druart 2017-01-12 09:51:30 UTC
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.
Comment 57 Jonathan Druart 2017-01-12 09:52:25 UTC
(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...
Comment 58 Kyle M Hall 2017-01-12 12:43:20 UTC
Followup "Bug 17234: Need to separate KEY and FOREIGN KEY checks" has been pushed to master!
Comment 59 Katrin Fischer 2017-01-15 16:03:44 UTC
These patches have been pushed to 16.11.x and will be in 16.11.02.
.
Comment 60 Jonathan Druart 2017-01-16 10:45:14 UTC
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:
Comment 61 Jonathan Druart 2017-01-16 10:45:38 UTC
(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.
Comment 62 Julian Maurice 2017-01-19 15:50:56 UTC
Pushed to 3.22.x for 3.22.15
Comment 63 Mason James 2017-01-27 02:49:24 UTC
Pushed to 16.05.x, for 16.05.08 release