Prio Notes:

Prio note: We’re prioritizing this above T362084, despite the lower “score”, because a database performance issue seems more important to us.

Given that this table is secondary data with the ability to self heal after reparse

I’m skeptical of this proclaimed ability; one important gap I’m aware of is that, as far as I know, we never remove “old” unused text entries from the storage – we rely on them being removed as soon as they become unused (which requires some locking tricks to make sure nothing breaks if a concurrent transaction re-uses the text entry). IMHO this needs more investigation before we can drop the locking.

It’s possible that we should change the approach we take to ensure people can’t see deleted terms in the cloud replicas / on Quarry (which is the main reason we care about cleaning them up, I think: we don’t want revdeleted labels with PII to stay visible there). Currently, we try pretty hard to actually delete the terms from the secondary storage, but this causes all sorts of locking headaches (T283198) and occasionally missing data (T309445). It might be better to never¹ delete terms from the primary storage, and instead just change the cloud replicas views to only return data that’s referenced (recursively) from wbt_item_terms or wbt_property_terms – shifting the responsibility of hiding sensitive data from the production databases to the cloud replicas. (I think I’ve commented something like this on another task before, but I can’t find it now.)

¹ (Or if we’re worried about the size of the secondary storage exploding – somehow still delete terms, but only if it seems like they’re not used and won’t be used. Maybe a job can scan for unused IDs, submit them to a second job to run a week later, and the second job actually deletes them if they’re still unused?)

I agree that the pruning part needs to be rethought and revamped but I'm failing to see how it's connected to this problem. This is explicitly about locking the rows in wbt_item_terms which is the head. Let's say for Q1234 you have this:

+------------+--------------+----------------------+
| wbit_id    | wbit_item_id | wbit_term_in_lang_id |
+------------+--------------+----------------------+
|  767752395 |     1234 |                    5 |
| 6682347558 |    1234 |                  172 |
|   95594787 |      1234 |                  192 |
|   34102141 |       1234 |                  237 |
|  561818268 |     1234 |                  274 |
+------------+--------------+----------------------+

And if you don't lock the rows in wbt_item_terms, and if all of these get corrupted and deleted and as result entries in wbt_term_in_lang also get deleted (and even entries in wbt_text_in_lang also get deleted). A reparse should fix all of this and reinsert any missing entries in all of the tables if they get deleted by the prune job. i.e. even if you corrupt from the head to the leaf via race conditions on the deletion job. It still should recover with reparse, from my understanding of how it is built. What am I missing here?

Also noting that I'm talking about the lock in ::acquireAndInsertTerms() and not the one on ::deleteTermsWithoutClean() as the latter happens much less frequently and doesn't lock contention from what I'm seeing.

Change #1050658 had a related patch set uploaded (by Hoo man; author: Hoo man):

[mediawiki/extensions/Wikibase@master] Don't use FOR UPDATE in acquireAndInsertTerms

https://gerrit.wikimedia.org/r/1050658

Change #1054359 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@master] DatabaseTermStoreWriterBase: Clarify best effort basis

https://gerrit.wikimedia.org/r/1054359

Change #1050658 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] Don't use FOR UPDATE in acquireAndInsertTerms

https://gerrit.wikimedia.org/r/1050658

Given that this table is secondary data with the ability to self heal after reparse

Minor correction: AFAICT we only update the term store on actual edits, not on e.g. ?action=purge or a “null edit” (I tested action=wbeditentity with data={}).

Moving back to review for the follow-up clarification patch.

Meanwhile, the specific query vanished from the slow queries dashboard (https://logstash.wikimedia.org/goto/e6310eb39c862c3180e927bf3109b01a):

Change #1054359 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] DatabaseTermStoreWriterBase: Clarify best effort basis

https://gerrit.wikimedia.org/r/1054359

If you measure the "write" times only (issues on the master vs. random replicas being slow), the change is much more visible https://logstash.wikimedia.org/goto/0474b926f6f8cf84846ec87b38335f53