Bug 33671

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).

Adding a follow-up to update dbrev accordingly.

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>

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>

Trivial: self SO

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?

(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.

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>

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>

Pushed to master for 23.05.

Nice work everyone, thanks!

*** Bug 33118 has been marked as a duplicate of this bug. ***

Nice work everyone!

Pushed to stable for 22.11.x

Missing dependencies for 22.05.x, no backport.

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?

(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.

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.

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.

(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 ?

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

(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.

Testers: ONLY apply the LAST patch please!

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).

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.

(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.

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>

Working for me too, thanks for the follow-up Marcel.

Pushed to master for 23.11.

Nice work everyone, thanks!

Thanks for all the hard work!

Pushed to 23.05.x for the next release

Nice work everyone!

Pushed to oldstable for 22.11.x

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.

(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

That worked,

Thank you!

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.