Skip to content

Adjust sqlite size calculation to account for WAL #231

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Feb 6, 2019
Merged

Adjust sqlite size calculation to account for WAL #231

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9c277a1
Adjust sqlite size calculation to account for WAL
rsgowman Feb 4, 2019
0d11da9
Review feedback
rsgowman Feb 4, 2019
111ab9a
review feedback
rsgowman Feb 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions firebase-firestore/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
# Unreleased
- [fixed] Fixed calculation of SQLite database size on Android 9 Pie devices.
Previous method could be off by a few MBs on these devices, potentially
delaying garbage collection.

# 18.0.1
- [fixed] Fixed an issue where Firestore would crash if handling write batches
Expand Down
1 change: 1 addition & 0 deletions firebase-firestore/firebase-firestore.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ android {
sourceCompatibility JavaVersion.VERSION_1_8
targetCompatibility JavaVersion.VERSION_1_8
}
testOptions.unitTests.includeAndroidResources = true
Copy link
Contributor

Choose a reason for hiding this comment

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

This is technically tangential to your PR, right? Please confirm you're including it intentionally.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, please confirm that including this flag now does not generate a warning.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is technically tangential to your PR, right? Please confirm you're including it intentionally.

Mostly tangential, though it causes the issue to manifest itself. (Also required by Ashwin's changes.)

Also, please confirm that including this flag now does not generate a warning.

This does not seem to introduce any new warnings.

}

dependencies {
Expand Down
24 changes: 20 additions & 4 deletions firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLitePersistence.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@
import com.google.firebase.firestore.util.Consumer;
import com.google.firebase.firestore.util.Logger;
import com.google.firebase.firestore.util.Supplier;
import java.io.File;
import java.io.UnsupportedEncodingException;
import java.net.URLEncoder;
import java.util.ArrayList;
Expand Down Expand Up @@ -77,7 +76,6 @@ public static String databaseName(String persistenceKey, DatabaseId databaseId)
private final OpenHelper opener;
private final LocalSerializer serializer;
private SQLiteDatabase db;
private File databasePath;
private boolean started;
private final SQLiteQueryCache queryCache;
private final SQLiteRemoteDocumentCache remoteDocumentCache;
Expand Down Expand Up @@ -106,7 +104,6 @@ public SQLitePersistence(
LruGarbageCollector.Params params) {
String databaseName = databaseName(persistenceKey, databaseId);
this.opener = new OpenHelper(context, databaseName);
this.databasePath = context.getDatabasePath(databaseName);
this.serializer = serializer;
this.queryCache = new SQLiteQueryCache(this, this.serializer);
this.remoteDocumentCache = new SQLiteRemoteDocumentCache(this, this.serializer);
Expand Down Expand Up @@ -199,7 +196,26 @@ <T> T runTransaction(String action, Supplier<T> operation) {
}

long getByteSize() {
return databasePath.length();
return getPageCount() * getPageSize();
}

/**
* Gets the page size of the database. Typically 4096.
*
* @see https://www.sqlite.org/pragma.html#pragma_page_size
*/
private long getPageSize() {
return query("PRAGMA page_size").firstValue(row -> row.getLong(/*column=*/ 0));
}

/**
* Gets the number of pages in the database file. Multiplying this with the page size yields the
* approximate size of the database on disk (including the WAL, if relevant).
*
* @see https://www.sqlite.org/pragma.html#pragma_page_count.
*/
private long getPageCount() {
return query("PRAGMA page_count").firstValue(row -> row.getLong(/*column=*/ 0));
}

/**
Expand Down