diff --git a/changelog/unreleased/41782 b/changelog/unreleased/41782 new file mode 100644 index 000000000000..38e866388c5a --- /dev/null +++ b/changelog/unreleased/41782 @@ -0,0 +1,14 @@ +Bugfix: Restore index usage for filecache writes on Oracle + +On Oracle every compare column of an upsert was wrapped in to_char(). That cast +is only needed for text and binary columns, which Oracle cannot compare +directly, but it was applied to all of them - and to_char(column) cannot use an +index on that column. Writes to the file cache compare storage and path_hash, +so uploads, renames and file scans could no longer use the unique index +fs_storage_path_hash and became very slow on large installations. + +Only text and binary compare columns are cast now, so every other comparison +uses its index again. + +https://github.com/owncloud/core/issues/41782 +https://github.com/owncloud/core/pull/41783 diff --git a/lib/private/DB/Adapter.php b/lib/private/DB/Adapter.php index 7e55f45fad3b..00c2865e2100 100644 --- a/lib/private/DB/Adapter.php +++ b/lib/private/DB/Adapter.php @@ -115,22 +115,44 @@ public function insertIfNotExist($table, $input, array $compare = null) { return $this->conn->executeUpdate($query, $inserts); } + /** + * Types to pass to the expression builder for the compare columns, keyed by + * column name. Only platforms that need to treat some column types + * differently in a comparison return anything here - see AdapterOCI8. + * + * A column that is missing from the result is compared as it is, which is + * what every platform except Oracle does with any type anyway, because + * ExpressionBuilder::eq() ignores its type argument. + * + * @param string $table table name including **PREFIX** + * @param string[] $compare columns that are compared to look for existing rows + * @return array column name => one of \OCP\DB\QueryBuilder\IQueryBuilder::PARAM_* + */ + protected function getCompareColumnTypes($table, array $compare) { + return []; + } + /** * Inserts, or updates a row into the database. Returns the inserted or updated rows * @param $table string table name including **PREFIX** * @param $input array the key=>value pairs to insert into the db row * @param $compare array columns that should be compared to look for existing arrays + * If this is null or an empty array, all keys of $input will be compared * @return int the number of rows affected by the operation * @throws DriverException|\RuntimeException */ public function upsert($table, $input, $compare) { - $this->conn->beginTransaction(); - $done = false; - if (empty($compare)) { $compare = \array_keys($input); } + // resolved before the transaction is opened, because it may query the schema + $compareTypes = $this->getCompareColumnTypes($table, $compare); + $isOracle = $this->conn->getDatabasePlatform() instanceof OraclePlatform; + + $this->conn->beginTransaction(); + $done = false; + // Construct the update query $qbu = $this->conn->getQueryBuilder(); $qbu->update($table); @@ -139,25 +161,19 @@ public function upsert($table, $input, $compare) { ->setParameter($col, $val); } foreach ($compare as $key) { - if ($input[$key] === null || ($input[$key] === '' && $this->conn->getDatabasePlatform() instanceof OraclePlatform)) { + if ($input[$key] === null || ($input[$key] === '' && $isOracle)) { $qbu->andWhere($qbu->expr()->isNull($key)); } else { - if ($this->conn->getDatabasePlatform() instanceof OraclePlatform) { - $qbu->andWhere( - $qbu->expr()->eq( - // needs to cast to char in order to compare with char - $qbu->createFunction('to_char(`'.$key.'`)'), // TODO does this handle empty strings on oracle correctly - $qbu->expr()->literal($input[$key]) - ) - ); - } else { - $qbu->andWhere( - $qbu->expr()->eq( - $key, - $qbu->expr()->literal($input[$key]) - ) - ); - } + $qbu->andWhere( + $qbu->expr()->eq( + $key, + $qbu->expr()->literal($input[$key]), + // on Oracle a large object column has to be cast to char in + // order to be comparable at all - every other column must be + // left alone, or the comparison cannot use an index + $compareTypes[$key] ?? null + ) + ); } } diff --git a/lib/private/DB/AdapterOCI8.php b/lib/private/DB/AdapterOCI8.php index e7a9a9e2e752..21e84bf85da1 100644 --- a/lib/private/DB/AdapterOCI8.php +++ b/lib/private/DB/AdapterOCI8.php @@ -24,7 +24,19 @@ namespace OC\DB; +use Doctrine\DBAL\Types\BlobType; +use Doctrine\DBAL\Types\TextType; +use OCP\DB\QueryBuilder\IQueryBuilder; + class AdapterOCI8 extends Adapter { + /** + * Large object columns per real table name, or null for a table whose + * columns could not be resolved. + * + * @var array + */ + private $lobColumns = []; + public function lastInsertId($table) { if ($table === null) { throw new \InvalidArgumentException('Oracle requires a table name to be passed into lastInsertId()'); @@ -48,4 +60,95 @@ public function fixupStatement($statement) { $statement = \str_ireplace('UNIX_TIMESTAMP()', self::UNIX_TIMESTAMP_REPLACEMENT, $statement); return $statement; } + + /** + * Oracle cannot compare a CLOB or a BLOB with `=` - the attempt fails with + * ORA-00932 - so those columns have to be wrapped in `to_char()`. Every + * other column has to be left alone: `to_char(col) = 'x'` is not sargable, + * so the comparison cannot use an index on `col` and degrades into a scan. + * + * If the column types cannot be resolved, all compare columns are cast. + * That is the behaviour which shipped before this distinction was made - + * slow, but it can never raise ORA-00932. + * + * @inheritdoc + */ + protected function getCompareColumnTypes($table, array $compare) { + $lobColumns = $this->getLobColumns($table); + + $types = []; + foreach ($compare as $key) { + if ($lobColumns === null || isset($lobColumns[$key])) { + $types[$key] = IQueryBuilder::PARAM_STR; + } + } + return $types; + } + + /** + * The names of all large object columns of the given table, as keys. + * + * The result is memoized per connection. A schema change within the same + * request is therefore not picked up, which is acceptable: the type of an + * existing column does not change underneath a running upsert. + * + * @param string $table table name including **PREFIX** + * @return array|null null if the columns could not be resolved + */ + private function getLobColumns($table) { + $tableName = $this->getRealTableName($table); + if (\array_key_exists($tableName, $this->lobColumns)) { + return $this->lobColumns[$tableName]; + } + + $lobColumns = null; + $reason = 'the table is not known to the schema manager'; + try { + // the identifier has to be quoted: ownCloud creates all tables and + // columns quoted, hence in lower case, while Oracle folds an unquoted + // identifier to upper case and would not find the table at all + $columns = $this->conn->getSchemaManager()->listTableColumns( + $this->conn->quoteIdentifier($tableName) + ); + if ($columns !== []) { + $lobColumns = []; + foreach ($columns as $column) { + $type = $column->getType(); + if ($type instanceof TextType || $type instanceof BlobType) { + // getName() and not the array key, because the key keeps the + // quotes of a reserved word like `oc_privatedata`.`user` + $lobColumns[$column->getName()] = true; + } + } + } + } catch (\Exception $e) { + $reason = $e->getMessage(); + } + + if ($lobColumns === null) { + // remember the failure as well, so this is logged once per table + \OC::$server->getLogger()->warning( + 'Could not determine the column types of "{table}", falling back to comparing all columns as char: {reason}', + ['app' => 'core', 'table' => $tableName, 'reason' => $reason] + ); + } + + $this->lobColumns[$tableName] = $lobColumns; + return $lobColumns; + } + + /** + * The table name as it exists in the database, for a name as it is passed to + * the query builder. Mirrors Connection::replaceTablePrefix(), which is not + * reachable from here. + * + * @param string $table table name including **PREFIX** + * @return string + */ + private function getRealTableName($table) { + if (\strpos($table, '*PREFIX*') === 0) { + $table = \substr($table, \strlen('*PREFIX*')); + } + return $this->conn->getPrefix() . $table; + } } diff --git a/lib/public/IDBConnection.php b/lib/public/IDBConnection.php index 0b444cb6df8a..98ce41e9ea6d 100644 --- a/lib/public/IDBConnection.php +++ b/lib/public/IDBConnection.php @@ -140,7 +140,9 @@ public function insertIfNotExist($table, $input, array $compare = null); * @param array $input data that should be inserted into the table (column name => value) * @param array|null $compare List of values that should be checked for "if not exists" * If this is null or an empty array, all keys of $input will be compared - * Please note: text fields (clob) must not be used in the compare array + * Please note: on Oracle a text field (clob) in the compare array is + * compared as char and therefore limited to 4000 bytes, and a binary + * field (blob) cannot be compared at all * @return int number of affected rows * @throws \Doctrine\DBAL\DBALException * @since 10.0.3 diff --git a/tests/lib/DB/AdapterTest.php b/tests/lib/DB/AdapterTest.php index 2cc82ac8df8d..2679b82967c7 100644 --- a/tests/lib/DB/AdapterTest.php +++ b/tests/lib/DB/AdapterTest.php @@ -24,7 +24,16 @@ use Doctrine\DBAL\DBALException; use Doctrine\DBAL\Driver\AbstractDriverException; use Doctrine\DBAL\Driver\DriverException; +use Doctrine\DBAL\Platforms\AbstractPlatform; +use Doctrine\DBAL\Platforms\OraclePlatform; +use Doctrine\DBAL\Platforms\SqlitePlatform; +use Doctrine\DBAL\Schema\AbstractSchemaManager; +use Doctrine\DBAL\Schema\Column; +use Doctrine\DBAL\Types\Type; +use Doctrine\DBAL\Types\Types; use OC\DB\Adapter; +use OC\DB\AdapterOCI8; +use OC\DB\Connection; use OCP\DB\QueryBuilder\IExpressionBuilder; use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\IDBConnection; @@ -186,4 +195,212 @@ public function testUpsertAndThrowOtherDriverExceptions() { $adapter = new Adapter($mockConn); $rows = $adapter->upsert('*PREFIX*appconfig', ['appid' => 'testadapter', 'configvalue' => 'test4-updated', 'configkey' => 'test4-updated'], ['appid', 'configkey']); } + + /** + * Runs an upsert against a stubbed connection and returns the type that was + * handed to the expression builder for each compare column. A column mapped + * to IQueryBuilder::PARAM_STR is the one that OCIExpressionBuilder wraps in + * to_char(); a column mapped to null is compared as it is. + * + * There is no Oracle job in CI, so the platform has to be faked here. + * + * @param string $adapterClass + * @param AbstractPlatform $platform + * @param string $table table name including *PREFIX* + * @param array $input column name => value + * @param array $compare columns to compare + * @param array|null $columns column name as stored => doctrine type name, + * or null to make the schema lookup fail + * @param string|null $expectedIdentifier the identifier the schema manager + * must be queried with, or null not to check it + * @return array compare column => type + */ + private function captureCompareTypes( + $adapterClass, + AbstractPlatform $platform, + $table, + array $input, + array $compare, + $columns, + $expectedIdentifier + ) { + $captured = []; + + $expr = $this->createMock(IExpressionBuilder::class); + $expr->method('literal')->willReturnCallback(function ($value) { + return "'$value'"; + }); + $expr->method('eq')->willReturnCallback(function ($x, $y, $type = null) use (&$captured) { + $captured[$x] = $type; + return "$x = $y"; + }); + + $qb = $this->createMock(IQueryBuilder::class); + $qb->method('expr')->willReturn($expr); + $qb->method('set')->willReturn($qb); + $qb->method('setValue')->willReturn($qb); + // pretend the update hit a row, so the insert is never attempted + $qb->method('execute')->willReturn(1); + + $schemaManager = $this->createMock(AbstractSchemaManager::class); + $listTableColumns = $schemaManager->method('listTableColumns'); + if ($expectedIdentifier !== null) { + $listTableColumns->with($expectedIdentifier); + } + if ($columns === null) { + $listTableColumns->willThrowException(new DBALException('unknown table')); + } else { + $portableColumns = []; + foreach ($columns as $name => $typeName) { + $column = new Column($name, Type::getType($typeName)); + // this is how AbstractSchemaManager keys the result: by the quoted name + $portableColumns[\strtolower($column->getQuotedName($platform))] = $column; + } + $listTableColumns->willReturn($portableColumns); + } + + $conn = $this->createMock(Connection::class); + $conn->method('getDatabasePlatform')->willReturn($platform); + $conn->method('getQueryBuilder')->willReturn($qb); + $conn->method('getSchemaManager')->willReturn($schemaManager); + $conn->method('getPrefix')->willReturn('oc_'); + $conn->method('quoteIdentifier')->willReturnCallback(function ($name) use ($platform) { + return $platform->quoteIdentifier($name); + }); + + $adapter = new $adapterClass($conn); + $this->assertEquals(1, $adapter->upsert($table, $input, $compare)); + + return $captured; + } + + /** + * The regression behind SE-1776: on Oracle the compare columns of a + * filecache upsert must not be wrapped in to_char(), because to_char(col) + * is not sargable - the unique index fs_storage_path_hash then cannot be + * used and every upload or rename degrades into an index skip scan. + */ + public function testUpsertOnOracleDoesNotCastNonLobCompareColumns() { + $types = $this->captureCompareTypes( + AdapterOCI8::class, + new OraclePlatform(), + '*PREFIX*filecache', + ['storage' => 1, 'path' => 'files/foo.txt', 'path_hash' => \md5('files/foo.txt')], + ['storage', 'path_hash'], + [ + 'storage' => Types::INTEGER, + 'path' => Types::STRING, + 'path_hash' => Types::STRING, + ], + '"oc_filecache"' + ); + + $this->assertSame(['storage' => null, 'path_hash' => null], $types); + } + + /** + * The other half: a text column in the compare array still has to be cast, + * because Oracle cannot compare a CLOB with = at all (ORA-00932). + */ + public function testUpsertOnOracleCastsLobCompareColumns() { + $types = $this->captureCompareTypes( + AdapterOCI8::class, + new OraclePlatform(), + '*PREFIX*appconfig', + ['appid' => 'core', 'configkey' => 'installedat', 'configvalue' => '1234567890'], + ['appid', 'configkey', 'configvalue'], + [ + 'appid' => Types::STRING, + 'configkey' => Types::STRING, + 'configvalue' => Types::TEXT, + ], + '"oc_appconfig"' + ); + + $this->assertSame([ + 'appid' => null, + 'configkey' => null, + 'configvalue' => IQueryBuilder::PARAM_STR, + ], $types); + } + + /** + * A column whose name is a reserved word is created quoted, and the schema + * manager returns it keyed by its quoted name - the lookup must still match + * the plain column name used in the compare array. + */ + public function testUpsertOnOracleCastsLobColumnWithQuotedName() { + $types = $this->captureCompareTypes( + AdapterOCI8::class, + new OraclePlatform(), + '*PREFIX*testtable', + ['key' => 'somekey', 'value' => 'somevalue'], + ['key', 'value'], + ['"key"' => Types::STRING, '"value"' => Types::TEXT], + '"oc_testtable"' + ); + + $this->assertSame(['key' => null, 'value' => IQueryBuilder::PARAM_STR], $types); + } + + /** + * If the column types cannot be resolved, every compare column is cast - + * the behaviour that shipped before large objects were told apart. It is + * slow, but it can never raise ORA-00932. + * + * Note that this class cannot use a data provider: its constructor takes no + * arguments, so PHPUnit has no way to hand a data set to an instance. + * + * @param array|null $columns + */ + private function assertUpsertOnOracleCastsEverything($columns) { + $types = $this->captureCompareTypes( + AdapterOCI8::class, + new OraclePlatform(), + '*PREFIX*filecache', + ['storage' => 1, 'path_hash' => \md5('files/foo.txt')], + ['storage', 'path_hash'], + $columns, + '"oc_filecache"' + ); + + $this->assertSame([ + 'storage' => IQueryBuilder::PARAM_STR, + 'path_hash' => IQueryBuilder::PARAM_STR, + ], $types); + } + + public function testUpsertOnOracleCastsEverythingWhenTheSchemaManagerThrows() { + $this->assertUpsertOnOracleCastsEverything(null); + } + + public function testUpsertOnOracleCastsEverythingWhenTheTableIsUnknown() { + $this->assertUpsertOnOracleCastsEverything([]); + } + + /** + * Every other platform compares all columns as they are, text columns + * included - ExpressionBuilder::eq() ignores the type argument there. + */ + public function testUpsertDoesNotCastAnythingOnOtherPlatforms() { + $types = $this->captureCompareTypes( + Adapter::class, + new SqlitePlatform(), + '*PREFIX*appconfig', + ['appid' => 'core', 'configkey' => 'installedat', 'configvalue' => '1234567890'], + ['appid', 'configkey', 'configvalue'], + [ + 'appid' => Types::STRING, + 'configkey' => Types::STRING, + 'configvalue' => Types::TEXT, + ], + null + ); + + $this->assertSame([ + 'appid' => null, + 'configkey' => null, + 'configvalue' => null, + ], $types); + } }