Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[core] Implement a vacuum strategy for the offline database #4342

Merged
merged 1 commit into from
Mar 17, 2016

Conversation

jfirebaugh
Copy link
Contributor

Enable PRAGMA auto_vacuum = INCREMENTAL, and perform a PRAGMA incremental_vacuum when deleting an offline region.

I'm targeting this for the release because enabling auto vacuum on an existing database has some pitfalls, and doing before offline sees widespread adoption will minimize the number of affected databases.

Fixes #4340.

👀 @tmpsantos @1ec5

@jfirebaugh jfirebaugh added this to the ios-v3.2.0 milestone Mar 16, 2016
@@ -458,6 +466,7 @@ void OfflineDatabase::deleteRegion(OfflineRegion&& region) {
stmt->run();

evict(0);
db->exec("PRAGMA incremental_vacuum");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think calling vacuum should be a separated API and not called automatically. I can think of 2 use cases for that:

  • In your app you want to remove several regions. Each deletion would be super slow because the database would be recreated on each VACUUM. Instead, the user interface would be designed in a way you delete all regions first and at the end call OffilineDatabase::vacuum(), committing to the operation.
  • This API could be used for vacuum the dangling pages from the cache.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that PRAGMA incremental_vacuum is not the same as VACUUM. In particular, my interpretation of the documentation is that PRAGMA incremental_vacuum is much cheaper: it works with a page structure that has already been prepared for incremental vacuum via PRAGMA auto_vacuum, and it doesn't compact partially filled pages of the database nor rewrite the entire database file like VACUUM does. I am not sure that a single PRAGMA incremental_vacuum after deleting several regions would be significantly cheaper than calling PRAGMA incremental_vacuum after each deletion. In fact, it might block other database commands for a longer continuous period of time.

Regarding the second point, we expect the number of free pages for the ambient cache to be low. The expected behavior is that the ambient cache fills up to the maximum allotted size, and then stays at that size. Vacuuming free pages from it would in fact be a deoptimization, because those pages would just need to be reallocated as soon as the cache filled back up to its maximum size.

I'd prefer to keep the vacuum strategy a private implementation detail for now. There are certainly some unknowns about how it will perform, but it's not clear to me that providing API control will help.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarifying.

@tmpsantos
Copy link
Contributor

@jfirebaugh 👍 + a comment.

Enable `PRAGMA auto_vacuum = INCREMENTAL`, and perform a `PRAGMA incremental_vacuum` when deleting an offline region.
@jfirebaugh jfirebaugh merged commit 5130ca8 into release-ios-3.2.0-android-4.0.0 Mar 17, 2016
@jfirebaugh jfirebaugh deleted the offline-vacuum branch March 17, 2016 21:18
@bleege bleege mentioned this pull request Mar 21, 2016
14 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants