Please stick to 80 char lines, and please format this like other SQL queries in our codebase

Can we make the error more clear if no result was matched?

Same feedback about SQL query

It's unclear this boolean indicates at the callsite. Can we instead take something like this?

olmAccountType: 'primary' | 'notifs',

FWIW the TablePlus "Beautify" command (⌘+I) and DataGrip "Format" command (⌘+⌥+L) format SQL queries with code style that pretty much matches ours

(they don't handle capitalization though)

Next diffs in this stack will call it in a responder.

As discussed in our 1:1, I'd like to avoid this pattern of having the START TRANSACTION in one query, and having the transaction closed in a separate query
Instead, the plan was to have a single query that opens and closes the transaction

As far as I remember we didn't explicitly decide one the particular approach but kept it open for both. It doesn't really matter but I just want to note that I didn't deliberately acted against what we agreed on without providing some tradeoffs discussion. I thought I can follow whichever approach I found working.

Basically the choice between my approach and yours comes down to a question, whether issuing plain START TRANSACTION query locks entire database while one query START TRANSACTION; SELECT...; UPDATE ....; COMMIT; makes database aware of what we are doing in the query so that it can optimize what is locked and what is not. Looks like it is really tough to find resources that answer this question. The most comprehensive material I found is this: http://dev.cs.ovgu.de/db/mysql/InnoDB-transaction-model.html. It states that locks on the database are on the row level. Additionally it states that InnoDB sets locks on the rows that it encounters when it scans the database as a results of executing query. In our dilemma there are no queries immediately after the START TRANSACTION so it doesn't have an information which rows to lock. It would be strange default behaviour to lock the entire database at once if there are no other queries. I understand we are looking for an explicit answer and the resources I found are not an explicit answer. However I couldn't find any resources suggesting that database might be locked by default if plain START TRANSACTION is issued.

Perhaps you felt the sleep was not great for performance (true), but we need to weigh that against the potential impact of locking the database while we issue multiple queries within a single transaction.

I definitely thought about performance. But another thing to consider is that while() { sleep() } os an anti-pattern especially when dealing with databases which have decades of research to solve those kinds of problems for developers. Being an open source project we should avoid implementing solution that might look suspicious to the community.

I will pursue looking for more explicit answer. Knowing we want to meet the monthly goal I will time box it. If I have no solution until the tomorrow's crypto sync (it is the middle of my working day) I will drop the research and go ahead with sleep and create follow up issue giving it high priority to revisit it quickly during the bug bash in the second half of may. My intuition on is that the best thing to do here is to create some source of worker process that operates sequentially by consuming some task queue, which is similar to what we have on native but on process level. I believe there must be some library to do so in JS. It is a common backed case and there are such libraries for other languages such as Celery for Python. Doing so however is out of the scope right now as we must meet the deadline.

This is a tricky one and I don't exactly know how it works. But it sounds reasonable that the engine could use the whole query to optimize locking strategy and limit the locking to just some rows / tables. It also depends on the isolation level we have configured for our db - for the lowest isolation levels it shouldn't matter, but for higher there should be significant difference in performance.

From the performance point of view, the locks will be acquired only for the duration of transaction, so if the transaction doesn't last for too long, even if it locks the entire database, it shouldn't have too high impact. Especially that every update / delete operation probably also locks tables, and we already have some delete queries which touch a lot of tables.

One thing about locking is that it isn't necessary as bad as it sounds, because transactions usually are implemented as hidden columns where transaction ids are stored. So the operation isn't blocked and is executed, and then later, when it gets committed, the ids are updated.

Disclaimer: most of this isn't based on any research, just on the things I remember, which may not always be accurate and depends heavily on the engine. I can spend some time doing the research, if we think it makes sense.

We should use Promise<mixed> here since we don't care about the return type.

Basically we always probably () => mixed over () => void when typing a parameter, since usually the latter means we don't care about the return

I decreased delay value following my discussion with @tomek . He reasoned that operations that take place inside a loop are fast and such high delay value like 2000ms decreases the chance that the caller of this method will be the very next to acquire the lock, while it should be the opposite.