Bug 13669 - Web installer fails to load sample data on MySQL 5.6+
Summary: Web installer fails to load sample data on MySQL 5.6+
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Installation and upgrade (web-based installer) (show other bugs)
Version: master
Hardware: All All
: P5 - low blocker (vote)
Target Milestone: Oldversions
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Whiteboard:
Keywords: dependency
Depends on:
Blocks: 17258
  Show dependency treegraph
 
Reported: 2015-02-04 23:05 UTC by Tomás Cohen Arazi
Modified: 2018-12-16 19:32 UTC (History)
12 users (show)

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


Attachments
Bug 13669: Use perl to load SQL statements from sample data (4.50 KB, patch)
2016-05-18 16:34 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13669: Use perl to load SQL statements from sample data (4.51 KB, patch)
2016-05-18 19:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13669: Use perl to load SQL statements from sample data (4.53 KB, patch)
2016-05-18 20:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13669: (followup) Fix kohastructure.sql so FK dependencies are sorted (110.35 KB, patch)
2016-05-18 20:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13669: Use perl to load SQL statements from sample data (4.59 KB, patch)
2016-05-18 21:47 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 13669: (followup) Fix kohastructure.sql so FK dependencies are sorted (110.40 KB, patch)
2016-05-18 21:47 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 13669: (alternative) use DBIx::RunSQL (3.22 KB, patch)
2016-05-19 00:38 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 13669: (alternative) use DBIx::RunSQL (3.27 KB, patch)
2016-05-19 01:29 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 13669: (followup) fix I18N files (7.15 KB, patch)
2016-05-19 04:26 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 13669: (followup) fix fr-FR files (1.35 MB, patch)
2016-05-19 04:26 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 13669: (followup) fix I18N files (7.26 KB, patch)
2016-05-19 04:45 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 13669: Re-adds error handling to load_sql (1.44 KB, patch)
2016-05-20 17:32 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 13669: Catch the errors to have them to the logs (2.15 KB, patch)
2016-05-21 08:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 13669: Catch the errors to have them to the logs (2.38 KB, patch)
2016-05-21 12:14 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 13669: Use perl to load SQL statements from sample data (4.66 KB, patch)
2016-05-21 12:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13669: (followup) Fix kohastructure.sql so FK dependencies are sorted (110.48 KB, patch)
2016-05-21 12:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13669: (alternative) use DBIx::RunSQL (3.34 KB, patch)
2016-05-21 12:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13669: Re-adds error handling to load_sql (1.52 KB, patch)
2016-05-21 12:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13669: Catch the errors to have them to the logs (2.43 KB, patch)
2016-05-21 12:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13669: followup to add DBIx::RunSQL dependency (894 bytes, patch)
2016-05-24 13:39 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 13669: followup to add DBIx::RunSQL dependency (1.05 KB, patch)
2016-05-24 14:19 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 13669: followup to add DBIx::RunSQL dependency (1.31 KB, patch)
2016-05-24 18:42 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2015-02-04 23:05:39 UTC
Due to a change in MySQL 5.6 [1] the web installer fails to load the kohastructure.sql and the chosen sample data files.

This is because C4::Installer::load_sql relies on a command line call to the mysql binary, which from now on doesn't support passing the user password as parameter on the command line, YAY!

[1] http://dev.mysql.com/doc/refman/5.6/en/upgrading-from-previous-series.html
Comment 1 Jonathan Druart 2015-07-15 07:59:24 UTC
Tomás, I have installed MySQL server 5.6 (Server version: 5.6.25 MySQL Community Server (GPL)) and it seems to work, could you confirm?

I have followed http://www.debiantutorials.com/install-mysql-server-5-6-debian-7-8/ and then install Koha, select all the sample data files and all is inserted :)
Comment 2 Jonathan Druart 2015-07-15 09:03:48 UTC
Whaou, crazy last hour.

So I decided to keep this version, just to test.
And... at least 2 things are broken (I stopped the experience quickly):
1/  prove t/db_dependent/Acquisition.t
t/db_dependent/Acquisition.t .. 1/87 DBD::mysql::st execute failed: Data too long for column 'budget_code' at row 1 at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1832.

Because aqbudgets.budget_code is varchar(30) and the test try to insert a 31 string chars: budget_code_test_getordersbybib

2/ On creating an order, Software error:

addorder.pl: DBIx::Class::Storage::DBI::_dbh_execute(): Field 'budgetgroup_id' doesn't have a default value at /home/koha/src/Koha/Acquisition/Order.pm line 47

And yes it does not have a default value.

Conclusion: We are not ready at all for MySQL 5.6
Comment 3 Jonathan Druart 2015-07-15 09:13:12 UTC
Ha, I should mention: It tooks me a while to reinstall mariadb-server:

% sudo apt-get install mariadb-server

The following extra packages will be installed:
  mariadb-server-10.0 mariadb-server-core-10.0
Suggested packages:
  mariadb-test tinyca
The following packages will be REMOVED:
  mysql-community-server
The following NEW packages will be installed:
  mariadb-server mariadb-server-10.0 mariadb-server-core-10.0

Preconfiguring packages ...
dpkg: mysql-community-server: dependency problems, but removing anyway as you requested:
 koha-deps depends on mysql-server | virtual-mysql-server; however:
  Package mysql-server is not installed.
  Package virtual-mysql-server is not installed.
  Package mysql-community-server which provides virtual-mysql-server is to be removed.
  Package mysql-server-5.5 which provides virtual-mysql-server is not installed.
  Package mariadb-server-10.0 which provides virtual-mysql-server is not installed.

(Reading database ... 66642 files and directories currently installed.)
Removing mysql-community-server (5.6.25-2debian8) ...
......
[info] MySQL Community Server 5.6.25 is stopped.
Processing triggers for man-db (2.6.7.1-1) ...
Selecting previously unselected package mariadb-server-10.0.
(Reading database ... 66539 files and directories currently installed.)
Preparing to unpack .../mariadb-server-10.0_10.0.19-1_i386.deb ...
/etc/init.d/mysql: line 38: mysqld: command not found
[info] MySQL Community Server  is already stopped.
Unpacking mariadb-server-10.0 (10.0.19-1) ...
dpkg: error processing archive /var/cache/apt/archives/mariadb-server-10.0_10.0.19-1_i386.deb (--unpack):
 trying to overwrite '/usr/share/mysql/errmsg-utf8.txt', which is also in package mysql-common 5.6.25-2debian8
dpkg-deb: error: subprocess paste was killed by signal (Broken pipe)
Selecting previously unselected package mariadb-server-core-10.0.
Preparing to unpack .../mariadb-server-core-10.0_10.0.19-1_i386.deb ...
Unpacking mariadb-server-core-10.0 (10.0.19-1) ...
dpkg: error processing archive /var/cache/apt/archives/mariadb-server-core-10.0_10.0.19-1_i386.deb (--unpack):
 trying to overwrite '/usr/share/mysql/hungarian/errmsg.sys', which is also in package mysql-common 5.6.25-2debian8
dpkg-deb: error: subprocess paste was killed by signal (Broken pipe)


% sudo apt-get remove mysql-server-5.6
Reading package lists... Done
Building dependency tree       
Reading state information... Done
Note, selecting 'mysql-community-server' instead of 'mysql-server-5.6'
You might want to run 'apt-get -f install' to correct these:
The following packages have unmet dependencies:
 koha-deps : Depends: mysql-server but it is not going to be installed or
                      virtual-mysql-server
 mariadb-server : Depends: mariadb-server-10.0 (>= 10.0.19-1) but it is not going to be installed
E: Unmet dependencies. Try 'apt-get -f install' with no packages (or specify a solution).

% sudo apt-get -f install
dpkg: error processing archive /var/cache/apt/archives/mariadb-server-core-10.0_10.0.19-1_i386.deb (--unpack):
 trying to overwrite '/usr/share/mysql/hungarian/errmsg.sys', which is also in package mysql-common 5.6.25-2debian8
dpkg-deb: error: subprocess paste was killed by signal (Broken pipe)
Preparing to unpack .../mariadb-server-10.0_10.0.19-1_i386.deb ...
/etc/init.d/mysql: line 38: mysqld: command not found
[info] MySQL Community Server  is already stopped.
Unpacking mariadb-server-10.0 (10.0.19-1) ...
dpkg: error processing archive /var/cache/apt/archives/mariadb-server-10.0_10.0.19-1_i386.deb (--unpack):
 trying to overwrite '/usr/share/mysql/errmsg-utf8.txt', which is also in package mysql-common 5.6.25-2debian8
dpkg-deb: error: subprocess paste was killed by signal (Broken pipe)
Processing triggers for man-db (2.6.7.1-1) ...
Errors were encountered while processing:
 /var/cache/apt/archives/mariadb-server-core-10.0_10.0.19-1_i386.deb
 /var/cache/apt/archives/mariadb-server-10.0_10.0.19-1_i386.deb
E: Sub-process /usr/bin/dpkg returned an error code (1)

Commenting /etc/apt/sources.list/mysql.list

% sudo apt-get update

% sudo apt-get -f install
If are sure you want to downgrade to 5.5, remove the file
/var/lib/mysql/debian-*.flag and try installing again.
dpkg: error processing archive /var/cache/apt/archives/mysql-server-5.5_5.5.43-0+deb8u1_i386.deb (--unpack):
 subprocess new pre-installation script returned error exit status 1

% sudo rm /var/lib/mysql/debian-10.0.flag

% sudo apt-get -f install 
[FAIL] Starting MySQL database server: mysqld . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . failed!

# I'm getting angry
% sudo apt-get purge mysql-server mysql-client mysql-common mysql-client-5.5 mysql-server-5.5

% sudo apt-get install mariadb-server mariadb-client

[ ok ] Starting MariaDB database server: mysqld.


\o/
Comment 4 Tomás Cohen Arazi 2015-07-15 09:15:39 UTC
> % sudo apt-get install mariadb-server mariadb-client
> 
> [ ok ] Starting MariaDB database server: mysqld.
> 
> 
> \o/

Weeeeee
Comment 5 Humberto Blanco 2016-03-31 19:58:30 UTC
Tomás, 
I have similar problem with mysql enterprise end i need to modify the database structure.so:

ALTER TABLE aqorders ALTER COLUMN budgetgroup_id  SET DEFAULT 0;
Comment 6 Jonathan Druart 2016-04-04 11:00:00 UTC
(In reply to Humberto Blanco from comment #5)
> Tomás, 
> I have similar problem with mysql enterprise end i need to modify the
> database structure.so:
> 
> ALTER TABLE aqorders ALTER COLUMN budgetgroup_id  SET DEFAULT 0;

This column does not seem to be used and could be removed.
In the meanwhile it would be better to set it to NULL (and remove the NOT NULL constraint) instead of set it to 0 (which does not make sense).
Comment 7 Jonathan Druart 2016-05-02 14:03:55 UTC
(In reply to Jonathan Druart from comment #6)
> (In reply to Humberto Blanco from comment #5)
> > Tomás, 
> > I have similar problem with mysql enterprise end i need to modify the
> > database structure.so:
> > 
> > ALTER TABLE aqorders ALTER COLUMN budgetgroup_id  SET DEFAULT 0;
> 
> This column does not seem to be used and could be removed.
> In the meanwhile it would be better to set it to NULL (and remove the NOT
> NULL constraint) instead of set it to 0 (which does not make sense).

See bug 16414.
Comment 8 Chris Cormack 2016-05-18 06:13:08 UTC
Yeah this is a blocker for release I reckom
Comment 9 Katrin Fischer 2016-05-18 06:17:08 UTC
This means people can't install on current the current Ubuntu LTS - we really need to fix it!
Comment 10 Jonathan Druart 2016-05-18 13:16:25 UTC
It seems that using MySQL v5.7, the web installer fails on step 3 with "using a password on command line interface can be insecure".
I don't think we can fix that modifying Koha code.
I think we should remove the password from the command line execute in C4::Installer::load_sql and let the administrators set a config file containing the password.
Comment 11 Tomás Cohen Arazi 2016-05-18 16:34:58 UTC Comment hidden (obsolete)
Comment 12 Tomás Cohen Arazi 2016-05-18 19:23:10 UTC Comment hidden (obsolete)
Comment 13 Tomás Cohen Arazi 2016-05-18 20:39:30 UTC Comment hidden (obsolete)
Comment 14 Tomás Cohen Arazi 2016-05-18 20:39:39 UTC Comment hidden (obsolete)
Comment 15 Chris Cormack 2016-05-18 21:47:18 UTC Comment hidden (obsolete)
Comment 16 Chris Cormack 2016-05-18 21:47:39 UTC Comment hidden (obsolete)
Comment 17 Chris Cormack 2016-05-18 21:48:54 UTC
This works, but it is slow, I think that is ok though, as you only run the web installer once :)
Comment 18 Bernardo Gonzalez Kriegel 2016-05-19 00:38:53 UTC Comment hidden (obsolete)
Comment 19 Chris Cormack 2016-05-19 00:40:27 UTC
OOhhh, that sounds a lot better, I'll test it out
Comment 20 Chris Cormack 2016-05-19 01:28:09 UTC
It works, but a new dependency eh? We need to update that too.

I've attached it as a signed off patch, but will let the RM and others decide if they want to add the new dependency.
Comment 21 Chris Cormack 2016-05-19 01:29:01 UTC Comment hidden (obsolete)
Comment 22 Tomás Cohen Arazi 2016-05-19 03:12:57 UTC
(In reply to Chris Cormack from comment #20)
> It works, but a new dependency eh? We need to update that too.
> 
> I've attached it as a signed off patch, but will let the RM and others
> decide if they want to add the new dependency.

Yeah, it all depends on the packaging complexity. Let's add the packaging manager to the talk.
Comment 23 Bernardo Gonzalez Kriegel 2016-05-19 04:26:49 UTC Comment hidden (obsolete)
Comment 24 Bernardo Gonzalez Kriegel 2016-05-19 04:26:54 UTC Comment hidden (obsolete)
Comment 25 Bernardo Gonzalez Kriegel 2016-05-19 04:34:50 UTC
(In reply to Chris Cormack from comment #20)
> It works, but a new dependency eh? We need to update that too.
> 
> I've attached it as a signed off patch, but will let the RM and others
> decide if they want to add the new dependency.

You are right, but SQL::SplitStatement also needs packaging.

Tested web installer in all languages, added fixes for almost all.
ru-RU and uk-UA need more work, but I need to sleep.
Comment 26 Bernardo Gonzalez Kriegel 2016-05-19 04:45:48 UTC Comment hidden (obsolete)
Comment 27 Jonathan Druart 2016-05-19 07:44:24 UTC
Bernardo, how did you generate new sample files?
Comment 28 Jonathan Druart 2016-05-19 07:53:50 UTC
What's the expected behavior if an error occurs,
Comment 29 Bernardo Gonzalez Kriegel 2016-05-19 11:37:31 UTC
Hi Jonathan,

(In reply to Jonathan Druart from comment #27)
> Bernardo, how did you generate new sample files?

Mmmm, sorry but can't understand exactly...
Just edited current files and fix what was needed

(In reply to Jonathan Druart from comment #28)
> What's the expected behavior if an error occurs,

A) de-DE, es-ES, fr-CA and pl-PL, 
same error for all langs

 * first 3 patches applied
 * (cd misc/translator/; ./translate install de-DE) , for example
 * drop/create database, run webinstaller
 * marc21, select all sample data

1) web installer error

Software error:

[SQL ERROR]: -- ISSN: 1041-5653
-- Refer to http://www.niso.org/standards/index.html

-- General Holdings: Type of Unit Designator
INSERT INTO `authorised_values` (category, authorised_value, lib) VALUES ('HINGS_UT','0','Information not available; Not applicable');INSERT INTO `authorised_values` (category, authorised_value, lib) VALUES ('HINGS_UT','a','Basic bibliographic unit')
For help

2) logs

install.pl: DBD::mysql::st execute 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 'INSERT INTO `authorised_values` (category, authorised_value, lib) VALUES ('HINGS' at line 5 at /usr/local/share/perl/5.18.2/DBIx/RunSQL.pm line 254...
and more lines

In this case I suppose that DBIx::RunSQL does not know how to parse 2 sql statments on the same line.

B) fr-FR
1) web installer

Software error:

[SQL ERROR]: 
INSERT INTO `auth_types` (`authtypecode`, `authtypetext`, `auth_tag_to_report`, `summary`) VALUES ('','Défaut','','')

2) logs

install.pl: DBD::mysql::st execute failed: Duplicate entry '' for key 'PRIMARY' at /usr/local/share/perl/5.18.2/DBIx/RunSQL.pm line 254.

In this case as both autorite_default.sql and 
autorites_norme_unimarc.sql are 'Obligatoire',
we have duplicate keys for 'Défaut' auth type 
and some other fields/subfields.

Here the second file was edited to join all inserts in
one statement (like EN marc21 files) instead of adding 
IGNORE to all INSERTs to prevent this error 

The other error is a space after final ';' on sample_labels.sql,
similar to case A)
Comment 30 Bernardo Gonzalez Kriegel 2016-05-19 14:03:22 UTC
Moving i18n files to Bug 16554.
Resetting to 'Signed off' for QA consideration, sorry for the noise
Comment 31 Jonathan Druart 2016-05-20 16:06:25 UTC
I think we should gracefully handle errors, as it was before.
If C4::Installer::load_file encounters an error, Koha will crash.
Comment 32 Bernardo Gonzalez Kriegel 2016-05-20 17:32:09 UTC Comment hidden (obsolete)
Comment 33 Bernardo Gonzalez Kriegel 2016-05-20 17:34:52 UTC
(In reply to Jonathan Druart from comment #31)
> I think we should gracefully handle errors, as it was before.
> If C4::Installer::load_file encounters an error, Koha will crash.

You are right, added a possible fix.
Comment 34 Jonathan Druart 2016-05-20 18:03:16 UTC
I don't know if we should fail or continue actually, hard to say.
I would say we should block anything if an error occurs (and so do not execute next files).
Otherwise there is a `force` parameter to set to DBIx::RunSQL we could use.
Comment 35 Tomás Cohen Arazi 2016-05-20 18:17:02 UTC
(In reply to Jonathan Druart from comment #34)
> I don't know if we should fail or continue actually, hard to say.
> I would say we should block anything if an error occurs (and so do not
> execute next files).
> Otherwise there is a `force` parameter to set to DBIx::RunSQL we could use.

Any behaviour change in that way would be a nice improvement, but it would need to be discussed. Also, that would be an enhancement.
This is a blocker bugfix so Koha works on newer operating systems, with backwards compatibility. Keeping the current behaviour, making it work. I think we should push it if the Packaging manager agrees to package DBIx::RunSQL (it only depends on DBI!) and add it to the repository.
Comment 36 Bernardo Gonzalez Kriegel 2016-05-20 18:34:39 UTC
(In reply to Tomás Cohen Arazi from comment #35)
> (In reply to Jonathan Druart from comment #34)
> > I don't know if we should fail or continue actually, hard to say.
> > I would say we should block anything if an error occurs (and so do not
> > execute next files).
> > Otherwise there is a `force` parameter to set to DBIx::RunSQL we could use.
> 
> Any behaviour change in that way would be a nice improvement, but it would
> need to be discussed. Also, that would be an enhancement.
> This is a blocker bugfix so Koha works on newer operating systems, with
> backwards compatibility. Keeping the current behaviour, making it work. I
> think we should push it if the Packaging manager agrees to package
> DBIx::RunSQL (it only depends on DBI!) and add it to the repository.

Was about to say something similar. 
Also 'force' seems to hide the problems, just tried it and can't get number of problems.
Comment 37 Jonathan Druart 2016-05-21 07:14:05 UTC
Yes, I agree: fix without change in the behavior.

That's why I think we should update the patch :)

Without the patch the errors are log into the Koha log file and there is not crash
With the patch, there is nothing in the log file and Koha crashes.
Comment 38 Jonathan Druart 2016-05-21 07:16:53 UTC
(In reply to Jonathan Druart from comment #37)
> Without the patch the errors are log into the Koha log file and there is not
> crash
> With the patch, there is nothing in the log file and Koha crashes.

Sorry, forgot to apply the last patch locally.
Comment 39 Jonathan Druart 2016-05-21 08:05:55 UTC Comment hidden (obsolete)
Comment 40 Bernardo Gonzalez Kriegel 2016-05-21 12:14:50 UTC Comment hidden (obsolete)
Comment 41 Bernardo Gonzalez Kriegel 2016-05-21 12:16:30 UTC
> 
> Sorry, forgot to apply the last patch locally.

Forgot to sing also :)
I've signed yours, can't sign mine.
Comment 42 Jonathan Druart 2016-05-21 12:33:38 UTC
Created attachment 51681 [details] [review]
Bug 13669: Use perl to load SQL statements from sample data

This patch changes C4::Installer::load_sql so it uses File::Slurp [1] to read
the SQL files, and SQL::SplitStatement to extract the statements from the full SQL
file so they can be passed to $dbh->do.

To test:
- On Mysql 5.5, run the webinstaller
=> SUCCESS: Everything works as expected
- Apply the patch
- Run
  $ mysql -uroot
  > DROP DATABASE koha_kohadev ; CREATE DATABASE koha_kohadev;
- Run the webinstaller
=> SUCCESS: Everything works as expected
- On Mysql 5.6+ (5.7 is implied)
- Run
  $ mysql -uroot
  > DROP DATABASE koha_kohadev ; CREATE DATABASE koha_kohadev;
- Run the webinstaller
=> FAIL: It cannot load the sql files due to a password-in-command-line error
- Apply the patch
- Make sure everything is clean (it should):
  $ mysql -uroot
  > DROP DATABASE koha_kohadev ; CREATE DATABASE koha_kohadev;
- Run the webinstaller
=> SUCCESS: EVerything works as expected
- Sign off :-D

[1] Note: This is a POC patch, in the sense that it does the job, fixes a nasty problem
but using File::Slurp to load the SQL files in memory comes with a big runtime penalty.
You will notice the install procedure is now much slower, for instance.

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 43 Jonathan Druart 2016-05-21 12:33:43 UTC
Created attachment 51682 [details] [review]
Bug 13669: (followup) Fix kohastructure.sql so FK dependencies are sorted

The current table creation order is left to mysql's strategy, which is not
suitable for parsing the SQL files and passing one statement at a time in
the current order.

This patch just moves table creation statements around so FK constraints are
defined for previously created tables.

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 44 Jonathan Druart 2016-05-21 12:33:47 UTC
Created attachment 51683 [details] [review]
Bug 13669: (alternative) use DBIx::RunSQL

Alternative POC solution, on top of
previous patches, feel free to obsolte.

This patch use DBIx::RunSQL->run_sql_file
to procees each sql file.

To test:
1) Apply all patches
2) Same test plan of patch 1

Timing test running web installer, marc21,
all sample data, time in seconds

a) without patch
structure   data
59.7        66.5
58.6        66.0

b) SQL::SplitStatement (patch 1+2)
59.4        101
59.7        102

c) DBIx::RunSQL (patch 3)
60.7        66.8
59.4        66.2

Tomas' version is a bit slower loading sample data,
all give similar results processing kohastructure.

New dependency to package: DBIx::RunSQL

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 45 Jonathan Druart 2016-05-21 12:33:54 UTC
Created attachment 51684 [details] [review]
Bug 13669: Re-adds error handling to load_sql

This patch prevents crashing in case an
error is detected when loading a file

To test:
1) Apply patch
2) Mangle kohastructure.sql or any sample
file adding and invalid SQL line
3) Run webinstaller and check that the error
is handled gracefully

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 46 Jonathan Druart 2016-05-21 12:33:59 UTC
Created attachment 51685 [details] [review]
Bug 13669: Catch the errors to have them to the logs

This patch redirect STDERR to a variable to retrieve the errors raised
by the DBMS when loading a sql file, it could be useful to debug errors.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
It's better of course, trying to load a failed fiel
it outputs mysl errors
DBD::mysql::st execute failed: You have an error in your SQL syntax...
No errors

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 47 Katrin Fischer 2016-05-22 17:39:02 UTC
I have installed the dependency from CPAN, made sure that the database was empty, applied the patches and ran the web installer in English. But I still get errors, any idea what's going wrong there?

Help?

[Sun May 22 19:38:37 2016] install.pl: Use of uninitialized value in numeric lt (<) at /usr/local/share/perl/5.22.1/DBIx/RunSQL.pm line 261. [Sun May 22 19:38:37 2016] install.pl: Use of uninitialized value in numeric lt (<) at /usr/local/share/perl/5.22.1/DBIx/RunSQL.pm line 261. [Sun May 22 19:38:37 2016] install.pl: Use of uninitialized value in numeric lt (<) at /usr/local/share/perl/5.22.1/DBIx/RunSQL.pm line 261. [Sun May 22 19:38:37 2016] install.pl: Use of uninitialized value in numeric lt (<) at /usr/local/share/perl/5.22.1/DBIx/RunSQL.pm line 261. [Sun May 22 19:38:37 2016] install.pl: Use of uninitialized value in numeric lt (<) at /usr/local/share/perl/5.22.1/DBIx/RunSQL.pm line 261. [Sun May 22 19:38:37 2016] install.pl: DBD::mysql::st execute failed: BLOB, TEXT, GEOMETRY or JSON column 'defaultvalue' can't have a default value at /usr/local/share/perl/5.22.1/DBIx/RunSQL.pm line 254.

Please contact your system administrator
Comment 48 Bernardo Gonzalez Kriegel 2016-05-22 22:11:26 UTC
(In reply to Katrin Fischer from comment #47)
> I have installed the dependency from CPAN, made sure that the database was
> empty, applied the patches and ran the web installer in English. But I still
> get errors, any idea what's going wrong there?
> 
...

I can confirm similar problems on Ubuntu 16.05, Myslq 5.7
My test where not done on this kind of system, sorry :(

In case the problem where from DBIX::RunSQL (which gives problems),
I wrote a revision using Tomas' approach (SQL::SplitStatment), and found
the same problems

[Sun May 22 18:49:18 2016] install.pl: DBD::mysql::db do failed: BLOB, TEXT, GEOMETRY or JSON column 'defaultvalue' can't have a default value at /home/bgkriegel/kohaclone/C4/Installer.pm line 443. [Sun May 22 18:49:51 2016] install.pl: DBD::mysql::db do failed: Invalid default value for 'created_on' at /home/bgkriegel/kohaclone/C4/Installer.pm line 443. [Sun May 22 18:49:52 2016] install.pl: DBD::mysql::db do failed: Cannot add foreign key constraint at /home/bgkriegel/kohaclone/C4/Installer.pm line 443. [Sun May 22 18:49:52 2016] install.pl: DBD::mysql::db do failed: Cannot add foreign key constraint at /home/bgkriegel/kohaclone/C4/Installer.pm line 443
Comment 49 Bernardo Gonzalez Kriegel 2016-05-23 01:05:27 UTC
Errors are not in the procedeure, I tested extensively but on msyql < 5.6, so this is specific to ver 5.6+ treatment of SQL, or 5.7 more specifically

1) table: auth_subfield_structure

line:  72   `defaultvalue` TEXT DEFAULT '',
error: (BLOB, TEXT, GEOMETRY or JSON column 'defaultvalue' can't have a default value): CREATE TABLE `auth_subfield_structure`

Seems that we can't have default values here.

Ref: http://dev.mysql.com/doc/refman/5.7/en/blob.html
->" BLOB and TEXT columns cannot have DEFAULT values."

2) table: virtualshelves
line:  2212   `created_on` TIMESTAMP NOT NULL, -- creation time
error: (Invalid default value for 'created_on'): CREATE TABLE `virtualshelves`

Seems that first TIMESTAMP column in a table not explicitly nullable
automatically gets DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP

Could it be DATETIME instead of TIMESTAMP??

Reference http://shankargopal.blogspot.com.ar/2013/03/mysql-566-timestamp-columns-and-default.html


3) and 4)
error: (Cannot add foreign key constraint): CREATE TABLE `virtualshelfcontents`
error: (Cannot add foreign key constraint): CREATE TABLE `virtualshelfshares`

This two because virtualshelves table not created

MORE
----
Fixing somehow previous kohastructure errors, we have more Errors in sample data

auth_subfield_structure, (Incorrect integer value: '' for column 'linkid' at row 1)
Here we have an error, '' instead of 0, MySQL < 5.6 tolerates this, 5.7 no

sample_patrons.sql, (Column 'address' cannot be null)

sample_quotes.sql, (Incorrect datetime value: '0000-00-00 00:00:00' for column 'timestamp' at row 1)

MySQL 5.7 does not like things like that.
Perhaps we can change the sql_mode for Koha globally, 
or fix all the sample data (not difficult) in all languages, 
and test/sign/QA everything... before release?

Ref: https://www.digitalocean.com/community/tutorials/how-to-prepare-for-your-mysql-5-7-upgrade
Comment 50 Jonathan Druart 2016-05-23 11:17:49 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #49)
> Errors are not in the procedeure, I tested extensively but on msyql < 5.6,
> so this is specific to ver 5.6+ treatment of SQL, or 5.7 more specifically
> 
> 1) table: auth_subfield_structure
> 
> line:  72   `defaultvalue` TEXT DEFAULT '',
> error: (BLOB, TEXT, GEOMETRY or JSON column 'defaultvalue' can't have a
> default value): CREATE TABLE `auth_subfield_structure`
> 
> Seems that we can't have default values here.
> 
> Ref: http://dev.mysql.com/doc/refman/5.7/en/blob.html
> ->" BLOB and TEXT columns cannot have DEFAULT values."

I get this one using --show-warnings on MariaDB 10.0.24
Fixed with 
diff --git a/installer/data/mysql/kohastructure.sql b/installer/data/mysql/kohastructure.sql
index 42bff46..ea220bb 100644
--- a/installer/data/mysql/kohastructure.sql
+++ b/installer/data/mysql/kohastructure.sql
@@ -69,7 +69,7 @@ CREATE TABLE `auth_subfield_structure` (
   `linkid` tinyint(1) NOT NULL default 0,
   `kohafield` varchar(45) NULL default '',
   `frameworkcode` varchar(10) NOT NULL default '',
-  `defaultvalue` TEXT DEFAULT '',
+  `defaultvalue` TEXT,

But I don't get the others

> MySQL 5.7 does not like things like that.
> Perhaps we can change the sql_mode for Koha globally, 
> or fix all the sample data (not difficult) in all languages, 
> and test/sign/QA everything... before release?

Yes, on another bug report.
Now you have Ubuntu 16.04, could you please submit a patch? :)
Comment 51 Kyle M Hall 2016-05-23 16:48:31 UTC
Pushed to master for 16.05! Thanks Tomas, Bernardo and Jonathan!
Comment 52 Bernardo Gonzalez Kriegel 2016-05-23 16:57:48 UTC
(In reply to Jonathan Druart from comment #50)
> Yes, on another bug report.
> Now you have Ubuntu 16.04, could you please submit a patch? :)

Will do :)
Comment 53 Bernardo Gonzalez Kriegel 2016-05-23 22:55:19 UTC
Mmm, we have a problem :(

RunSQL prints loads of 'warnings', they are catched and treated as
errors because they go to stderr.

RunSQL have this line

261             } elsif( 0 < $sth->{NUM_OF_FIELDS} ) {

but $sth from execute can have that parameter undef for create/delete,
and that seems to be the case, not an error but a warning.

Now we can't install, even on MySQL < 5.7 !!

Can we revert (in another Bug) to something similar to my error treatment?
Displayed errors where not as fancy, but worked.
Comment 54 Bernardo Gonzalez Kriegel 2016-05-24 01:57:32 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #52)
> (In reply to Jonathan Druart from comment #50)
> > Yes, on another bug report.
> > Now you have Ubuntu 16.04, could you please submit a patch? :)
> 
> Will do :)

Bug 16572 as POC solution to my last comment
Bug 16573 for Ubuntu 16.04 fix
Comment 55 Katrin Fischer 2016-05-24 05:10:53 UTC
Thx all for working on this! I have Ubuntu 16.04 and MySQL 5.7 - so I can test this tomorrow again at the hackfest if stil needed.
Comment 56 Marc Véron 2016-05-24 06:05:00 UTC
Is the following related to this bug?

Can't locate DBIx/RunSQL.pm in @INC (@INC contains: /usr/share/kohaclone /etc/perl /usr/local/lib/perl/5.14.2 /usr/local/share/perl/5.14.2 /usr/lib/perl5 /usr/share/perl5 /usr/lib/perl/5.14 /usr/share/perl/5.14 /usr/local/lib/site_perl .) at /usr/share/kohaclone/C4/Installer.pm line 23.
BEGIN failed--compilation aborted at /usr/share/kohaclone/C4/Installer.pm line 23.
Compilation failed in require at /usr/share/kohaclone/about.pl line 34.
BEGIN failed--compilation aborted at /usr/share/kohaclone/about.pl line 34.

It seems that a dependency is missing in PerlDependencies.pm
Comment 57 Katrin Fischer 2016-05-24 06:07:10 UTC
Yes it is related - and I think it needs to be packaged, I couldn't find it in latest Ubuntu and installed from CPAN for testing.
Comment 58 Kyle M Hall 2016-05-24 10:42:57 UTC
(In reply to Katrin Fischer from comment #57)
> Yes it is related - and I think it needs to be packaged, I couldn't find it
> in latest Ubuntu and installed from CPAN for testing.

Just to note, DBIx::RunSQL should be trivial to package. As far as I can tell, it requires no external dependencies. At least nothing we don't already have except for Text::Table which appears to be optional.
Comment 59 Kyle M Hall 2016-05-24 10:45:02 UTC
So, with the release so close, how should we deal with this? Revert? Push forward with Bug 16572 and Bug 16573? Or do we have other options?
Comment 60 Bernardo Gonzalez Kriegel 2016-05-24 11:19:59 UTC
(In reply to Kyle M Hall from comment #59)
> So, with the release so close, how should we deal with this? Revert? Push
> forward with Bug 16572 and Bug 16573? Or do we have other options?

With Bug 16572 + new perl dependency DBIx::RunSQL we can install where we used to, not systems with MySQL 5.7. 

To add support for mysql 5.7 we need Bug 16573 + another one to fix i18n installer files. I can write it, but we need someone to test, and that in case this kind of fix is considered valid.
Comment 61 Kyle M Hall 2016-05-24 13:03:11 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #60)
> (In reply to Kyle M Hall from comment #59)
> > So, with the release so close, how should we deal with this? Revert? Push
> > forward with Bug 16572 and Bug 16573? Or do we have other options?
> 
> With Bug 16572 + new perl dependency DBIx::RunSQL we can install where we
> used to, not systems with MySQL 5.7. 
> 
> To add support for mysql 5.7 we need Bug 16573 + another one to fix i18n
> installer files. I can write it, but we need someone to test, and that in
> case this kind of fix is considered valid.

Thanks! If you write it I'm sure we can find some testers!
Comment 62 Bernardo Gonzalez Kriegel 2016-05-24 13:20:58 UTC
(In reply to Kyle M Hall from comment #61)
> 
> Thanks! If you write it I'm sure we can find some testers!

Will do!

Do you need another bug to add DBIx::RunSQL dependency ?
Comment 63 Tomás Cohen Arazi 2016-05-24 13:25:10 UTC
Just a followup!
Comment 64 Bernardo Gonzalez Kriegel 2016-05-24 13:39:14 UTC Comment hidden (obsolete)
Comment 65 Marc Véron 2016-05-24 14:19:54 UTC Comment hidden (obsolete)
Comment 66 Bernardo Gonzalez Kriegel 2016-05-24 18:42:36 UTC
Created attachment 51746 [details] [review]
Bug 13669: followup to add DBIx::RunSQL dependency

Just that

To test:
1) run koha_perl_deps and check it show up

The module appears now on the About page.
Signed-off-by: Marc Véron <veron@veron.ch>

Ammended patch, only change is DBIx::RunSQL version,
now 0.14 :)

Module's author kindly accept to upgrade it, in particular
this makes Bug 16572 innecesary and is not needed to install
without problems.

Tested install on Ubuntu 14.04/Mysql 5.5.49, marc21 + all sample
Comment 67 Julian Maurice 2016-05-25 08:21:28 UTC
Patches pushed to 3.22.x, will be in 3.22.7
Comment 68 Kyle M Hall 2016-05-25 10:07:19 UTC
Excellent! Followup pushed to master! Thanks Bernardo!
Comment 69 Julian Maurice 2016-05-31 09:01:22 UTC
I forgot the last followup in 3.22.7, it will be in 3.22.8
Comment 70 Tomás Cohen Arazi 2016-05-31 10:33:37 UTC
(In reply to Julian Maurice from comment #69)
> I forgot the last followup in 3.22.7, it will be in 3.22.8

Julian, have the PM add it on packaging!