Bug 15207 - Error on upgrade from 3.20.5 to 3.22 beta
Summary: Error on upgrade from 3.20.5 to 3.22 beta
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 major (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 14205
Blocks:
  Show dependency treegraph
 
Reported: 2015-11-18 11:20 UTC by Zeno Tajoli
Modified: 2016-06-21 21:39 UTC (History)
5 users (show)

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


Attachments
patch for fix update (2.89 KB, patch)
2015-11-18 14:43 UTC, Zeno Tajoli
Details | Diff | Splinter Review
Warnings after patch (46.16 KB, image/png)
2015-11-18 21:59 UTC, Nick Clemens
Details
New version of patch for fix update (3.37 KB, patch)
2015-11-19 15:23 UTC, Zeno Tajoli
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15207: To not show errors on update from 3.20.x to 3.22.x (3.56 KB, patch)
2015-11-19 20:17 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 15207: (followup) add collate info to sound alerts table (1.61 KB, patch)
2015-11-20 01:32 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 15207: Follow-up - change check for existing index (1.38 KB, patch)
2015-11-21 11:44 UTC, Katrin Fischer
Details | Diff | Splinter Review
Not show errors on update from 3.20 (3.88 KB, patch)
2015-11-21 13:21 UTC, Zeno Tajoli
Details | Diff | Splinter Review
Not show errors on update from 3.20 (4.07 KB, patch)
2015-11-21 15:29 UTC, Zeno Tajoli
Details | Diff | Splinter Review
To not show errors on update from 3.20.x to 3.22.x (4.17 KB, patch)
2015-11-23 23:08 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 15207: Avoid warnings when adding constraint course_reserves_ibfk_2 (2.21 KB, patch)
2015-11-25 13:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15207: Avoid warnings when adding constraint course_reserves_ibfk_2 (2.27 KB, patch)
2015-11-25 14:27 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[PASSED QA] Bug 15207: Avoid warnings when adding constraint course_reserves_ibfk_2 (2.33 KB, patch)
2015-11-25 15:01 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Zeno Tajoli 2015-11-18 11:20:40 UTC

    
Comment 1 Zeno Tajoli 2015-11-18 11:24:40 UTC
Hi,

I try to do an update from 3.20.5 to master (3.22 beta at db level).

I have done a standard marc21 english install using all sql files, mandatory and optional.

I receive two errors:

updatedatabase.pl: DBD::mysql::db do failed: Can't create table 'koha.#sql-3a4_7f' (errno: 121) [for Statement "
updatedatabase.pl: ALTER IGNORE TABLE course_reserves
updatedatabase.pl: add CONSTRAINT course_reserves_ibfk_2
updatedatabase.pl: FOREIGN KEY (ci_id) REFERENCES course_items (ci_id)
updatedatabase.pl: ON DELETE CASCADE ON UPDATE CASCADE
updatedatabase.pl: "] at /production/koha/install/bug14960/installer/data/mysql/updatedatabase.pl line 10804.

updatedatabase.pl: DBD::mysql::db do failed: Duplicate entry '1-self_checkout' for key 'PRIMARY' [for Statement "
updatedatabase.pl: INSERT INTO permissions (module_bit, code, description)
updatedatabase.pl: VALUES (1, 'self_checkout', 'Perform self checkout at the OPAC. It should be used for the patron matching the AutoSelfCheckID')
updatedatabase.pl: "] at /production/koha/install/bug14960/installer/data/mysql/updatedatabase.pl line 10871.
Comment 2 Zeno Tajoli 2015-11-18 11:53:24 UTC
The first problem  (about change in table course_reserves, bug 14205) is not
a real problem for y test situation.
Course Reserve is not active and the new costraint,  course_reserves_ibfk_2,
it was added without problems
Comment 3 Zeno Tajoli 2015-11-18 12:15:35 UTC
The second error is present because the update 3.21.00.027 starts so:

dbh->do(q|
        INSERT INTO permissions (module_bit, code, description)
        VALUES (1, 'self_checkout', 'Perform self checkout at the OPAC. It should be used for the patron matching the AutoSelfCheckID')
    |);

But in 3.20.5 in file  installer/data/mysql/en/mandatory/userpermissions.sql I see: 

INSERT INTO permissions (module_bit, code, description) VALUES
[...]   
( 1, 'self_checkout', 'Perform self checkout at the OPAC. It should be used for the patron matching the AutoSelfCheckID'),

So in 3.20.x the code 'self_checkout' could be present.

I don't know if it is better to 'INSERT IGNORE' or other statement.
A discussion on the topic here:
http://stackoverflow.com/questions/548541/insert-ignore-vs-insert-on-duplicate-key-update

Tecnaly speaking also this error is not blocking, the code 'self_checkout' is present in the table permissions with a correct value.
Comment 4 Zeno Tajoli 2015-11-18 13:27:43 UTC
About the first error:

The motivation of the error is that in 3.20.5 the second constraint on the table
 course_reserves is present in its version onf kohastructure.sql:

`course_reserves_ibfk_2` FOREIGN KEY (`ci_id`) REFERENCES `course_items` (`ci_id`) ON DELETE CASCADE ON UPDATE CASCADE
in http://git.koha-community.org/gitweb/?p=koha.git;a=blob;f=installer/data/mysql/kohastructure.sql;h=52eb88d1c35a3cfd825cd16412b49929ab6153ae;hb=refs/heads/3.20.x#l627

We can see here:
http://stackoverflow.com/questions/12623651/error-error-1005-cant-create-table-errno-121
Comment 5 Zeno Tajoli 2015-11-18 14:43:07 UTC Comment hidden (obsolete)
Comment 6 Nick Clemens 2015-11-18 21:59:55 UTC
Created attachment 44970 [details]
Warnings after patch

Got rid of second error, but just pushed the first down a few lines
Comment 7 M. Tompsett 2015-11-18 23:49:13 UTC
Comment on attachment 44960 [details] [review]
patch for fix update

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

::: installer/data/mysql/updatedatabase.pl
@@ +10816,5 @@
> +                add CONSTRAINT course_reserves_ibfk_2
> +                    FOREIGN KEY (ci_id) REFERENCES course_items (ci_id)
> +                    ON DELETE CASCADE ON UPDATE CASCADE
> +        });
> +    };

What if someone created it incorrectly?
Why not leave the do logic as it was, but insert a delete foreign key if the count>0?
Comment 8 Zeno Tajoli 2015-11-19 08:12:28 UTC
(In reply to Nick Clemens from comment #6)
> Created attachment 44970 [details]
> Warnings after patch
> 
> Got rid of second error, but just pushed the first down a few lines

Nick, can you try to understand why in your set up those instructions: my $count_course_reserves_ibfk_2 = $dbh->selectrow_arrayref(q|
+        SELECT COUNT(*)
+        FROM information_schema.table_constraints 
+        WHERE  table_schema = schema() 
+               AND table_name = 'course_reserves' 
+               AND CONSTRAINT_NAME = 'course_reserves_ibfk_2'
+    |);

did not retreive the presence of FK "course_reserves_ibfk_2" ?
Because I don't have idea how to repoduce your situation.
Comment 9 Nick Clemens 2015-11-19 12:00:59 UTC
Sure, my test plan on kohadevbox:
1 - Checkout 3.20.x
2 - drop db, create db,
3 - run web installer (upgrade)
4 - checkout master
5 - run webinstaller
6 - got errors described
7 - checkout 3.20.x
8 - drop db, create db
9 - run web installer
10- - checkout master
11 - apply patch
12 - run websintaller (upgrade)
Comment 10 Zeno Tajoli 2015-11-19 12:58:15 UTC
Nick,
can you tell us your version of MySQL ?
Or do you use MariaBD ?
Comment 11 Zeno Tajoli 2015-11-19 15:23:41 UTC Comment hidden (obsolete)
Comment 12 Nick Clemens 2015-11-19 15:25:25 UTC
On Debian Jessie:

mysql Ver 14.14 Distrib 5.5.46, for debian-linux-gnu (x86_64) using readline 6.3
Perl 5.020002
Server version: Apache/2.4.10 (Debian)
Comment 13 Zeno Tajoli 2015-11-19 15:28:16 UTC
Exactly my enviroment (Debian, MySQL and Apache versions).
Try the new version !!
Comment 14 Bernardo Gonzalez Kriegel 2015-11-19 20:17:01 UTC Comment hidden (obsolete)
Comment 15 Bernardo Gonzalez Kriegel 2015-11-20 01:32:52 UTC Comment hidden (obsolete)
Comment 16 Katrin Fischer 2015-11-20 18:23:55 UTC
Hm, I think it doesn't work for me. I created a brand new database on a branch running 3.20.5. Then I switched back to master, applied the patches and ran the database update. This is the result:

    [Fri Nov 20 19:22:16 2015] updatedatabase.pl: DBD::mysql::db do failed: Can't create table 'koha.#sql-5b5_3f2' (errno: 121) [for Statement "
    [Fri Nov 20 19:22:16 2015] updatedatabase.pl: ALTER IGNORE TABLE course_reserves
    [Fri Nov 20 19:22:16 2015] updatedatabase.pl: add CONSTRAINT course_reserves_ibfk_2
    [Fri Nov 20 19:22:16 2015] updatedatabase.pl: FOREIGN KEY (ci_id) REFERENCES course_items (ci_id)
    [Fri Nov 20 19:22:16 2015] updatedatabase.pl: ON DELETE CASCADE ON UPDATE CASCADE
    [Fri Nov 20 19:22:16 2015] updatedatabase.pl: "] at /home/katrin/kohaclone/installer/data/mysql/updatedatabase.pl line 10814.
Comment 17 Katrin Fischer 2015-11-20 18:26:24 UTC
This is my 'before' course_reserves table copied from the mysqldump:

DROP TABLE IF EXISTS `course_reserves`;
/*!40101 SET @saved_cs_client     = @@character_set_client */;
/*!40101 SET character_set_client = utf8 */;
CREATE TABLE `course_reserves` (
  `cr_id` int(11) NOT NULL AUTO_INCREMENT,
  `course_id` int(11) NOT NULL,
  `ci_id` int(11) NOT NULL,
  `staff_note` mediumtext COLLATE utf8_unicode_ci,
  `public_note` mediumtext COLLATE utf8_unicode_ci,
  `timestamp` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP,
  PRIMARY KEY (`cr_id`),
  UNIQUE KEY `pseudo_key` (`course_id`,`ci_id`),
  KEY `course_id` (`course_id`),
  KEY `course_reserves_ibfk_2` (`ci_id`),
  CONSTRAINT `course_reserves_ibfk_1` FOREIGN KEY (`course_id`) REFERENCES `courses` (`course_id`),
  CONSTRAINT `course_reserves_ibfk_2` FOREIGN KEY (`ci_id`) REFERENCES `course_items` (`ci_id`) ON DELETE CASCADE ON UPDATE CASCADE
) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci;
/*!40101 SET character_set_client = @saved_cs_client */;
Comment 18 Katrin Fischer 2015-11-21 11:31:27 UTC
Could something like this here work?
4234 sub stocknumber_checker { #code reused later on
4235   my @row;
4236   #drop the obsolete itemSStocknumber idx if it exists
4237   @row = $dbh->selectrow_array("SHOW INDEXES FROM items WHERE key_name='itemsstocknumberidx'");
4238   $dbh->do("ALTER TABLE `items` DROP INDEX `itemsstocknumberidx`;") if @row;
4239 
4240   #check itemstocknumber idx; remove it if it is unique
4241   @row = $dbh->selectrow_array("SHOW INDEXES FROM items WHERE key_name='itemstocknumberidx' AND non_unique=0");
4242   $dbh->do("ALTER TABLE `items` DROP INDEX `itemstocknumberidx`;") if @row;
4243 
4244   #add itemstocknumber index non-unique IF it still not exists
4245   @row = $dbh->selectrow_array("SHOW INDEXES FROM items WHERE key_name='itemstocknumberidx'");
4246   $dbh->do("ALTER TABLE items ADD INDEX itemstocknumberidx (stocknumber);") unless @row;
4247 }
Comment 19 Katrin Fischer 2015-11-21 11:39:41 UTC
Trying it out now.
Comment 20 Katrin Fischer 2015-11-21 11:44:05 UTC Comment hidden (obsolete)
Comment 21 Katrin Fischer 2015-11-21 11:45:06 UTC
I have attached a follow-up - with this patch no errors show for me. But can someone please double-check?
Comment 22 Zeno Tajoli 2015-11-21 13:21:44 UTC Comment hidden (obsolete)
Comment 23 Zeno Tajoli 2015-11-21 13:25:20 UTC
Hi to all,

I cherry-pick the code of Katrin and Bernardo and I write a new version of the patch.
Now the check is based on presence of index, not on FK in the INFORMATION_SCHEMA of mysql.

So probalby it is more robust.
New cycle of sign-off/QA
Comment 24 Katrin Fischer 2015-11-21 13:31:36 UTC
Hi Zeno, thx for your work on this. We usually try to keep authorship of code. The other option here would be to sign off the 2 follow-ups from Bernardo and me - and move this to signed off.
Comment 25 Zeno Tajoli 2015-11-21 15:22:00 UTC
Hi Katrin,

Infact I'outsite my standard pc and so I need to rewrite the patch from scratch.
I think the second option is the more easy to do. Clearly we need that others person do sign-off and QA.
Comment 26 Zeno Tajoli 2015-11-21 15:29:34 UTC Comment hidden (obsolete)
Comment 27 Zeno Tajoli 2015-11-21 15:30:51 UTC
I upload a new version of the patch more correct about authorship of the code.
Comment 28 Katrin Fischer 2015-11-22 20:56:00 UTC
I'd be happier with the 3 separate patches from before - I had tested them all together with my changes and they seemed to work really well now. I also think no additional changes are included in the new unified patch - so there should be no difference.
Comment 29 Bernardo Gonzalez Kriegel 2015-11-23 13:42:37 UTC
No problem for my part about authorship, we need to solve this :)
What we do: 3 patches or one?
Comment 30 Brendan Gallagher 2015-11-23 23:08:28 UTC Comment hidden (obsolete)
Comment 31 Katrin Fischer 2015-11-24 00:33:39 UTC
Thx Bernardo and Brendan, I will try to retest this tomorrow for QA - but not sad at all if someone beats me to it :)
Comment 32 Jonathan Druart 2015-11-25 13:16:20 UTC
I will provide a counter patch to simplify this change.
Comment 33 Jonathan Druart 2015-11-25 13:21:37 UTC
Follow-up added on bug 11431 comment 163.
There is no need to specify the collate for the columns.
Comment 34 Jonathan Druart 2015-11-25 13:33:36 UTC
(In reply to Zeno Tajoli from comment #3)
> The second error is present because the update 3.21.00.027 starts so:
> 
> dbh->do(q|
>         INSERT INTO permissions (module_bit, code, description)
>         VALUES (1, 'self_checkout', 'Perform self checkout at the OPAC. It
> should be used for the patron matching the AutoSelfCheckID')
>     |);
> 
> But in 3.20.5 in file  installer/data/mysql/en/mandatory/userpermissions.sql
> I see: 
> 
> INSERT INTO permissions (module_bit, code, description) VALUES
> [...]   
> ( 1, 'self_checkout', 'Perform self checkout at the OPAC. It should be used
> for the patron matching the AutoSelfCheckID'),
> 
> So in 3.20.x the code 'self_checkout' could be present.

Follow-up added on bug 14298.
Comment 35 Jonathan Druart 2015-11-25 13:55:01 UTC Comment hidden (obsolete)
Comment 36 Tomás Cohen Arazi 2015-11-25 14:27:17 UTC Comment hidden (obsolete)
Comment 37 Katrin Fischer 2015-11-25 15:01:31 UTC
Created attachment 45142 [details] [review]
[PASSED QA] Bug 15207: Avoid warnings when adding constraint course_reserves_ibfk_2

Bug 14205 added a new foreign key on the course_reserve table.
As this dbrev has been backported to the stable version (3.20.x), the
next dbrev should take care of this existing constraint to avoid
warnings.

Test plan:
1/ replace the modified dbrev with 3.21.00.XXX to make sure it will be
executed
2/ Make sure you have the KEY and the FK on the course_reserves table:
sql> alter table course_reserves add CONSTRAINT `course_reserves_ibfk_2`
FOREIGN KEY (`ci_id`) REFERENCES `course_items` (`ci_id`) ON DELETE
CASCADE ON UPDATE CASCADE;
sql> alter table course_reserves ADD KEY `course_reserves_ibfk_2` (`ci_id`);

3/ SHOW CREATE TABLE course_reserves
should show that the table is not in sync with the kohastructure.sql file
4/ Execute the dbrev
5/ SHOW CREATE TABLE course_reserves
should show that the table is in sync with the kohastructure.sql file

Repeat 4 and 5, confirm you don't get any error.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 38 Tomás Cohen Arazi 2015-11-25 15:30:57 UTC
Patch pushed to master.

Thanks Zeno, Bernardo, Katrin and Jonathan :-D