From d16bfb0a30882ef80f844f9fbb0775505caeeda2 Mon Sep 17 00:00:00 2001
From: Aaron Schulz <aschulz@wikimedia.org>
Date: Wed, 6 Jun 2018 14:38:47 -0700
Subject: [PATCH] rdbms: fix Sqlite::tableExists() method to avoid
 STATUS_TRX_ERROR

Sqlite used the base implementation of trying a SELECT 1 query and
seeing if it failed. Instead, make it use the sqlite_master table.
Also remove the base version of that method since it would always
cause this problem and all subclasses have proper implementations.

Make LoadBalancerTest::assertWriteAllowed() more explicit and add
more assertions there.

Change-Id: I6c7b0bea8894c45dfe8931748d6687f0e5d1e101
---
 includes/libs/rdbms/database/Database.php     | 13 +---------
 .../libs/rdbms/database/DatabaseSqlite.php    | 13 ++++++++++
 .../phpunit/includes/db/LoadBalancerTest.php  | 26 ++++++++++++-------
 .../libs/rdbms/database/DatabaseTest.php      |  1 +
 4 files changed, 31 insertions(+), 22 deletions(-)

diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php
index 16e654fe7c..ded11404c2 100644
--- a/includes/libs/rdbms/database/Database.php
+++ b/includes/libs/rdbms/database/Database.php
@@ -1925,18 +1925,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
 		}
 	}
 
-	public function tableExists( $table, $fname = __METHOD__ ) {
-		$tableRaw = $this->tableName( $table, 'raw' );
-		if ( isset( $this->sessionTempTables[$tableRaw] ) ) {
-			return true; // already known to exist
-		}
-
-		$table = $this->tableName( $table );
-		$ignoreErrors = true;
-		$res = $this->query( "SELECT 1 FROM $table LIMIT 1", $fname, $ignoreErrors );
-
-		return (bool)$res;
-	}
+	abstract public function tableExists( $table, $fname = __METHOD__ );
 
 	public function indexUnique( $table, $index ) {
 		$indexInfo = $this->indexInfo( $table, $index );
diff --git a/includes/libs/rdbms/database/DatabaseSqlite.php b/includes/libs/rdbms/database/DatabaseSqlite.php
index 2125c70fd5..2ebb1a913e 100644
--- a/includes/libs/rdbms/database/DatabaseSqlite.php
+++ b/includes/libs/rdbms/database/DatabaseSqlite.php
@@ -519,6 +519,19 @@ class DatabaseSqlite extends Database {
 		return $this->lastAffectedRowCount;
 	}
 
+	function tableExists( $table, $fname = __METHOD__ ) {
+		$tableRaw = $this->tableName( $table, 'raw' );
+		if ( isset( $this->sessionTempTables[$tableRaw] ) ) {
+			return true; // already known to exist
+		}
+
+		$encTable = $this->addQuotes( $tableRaw );
+		$res = $this->query(
+			"SELECT 1 FROM sqlite_master WHERE type='table' AND name=$encTable" );
+
+		return $res->numRows() ? true : false;
+	}
+
 	/**
 	 * Returns information about an index
 	 * Returns false if the index does not exist
diff --git a/tests/phpunit/includes/db/LoadBalancerTest.php b/tests/phpunit/includes/db/LoadBalancerTest.php
index cf605c19bd..d6b43e530b 100644
--- a/tests/phpunit/includes/db/LoadBalancerTest.php
+++ b/tests/phpunit/includes/db/LoadBalancerTest.php
@@ -190,32 +190,38 @@ class LoadBalancerTest extends MediaWikiTestCase {
 
 	private function assertWriteAllowed( Database $db ) {
 		$table = $db->tableName( 'some_table' );
+		// Trigger a transaction so that rollback() will remove all the tables.
+		// Don't do this for MySQL/Oracle as they auto-commit transactions for DDL
+		// statements such as CREATE TABLE.
+		$useAtomicSection = in_array( $db->getType(), [ 'sqlite', 'postgres', 'mssql' ], true );
 		try {
 			$db->dropTable( 'some_table' ); // clear for sanity
+			$this->assertNotEquals( $db::STATUS_TRX_ERROR, $db->trxStatus() );
 
-			// Trigger DBO_TRX to create a transaction so the flush below will
-			// roll everything here back in sqlite. But don't actually do the
-			// code below inside an atomic section becaue MySQL and Oracle
-			// auto-commit transactions for DDL statements like CREATE TABLE.
-			$db->startAtomic( __METHOD__ );
-			$db->endAtomic( __METHOD__ );
-
+			if ( $useAtomicSection ) {
+				$db->startAtomic( __METHOD__ );
+			}
 			// Use only basic SQL and trivial types for these queries for compatibility
 			$this->assertNotSame(
 				false,
 				$db->query( "CREATE TABLE $table (id INT, time INT)", __METHOD__ ),
 				"table created"
 			);
+			$this->assertNotEquals( $db::STATUS_TRX_ERROR, $db->trxStatus() );
 			$this->assertNotSame(
 				false,
 				$db->query( "DELETE FROM $table WHERE id=57634126", __METHOD__ ),
 				"delete query"
 			);
+			$this->assertNotEquals( $db::STATUS_TRX_ERROR, $db->trxStatus() );
 		} finally {
-			// Drop the table to clean up, ignoring any error.
-			$db->query( "DROP TABLE $table", __METHOD__, true );
-			// Rollback the DBO_TRX transaction for sqlite's benefit.
+			if ( !$useAtomicSection ) {
+				// Drop the table to clean up, ignoring any error.
+				$db->dropTable( 'some_table' );
+			}
+			// Rollback the atomic section for sqlite's benefit.
 			$db->rollback( __METHOD__, 'flush' );
+			$this->assertNotEquals( $db::STATUS_TRX_ERROR, $db->trxStatus() );
 		}
 	}
 
diff --git a/tests/phpunit/includes/libs/rdbms/database/DatabaseTest.php b/tests/phpunit/includes/libs/rdbms/database/DatabaseTest.php
index c12882b818..8f4aae365f 100644
--- a/tests/phpunit/includes/libs/rdbms/database/DatabaseTest.php
+++ b/tests/phpunit/includes/libs/rdbms/database/DatabaseTest.php
@@ -440,6 +440,7 @@ class DatabaseTest extends PHPUnit\Framework\TestCase {
 			'numFields', 'numRows',
 			'open',
 			'strencode',
+			'tableExists'
 		];
 		$db = $this->getMockBuilder( Database::class )
 			->disableOriginalConstructor()
-- 
2.20.1