Bug 33368 - borrowers.flags is about to reach the limit
Summary: borrowers.flags is about to reach the limit
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low critical (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-03-30 07:54 UTC by Jonathan Druart
Modified: 2023-12-28 20:44 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.05.00,22.11.05


Attachments
Bug 33368: Extends borrowers.flags to bigint (4.76 KB, patch)
2023-03-30 08:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33368: DBIC (3.78 KB, patch)
2023-03-30 08:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33368: Extends borrowers.flags to bigint (4.81 KB, patch)
2023-03-30 08:11 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33368: DBIC (3.83 KB, patch)
2023-03-30 08:11 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33368: Extends borrowers.flags to bigint (4.86 KB, patch)
2023-04-05 00:03 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 33368: DBIC (3.89 KB, patch)
2023-04-05 00:03 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 33368: Extends borrowers.flags to bigint (4.92 KB, patch)
2023-04-06 15:42 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 33368: DBIC (3.95 KB, patch)
2023-04-06 15:42 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2023-03-30 07:54:32 UTC
it's a int(11) which means its limit is 2^31

We have currently 29 permissions, there is only one remaining, at 31 permissions the permission system won't support a case where a patron has all permissions.
Comment 1 Jonathan Druart 2023-03-30 08:02:34 UTC
Created attachment 148952 [details] [review]
Bug 33368: Extends borrowers.flags to bigint

it's a int(11) which means its limit is 2^31

We have currently 29 permissions, there is only one remaining, at 31 permissions
the permission system won't support a case where a patron has all permissions.

Test plan:
Don't apply this patch
Add another permission
INSERT INTO userflags (bit, flag, flagdesc, defaulton) VALUES (30, 'new permission 1', '', 0) ;
Go to the interface, tick all the boxes
select flags from borrowers where borrowernumber=5;
=> 2113928830
Remember:
2^31  2147483648
2^32  4294967296

Add another one
INSERT INTO userflags (bit, flag, flagdesc, defaulton) VALUES (31, 'new permission 2', '', 0) ;
Repeat the step before
=> boom
CGI::Compile::ROOT::kohadevbox_koha_members_member_2dflags_2epl::member_2dflags_2epl(): DBI Exception: DBD::mysql::st execute failed: Out of range value for column 'flags' at row 1 at /usr/share/perl5/CGI/Emulate/PSGI.pm line 30

Apply the patch, try again

QA note: That's ofc not the best solution, but the easiest for now
Comment 2 Jonathan Druart 2023-03-30 08:02:37 UTC
Created attachment 148953 [details] [review]
Bug 33368: DBIC
Comment 3 Nick Clemens 2023-03-30 08:11:54 UTC
Created attachment 148954 [details] [review]
Bug 33368: Extends borrowers.flags to bigint

it's a int(11) which means its limit is 2^31

We have currently 29 permissions, there is only one remaining, at 31 permissions
the permission system won't support a case where a patron has all permissions.

Test plan:
Don't apply this patch
Add another permission
INSERT INTO userflags (bit, flag, flagdesc, defaulton) VALUES (30, 'new permission 1', '', 0) ;
Go to the interface, tick all the boxes
select flags from borrowers where borrowernumber=5;
=> 2113928830
Remember:
2^31  2147483648
2^32  4294967296

Add another one
INSERT INTO userflags (bit, flag, flagdesc, defaulton) VALUES (31, 'new permission 2', '', 0) ;
Repeat the step before
=> boom
CGI::Compile::ROOT::kohadevbox_koha_members_member_2dflags_2epl::member_2dflags_2epl(): DBI Exception: DBD::mysql::st execute failed: Out of range value for column 'flags' at row 1 at /usr/share/perl5/CGI/Emulate/PSGI.pm line 30

Apply the patch, try again

QA note: That's ofc not the best solution, but the easiest for now

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 4 Nick Clemens 2023-03-30 08:11:57 UTC
Created attachment 148955 [details] [review]
Bug 33368: DBIC

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 5 Victor Grousset/tuxayo 2023-04-05 00:03:06 UTC
Created attachment 149150 [details] [review]
Bug 33368: Extends borrowers.flags to bigint

it's a int(11) which means its limit is 2^31

We have currently 29 permissions, there is only one remaining, at 31 permissions
the permission system won't support a case where a patron has all permissions.

Test plan:
Don't apply this patch
Add another permission
INSERT INTO userflags (bit, flag, flagdesc, defaulton) VALUES (30, 'new permission 1', '', 0) ;
Go to the interface, tick all the boxes
select flags from borrowers where borrowernumber=5;
=> 2113928830
Remember:
2^31  2147483648
2^32  4294967296

Add another one
INSERT INTO userflags (bit, flag, flagdesc, defaulton) VALUES (31, 'new permission 2', '', 0) ;
Repeat the step before
=> boom
CGI::Compile::ROOT::kohadevbox_koha_members_member_2dflags_2epl::member_2dflags_2epl(): DBI Exception: DBD::mysql::st execute failed: Out of range value for column 'flags' at row 1 at /usr/share/perl5/CGI/Emulate/PSGI.pm line 30

Apply the patch, try again

QA note: That's ofc not the best solution, but the easiest for now

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 6 Victor Grousset/tuxayo 2023-04-05 00:03:08 UTC
Created attachment 149151 [details] [review]
Bug 33368: DBIC

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 7 Victor Grousset/tuxayo 2023-04-05 00:13:01 UTC
It works! However I don't get why because bigint(11) still has 11 for the number of digits. But it works up to at least 40 bits/flags. And MariaDB's doc doesn't have anything suspicious like the number for bigint being there for compatibility/historical reasons and not being used.
https://mariadb.com/kb/en/bigint/

Anyway, second signoff added so it can still be used as a signoff and Nick can pass QA if they know the answer to the above and don't want to wait.
Comment 8 Tomás Cohen Arazi 2023-04-05 01:17:13 UTC
(In reply to Victor Grousset/tuxayo from comment #7)
> It works! However I don't get why because bigint(11) still has 11 for the
> number of digits. But it works up to at least 40 bits/flags. And MariaDB's
> doc doesn't have anything suspicious like the number for bigint being there
> for compatibility/historical reasons and not being used.
> https://mariadb.com/kb/en/bigint/
> 
> Anyway, second signoff added so it can still be used as a signoff and Nick
> can pass QA if they know the answer to the above and don't want to wait.

(11) i.e. the 'display width' is not relevant to the storage size, which is the problem here. Any INT data type will have the same known limit.

It is only relevant if you define things like zero  padding, and then you know it will fill with zeroes up to 11 digits.

For reference: https://dev.mysql.com/doc/refman/5.7/en/numeric-type-syntax.html#:~:text=The%20maximum%20display%20width%20is,digits%20that%20can%20be%20stored.
Comment 9 Tomás Cohen Arazi 2023-04-06 15:42:47 UTC
Created attachment 149239 [details] [review]
Bug 33368: Extends borrowers.flags to bigint

it's a int(11) which means its limit is 2^31

We have currently 29 permissions, there is only one remaining, at 31 permissions
the permission system won't support a case where a patron has all permissions.

Test plan:
Don't apply this patch
Add another permission
INSERT INTO userflags (bit, flag, flagdesc, defaulton) VALUES (30, 'new permission 1', '', 0) ;
Go to the interface, tick all the boxes
select flags from borrowers where borrowernumber=5;
=> 2113928830
Remember:
2^31  2147483648
2^32  4294967296

Add another one
INSERT INTO userflags (bit, flag, flagdesc, defaulton) VALUES (31, 'new permission 2', '', 0) ;
Repeat the step before
=> boom
CGI::Compile::ROOT::kohadevbox_koha_members_member_2dflags_2epl::member_2dflags_2epl(): DBI Exception: DBD::mysql::st execute failed: Out of range value for column 'flags' at row 1 at /usr/share/perl5/CGI/Emulate/PSGI.pm line 30

Apply the patch, try again

QA note: That's ofc not the best solution, but the easiest for now

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 10 Tomás Cohen Arazi 2023-04-06 15:42:50 UTC
Created attachment 149240 [details] [review]
Bug 33368: DBIC

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 11 Tomás Cohen Arazi 2023-04-06 15:53:15 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 12 Victor Grousset/tuxayo 2023-04-08 03:00:20 UTC
(In reply to Tomás Cohen Arazi from comment #8)
> (11) i.e. the 'display width' 

ok!!! Now I really get this part from MariaDB's doc: «If a column has been set to ZEROFILL, all values will be prepended by zeros so that the BIGINT value contains a number of M digits.»

> is not relevant to the storage size,

I though it was to have some business domain rules enforced in the DB so it was on top of storage constraints.



> For reference:
> https://dev.mysql.com/doc/refman/5.7/en/numeric-type-syntax.html#:~:
> text=The%20maximum%20display%20width%20is,digits%20that%20can%20be%20stored.

thanks, it's clearer
Comment 13 Jacob O'Mara 2023-04-16 20:38:07 UTC
Nice work, thanks everyone!

Pushed to 22.11.x for the next release.