Skip updating unchanged user settings #2128
Conversation
|
@andreas-p, thanks for your PR! By analyzing the history of the files in this pull request, we identified @MorrisJobke, @icewind1991 and @bartv2 to be potential reviewers. |
| @@ -212,8 +212,15 @@ public function setUserValue($userId, $appName, $key, $value, $preCondition = nu | |||
| throw new \UnexpectedValueException('Only integers, floats and strings are allowed as value'); | |||
| } | |||
|
|
|||
| // TODO - FIXME | |||
| $this->fixDIInit(); | |||
| $curval=$this->getUserValue($userId, $appName, $key); | |||
There was a problem hiding this comment.
Could you add spaces around the equal sign.
| $curval=$this->getUserValue($userId, $appName, $key); | ||
| if (isset($preCondition)) { | ||
| if ($curval !== $preCondition) | ||
| throw new OCP\PreConditionNotMetException(); |
There was a problem hiding this comment.
Please add curly brackets for all if statements.
|
@rullzer just pointed out that it is already partially done in #1734 @andreas-p Could you rebase on top of current master and combine your changes with the current master version. |
* this PR makes sure to warm up the cache for that user * then the logic within the "if is in cache" code can be used to reduce needed queries * inspired by @andreas-p - #2128 Signed-off-by: Morris Jobke <hey@morrisjobke.de>
|
Thanks a lot for your PR - while looking at it I noticed another bug in the newly introduced code. So I decided to fix this together with your improvement. See #2147 for this PR. I will close this PR here then - nevertheless: Thanks for this, because you helped to spot this issue 🙌 Feel free to maybe have a look at our starter issues: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22starter+issue%22 |
* this PR makes sure to warm up the cache for that user * then the logic within the "if is in cache" code can be used to reduce needed queries * inspired by @andreas-p - #2128 Signed-off-by: Morris Jobke <hey@morrisjobke.de>
This partially addresses #366
This small check will skip updating the user settings if the value doesn't change. Each unnecessary update will create two database roundtrips (one failed insert, one update). In case of PostgreSQL, a multitude of constraint violations is logged, and the table is bloated.
I found an average ajax call execution time reduced by 10% by suppressing unnecessary user_ldap:uid and user_ldap:displayName updates.