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.
Created attachment 180887 [details] [review] Bug 39618: Add index to borrowers.preferred_name column To test: 1. APPLY PATCH 2. updatedatabase
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?
Created attachment 180926 [details] [review] Bug 39618: Add KEY to kohastructure.sql
(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.
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
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!
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.
(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!
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.
Created attachment 181528 [details] [review] Bug 39618: Add index to borrowers.preferred_name column To test: 1. APPLY PATCH 2. updatedatabase
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>
I feel like this is bordering on a bugfix, would you agree?
(In reply to Katrin Fischer from comment #12) > I feel like this is bordering on a bugfix, would you agree? Yes, I agree.
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>
Pushed for 25.05! Well done everyone, thank you!
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) )
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.
Hello, the operative system is Debian GNU/Linux 11 (bullseye) and the DBMS is 8.0.31 MySQL Community Server - GPL.
(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)
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.
Created attachment 182247 [details] [review] Bug 39618: (QA follow-up) add prefix length to index key
Hopefully I've added that right... Note you can use "show indexes from borrowers" to double-check the "Sub_part" length for the indexes.
Follow-up pushed to main. @David: can you explain about your choice of 768?
(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
Bug 28267 changed it so that we require DYNAMIC rows so 768 again makes the most sense I reckon.
Follow-up pushed, explanation provided - removing keyword.