From 30fb30dfb59ae5b68d0ba570e14deb7463374044 Mon Sep 17 00:00:00 2001 From: Lee Shombert Date: Thu, 20 Jul 2023 16:58:04 -0700 Subject: [PATCH] Fix SQL crash and tests Bug: 291766410 This fixes a problem in the prepared statement cache handling. If a DDL statement is executed, the cache should be invalidated. The existing code evicted all statements from all caches synchronously with the DDL statement. This meant that caches in use by other threads were modified by the thread executing the DDL statement. This caused crashes inside sqlite under stress. The fix uses a sequence number that lets cache uses know if a prepared statement is stale. Stale statements are disposed of in the thread that owns the cache as they are fetched. Statements that are not fetched are evicted normally by the LRU cache. This change appears to be performance-neutral. The database performance tests show a +-2% skew with respect to baseline. The fix solves issues with SQLiteDatabaseTest.testStressDDLEvicts(). That test has been changed to run 5000 iterations instead of 1000, to provide a more stressful test. Two other unit tests are fixed: * DatabaseGeneralTest.testOpenDatabaseLookasidConfig(). That test was broken when the return value of SQLiteDebug.getDatabaseInfo() was modified with an extra array entry. * DatabaseErrorHandlerTest.testDatabaseIsCorrupt(). The commit that broke the test is unknown. There are no errors in the unit tests below. Test: atest * FrameworksCoreTests:android.database * CtsDatabaseTestCases Change-Id: I117241f33584cf94a1e5ca00d18268f7fbc45c3e --- .../database/sqlite/SQLiteConnection.java | 109 ++++++++++++------ .../database/sqlite/SQLiteConnectionPool.java | 11 +- .../database/DatabaseErrorHandlerTest.java | 50 ++++---- .../android/database/DatabaseGeneralTest.java | 36 +++--- 4 files changed, 130 insertions(+), 76 deletions(-) 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();