Bug 15207

Summary: Error on upgrade from 3.20.5 to 3.22 beta
Product: Koha Reporter: Zeno Tajoli <ztajoli>
Component: Installation and upgrade (web-based installer)Assignee: Jonathan Druart <jonathan.druart+koha>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: bgkriegel, jonathan.druart+koha, katrin.fischer, nick, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11431
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14298
Change sponsored?: Sponsored Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 14205    
Bug Blocks:    
Attachments: patch for fix update
Warnings after patch
New version of patch for fix update
[SIGNED-OFF] Bug 15207: To not show errors on update from 3.20.x to 3.22.x
Bug 15207: (followup) add collate info to sound alerts table
Bug 15207: Follow-up - change check for existing index
Not show errors on update from 3.20
Not show errors on update from 3.20
To not show errors on update from 3.20.x to 3.22.x
Bug 15207: Avoid warnings when adding constraint course_reserves_ibfk_2
Bug 15207: Avoid warnings when adding constraint course_reserves_ibfk_2
[PASSED QA] Bug 15207: Avoid warnings when adding constraint course_reserves_ibfk_2

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 Mark 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