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
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 :)
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
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/
> % sudo apt-get install mariadb-server mariadb-client > > [ ok ] Starting MariaDB database server: mysqld. > > > \o/ Weeeeee
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;
(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).
(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.
Yeah this is a blocker for release I reckom
This means people can't install on current the current Ubuntu LTS - we really need to fix it!
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.
Created attachment 51596 [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.
Created attachment 51597 [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.
Created attachment 51598 [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.
Created attachment 51599 [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.
Created attachment 51600 [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>
Created attachment 51601 [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>
This works, but it is slow, I think that is ok though, as you only run the web installer once :)
Created attachment 51607 [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
OOhhh, that sounds a lot better, I'll test it out
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.
Created attachment 51608 [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>
(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.
Created attachment 51610 [details] [review] Bug 13669: (followup) fix I18N files This patch fix web installer for de-DE, es-ES, fr-CA, nb-NO and pl-PL To test: 1) Apply patch 2) Try web installer for any/all listed languages. There must be no problems for marc21 + all sample files
Created attachment 51611 [details] [review] Bug 13669: (followup) fix fr-FR files Rewrite autorites_norme_unimarc.sql to use only one INSERT IGNORE. New load method complains about duplicate keys, and both auth files are mandatory! Also removed an exotic blank space on sample_labels.sql Tested with unimarc_complet and all sample files. To test 1) Apply the patch 2) Try fr-FR web installer, unimarc_complet + all sample files There must be no errors Don't know if fr-FR marc21 and unimarc_lecture_pub are used, need a look from french users.
(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.
Created attachment 51612 [details] [review] Bug 13669: (followup) fix I18N files This patch fix web installer for de-DE, es-ES, fr-CA, nb-NO and pl-PL To test: 1) Apply patch 2) Try web installer for any/all listed languages. a) de-DE, es-ES, fr-CA and pl-PL There must be no problems for marc21 + all sample files b) nb-NO There must be no problems for normarc + all sample files Fixed test plan
Bernardo, how did you generate new sample files?
What's the expected behavior if an error occurs,
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)
Moving i18n files to Bug 16554. Resetting to 'Signed off' for QA consideration, sorry for the noise
I think we should gracefully handle errors, as it was before. If C4::Installer::load_file encounters an error, Koha will crash.
Created attachment 51671 [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
(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.
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.
(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.
(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.
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.
(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.
Created attachment 51672 [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.
Created attachment 51680 [details] [review] [SIGNED-OFF] 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
> > Sorry, forgot to apply the last patch locally. Forgot to sing also :) I've signed yours, can't sign mine.
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>
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>
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>
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>
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>
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
(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
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
(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? :)
Pushed to master for 16.05! Thanks Tomas, Bernardo and Jonathan!
(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 :)
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.
(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
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.
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
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.
(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.
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?
(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.
(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!
(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 ?
Just a followup!
Created attachment 51736 [details] [review] Bug 13669: followup to add DBIx::RunSQL dependency Just that To test: 1) run koha_perl_deps and check it show up
Created attachment 51738 [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>
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
Patches pushed to 3.22.x, will be in 3.22.7
Excellent! Followup pushed to master! Thanks Bernardo!
I forgot the last followup in 3.22.7, it will be in 3.22.8
(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!
I stumbled over RunSQL while trying to add a CREATE FUNCTION in a Koha installer script. The script can be run fine with mysql client but not via RunSQL. Why? In order to create a function or procedure you need to change the delimiter with DELIMITER // (or some other character). This is not a MySQL command. RunSQL will crash on it, but the mysql client knows about it.
IN the period between this push and the the push of bug 26947, the /*! version dependent lines at the start of kohastructure appear to have been silently ignored by DBIx::RunSQL.