Bug 39618 - Add a non-unique index/key to borrowers table for preferred_name
Summary: Add a non-unique index/key to borrowers table for preferred_name
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Nick Clemens (kidclamp)
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2025-04-11 20:33 UTC by Lucas Gass (lukeg)
Modified: 2025-05-16 15:21 UTC (History)
4 users (show)

See Also:
GIT URL:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
25.05.00
Circulation function:


Attachments
Bug 39618: Add index to borrowers.preferred_name column (1.40 KB, patch)
2025-04-11 20:42 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 39618: Add KEY to kohastructure.sql (1.82 KB, patch)
2025-04-14 18:12 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 39618: Add index to borrowers.preferred_name column (2.22 KB, patch)
2025-04-25 13:24 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 39618: Add index to borrowers.preferred_name column (2.26 KB, patch)
2025-04-25 20:07 UTC, Roman Dolny
Details | Diff | Splinter Review
Bug 39618: Add index to borrowers.preferred_name column (2.33 KB, patch)
2025-05-02 15:22 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 39618: (QA follow-up) add prefix length to index key (1.76 KB, patch)
2025-05-11 23:52 UTC, David Cook
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lucas Gass (lukeg) 2025-04-11 20:33:22 UTC
We have seen some slowdown in patron searches since upgrading sites to 24.11 that seem to be tied to the introduction of preferred_name. 

Adding a key for preferred_name seems to speed up queries by quite a bit when filtering or sorting by preferred_name. 

This has been most notable in the patron autocomplete where queries went from over 5 seconds without, to .2 seconds with.
Comment 1 Lucas Gass (lukeg) 2025-04-11 20:42:18 UTC
Created attachment 180887 [details] [review]
Bug 39618: Add index to borrowers.preferred_name column

To test:
1. APPLY PATCH
2. updatedatabase
Comment 2 David Cook 2025-04-13 23:58:26 UTC
That's really interesting.

Can you explain the patch a bit more? 

Marking Failed QA since the patch is missing a change for kohastructure.sql. I double-checked main and it doesn't look like that index already exists there.

Also, in the atomic update, it only adds the index if the "debit_id" index doesn't exist in the "article_requests" table? I suspect that must be a mistake?
Comment 3 Lucas Gass (lukeg) 2025-04-14 18:12:08 UTC
Created attachment 180926 [details] [review]
Bug 39618: Add KEY to kohastructure.sql
Comment 4 Lucas Gass (lukeg) 2025-04-14 18:20:47 UTC
(In reply to David Cook from comment #2)
> That's really interesting.
> 
> Can you explain the patch a bit more? 

 Without an index a search on the borrowers table for 'Bri', 'Bria', or 'Brian' will read through every row checking each 1 by 1. With an index the DB lookup starts with 'Bri', 'Bria', or 'Brian' as a starting point and is much faster. 

I mention 'Bri', 'Bria', or 'Brian' because these may all be lookups w/ the autocomplete feature as you continue to type the name 'Brian', letter by letter.
Comment 5 Nick Clemens (kidclamp) 2025-04-14 18:26:23 UTC
Essentially we found that if libraries add 'preferred_name' to DefaultPatronSearchFields system preference it slows down the sql queries


SIMPLE EXAMPLE
MariaDB [koha_kohadev]> SELECT COUNT(*) FROM borrowers WHERE preferred_name LIKE 'aaron%';
+----------+
| COUNT(*) |
+----------+
|      992 |
+----------+
1 row in set (0.052 sec)

MariaDB [koha_kohadev]> ALTER TABLE `borrowers` ADD KEY `preferred_name` (`preferred_name`);
Query OK, 0 rows affected, 1 warning (0.155 sec)
Records: 0  Duplicates: 0  Warnings: 1

MariaDB [koha_kohadev]> SELECT COUNT(*) FROM borrowers WHERE preferred_name LIKE 'aaron%';
+----------+
| COUNT(*) |
+----------+
|      992 |
+----------+
1 row in set (0.002 sec)

MariaDB [koha_kohadev]> 


In production with real Koha generated queries it went from several seconds per search to under 1 second per search
Comment 6 David Cook 2025-04-14 22:54:03 UTC
Sorry folks. I understood what Lucas said in the Description. I just wanted to know if I was actually seeing mistakes in the first patch or if there was a reason the kohastructure.sql change was missing and the index_exists() was different to what I expected. I should've worded my comment differently. My bad!
Comment 7 David Cook 2025-04-14 22:56:38 UTC
I love index related patches haha. Recently, on a different project, adding one (partial) index took a data heavy query down from 2 minutes to 1 second in some cases. 

I'll QA this later today once I've gotten through my emails.
Comment 8 Lucas Gass (lukeg) 2025-04-14 23:49:16 UTC
(In reply to David Cook from comment #6)
> Sorry folks. I understood what Lucas said in the Description. I just wanted
> to know if I was actually seeing mistakes in the first patch or if there was
> a reason the kohastructure.sql change was missing and the index_exists() was
> different to what I expected. I should've worded my comment differently. My
> bad!

Nope, that was a lazy mistake on my part. My bad!
Comment 9 David Cook 2025-04-17 07:09:25 UTC
Oops I thought this was already signed off. I guess I meant sign off rather than QA.

This is looking good at a glance.

I do notice that it doesn't follow the pattern of most of the other indexes (ie it's missing the "_idx" suffix at the end of the index name), but that's fine I think. Not a real issue.

My test plan:
0. Apply the patch
1. sudo koha-mysql kohadev
2. show indexes from borrowers
3. sudo koha-upgrade-schema kohadev
NOTE successful database update
4. sudo koha-mysql kohadev
5. show indexes from borrowers
NOTE indexes added to table
6. sudo koha-upgrade-schema kohadev
NOTE It says it's added the index again (but it hasn't - phew)
7. sudo koha-mysql kohadev
8. show indexes from borrowers
NOTE indexes hasn't been added a second time

Sorry to say this but I think I am going to mark "Failed QA" again, as I think that "say_success" in the atomic update shouldn't be shown if the database update hasn't been applied.
Comment 10 Nick Clemens (kidclamp) 2025-04-25 13:24:07 UTC
Created attachment 181528 [details] [review]
Bug 39618: Add index to borrowers.preferred_name column

To test:
1. APPLY PATCH
2. updatedatabase
Comment 11 Roman Dolny 2025-04-25 20:07:07 UTC
Created attachment 181555 [details] [review]
Bug 39618: Add index to borrowers.preferred_name column

To test:
1. APPLY PATCH
2. updatedatabase

Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Comment 12 Katrin Fischer 2025-04-28 16:13:28 UTC
I feel like this is bordering on a bugfix, would you agree?
Comment 13 Lucas Gass (lukeg) 2025-04-28 16:25:45 UTC
(In reply to Katrin Fischer from comment #12)
> I feel like this is bordering on a bugfix, would you agree?

Yes, I agree.
Comment 14 Martin Renvoize (ashimema) 2025-05-02 15:22:29 UTC
Created attachment 181872 [details] [review]
Bug 39618: Add index to borrowers.preferred_name column

To test:
1. APPLY PATCH
2. updatedatabase

Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 15 Katrin Fischer 2025-05-06 16:54:04 UTC
Pushed for 25.05!

Well done everyone, thank you!
Comment 16 Adolfo Rodríguez Taboada 2025-05-09 09:08:04 UTC
This patch returned the following error in my local instance.

ERROR: {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: BLOB/TEXT column 'preferred_name' used in key specification without a key length 

The field preferred_name is defined as longtext so I understand the key for  preferred_name should have a length like the index of middle_name

CREATE INDEX middle_name_idx ON borrowers ( middle_name(768) )
Comment 17 Katrin Fischer 2025-05-09 10:49:58 UTC
Hi Adolfo, can you please tell us the OS, DBMS and version of your DBMS? I reckon there is some version difference at play here as it worked well in my tests.
Comment 18 Adolfo Rodríguez Taboada 2025-05-09 11:07:23 UTC
Hello, the operative system is Debian GNU/Linux 11 (bullseye) and the DBMS is 8.0.31 MySQL Community Server - GPL.
Comment 19 David Cook 2025-05-11 23:33:31 UTC
(In reply to Adolfo Rodríguez Taboada from comment #16)
> This patch returned the following error in my local instance.
> 
> ERROR: {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: BLOB/TEXT column
> 'preferred_name' used in key specification without a key length 
> 
> The field preferred_name is defined as longtext so I understand the key for 
> preferred_name should have a length like the index of middle_name
> 
> CREATE INDEX middle_name_idx ON borrowers ( middle_name(768) )

I think that Adolfo makes a good point. At the very least, we should be consistent in the adding of indexes, and middle name is a good one to copy.

On a fairly new KTD, I dropped "preferred_name_idx" and re-added it. While there wasn't a noisy error message, I did notice that there was a warning in KTD:

MariaDB [koha_kohadev]> ALTER TABLE `borrowers` ADD KEY `preferred_name_idx` (`preferred_name`);
Query OK, 0 rows affected, 1 warning (0.050 sec)
Records: 0  Duplicates: 0  Warnings: 1

MariaDB [koha_kohadev]> show warnings;
+-------+------+----------------------------------------------------------+
| Level | Code | Message                                                  |
+-------+------+----------------------------------------------------------+
| Note  | 1071 | Specified key was too long; max key length is 3072 bytes |
+-------+------+----------------------------------------------------------+
1 row in set (0.000 sec)
Comment 20 David Cook 2025-05-11 23:47:14 UTC
I just tried using the web installer and it hides the warning. 

--

In KTD we're using STRICT_TRANS_TABLES which should be activating strict mode. According to MySQL 8.4 website: "Strict mode produces an error for attempts to create a key that exceeds the maximum key length. When strict mode is not enabled, this results in a warning and truncation of the key to the maximum key length."

--

I can't find any useful information on the MariaDB website about it. 

--

In any case, we should add a follow-up for this one to bring it in line with our other indexes and prevent issues for people.
Comment 21 David Cook 2025-05-11 23:52:05 UTC
Created attachment 182247 [details] [review]
Bug 39618: (QA follow-up) add prefix length to index key
Comment 22 David Cook 2025-05-11 23:52:40 UTC
Hopefully I've added that right...

Note you can use "show indexes from borrowers" to double-check the "Sub_part" length for the indexes.
Comment 23 Katrin Fischer 2025-05-12 15:42:37 UTC
Follow-up pushed to main.

@David: can you explain about your choice of 768?
Comment 24 David Cook 2025-05-13 02:15:13 UTC
(In reply to Katrin Fischer from comment #23)
> Follow-up pushed to main.
> 
> @David: can you explain about your choice of 768?

It's two-fold.

First, middle_name and other longtext column indexes are configured the same way.

Second, it's what MySQL says to do: "For InnoDB tables that use COMPRESSED or DYNAMIC row format, index key prefixes longer than 767 bytes (up to 3072 bytes) are permitted. Tables created with these row formats enable you to index a maximum of 1024 or 768 characters for utf8mb3 or utf8mb4 columns, respectively."

768 characters at 4 bytes each = 3072 total bytes

https://dev.mysql.com/doc/refman/8.4/en/charset-unicode-conversion.html
Comment 25 David Cook 2025-05-13 02:17:45 UTC
Bug 28267 changed it so that we require DYNAMIC rows so 768 again makes the most sense I reckon.
Comment 26 Katrin Fischer 2025-05-16 15:21:31 UTC
Follow-up pushed, explanation provided - removing keyword.