diff --git a/core/java/android/database/sqlite/SQLiteConnection.java b/core/java/android/database/sqlite/SQLiteConnection.java index b7489229a424f..706e75e4c64c2 100644 --- a/core/java/android/database/sqlite/SQLiteConnection.java +++ b/core/java/android/database/sqlite/SQLiteConnection.java @@ -113,9 +113,6 @@ public final class SQLiteConnection implements CancellationSignal.OnCancelListen private final boolean mIsReadOnlyConnection; private PreparedStatement mPreparedStatementPool; - // A lock access to the statement cache. - private final Object mCacheLock = new Object(); - @GuardedBy("mCacheLock") private final PreparedStatementCache mPreparedStatementCache; // The recent operations log. @@ -596,9 +593,7 @@ public final class SQLiteConnection implements CancellationSignal.OnCancelListen mConfiguration.updateParametersFrom(configuration); // Update prepared statement cache size. - synchronized (mCacheLock) { - mPreparedStatementCache.resize(configuration.maxSqlCacheSize); - } + mPreparedStatementCache.resize(configuration.maxSqlCacheSize); if (foreignKeyModeChanged) { setForeignKeyModeFromConfiguration(); @@ -630,12 +625,12 @@ public final class SQLiteConnection implements CancellationSignal.OnCancelListen mOnlyAllowReadOnlyOperations = readOnly; } - // Called by SQLiteConnectionPool only. - // Returns true if the prepared statement cache contains the specified SQL. + // Called by SQLiteConnectionPool only to decide if this connection has the desired statement + // already prepared. Returns true if the prepared statement cache contains the specified SQL. + // The statement may be stale, but that will be a rare occurrence and affects performance only + // a tiny bit, and only when database schema changes. boolean isPreparedStatementInCache(String sql) { - synchronized (mCacheLock) { - return mPreparedStatementCache.get(sql) != null; - } + return mPreparedStatementCache.get(sql) != null; } /** @@ -1070,28 +1065,41 @@ public final class SQLiteConnection implements CancellationSignal.OnCancelListen /** * Return a {@link #PreparedStatement}, possibly from the cache. */ - @GuardedBy("mCacheLock") private PreparedStatement acquirePreparedStatementLI(String sql) { ++mPool.mTotalPrepareStatements; - PreparedStatement statement = mPreparedStatementCache.get(sql); + PreparedStatement statement = mPreparedStatementCache.getStatement(sql); + long seqNum = mPreparedStatementCache.getLastSeqNum(); + boolean skipCache = false; if (statement != null) { if (!statement.mInUse) { - statement.mInUse = true; - return statement; + if (statement.mSeqNum == seqNum) { + // This is a valid statement. Claim it and return it. + statement.mInUse = true; + return statement; + } else { + // This is a stale statement. Remove it from the cache. Treat this as if the + // statement was never found, which means we should not skip the cache. + mPreparedStatementCache.remove(sql); + statement = null; + // Leave skipCache == false. + } + } else { + // The statement is already in the cache but is in use (this statement appears to + // be not only re-entrant but recursive!). So prepare a new copy of the statement + // but do not cache it. + skipCache = true; } - // The statement is already in the cache but is in use (this statement appears - // to be not only re-entrant but recursive!). So prepare a new copy of the - // statement but do not cache it. - skipCache = true; } ++mPool.mTotalPrepareStatementCacheMiss; - final long statementPtr = nativePrepareStatement(mConnectionPtr, sql); + final long statementPtr = mPreparedStatementCache.createStatement(sql); + seqNum = mPreparedStatementCache.getLastSeqNum(); try { final int numParameters = nativeGetParameterCount(mConnectionPtr, statementPtr); final int type = DatabaseUtils.getSqlStatementType(sql); final boolean readOnly = nativeIsReadOnly(mConnectionPtr, statementPtr); - statement = obtainPreparedStatement(sql, statementPtr, numParameters, type, readOnly); + statement = obtainPreparedStatement(sql, statementPtr, numParameters, type, readOnly, + seqNum); if (!skipCache && isCacheable(type)) { mPreparedStatementCache.put(sql, statement); statement.mInCache = true; @@ -1112,15 +1120,12 @@ public final class SQLiteConnection implements CancellationSignal.OnCancelListen * Return a {@link #PreparedStatement}, possibly from the cache. */ PreparedStatement acquirePreparedStatement(String sql) { - synchronized (mCacheLock) { - return acquirePreparedStatementLI(sql); - } + return acquirePreparedStatementLI(sql); } /** * Release a {@link #PreparedStatement} that was originally supplied by this connection. */ - @GuardedBy("mCacheLock") private void releasePreparedStatementLI(PreparedStatement statement) { statement.mInUse = false; if (statement.mInCache) { @@ -1148,9 +1153,7 @@ public final class SQLiteConnection implements CancellationSignal.OnCancelListen * Release a {@link #PreparedStatement} that was originally supplied by this connection. */ void releasePreparedStatement(PreparedStatement statement) { - synchronized (mCacheLock) { - releasePreparedStatementLI(statement); - } + releasePreparedStatementLI(statement); } private void finalizePreparedStatement(PreparedStatement statement) { @@ -1327,9 +1330,7 @@ public final class SQLiteConnection implements CancellationSignal.OnCancelListen mRecentOperations.dump(printer); if (verbose) { - synchronized (mCacheLock) { - mPreparedStatementCache.dump(printer); - } + mPreparedStatementCache.dump(printer); } } @@ -1430,7 +1431,7 @@ public final class SQLiteConnection implements CancellationSignal.OnCancelListen } private PreparedStatement obtainPreparedStatement(String sql, long statementPtr, - int numParameters, int type, boolean readOnly) { + int numParameters, int type, boolean readOnly, long seqNum) { PreparedStatement statement = mPreparedStatementPool; if (statement != null) { mPreparedStatementPool = statement.mPoolNext; @@ -1444,6 +1445,7 @@ public final class SQLiteConnection implements CancellationSignal.OnCancelListen statement.mNumParameters = numParameters; statement.mType = type; statement.mReadOnly = readOnly; + statement.mSeqNum = seqNum; return statement; } @@ -1461,10 +1463,10 @@ public final class SQLiteConnection implements CancellationSignal.OnCancelListen return sql.replaceAll("[\\s]*\\n+[\\s]*", " "); } - void clearPreparedStatementCache() { - synchronized (mCacheLock) { - mPreparedStatementCache.evictAll(); - } + // Update the database sequence number. This number is stored in the prepared statement + // cache. + void setDatabaseSeqNum(long n) { + mPreparedStatementCache.setDatabaseSeqNum(n); } /** @@ -1502,6 +1504,10 @@ public final class SQLiteConnection implements CancellationSignal.OnCancelListen // True if the statement is in the cache. public boolean mInCache; + // The database schema ID at the time this statement was created. The ID is left zero for + // statements that are not cached. This value is meaningful only if mInCache is true. + public long mSeqNum; + // True if the statement is in use (currently executing). // We need this flag because due to the use of custom functions in triggers, it's // possible for SQLite calls to be re-entrant. Consequently we need to prevent @@ -1510,10 +1516,41 @@ public final class SQLiteConnection implements CancellationSignal.OnCancelListen } private final class PreparedStatementCache extends LruCache { + // The database sequence number. This changes every time the database schema changes. + private long mDatabaseSeqNum = 0; + + // The database sequence number from the last getStatement() or createStatement() + // call. The proper use of this variable depends on the caller being single threaded. + private long mLastSeqNum = 0; + public PreparedStatementCache(int size) { super(size); } + public synchronized void setDatabaseSeqNum(long n) { + mDatabaseSeqNum = n; + } + + // Return the last database sequence number. + public long getLastSeqNum() { + return mLastSeqNum; + } + + // Return a statement from the cache. Save the database sequence number for the caller. + public synchronized PreparedStatement getStatement(String sql) { + mLastSeqNum = mDatabaseSeqNum; + return get(sql); + } + + // Return a new native prepared statement and save the database sequence number for the + // caller. This does not modify the cache in any way. However, by being synchronized, + // callers are guaranteed that the sequence number did not change across the native + // preparation step. + public synchronized long createStatement(String sql) { + mLastSeqNum = mDatabaseSeqNum; + return nativePrepareStatement(mConnectionPtr, sql); + } + @Override protected void entryRemoved(boolean evicted, String key, PreparedStatement oldValue, PreparedStatement newValue) { diff --git a/core/java/android/database/sqlite/SQLiteConnectionPool.java b/core/java/android/database/sqlite/SQLiteConnectionPool.java index b35a2e4eb1b53..ad335b62f05e4 100644 --- a/core/java/android/database/sqlite/SQLiteConnectionPool.java +++ b/core/java/android/database/sqlite/SQLiteConnectionPool.java @@ -111,6 +111,13 @@ public final class SQLiteConnectionPool implements Closeable { @GuardedBy("mLock") private IdleConnectionHandler mIdleConnectionHandler; + // The database schema sequence number. This counter is incremented every time a schema + // change is detected. Every prepared statement records its schema sequence when the + // statement is created. The prepared statement is not put back in the cache if the sequence + // number has changed. The counter starts at 1, which allows clients to use 0 as a + // distinguished value. + private long mDatabaseSeqNum = 1; + // whole execution time for this connection in milliseconds. private final AtomicLong mTotalStatementsTime = new AtomicLong(0); @@ -1127,10 +1134,12 @@ public final class SQLiteConnectionPool implements Closeable { } void clearAcquiredConnectionsPreparedStatementCache() { + // Invalidate prepared statements that have an earlier schema sequence number. synchronized (mLock) { + mDatabaseSeqNum++; if (!mAcquiredConnections.isEmpty()) { for (SQLiteConnection connection : mAcquiredConnections.keySet()) { - connection.clearPreparedStatementCache(); + connection.setDatabaseSeqNum(mDatabaseSeqNum); } } } diff --git a/core/tests/coretests/src/android/database/DatabaseErrorHandlerTest.java b/core/tests/coretests/src/android/database/DatabaseErrorHandlerTest.java index 91c7687176d4b..96775846c66f8 100644 --- a/core/tests/coretests/src/android/database/DatabaseErrorHandlerTest.java +++ b/core/tests/coretests/src/android/database/DatabaseErrorHandlerTest.java @@ -19,10 +19,13 @@ package android.database; import android.content.Context; import android.database.sqlite.SQLiteDatabase; import android.database.sqlite.SQLiteDiskIOException; +import android.database.sqlite.SQLiteDatabaseCorruptException; + import android.database.sqlite.SQLiteException; import android.test.AndroidTestCase; import android.util.Log; +import java.util.concurrent.atomic.AtomicBoolean; import java.io.BufferedWriter; import java.io.File; import java.io.FileWriter; @@ -61,7 +64,6 @@ public class DatabaseErrorHandlerTest extends AndroidTestCase { assertTrue(mDatabaseFile.exists()); } - public void testDatabaseIsCorrupt() throws IOException { mDatabase.execSQL("create table t (i int);"); // write junk into the database file @@ -70,30 +72,23 @@ public class DatabaseErrorHandlerTest extends AndroidTestCase { writer.close(); assertTrue(mDatabaseFile.exists()); // since the database file is now corrupt, doing any sql on this database connection - // should trigger call to MyDatabaseCorruptionHandler.onCorruption + // should trigger call to MyDatabaseCorruptionHandler.onCorruption. A corruption + // exception will also be throws. This seems redundant. try { mDatabase.execSQL("select * from t;"); fail("expected exception"); - } catch (SQLiteDiskIOException e) { - /** - * this test used to produce a corrupted db. but with new sqlite it instead reports - * Disk I/O error. meh.. - * need to figure out how to cause corruption in db - */ - // expected - if (mDatabaseFile.exists()) { - mDatabaseFile.delete(); - } - } catch (SQLiteException e) { - + } catch (SQLiteDatabaseCorruptException e) { + // Expected result. } - // database file should be gone + + // The database file should be gone. assertFalse(mDatabaseFile.exists()); - // after corruption handler is called, the database file should be free of - // database corruption - SQLiteDatabase db = SQLiteDatabase.openOrCreateDatabase(mDatabaseFile.getPath(), null, + // After corruption handler is called, the database file should be free of + // database corruption. Reopen it. + mDatabase = SQLiteDatabase.openOrCreateDatabase(mDatabaseFile.getPath(), null, new MyDatabaseCorruptionHandler()); - assertTrue(db.isDatabaseIntegrityOk()); + assertTrue(mDatabase.isDatabaseIntegrityOk()); + // The teadDown() routine will close the database. } /** @@ -102,8 +97,21 @@ public class DatabaseErrorHandlerTest extends AndroidTestCase { * corrupt before deleting the file. */ public class MyDatabaseCorruptionHandler implements DatabaseErrorHandler { + private final AtomicBoolean mEntered = new AtomicBoolean(false); public void onCorruption(SQLiteDatabase dbObj) { - boolean databaseOk = dbObj.isDatabaseIntegrityOk(); + boolean databaseOk = false; + if (!mEntered.get()) { + // The integrity check can retrigger the corruption handler if the database is, + // indeed, corrupted. Use mEntered to detect recursion and to skip retrying the + // integrity check on recursion. + mEntered.set(true); + databaseOk = dbObj.isDatabaseIntegrityOk(); + } + // At this point the database state has been detected and there is no further danger + // of recursion. Setting mEntered to false allows this object to be reused, although + // it is not obvious how such reuse would work. + mEntered.set(false); + // close the database try { dbObj.close(); @@ -122,4 +130,4 @@ public class DatabaseErrorHandlerTest extends AndroidTestCase { } } } -} \ No newline at end of file +} diff --git a/core/tests/coretests/src/android/database/DatabaseGeneralTest.java b/core/tests/coretests/src/android/database/DatabaseGeneralTest.java index 95b0e325c1fab..e8d90f552c3bb 100644 --- a/core/tests/coretests/src/android/database/DatabaseGeneralTest.java +++ b/core/tests/coretests/src/android/database/DatabaseGeneralTest.java @@ -914,6 +914,24 @@ public class DatabaseGeneralTest extends AndroidTestCase implements PerformanceT verifyLookasideStats(true); } + void verifyLookasideStats(boolean expectDisabled) { + boolean dbStatFound = false; + SQLiteDebug.PagerStats info = SQLiteDebug.getDatabaseInfo(); + for (SQLiteDebug.DbStats dbStat : info.dbStats) { + if (dbStat.dbName.endsWith(mDatabaseFile.getName()) && !dbStat.arePoolStats) { + dbStatFound = true; + Log.i(TAG, "Lookaside for " + dbStat.dbName + " " + dbStat.lookaside); + if (expectDisabled) { + assertTrue("lookaside slots count should be zero", dbStat.lookaside == 0); + } else { + assertTrue("lookaside slots count should be greater than zero", + dbStat.lookaside > 0); + } + } + } + assertTrue("No dbstat found for " + mDatabaseFile.getName(), dbStatFound); + } + @SmallTest public void testOpenParamsSetLookasideConfigValidation() { try { @@ -930,24 +948,6 @@ public class DatabaseGeneralTest extends AndroidTestCase implements PerformanceT } } - void verifyLookasideStats(boolean expectDisabled) { - boolean dbStatFound = false; - SQLiteDebug.PagerStats info = SQLiteDebug.getDatabaseInfo(); - for (SQLiteDebug.DbStats dbStat : info.dbStats) { - if (dbStat.dbName.endsWith(mDatabaseFile.getName())) { - dbStatFound = true; - Log.i(TAG, "Lookaside for " + dbStat.dbName + " " + dbStat.lookaside); - if (expectDisabled) { - assertTrue("lookaside slots count should be zero", dbStat.lookaside == 0); - } else { - assertTrue("lookaside slots count should be greater than zero", - dbStat.lookaside > 0); - } - } - } - assertTrue("No dbstat found for " + mDatabaseFile.getName(), dbStatFound); - } - @LargeTest public void testDefaultDatabaseErrorHandler() { DefaultDatabaseErrorHandler errorHandler = new DefaultDatabaseErrorHandler();