Use transactions when renaming directory contents

[icewind1991]

Or "how to save 99% execution time with 2 simple lines" 😄

From 33s to ~600ms when moving ~5000 files (comparison)

For #13391

cc @PVince81 @DeepDiver1975

[PVince81]

Wow, okay

[PVince81]

While this might speed up database update, the time window where the filesystem contents and cache do not match will still exist (and any file operation happening in-between will trigger a scanner process that will try and update it). So that ticket will probably need additional fixes.

[PVince81]

• needs to be tested on SQLite too

Do we have a unit test that tests this part of code ? (I suppose we do)

[MorrisJobke]

Wow o.O We should check our code more often for such stuff.

[icewind1991]

So that ticket will probably need additional fixes.

Agree, this should solve the majority of the issues though

[MorrisJobke]

@icewind1991 So this is for a rename of a folder, right?

[icewind1991]

@icewind1991 So this is for a rename of a folder, right?

Yes, for my test I created a folder with 5k small files in it

[icewind1991]

Do we have a unit test that tests this part of code ? (I suppose we do)

Yes

[scrutinizer-notifier]

The inspection completed: 4 new issues

[ghost]

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/8913/
Test PASSed.

[MorrisJobke]

Same here. Tested with SQLite and 10.000 files from 32s to 0.9s - compare

Works and code looks good 👍

[MorrisJobke]

Let me test with mysql and postgres again.

[MorrisJobke]

Works with Postgres 9.4 and Mysql 5.6 too.

[MorrisJobke]

I guess this reduces my problem with the data loss while renaming such a folder a lot.

@DeepDiver1975 @karlitschek I would propose this for backporting at least to stable8 (for 8.0.1 maybe)

[karlitschek]

Sorry. We only backport serious bugfixes. The risk for something is too high.

[icewind1991]

We can do even better: #13956 uses some dark sql rituals to bring the whole thing down to a single query.

[DeepDiver1975]

uses some dark sql rituals

like this ....

[LukasReschke]

Black magic has been verified by me: 👍

[PVince81]

@karlitscheck so... data loss on folder renames isn't a serious bug ?

[MorrisJobke]

@karlitschek ^

[d10r]

If you care about a users opinion (using it at our company, ~25 users): I consider this issue to be critical, not just serious. We started to plan migrating to another sync solution because of occasional data loss with OC.
The additional Apps OC offers are nice, but if the core feature isn't reliable, that's a killer for us.

[christianrj]

@bcg-didi Same for us (~31 users). I reported various bugs related to file operations, none of them was fixed so far, and some of them was closed for no reason. If these bugs are not resolved soon, we will migrate to another sync solution too (which we are already testing).

[MorrisJobke]

@karlitschek Should I prepare a backport PR?

[DeepDiver1975]

Should I prepare a backport PR?

yes please - thx

[MorrisJobke]

Backport: #14752

[Helios07]

Hey guys,
I do not want to play the bad cop here, but as far as I understand this function, there is a serious risk for errors because of the transaction.
The "commit" of the transaction will be executed either the executeAudited()-function succeeds or not. I cannot see any error handling here and no rollback. I would expect a "try" and a "catch" routine here like it is written here: http://php.net/manual/de/pdo.transactions.php
Please do not underestimate the risk of failing database actions. We run a huge owncloud (20k users at the moment and rising daily) with a clustered database. In a database cluster it is quite normal that you have deadlocks when two application servers write to different database nodes. Without transactions this is not a critical problem because you can make Galera (the cluster solution) retry the commands. But as far as I know, with transactions, such a retry is not possible.

So to conclude this, please prove me wrong, but I can see no error handling here which could corrupt the data!

[MorrisJobke]

@icewind1991 @PVince81 Please have a look at the comment. Is there an error handling in place? I don't think so. What can we do in this case? Retry for ourself? or rollback this somehow?

[PVince81]

Not sure. It seems we have autocommit enabled here https://github.com/owncloud/core/blob/master/lib/private/db/connectionfactory.php#L109.
So if I understand well, if an exception occurs the instruction pointer will jump to the next "catch" block which is likely to be far away, and does NOT contain any rollback code.

We probably need to find all locations in the code that have transactions and add a catch block that rolls back, as advised here: http://doctrine-dbal.readthedocs.org/en/latest/reference/transactions.html#transactions

[MorrisJobke]

@Helios07 @icewind1991 @PVince81 to not forget about this I opened a technical debt ticket: #16445

[Helios07]

Thank you very much.

[danimo]

Ping? We just got owncloud/client#3275, and I think it's really serious to resolve this properly. /cc @DeepDiver1975

[hoho508]

#16558

[PVince81]

@danimo this PR was already merged a while ago

[PVince81]

@icewind1991 any reason why the update of the actual parent folder isn't inside the transaction ? https://github.com/owncloud/core/pull/13948/files#diff-fb4e3a03c02988e8dde2af0b55f7b96fR421