Bug 6160: Deadlock removed

[obel1x]

This is a workaround to avoid Deadlocks and duplicate Keys in UPDATE SELECT- Statement as described in #6160 and maybe the cause for #9305 #12228 #6899 #6187

[MorrisJobke]

Could you remove the whole comment - that's what we have version control for. 😉

[obel1x]

Ah ok, thanks for the hint. Lack of me beeing new to github. Will do this if we agree if the changes can be done that way. Currently i don't know someone can do it better.

[MorrisJobke]

Code itself makes a lot of sense and seem to work as stuff that uses those methods (group creation, app config setting, file upload,...) still work fine. 👍

[nickvergessen]

Well this just leaves more time for concurrent queries to break it between the select and the insert. Exactly the reason why the method was created and used in first place.

[rullzer]

Can't we just wrap it in an transaction?
Also. Why not use the query builder?

[MorrisJobke]

@nickvergessen @rullzer What do you think about #6899 (comment) ? If you say that this is okay, I would port it by tomorrow so it will be in beta 1.

[obel1x]

yep, this is why it was first meant to be a version for testing and to see if it solves the problem in general.
In theory original used query really looks better, but all dbms seem to have large problems with it, not working as expected and even causing duplicate keys sometimes. Sad, but understandable as dmbs are trimmed for integrity and performance.

As for the aspect that there could be a concurrent access doing it that way: In almost any cases only the select is done, while entry is already there. You don't need lock for this. The update is only done once: when a file is created. I don't now how often the same file (of the same user) is created in the same moment twice, but i currently cannot think about the constellation this happens. You could use transaction + locking for this, but that would mean a lot of overhead for a each select. I would expect performance impacts then. I wouldn't do this, while even in worst case you could get duplicate entries and that's it.

So looking at the changes the second time it even improves performance compared to the original a lot while most time file is already there. In this case my changes are only doing selects (that can be speed up). But for the original Update...Select MySql wants to copy the table in temporary table first - each time says the docs. And this not only if the file is created - but for every time the function is used.

So finally original query bring dbms in trouble causing deadlocks and duplicate values and is inefficient either way each time the function is used. I would definitely change it somehow.
So please feel free to make adjustments as you proposed or just not merge and do it otherwise, but keep in mind that users are more interested in (fast) working programs than good looking code.
Btw @MorrisJobke: as said i really disadvice to solve Deadlocks with retrys - they should not happen at all and should be handled with better coding. If you hide them by retrying you loose the ability to solve the real issue. In general, better exception handling is fine.

At last: I have not used query builder, because i don't know what it is. I used text editor (kwrite). But i am new and willing to learn. Can you point me to some explanations to query builder to use for nc?

Nevermind what i am talking to the things above: Very good work you are doing with NC, keep it up!

[kesselb]

retrywrapper is a nice feature but will not help in this case. The insert query is not locked by another query its locked by itself. This could explain #6899 (comment). (Not true 😕)

server/lib/private/Files/Cache/Cache.php

Line 271 in a661f04

if (\OC::$server->getDatabaseConnection()->insertIfNotExist('*PREFIX*filecache', $values, [

should be replaced with something like this because they unique index is there anywhere and most reports for this deadlocks are from oc_filecache.

try {
// do insert
} catch (UniqueConstraintViolationException $e) {
// ignore when already exist
}

[obel1x]

retrywrapper is a nice feature but will not help in this case. The insert query is not locked by another query its locked by itself. This could explain #6899 (comment). (Not true confused)

why was the deadlock not caused by itself? for me it seemed so very much as i were the only user that time doing no other things with it.

server/lib/private/Files/Cache/Cache.php

Line 271 in a661f04
if (\OC::$server->getDatabaseConnection()->insertIfNotExist('PREFIXfilecache', $values, [
should be replaced with something like this because they unique index is there anywhere and most reports for this deadlocks are from oc_filecache.

try {
// do insert
} catch (UniqueConstraintViolationException $e) {
// ignore when already exist
}

I saw the function beeing used also in other context, not sure whether there is always an index on the table. Even if so, i wouldn't let the dbms run into execptions first if i could prevent them.

So how do we proceed? As for now i have not seen any negative test results/aspects further, but at least three Issues beeing confirmed to be solved by it and i guess around 10 Issues will be solved further. I would rate this beeing mayor, as users are quiet upset seeing that many errors. All those errors are gone with that merge.

Would you agree merging it if i remove the comments as @MorrisJobke suggested?

[kesselb]

Would you agree merging it if i remove the comments as @MorrisJobke suggested?

Sure. I'm not against your pr but I think its the wrong way 🙈

[MorrisJobke]

Well this just leaves more time for concurrent queries to break it between the select and the insert. Exactly the reason why the method was created and used in first place.

I can fully understand this one, because now the time is higher, because it needs to go back to PHP instead of being just a sub select, which has less time, but still the option to cause this. I guess we need to catch an exception here. There is just no further way to eliminate this except in an atomic SQL function, which is not present in all DBs.

So the approach would be to keep this method how it is currently, because theoretically it makes things worse:

• before: INSERT with SUB-SELECT
• after: SELECT in PHP and then fire an INSERT

Or do I miss something here? I don't know why it makes something better, but the obvious approach is to additionally to this method call also wrap the method call into a try catch with the exception that is expected (most likely a UniqueConstraintException), but unfortunately you cannot know if this is the case for all queries or should we just do this by default inside the function?

[MorrisJobke]

I saw the function beeing used also in other context, not sure whether there is always an index on the table. Even if so, i wouldn't let the dbms run into execptions first if i could prevent them.

That is what I wanted to describe above - there is no guarantee (except a single SQL statement itself which is currently not available on all supported platforms) to do this. So triggering and catching the exception is unfortunately the only way to go for now.

[MorrisJobke]

There are exact 26 usages (over all apps + core - apps == all apps in our app store + the ones I could find on GitHub/GitLab):

[MorrisJobke]

I guess if we focus on the stuff in apps/ and lib/ we covered most of the critical cases IMO.

[nickvergessen]

So the approach would be to keep this method how it is currently, because theoretically it makes things worse:

that is exactly my thought too

[obel1x]

year, don't get me wrong, i really understand the bad feeling that in theory it worse and thats why i would really also prefer better ways. But as for now i did not see the other approaches beeing better. Again i can only say that causing exeptions for me is really a wrong way imho. And the theoretical timeslot between updates is really very much theory. Think of when this will happen and how often you will see it - same file to be created at the same time from different users? And what would happen then? An exception 👍 Or am i missing anything?

So in practical for me it has even speed up NC while the complexity of update..select at dmbs level to find out, that file is there in 99% is high dbms load.

Well, its up to you to decide. At my work i need to do those nasty decisions every day ;)
I now did the changes. Let me know if i can do anything more.
Hf, Daniel

[MorrisJobke]

And the theoretical timeslot between updates is really very much theory. Think of when this will happen and how often you will see it - same file to be created at the same time from different users? And what would happen then? An exception 👍 Or am i missing anything?

It is actually not that theoretical - just look into the linked tickets. And if we are talking about short time slots, then look into this bug here: owncloud/core#24052

That one took me a long time to debug with on a high volume instance. It was basically this:

if($this->isCached($cacheKey)) {
    # here the cached values TTL ended and it got not cached anymore
    $this->getCached($cacheKey); # 💥 
}

Keep in mind that the TTL (time to live) was IIRC 12h. And this one happened on the instance quite often (a handful of times per day). The time between those two entries is far less than a fraction of a millisecond and it hit on time for a 12h timeout to hit this exact spot. Never say: "it is super unlikely" - at some point it will bite you no matter how unlikely it is. So better be safe here and catch the exception to be really bullet proof and not only theoretically safe. 😉

[MorrisJobke]

I started with the catching approach - first one is the issue in the locking: #12366

[MorrisJobke]

List to have a look at:

• apps/files_external/lib/Service/DBConfigService.php:303
• apps/files_external/lib/Service/DBConfigService.php:325
• apps/files_external/lib/Service/DBConfigService.php:341
• apps/files_sharing/lib/External/Manager.php:180
• apps/user_ldap/lib/Mapping/AbstractMapping.php:249
• apps/user_ldap/lib/Mapping/AbstractMapping.php:250
• lib/private/AppConfig.php:168
• lib/private/DB/MigrationService.php:270
• lib/private/Files/Cache/Cache.php:271 Catch and ignore UniqueConstraintViolationException in file cache insert #12368 (issue: Doctrine\DBAL\Exception\UniqueConstraintViolationException: An exception occurred while executing 'INSERT INTO "oc_filecache"  #6160)
• lib/private/Files/Cache/Storage.php:76
• lib/private/Files/Config/UserMountCache.php:175
• lib/private/Files/Type/Loader.php:117
• lib/private/Group/Database.php:101
• lib/private/Lock/DBLockingProvider.php:136 Catch and ignore UniqueConstraintViolationException in file locking init #12366 (issue: Race condition with locks in database - unique constraint violation on oc_file_locks #9305)
• lib/private/Tags.php:501
• lib/private/Tags.php:688

[MorrisJobke]

I moved this list to #12369

[MorrisJobke]

Let's go for #12371. This is basically what @blizzz @nickvergessen and @danielkesselberg suggested as well.

[MorrisJobke]

@obel1x Thanks for your contribution anyways. It was a good start and helped me to understand the underlying problem. 👍

You could also check out the other issues - maybe some that are suited for people that are not too familiar with the code base: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

[MorrisJobke]

@nickvergessen @blizzz It's possible that we still need this to remove the deadlocks that could also occur, because we have a sub query. 🤔

[MorrisJobke]

Two big ones are addressed in #12411 (file cache) and #12413 (file locks)