From 3babcdc4a8153df8b343670694482688c18557f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <1005065+DeepDiver1975@users.noreply.github.com> Date: Fri, 21 Aug 2026 13:46:45 +0200 Subject: [PATCH 1/2] fix(db): only cast text and binary compare columns to char on Oracle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adapter::upsert() wrapped every compare column in to_char() on Oracle. The cast is needed for CLOB and BLOB columns, which Oracle refuses to compare with = at all (ORA-00932), but to_char(column) is not sargable, so an index on the column can no longer be used. Cache::put() compares storage and path_hash - exactly the columns of the unique index fs_storage_path_hash - so every upload, rename and file scan degraded into an index skip scan. The compare column types are resolved from the schema now, and only text and binary columns are cast. If the types cannot be resolved, every column is cast, which is the previous behaviour and can never raise ORA-00932. https://github.com/owncloud/core/issues/41782 Co-Authored-By: Claude Opus 5 Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com> --- changelog/unreleased/41782 | 14 +++ lib/private/DB/Adapter.php | 56 ++++++--- lib/private/DB/AdapterOCI8.php | 103 ++++++++++++++++ lib/public/IDBConnection.php | 4 +- tests/lib/DB/AdapterTest.php | 214 +++++++++++++++++++++++++++++++++ 5 files changed, 370 insertions(+), 21 deletions(-) create mode 100644 changelog/unreleased/41782 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..061008919e5d 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,209 @@ 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); + } + + public function providesUnresolvableSchema() { + return [ + 'schema manager throws' => [null], + 'table is unknown' => [[]], + ]; + } + + /** + * 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. + * + * @dataProvider providesUnresolvableSchema + * @param array|null $columns + */ + public function testUpsertOnOracleCastsEverythingWhenSchemaIsUnavailable($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); + } + + /** + * 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); + } } From 8b12b7aec21f84a134e025e5275b0a72877530fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <1005065+DeepDiver1975@users.noreply.github.com> Date: Fri, 21 Aug 2026 14:03:27 +0200 Subject: [PATCH 2/2] test(db): replace the data provider with explicit test methods MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AdapterTest has a constructor without arguments, so PHPUnit cannot hand a data set to an instance and the provider arguments were dropped. Co-Authored-By: Claude Opus 5 Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com> --- tests/lib/DB/AdapterTest.php | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/tests/lib/DB/AdapterTest.php b/tests/lib/DB/AdapterTest.php index 061008919e5d..2679b82967c7 100644 --- a/tests/lib/DB/AdapterTest.php +++ b/tests/lib/DB/AdapterTest.php @@ -343,22 +343,17 @@ public function testUpsertOnOracleCastsLobColumnWithQuotedName() { $this->assertSame(['key' => null, 'value' => IQueryBuilder::PARAM_STR], $types); } - public function providesUnresolvableSchema() { - return [ - 'schema manager throws' => [null], - 'table is unknown' => [[]], - ]; - } - /** * 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. * - * @dataProvider providesUnresolvableSchema + * 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 */ - public function testUpsertOnOracleCastsEverythingWhenSchemaIsUnavailable($columns) { + private function assertUpsertOnOracleCastsEverything($columns) { $types = $this->captureCompareTypes( AdapterOCI8::class, new OraclePlatform(), @@ -375,6 +370,14 @@ public function testUpsertOnOracleCastsEverythingWhenSchemaIsUnavailable($column ], $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.