Adjust sqlite size calculation to account for WAL #231
Conversation
SQLite supports two methods of commiting; a rollback journal and a write-ahead-log (WAL). By default, a rollback journal is used. However, on Android 9 (P), the default changed slightly to a WAL-compatible mode: https://source.android.com/devices/tech/perf/compatibility-wal. This mode causes a WAL to be written to disk. Our prior method of calculating the database size did not take the WAL into account. In practice, this isn't too awful, as the contents of the WAL would eventually be written back into the main database file, but in the meantime, our calculation could be off by upwards of a few MB. This commit changes the size calulation to ask sqlite rather than looking at the size of the file on disk.
| @@ -83,6 +83,7 @@ android { | |||
| sourceCompatibility JavaVersion.VERSION_1_8 | |||
| targetCompatibility JavaVersion.VERSION_1_8 | |||
| } | |||
| testOptions.unitTests.includeAndroidResources = true | |||
There was a problem hiding this comment.
This is technically tangential to your PR, right? Please confirm you're including it intentionally.
There was a problem hiding this comment.
Also, please confirm that including this flag now does not generate a warning.
There was a problem hiding this comment.
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.
firebase-firestore/CHANGELOG.md
Outdated
| @@ -1,4 +1,7 @@ | |||
| # Unreleased | |||
| - [fixed] Fixed calculation of SQLite database size on Android 9 (P) devices. | |||
There was a problem hiding this comment.
"Android 9 (P)" is a nonstandard way of referring to this. Android documentation calls this "Android 9 Pie". We should follow since the changelog gets rolled up into our public release notes.
| @@ -83,6 +83,7 @@ android { | |||
| sourceCompatibility JavaVersion.VERSION_1_8 | |||
| targetCompatibility JavaVersion.VERSION_1_8 | |||
| } | |||
| testOptions.unitTests.includeAndroidResources = true | |||
There was a problem hiding this comment.
Also, please confirm that including this flag now does not generate a warning.
| * @see https://www.sqlite.org/pragma.html | ||
| */ | ||
| private long pragma(String pragmaName) { | ||
| Cursor cursor = db.rawQuery("PRAGMA " + pragmaName, null); |
There was a problem hiding this comment.
As a matter of principle we should avoid composing SQL through string concatenation. This construct is subject to SQL injection attacks if we're not careful. Better to just have the caller pass the whole query string.
There was a problem hiding this comment.
As discussed, it's not possible to use a prepared statement here. We could instead allow the caller to pass the entire sql statement, but that's probably worse. Instead, we decided to un-refactor this. (Done).
| * @param pragmaName Must be a valid 'pragma-name' and must refer to a long value. | ||
| * @see https://www.sqlite.org/pragma.html | ||
| */ | ||
| private long pragma(String pragmaName) { |
There was a problem hiding this comment.
Not all pragmas resolve to a single long value. Maybe name this simpleQueryForLong?
I suggest that name because for queries that resolve to a single 1x1 result, SQLite supports running them with SQLiteStatement.simpleQueryForLong in a way that avoids allocating a cursor.
| * @see https://www.sqlite.org/pragma.html#pragma_page_size | ||
| */ | ||
| private long getPageSize() { | ||
| Cursor cursor = db.rawQuery("PRAGMA page_size", null); |
There was a problem hiding this comment.
You could cut down on the verbosity here by writing this as:
return query("PRAGMA page_size").firstValue(row -> row.getLong(0));
|
@rsgowman: The following test failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
SQLite supports two methods of commiting; a rollback journal and a
write-ahead-log (WAL). By default, a rollback journal is used. However,
on Android 9 (P), the default changed slightly to a WAL-compatible mode:
https://source.android.com/devices/tech/perf/compatibility-wal. This
mode causes a WAL to be written to disk.
Our prior method of calculating the database size did not take the WAL
into account. In practice, this isn't too awful, as the contents of the
WAL would eventually be written back into the main database file, but in
the meantime, our calculation could be off by upwards of a few MB. This
commit changes the size calulation to ask sqlite rather than looking at
the size of the file on disk.