Bug 33671 - Database update 22.06.00.048 breaks update process
Summary: Database update 22.06.00.048 breaks update process
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Installation and upgrade (web-based installer) (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Marcel de Rooy
QA Contact: Martin Renvoize
URL:
Keywords: rel_22_11_candidate
: 33118 (view as bug list)
Depends on: 30472
Blocks:
  Show dependency treegraph
 
Reported: 2023-05-03 21:48 UTC by Katrin Fischer
Modified: 2023-12-28 20:47 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.02,23.05.00,22.11.08,22.11.06


Attachments
Bug 33671: Fix for 30472 - Remove null-guarantors (1.75 KB, patch)
2023-05-04 07:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33671: Update POD, remove unneeded check in relationship->guarantor (2.04 KB, patch)
2023-05-04 07:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[PUSHED] Bug 33671: Fix for 30472 - Remove null-guarantors (1.79 KB, patch)
2023-05-09 08:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PUSHED] Bug 33671: Update POD, remove unneeded check in relationship->guarantor (2.07 KB, patch)
2023-05-09 08:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33671: (follow-up) Drop FK constraint during column change (2.57 KB, patch)
2023-07-12 14:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33671: (follow-up) Disable FK check for column change (2.47 KB, patch)
2023-07-17 09:25 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33671: (follow-up) Disable FK check for column change (2.50 KB, patch)
2023-07-17 09:55 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2023-05-03 21:48:55 UTC
We've had 2 reports of this database update breaking the update on bug 30472. The database update altered the table without checking/dealing with existing NULL values:

PabloAB:

Upgrading to 22.11.02-1 from an updated 22.05 on a Debian 11:

Upgrade to 22.06.00.048  [18:33:27]: Bug 30472 - borrower_relationships.guarantor_id NOT NULL
ERROR - {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Cannot change column 'guarantor_id': used in a foreign key constraint 'r_guarantor' at /usr/share/koha/lib/C4/Installer.pm line 739

Maybe the same issue as on bug 32399?

------

Mona Panchaud:

Hello.

While upgrading from 22.05 to 22.11 we are running into the following problem that is related to this change:

"Upgrade to 22.06.00.048 [20:19:53]: Bug 30472 - borrower_relationships.guarantor_id NOT NULL
ERROR: {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Cannot change column 'guarantor_id': used in a foreign key constraint 'r_guarantor' at /usr/share/koha/lib/C4/Installer.pm line 743"

Is there any workaround?
What can we do to get the upgrade working?
Comment 1 Marcel de Rooy 2023-05-04 06:47:02 UTC
To get it working right away, just do this in mysql:

DELETE FROM borrower_relationships WHERE guarantor_id IS NULL

This is just bad data. You cannot add such relations (anymore).
Comment 2 Marcel de Rooy 2023-05-04 06:47:32 UTC
Adding a follow-up to update dbrev accordingly.
Comment 3 Marcel de Rooy 2023-05-04 07:15:57 UTC
Created attachment 150629 [details] [review]
Bug 33671: Fix for 30472 - Remove null-guarantors

These guarantors cannot be added. They are of no use.

NOTE FOR QA/RM: I do not think that it is needed to add a new
db rev for this change since it happens seldom. But if you think
that it is, just copy it to atomicupdate/change bugno.

Test plan:
[1] Check your table definition and modify it:
    alter table borrower_relationships modify column `guarantor_id` int(11) NULL;
[2] Add an empty guarantor:
    insert into borrower_relationships (guarantee_id,relationship) values (SOME_PATRON_ID,'father');
[3] Run the dbrev and verify that the inserted record has been removed.
[4] Check table definition with 'show create table borrower_relationships'.
    You should see NOT NULL with guarantor_id.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 4 Marcel de Rooy 2023-05-04 07:15:59 UTC
Created attachment 150630 [details] [review]
Bug 33671: Update POD, remove unneeded check in relationship->guarantor

[1] Update POD for $patron->guarantor_relationships
[2] Remove check from Patron::Relationship->guarantor

Test plan:
Run t/db_dependent/Koha/Patron.t
Run t/db_dependent/Patron/Relationships.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 5 Marcel de Rooy 2023-05-04 07:16:25 UTC
Trivial: self SO
Comment 6 Mona Panchaud 2023-05-04 11:13:15 UTC
Thanks for taking a look at this and adding a fix!

Just to be sure and for my understanding: The data that is deleted is not used by anything else?

Will this fix get into the 22.11 branch so that we can try the update again in a straightforward way?
Comment 7 Katrin Fischer 2023-05-04 11:27:41 UTC
(In reply to Mona Panchaud from comment #6)
> Thanks for taking a look at this and adding a fix!
> 
> Just to be sure and for my understanding: The data that is deleted is not
> used by anything else?

The new data structure won't allow these to be added, which is why you get the complaint in the update. So it should be safe as this type of data can no longer be generated. 

> Will this fix get into the 22.11 branch so that we can try the update again
> in a straightforward way?

The fix is now going through the QA process and when everything is ok the goal will be to also include in the stable branches.
Comment 8 Jonathan Druart 2023-05-09 08:24:23 UTC
Created attachment 150824 [details] [review]
[PUSHED] Bug 33671: Fix for 30472 - Remove null-guarantors

These guarantors cannot be added. They are of no use.

NOTE FOR QA/RM: I do not think that it is needed to add a new
db rev for this change since it happens seldom. But if you think
that it is, just copy it to atomicupdate/change bugno.

Test plan:
[1] Check your table definition and modify it:
    alter table borrower_relationships modify column `guarantor_id` int(11) NULL;
[2] Add an empty guarantor:
    insert into borrower_relationships (guarantee_id,relationship) values (SOME_PATRON_ID,'father');
[3] Run the dbrev and verify that the inserted record has been removed.
[4] Check table definition with 'show create table borrower_relationships'.
    You should see NOT NULL with guarantor_id.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 9 Jonathan Druart 2023-05-09 08:24:26 UTC
Created attachment 150825 [details] [review]
[PUSHED] Bug 33671: Update POD, remove unneeded check in relationship->guarantor

[1] Update POD for $patron->guarantor_relationships
[2] Remove check from Patron::Relationship->guarantor

Test plan:
Run t/db_dependent/Koha/Patron.t
Run t/db_dependent/Patron/Relationships.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 10 Tomás Cohen Arazi 2023-05-10 17:19:55 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 11 Jonathan Druart 2023-05-11 12:40:47 UTC
*** Bug 33118 has been marked as a duplicate of this bug. ***
Comment 12 Matt Blenkinsop 2023-05-15 12:27:25 UTC
Nice work everyone!

Pushed to stable for 22.11.x
Comment 13 Lucas Gass 2023-06-15 19:01:23 UTC
Missing dependencies for 22.05.x, no backport.
Comment 14 Pablo AB 2023-07-04 22:38:29 UTC
Not sure if it's still mandatory to run before the update the
`DELETE FROM borrower_relationships WHERE guarantor_id IS NULL`, but the errors still happen on the same upgrade, from 22.05 to 22.11, but on Installer.pm line 741.  The procedure continues till the end, resulting in a broken Koha.

If I understand Katrin Fischer comment 7 correctly, these issues shouldn't happen.

Now rereading Gass comment 13, "Missing dependencies for 22.05.x, no backport". 

Why this issue is marked as RESOLVED, if aims directly to 22.05? If the DELETE query solve the issue, couldn't the installer do it transparently?
Comment 15 Marcel de Rooy 2023-07-05 11:21:11 UTC
(In reply to Pablo AB from comment #14)
> Not sure if it's still mandatory to run before the update the
> `DELETE FROM borrower_relationships WHERE guarantor_id IS NULL`, but the
> errors still happen on the same upgrade, from 22.05 to 22.11, but on
> Installer.pm line 741.  The procedure continues till the end, resulting in a
> broken Koha.
> 
> If I understand Katrin Fischer comment 7 correctly, these issues shouldn't
> happen.
> 
> Now rereading Gass comment 13, "Missing dependencies for 22.05.x, no
> backport". 
> 
> Why this issue is marked as RESOLVED, if aims directly to 22.05? If the
> DELETE query solve the issue, couldn't the installer do it transparently?

Hi Pablo,
We cannot help you if you do not provide more details.
Trying to respond to a few of your comments:
The DELETE you mention is now part of the dbrev (220600048). This number 2206 tells you that it is part of 22.11. So when you upgrade from 22.05 to 22.11 you should have it. (Note: obviously this depends on your actual 22.11 version too, use the latest.)
If you would upgrade from X to 22.05, there is no need for this fix. So it was not backported to 22.05.
Comment 16 Marcel de Rooy 2023-07-06 05:51:32 UTC
Pablo responded:
> If the fix of this issue not apply to 22.05, then there is a confusion right from the start. 

This is again a misunderstanding! The fix is part of 22.11. So if you are upgrading from 22.05 to 22.11 you will be affected.
You are referring to 22.11.02. Please use the latest 22.11 now.

> Maybe worth to mention that that Koha was a fresh installation on 20.05, and never touched directly the db, so if there was a NULL guarantor_id on borrower_relationships, Koha create it.
> On a few days I will get the VM rollbacked to check, but on a very similar instance SELECT COUNT(*) FROM borrower_relationships WHERE guarantor_id IS NULL; return zero rows.

Koha does not create guarantors out of the blue.
It should be impossible to have FK constraint error on an empty table.

Any comments on Koha version, SQL version, SHOW CREATE TABLE borrower_relationships, and its records, etc. may help us further. Please repeat with the latest 22.11 and report back.
Comment 17 Pablo AB 2023-07-06 19:33:05 UTC
I'm upgrading on a Debian 11 from Koha 22.05.12-1 to 22.11, and it is in this upgrade/transition where I came across this issue. I can't reach 22.11 due to this.

SELECT version()\G
version(): 10.1.41-MariaDB-0+deb9u1

SELECT * FROM borrower_relationships;
Empty set (0.000 sec)

SHOW CREATE TABLE borrower_relationships\G
*************************** 1. row ***************************
       Table: borrower_relationships
Create Table: CREATE TABLE `borrower_relationships` (
  `id` int(11) NOT NULL AUTO_INCREMENT,
  `guarantor_id` int(11) DEFAULT NULL,
  `guarantee_id` int(11) NOT NULL,
  `relationship` varchar(100) COLLATE utf8mb4_unicode_ci NOT NULL,
  PRIMARY KEY (`id`),
  UNIQUE KEY `guarantor_guarantee_idx` (`guarantor_id`,`guarantee_id`),
  KEY `r_guarantee` (`guarantee_id`),
  CONSTRAINT `r_guarantee` FOREIGN KEY (`guarantee_id`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE CASCADE ON UPDATE CASCADE,
  CONSTRAINT `r_guarantor` FOREIGN KEY (`guarantor_id`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE CASCADE ON UPDATE CASCADE
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci


The DELETE I mention is only based on comment 1 of this issue.

I originally wrote bug 33118, marked as duplicate of this one.
Comment 18 Marcel de Rooy 2023-07-07 06:05:28 UTC
(In reply to Pablo AB from comment #17)

> SELECT version()\G
> version(): 10.1.41-MariaDB-0+deb9u1

I would recommend to upgrade your MariaDB server to the level that would be normal for Debian 11. This is older.

> SELECT * FROM borrower_relationships;
> Empty set (0.000 sec)

So there should be really no harm in adding a FK.

> SHOW CREATE TABLE borrower_relationships\G
> *************************** 1. row ***************************
>        Table: borrower_relationships
> Create Table: CREATE TABLE `borrower_relationships` (
>   `id` int(11) NOT NULL AUTO_INCREMENT,
>   `guarantor_id` int(11) DEFAULT NULL,
>   `guarantee_id` int(11) NOT NULL,
>   `relationship` varchar(100) COLLATE utf8mb4_unicode_ci NOT NULL,
>   PRIMARY KEY (`id`),
>   UNIQUE KEY `guarantor_guarantee_idx` (`guarantor_id`,`guarantee_id`),
>   KEY `r_guarantee` (`guarantee_id`),
>   CONSTRAINT `r_guarantee` FOREIGN KEY (`guarantee_id`) REFERENCES
> `borrowers` (`borrowernumber`) ON DELETE CASCADE ON UPDATE CASCADE,
>   CONSTRAINT `r_guarantor` FOREIGN KEY (`guarantor_id`) REFERENCES
> `borrowers` (`borrowernumber`) ON DELETE CASCADE ON UPDATE CASCADE
> ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci

This looks perfectly fine.

Could you test this on the mysql command line too:
ALTER TABLE borrower_relationships CHANGE COLUMN guarantor_id guarantor_id int(11) NOT NULL
And revert again with:
ALTER TABLE borrower_relationships CHANGE COLUMN guarantor_id guarantor_id int(11) DEFAULT NULL
What is the error message, code ?

It would not surprise me if it would not fail for a higher MariaDB ?
Comment 19 Pablo AB 2023-07-10 03:43:24 UTC
>> version(): 10.1.41-MariaDB-0+deb9u1
> I would recommend to upgrade your MariaDB server to the level that would be normal for Debian 11. This is older.

DB server is on a different host and managed by a different sector. 

ALTER TABLE borrower_relationships CHANGE COLUMN guarantor_id guarantor_id int(11) NOT NULL;
ERROR 1832 (HY000): Cannot change column 'guarantor_id': used in a foreign key constraint 'r_guarantor'

I would try to upgrade the MariaDB and test again. At least we find out that this error is not only caused by bad data.
Comment 20 Marcel de Rooy 2023-07-12 13:53:58 UTC
(In reply to Pablo AB from comment #19)
> ALTER TABLE borrower_relationships CHANGE COLUMN guarantor_id guarantor_id
> int(11) NOT NULL;
> ERROR 1832 (HY000): Cannot change column 'guarantor_id': used in a foreign
> key constraint 'r_guarantor'

Ah. Thats helpful. I think we'd better change the dbrev. Remove the constraint, change column and add constraint back again. You could try that manually too.
Comment 21 Marcel de Rooy 2023-07-12 14:36:54 UTC Comment hidden (obsolete)
Comment 22 Marcel de Rooy 2023-07-12 14:38:39 UTC Comment hidden (obsolete)
Comment 23 Marcel de Rooy 2023-07-12 14:49:34 UTC
Testers: ONLY apply the LAST patch please!
Comment 24 Tomás Cohen Arazi 2023-07-13 14:17:26 UTC
I got the same error updating on a MariaDB 10.4-run system, but with DBRev 211200048. And I think the right solution is to lift the FK checks (in that case, it is just renaming a FK column, not adding a fk, but the error was the same).
Comment 25 Marcel de Rooy 2023-07-17 09:25:10 UTC
Created attachment 153533 [details] [review]
Bug 33671: (follow-up) Disable FK check for column change

See also BZ report. As suggested by Tomas.

NOTE: The dbrev is extended too for removing records with a failing guarantor_id
foreign key (theoretically not present obviously).

Test plan:
1) cp installer/data/mysql/db_revs/220600048.pl installer/data/mysql/atomicupdate/
2) Run updatedatabase.pl
3) Remove copied file

Bonus:
Manually remove FK constraint before running updatedatabase.
Insert a record with bad guarantor_id into relationships manually.
Run the update again.
You should not see the message that the constraint is removed.
Is the bad record count reported and record removed?

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
[EDIT] Simplified. No longer removes the FK. Just disables during ALTER.
Comment 26 Marcel de Rooy 2023-07-17 09:26:42 UTC
(In reply to Tomás Cohen Arazi from comment #24)
> I got the same error updating on a MariaDB 10.4-run system, but with DBRev
> 211200048. And I think the right solution is to lift the FK checks (in that
> case, it is just renaming a FK column, not adding a fk, but the error was
> the same).

Patch adjusted accordingly. Thanks.
Moved to SO queue. Would be nice to have a QA stamp still.
Tested myself and got feedback from Pablo and Tomas.
Comment 27 Martin Renvoize 2023-07-17 09:55:44 UTC
Created attachment 153536 [details] [review]
Bug 33671: (follow-up) Disable FK check for column change

See also BZ report. As suggested by Tomas.

NOTE: The dbrev is extended too for removing records with a failing guarantor_id
foreign key (theoretically not present obviously).

Test plan:
1) cp installer/data/mysql/db_revs/220600048.pl installer/data/mysql/atomicupdate/
2) Run updatedatabase.pl
3) Remove copied file

Bonus:
Manually remove FK constraint before running updatedatabase.
Insert a record with bad guarantor_id into relationships manually.
Run the update again.
You should not see the message that the constraint is removed.
Is the bad record count reported and record removed?

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
[EDIT] Simplified. No longer removes the FK. Just disables during ALTER.
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 28 Martin Renvoize 2023-07-17 09:56:09 UTC
Working for me too, thanks for the follow-up Marcel.
Comment 29 Tomás Cohen Arazi 2023-07-18 17:09:04 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 30 Martin Renvoize 2023-07-19 08:30:43 UTC
Thanks for all the hard work!

Pushed to 23.05.x for the next release
Comment 31 Matt Blenkinsop 2023-07-19 10:15:31 UTC
Nice work everyone!

Pushed to oldstable for 22.11.x
Comment 32 Jeremiah 2023-07-20 05:31:17 UTC
Hello,

I updated the koha files on 7/19/2023.
I am encountering the same error:

Upgrade to 22.06.00.048 [22:23:00]: Bug 30472 - borrower_relationships.guarantor_id NOT NULL
ERROR: {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Cannot change column 'guarantor_id': used in a foreign key constraint 'r_guarantor' at /usr/share/koha/lib/C4/Installer.pm line 741

Based on the bug comments, it looks like this is something in the works.
Any solutions I can try in the meantime?

Any help is appreciated.
Comment 33 Marcel de Rooy 2023-07-20 05:38:10 UTC
(In reply to Jeremiah from comment #32)
> Hello,
> 
> I updated the koha files on 7/19/2023.
> I am encountering the same error:
> 
> Upgrade to 22.06.00.048 [22:23:00]: Bug 30472 -
> borrower_relationships.guarantor_id NOT NULL
> ERROR: {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Cannot change
> column 'guarantor_id': used in a foreign key constraint 'r_guarantor' at
> /usr/share/koha/lib/C4/Installer.pm line 741
> 
> Based on the bug comments, it looks like this is something in the works.
> Any solutions I can try in the meantime?
> 
> Any help is appreciated.

I guess you missed the latest change on this dbrev.
You could apply the last follow-up to your install or pick the SQL statements from the last patch and run them under mysql client.

Especially:
SET FOREIGN_KEY_CHECKS=0;
ALTER TABLE borrower_relationships CHANGE COLUMN guarantor_id guarantor_id int(11) NOT NULL
SET FOREIGN_KEY_CHECKS=1
Comment 34 Jeremiah 2023-07-20 18:40:25 UTC
That worked,

Thank you!
Comment 35 Pablo AB 2023-08-11 17:01:54 UTC
I upgraded MariaDB from 10.1 to 10.6 and tried to upgrade (from 22.05) again. I successfully passed through the `Bug 30472` section!
Still having issues, now on Bug 24860 (Can't create table `koha_myinstance`.`reserves` (errno: 150 "Foreign key constraint is incorrectly formed")). I'll file a new bug.