Avoid inserting multiple locks if a lock already exists #4694
Conversation
|
@rijkvanzanten Could you please take a look? |
| trxOrKnex.from(trxOrKnex.raw("?? (??)", [lockTableWithSchema, "is_locked"])).insert(function () { | ||
| return this.select(trxOrKnex.raw("?", [0])).whereNotExists(function () { | ||
| return this.select("*").from(lockTableWithSchema); | ||
| }); | ||
| }) |
There was a problem hiding this comment.
While I believe this achieves what it aims to achieve, it's a little hard to mentally parse what's going on here. Should we extract it into a separate function, just so we can keep this bigger .then chain block a little easier to understand?
There was a problem hiding this comment.
👍 Makes sense. I extracted the logic into a helper function and updated the PR. Let me know what you think.
There is currently a race in ensureTable function that could result in multiple rows being insert into the locks table. Multiple locks in the lock table is a non-recoverable state where no lock can later be acquired. The reason for the race is that the number of locks are first retrieved from the lock table, and a new lock is then inserted if that number is zero. It is possible that a lock is inserted into the locks table right after the number of locks were first retrived, resulting in another lock being inserted. This is fixed by using an insert into select where no row is inserted if there is already a row in table, which is the desired end state.
|
@stigerland Will this work correctly if lock already exists prior to migrations starting? It would still fail as it did before? |
|
@kibertoad The change ensures that exactly one lock row is inserted into the lock table. If a lock row is missing, it will be inserted. If there is already a lock row, nothing is done and it is left untouched. Previously, the race could result in multiple rows being inserted into the lock table if there is one already. Which at least caused errors later in the migration since it is assumed that there is only one row in the lock table. When a lock is needed, it will still try to require the lock and it should fail if it is not able to acquire it. That part of the code is left untouched. |
|
@rijkvanzanten Waiting for your approval, then we can merge :) |
There is currently a race in ensureTable function that could result in multiple rows being insert into the locks table. Multiple locks in the lock table is a non-recoverable state where no lock can later
be acquired.
The reason for the race is that the number of locks are first retrieved from the lock table, and a new lock is then inserted if that number
is zero. It is possible that a lock is inserted into the locks table right after the number of locks were first retrived, resulting in another lock being inserted.
This is fixed by using an insert into select where no row is inserted if
there is already a row in table, which is the desired end state.