Close cursor early in calculateFolderSize #13752

PVince81 · 2015-01-29T10:45:15Z

This method triggers additional queries in $this->update() so to avoid
potential database locks or delays, we close the cursor as soon as it is not needed any more

This seems to fix the "database is locked" issue happening here during upload: #12429 (I have an environment where it could be reproduced consistently)

@DeepDiver1975 @guruz @icewind1991

This method triggers additional queries in $this->update() so to avoid potential database locks or delays, we close the cursor as soon as it is not needed any more

scrutinizer-notifier · 2015-01-29T11:00:03Z

The inspection completed: 5 new issues

DeepDiver1975 · 2015-01-29T11:05:01Z

refs #13705

PVince81 · 2015-01-29T11:08:44Z

checked the md5 of the uploaded files, all was well.
See the original issue for sample images (a folder full of small pics)

I just tried the following (all was with encryption enabled), upload in parallel from:

Firefox upload into a folder "sub1"
Chromium upload into a folder "sub2"
Sync client upload into a folder "sub3"

Then checked md5, all fine. No database is locked exception.

Next step: upload into same folder.

ghost · 2015-01-29T11:12:38Z

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

PVince81 · 2015-01-29T11:30:15Z

Upload different files into the same folder seemed to work, too.

I'll see if I can test parallel uploads now with the sync client.

PVince81 · 2015-01-29T11:32:44Z

If that works well we might want to backport this @karlitschek

PVince81 · 2015-01-29T12:00:44Z

I tried with the new sync client master (owncloud/client@43d6dbb) and I observed this:
Without the fix: massive floodage of the logs with "database is locked"

With this fix: all worked fine, nothing in log!

Please review, this will also fix parallel upload + sqlite

PVince81 · 2015-01-29T12:06:00Z

Fixes #12429

PeterPablo · 2015-01-29T12:07:17Z

lib/private/files/cache/cache.php

@@ -596,6 +596,7 @@ public function calculateFolderSize($path, $entry = null) {
 				'WHERE `parent` = ? AND `storage` = ?';
 			$result = \OC_DB::executeAudited($sql, array($id, $this->getNumericStorageId()));
 			if ($row = $result->fetchRow()) {
+				$result->closeCursor();


Isn't it slightly better to move this line outside the if check, add an additional statement $resultRow = $result->fetchRow(); and change the if-statement to if ($row = $resultRow) { ?

or simply if($resultRow) ;)

It would be slightly better, yes. But I think the current approach is still acceptable.

I preferred investing my time in testing this bad bug and making sure it work instead of making the code perfect. (note: still busy testing)

Feel free to make the change yourself if you think that's very important.

@PVince81
I am probably not up to it, since I don't have a test environment and am only following the changes. The advantage of my proposed approach is that the connection to the database is freed at the earliest point and additionally in a single place. This might prevent regressions, once other parts of that function are touched or additional cases added to the if statement.

If you still think I should provide a commit without me having a test environment and practically zero php-experience, I would provide it. Should I then create a new PR with a feature branch having your commit against HEAD of master?

I don't think the performance gain for this change will be that much, but anyway if you'd like to contribute, you should fork the core repo and then submit PRs of a feature branch to core master.

You are saying you have zero PHP experience and yet are making suggestions to improve PHP code ? 😉
Anyway, there's always opportunities to learn.

If you are interested in contributing code/changes, I suggest to setup a dev env http://doc.owncloud.org/server/7.0/developer_manual/general/devenv.html
Then fork the project and submit a PR to master from a feature branch.

Please note that we are currently in the final phase of 8.0 release (so a bit of a rush to finish it), so any non-critical PRs will have to wait for 8.1. 😄

Thanks for elaborating. I knew most about it, yet primarily wanted to know, if you consider this kind of change too much of nitpicking. Certainly no rush is needed for this kind of change, so don't bother with me right now.
No php, but isn't it all the same at an abstract level? ;-)

PVince81 · 2015-01-29T12:38:42Z

Uploading to a shared folder as recipient with parallel upload also seems to work fine.
I did see some harmless warnings, possibly unrelated: #13761

icewind1991 · 2015-01-29T13:14:38Z

Looks good 👍

MorrisJobke · 2015-01-29T13:23:14Z

Tested parallel upload ... no suspicious warnings/errors 👍

Close cursor early in calculateFolderSize

PVince81 · 2015-01-29T13:46:32Z

@karlitschek backport to OC 7 ?

karlitschek · 2015-01-29T13:51:42Z

Please backport! Thanks

MorrisJobke · 2015-01-29T13:53:53Z

stable7 234f33e

guruz · 2015-01-29T14:23:47Z

Please backport! Thanks

... but then #13615 (comment) needs to be included in oC and backported too.
Not sure if this is clear, but you cannot sanely use sqlite3 without a busy wait handler (or manually handling BUSY return value). This is a regression, the handler had been there in oC5.

@PVince81 It would be good to test that PROPFIND lock case before your "fix" here with and without #13615

PVince81 · 2015-01-29T14:40:32Z

I think even if the PROPFIND case doesn't work, this fix already fixes the upload case.
So in the worst case it means we need additional fixing.

Can whoever was having the PROPFIND lock issue help testing this ? @davivel ?

PVince81 · 2015-01-29T14:41:57Z

I think this PR here doesn't fix a "BUSY" case but really a "LOCK" case. The SELECT statement was still open while we were trying to do UPDATEs on the same table in some situations, which would directly lock because the same PHP request cannot wait for itself (deadlock)

guruz · 2015-01-29T14:43:22Z

Thanks for your explanations on IRC, you're right of course. It's two independant things.. :)

guruz · 2015-01-29T17:02:16Z

Followup: #13772

PVince81 · 2015-01-29T19:49:34Z

Tested the backport on stable7, also works fine with parallel uploads.

DeepDiver1975 · 2015-01-30T14:03:16Z

stable7 234f33e

this backport did let stable7 to fall apart - #13788

We will stop this cherry-pick nightmare ....

karlitschek · 2015-01-30T14:04:10Z

:-( @DeepDiver1975 agreed

Close cursor early in calculateFolderSize

35ab770

This method triggers additional queries in $this->update() so to avoid potential database locks or delays, we close the cursor as soon as it is not needed any more

This was referenced Jan 29, 2015

Add max parallels to getcapabiltilies #13628

Closed

"General error: 5 database is locked" while uploading small files #12429

Closed

PVince81 added this to the 8.0-current milestone Jan 29, 2015

PVince81 mentioned this pull request Jan 29, 2015

Make sure cursors are closed everywhere / Database is locked #10566

Closed

PeterPablo reviewed Jan 29, 2015
View reviewed changes

MorrisJobke added a commit that referenced this pull request Jan 29, 2015

Merge pull request #13752 from owncloud/closecursor1

acf0582

Close cursor early in calculateFolderSize

MorrisJobke merged commit acf0582 into master Jan 29, 2015

MorrisJobke deleted the closecursor1 branch January 29, 2015 13:23

PVince81 added the Backport-Request label Jan 29, 2015

PVince81 mentioned this pull request Jan 29, 2015

ownCloud 7.0.3 stable problems due to missing shareKey (sqlite3, SQLSTATE[HY000]: General error: 5 database is locked) #12428

Closed

MorrisJobke added 7.0-current and removed Backport-Request labels Jan 29, 2015

karlitschek mentioned this pull request Jan 29, 2015

Concurrent PROPFINDs result in error 500 #13705

Closed

DeepDiver1975 mentioned this pull request Jan 30, 2015

[stable7] oracle unit test failing #13788

Closed

lock bot locked as resolved and limited conversation to collaborators Aug 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Close cursor early in calculateFolderSize #13752

Close cursor early in calculateFolderSize #13752

PVince81 commented Jan 29, 2015

scrutinizer-notifier commented Jan 29, 2015

DeepDiver1975 commented Jan 29, 2015

PVince81 commented Jan 29, 2015

ghost commented Jan 29, 2015

PVince81 commented Jan 29, 2015

PVince81 commented Jan 29, 2015

PVince81 commented Jan 29, 2015

PVince81 commented Jan 29, 2015

PeterPablo Jan 29, 2015

MorrisJobke Jan 29, 2015

PVince81 Jan 29, 2015

PVince81 Jan 29, 2015

PeterPablo Jan 29, 2015

PVince81 Jan 29, 2015

PeterPablo Jan 29, 2015

PVince81 commented Jan 29, 2015

icewind1991 commented Jan 29, 2015

MorrisJobke commented Jan 29, 2015

PVince81 commented Jan 29, 2015

karlitschek commented Jan 29, 2015

MorrisJobke commented Jan 29, 2015

guruz commented Jan 29, 2015

PVince81 commented Jan 29, 2015

PVince81 commented Jan 29, 2015

guruz commented Jan 29, 2015

guruz commented Jan 29, 2015

PVince81 commented Jan 29, 2015

DeepDiver1975 commented Jan 30, 2015

karlitschek commented Jan 30, 2015

Close cursor early in calculateFolderSize #13752

Close cursor early in calculateFolderSize #13752

Conversation

PVince81 commented Jan 29, 2015

scrutinizer-notifier commented Jan 29, 2015

DeepDiver1975 commented Jan 29, 2015

PVince81 commented Jan 29, 2015

ghost commented Jan 29, 2015

PVince81 commented Jan 29, 2015

PVince81 commented Jan 29, 2015

PVince81 commented Jan 29, 2015

PVince81 commented Jan 29, 2015

PeterPablo Jan 29, 2015

Choose a reason for hiding this comment

MorrisJobke Jan 29, 2015

Choose a reason for hiding this comment

PVince81 Jan 29, 2015

Choose a reason for hiding this comment

PVince81 Jan 29, 2015

Choose a reason for hiding this comment

PeterPablo Jan 29, 2015

Choose a reason for hiding this comment

PVince81 Jan 29, 2015

Choose a reason for hiding this comment

PeterPablo Jan 29, 2015

Choose a reason for hiding this comment

PVince81 commented Jan 29, 2015

icewind1991 commented Jan 29, 2015

MorrisJobke commented Jan 29, 2015

PVince81 commented Jan 29, 2015

karlitschek commented Jan 29, 2015

MorrisJobke commented Jan 29, 2015

guruz commented Jan 29, 2015

PVince81 commented Jan 29, 2015

PVince81 commented Jan 29, 2015

guruz commented Jan 29, 2015

guruz commented Jan 29, 2015

PVince81 commented Jan 29, 2015

DeepDiver1975 commented Jan 30, 2015

karlitschek commented Jan 30, 2015