fix: Make authtoken activity updater simpler to avoid deadlocks #40971
Conversation
Signed-off-by: Christoph Wurst <[email protected]>
| @@ -234,7 +212,6 @@ public function updateActivity(IToken $token, int $now): void { | |||
| ->set('last_activity', $qb->createNamedParameter($now, IQueryBuilder::PARAM_INT)) | |||
| ->where( | |||
| $qb->expr()->eq('id', $qb->createNamedParameter($token->getId(), IQueryBuilder::PARAM_INT), IQueryBuilder::PARAM_INT), | |||
| $qb->expr()->lt('last_activity', $qb->createNamedParameter($now - 15, IQueryBuilder::PARAM_INT), IQueryBuilder::PARAM_INT) | |||
There was a problem hiding this comment.
Do we have a way to know current last_activity of the token?
If so, we could write this:
$qb->expr()->eq('last_activity', $qb->createNamedParameter($token->lastActivity(), IQueryBuilder::PARAM_INT), IQueryBuilder::PARAM_INT)I think it can help to reduce locking
There was a problem hiding this comment.
This is a micro optimization and will possibly not show any significant
performance improvement. Yet in setups with a DB cluster it means that
the write node has to send fewer changes to the read nodes due to the
lower number of actual changes.
This is exactly what I'm wondering about. Not sure if DB clusters are optimized enough to not resync changes when the query is "empty", but the idea was to avoid contacting the master.
There was a problem hiding this comment.
@Altahrim I thought about that too. We definitely have access to the previous last_activity value. But then I wonder if the additional clause in WHERE makes the query less or more prone to locking issues. The row is already looked up by the primary key. I can't imagine there being much difference between a less than and an equals check. They should be comparably fast/slow.
There was a problem hiding this comment.
Not sure if DB clusters are optimized enough to not resync changes when the query is "empty"
If I recall correctly, Galera enforce row replication, so no change should lead to no replication sent.
The row is already looked up by the primary key.
I am agree. I would like to be sure it doesn't lock anything else because of the less than.
I generally saw equal instead of less than to check if row as been updated.
Maybe we can simply avoid this request PHP side if we detect a close enough timestamp?
|
Did you manage to reproduce the issue somewhere? |
|
No, I was not. The deadlocks are only showing on a production instance backed by a Galera cluster. |
|
The change doesn't show any improvements on a affected production instance. |
Summary
#29357 made the query more complex to avoid too many UPDATE queries, but this can lead to deadlock situations. This patch reverts the optimization. This means there might be some lost updates in concurrent situations and there will be a lot more actual UPDATEs.
TODO
Checklist