[1.x] Improve unique constraint handling for high traffic applications #104
Conversation
| $exists = $this->newQuery() | ||
| ->where('name', $feature) | ||
| ->where('scope', $serialized = Feature::serializeScope($scope)) | ||
| ->exists(); | ||
|
|
||
| if (! $exists) { | ||
| return false; | ||
| } | ||
|
|
||
| $this->newQuery() | ||
| return (bool) $this->newQuery() | ||
| ->where('name', $feature) | ||
| ->where('scope', $serialized) | ||
| ->where('scope', Feature::serializeScope($scope)) | ||
| ->update([ | ||
| 'value' => json_encode($value, flags: JSON_THROW_ON_ERROR), | ||
| static::UPDATED_AT => Carbon::now(), | ||
| ]); | ||
|
|
||
| return true; |
There was a problem hiding this comment.
This was needlessly doing two queries: 1 to check the existence and another to make the update.
Updated to only do a single update statement to better support high traffic applications and reduce queries.
| if (! $this->update($feature, $scope, $value)) { | ||
| $this->insert($feature, $scope, $value); | ||
| } | ||
| $this->newQuery()->upsert([ | ||
| 'name' => $feature, | ||
| 'scope' => Feature::serializeScope($scope), | ||
| 'value' => json_encode($value, flags: JSON_THROW_ON_ERROR), | ||
| static::CREATED_AT => $now = Carbon::now(), | ||
| static::UPDATED_AT => $now, | ||
| ], uniqueBy: ['name', 'scope'], update: ['value', 'updated_at']); |
There was a problem hiding this comment.
When telling Pennant to set a feature, we now utilise upsert instead of attempting to update and then insert if there is nothing to update. This has a positive impact on the number of database queries the library runs.
|
@timacdonald This might be the wrong place to ask, but since it's related, asking here (please let me know if you prefer me creating a issue instead, I'll do that right away) Can we have If it makes sense, I can raise a PR for the same. This will primarily help with overriding functions in DatabaseDriver. Currently, I just need to override the "writes" to the DB. But, since we have a write inside the |
|
I don't see anything wrong with an |
fixes #101
Problem
Laravel Pennant implements a unique constraint that can cause race conditions:
pennant/database/migrations/2022_11_01_000001_create_features_table.php
Line 23 in 83178d7
The race condition can be illustrated with the following application setup:
To simulate the race condition, you may invoke the script:
Solution
We now retry when hitting a
UniqueConstraintViolationException. For a single feature , we only retry once, i.e., two total attempts. For several individual features, viagetAll, we retry twice, i.e., 3 total attempts.This is similar to how the framework handles these internally with
createOrFirst.Why not a cache lock
For indivdual features, we could use a cache lock.
When retrieving several features at a time, like you might do with
Feature::loadMissing([/* ... */]);to avoid n+1 queries, a cache lock becomes complicated.We would need an indivudal cache lock for each feature you are loading. We would need to aquire the cache lock before retrieving, which means every single feature retrievial now requires a cache lock, even if it will never result in a race condition.
If you are using the database driver, now you have increased the queries required to retrieve a feature.
I feel that simply retrying for those rare cases where a race condition will occur is the best way forward. It means 99% of retrievals happen with not speed impact and there is a small cost when a race condition happens to occur.