diff --git a/.github/workflows/php-unit.yml b/.github/workflows/php-unit.yml index e5940c0f9fbb..8eecc5e06324 100644 --- a/.github/workflows/php-unit.yml +++ b/.github/workflows/php-unit.yml @@ -88,6 +88,28 @@ jobs: run: | make install-composer-deps + - name: TEMPORARY DEBUG - Oracle client + if: startsWith(matrix.database,'oracle:') + env: + DSN: "(DESCRIPTION=(ADDRESS=(PROTOCOL=TCP)(HOST=127.0.0.1)(PORT=1521))(CONNECT_DATA=(SERVICE_NAME=FREEPDB1)))" + run: | + php -r 'echo "oci8: ", phpversion("oci8"), "\n";' + php -i | grep -iE "oracle|instant" || true + ls -l /opt/hostedtoolcache/setup-php/*/instantclient* 2>/dev/null || true + ldd "$(php -r 'echo ini_get("extension_dir");')/oci8.so" || true + echo "--- raw tcp to 1521 ---" + timeout 10 bash -c 'cat < /dev/null > /dev/tcp/127.0.0.1/1521' && echo "tcp ok" || echo "tcp failed" + echo "--- oci_connect (60s budget) ---" + timeout -s KILL 60 php -d display_errors=1 -r ' + $t = microtime(true); + $c = @oci_connect("owncloud", "owncloud", getenv("DSN")); + printf("connect=%s after %.1fs\n", $c ? "OK" : "FAIL", microtime(true) - $t); + if (!$c) { print_r(oci_error()); exit(1); } + $s = oci_parse($c, "select banner from v\$version"); + oci_execute($s); + print_r(oci_fetch_row($s)); + ' || echo "EXIT=$? (124/137 means it hung)" + - name: Install Server env: DATA_DIRECTORY: ${{ github.workspace }}/data @@ -126,7 +148,63 @@ jobs: --database-pass=${DB_PASSWORD}" fi - php occ ${install_cmd} + if [[ "${DB_TYPE}" == "oci" ]]; then + export DSN="(DESCRIPTION=(ADDRESS=(PROTOCOL=TCP)(HOST=${DB_HOST})(PORT=1521))(CONNECT_DATA=(SERVICE_NAME=${ORACLE_SERVICE_NAME})))" + cat > /tmp/oracle-watch.php <<'WATCHEOF' + /tmp/oracle-top-sql.php <<'TOPEOF' + /dev/null; do + sleep 60 + echo " --- elapsed=$(( $(date +%s) - START ))s ---" + timeout -s KILL 30 php /tmp/oracle-watch.php || true + done + wait $INSTALL_PID && INSTALL_RC=0 || INSTALL_RC=$? + timeout -s KILL 60 php /tmp/oracle-top-sql.php || true + if [[ $INSTALL_RC -ne 0 ]]; then exit $INSTALL_RC; fi + else + php occ ${install_cmd} + fi echo "enabling apps" php occ app:enable files_sharing php occ app:enable files_trashbin diff --git a/changelog/unreleased/41815-oracle-path-hash b/changelog/unreleased/41815-oracle-path-hash new file mode 100644 index 000000000000..8c682fdb53d7 --- /dev/null +++ b/changelog/unreleased/41815-oracle-path-hash @@ -0,0 +1,13 @@ +Bugfix: Avoid a deprecation notice when hashing the file cache path on Oracle + +Oracle cannot store empty strings, so the file cache converts them to null +before writing a row. For the storage root, whose path is the empty string, that +left md5() being called with null. PHP 8 reports that as a deprecated implicit +null to string conversion: noise in the log whenever a storage root is inserted, +and an error under PHPUnit's strict error handling. The stored path_hash itself +was never wrong, because md5(null) coerces to md5(''). + +The value is now cast to a string before hashing. + +https://github.com/owncloud/core/pull/41808 +https://github.com/owncloud/core/pull/41815 diff --git a/changelog/unreleased/41819 b/changelog/unreleased/41819 new file mode 100644 index 000000000000..a5bb06b8fc0e --- /dev/null +++ b/changelog/unreleased/41819 @@ -0,0 +1,30 @@ +Bugfix: Speed up Oracle schema introspection + +Installing and upgrading ownCloud on Oracle took an unreasonably long time. A +fresh `occ maintenance:install` on the 10.16 branch needed over 40 minutes, +while the same install on the master branch finished in well under a minute. + +The cause was the bundled doctrine/dbal 2.13, which introspects a schema by +describing every table on its own: for each table it issues one query for the +columns, one for the indexes, one for the foreign keys and one for the table +comment. Each of those queries inlines the table name as a literal, so Oracle +cannot share cursors between them and hard parses every single one, which +costs a few hundred milliseconds each. The migration code then asks for the +full schema once per applied migration, so the number of queries grows with +the number of tables multiplied by the number of migrations. With 68 +migrations and roughly 48 tables that added up to thousands of hard parsed +queries. + +Oracle schema introspection now reads the whole data dictionary with a fixed +number of queries instead of four per table. Reading a 48 table schema went +down from 194 queries to 6, and `occ maintenance:install` against Oracle on +PHP 7.4 went down from 43 minutes to 30 seconds. The resulting schema is +unchanged; it is compared against the previous implementation in the test +suite. + +doctrine/dbal does the same thing natively from version 3.4 onwards, which is +why the master branch was never affected. Upgrading doctrine/dbal on the 10.16 +branch is not an option, because its 3.x line changes public API that +third-party apps use. + +https://github.com/owncloud/core/pull/41819 diff --git a/composer.json b/composer.json index fa6faa07e713..d9d00d06e98f 100644 --- a/composer.json +++ b/composer.json @@ -2,6 +2,10 @@ "name": "owncloud/core", "description": "A safe home for all your data", "license": "AGPL-3.0-or-later", + "repositories": [ + { "type": "vcs", "url": "https://github.com/DeepDiver1975/streams" }, + { "type": "vcs", "url": "https://github.com/DeepDiver1975/SMB" } + ], "config" : { "vendor-dir": "lib/composer", "optimize-autoloader": true, diff --git a/composer.lock b/composer.lock index ff15f4828b18..fdec1686836e 100644 --- a/composer.lock +++ b/composer.lock @@ -4,7 +4,7 @@ "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies", "This file is @generated automatically" ], - "content-hash": "344e9958cd73533e9bdb08a414d19df0", + "content-hash": "68e036cb98d1342a175e02d310e0b3dc", "packages": [ { "name": "bantu/ini-get-wrapper", @@ -1349,9 +1349,15 @@ "version": "v3.7.0", "source": { "type": "git", - "url": "https://codeberg.org/icewind/SMB", + "url": "https://github.com/DeepDiver1975/SMB.git", "reference": "e6904cbe75f678335092f4861c60c656b1a99e84" }, + "dist": { + "type": "zip", + "url": "https://api.github.com/repos/DeepDiver1975/SMB/zipball/e6904cbe75f678335092f4861c60c656b1a99e84", + "reference": "e6904cbe75f678335092f4861c60c656b1a99e84", + "shasum": "" + }, "require": { "icewind/streams": ">=0.7.3", "php": ">=7.2" @@ -1368,7 +1374,25 @@ "Icewind\\SMB\\": "src/" } }, - "notification-url": "https://packagist.org/downloads/", + "autoload-dev": { + "psr-4": { + "Icewind\\SMB\\Test\\": "tests/" + } + }, + "scripts": { + "lint": [ + "parallel-lint --exclude src --exclude vendor --exclude target --exclude build ." + ], + "cs:check": [ + "php-cs-fixer fix --dry-run --diff" + ], + "cs:fix": [ + "php-cs-fixer fix" + ], + "psalm": [ + "psalm.phar" + ] + }, "license": [ "MIT" ], @@ -1379,6 +1403,10 @@ } ], "description": "php wrapper for smbclient and libsmbclient-php", + "support": { + "source": "https://github.com/DeepDiver1975/SMB/tree/v3.7.0", + "issues": "https://github.com/DeepDiver1975/SMB/issues" + }, "time": "2024-11-11T14:08:34+00:00" }, { @@ -1386,9 +1414,15 @@ "version": "v0.7.8", "source": { "type": "git", - "url": "https://codeberg.org/icewind/streams", + "url": "https://github.com/DeepDiver1975/streams.git", "reference": "cb2bd3ed41b516efb97e06e8da35a12ef58ba48b" }, + "dist": { + "type": "zip", + "url": "https://api.github.com/repos/DeepDiver1975/streams/zipball/cb2bd3ed41b516efb97e06e8da35a12ef58ba48b", + "reference": "cb2bd3ed41b516efb97e06e8da35a12ef58ba48b", + "shasum": "" + }, "require": { "php": ">=7.1" }, @@ -1403,7 +1437,11 @@ "Icewind\\Streams\\": "src/" } }, - "notification-url": "https://packagist.org/downloads/", + "autoload-dev": { + "psr-4": { + "Icewind\\Streams\\Tests\\": "tests/" + } + }, "license": [ "MIT" ], @@ -1414,6 +1452,10 @@ } ], "description": "A set of generic stream wrappers", + "support": { + "source": "https://github.com/DeepDiver1975/streams/tree/v0.7.8", + "issues": "https://github.com/DeepDiver1975/streams/issues" + }, "time": "2024-12-05T14:36:22+00:00" }, { diff --git a/lib/private/DB/Connection.php b/lib/private/DB/Connection.php index bbb97a7a30b4..0984e6b164c8 100644 --- a/lib/private/DB/Connection.php +++ b/lib/private/DB/Connection.php @@ -35,6 +35,7 @@ use Doctrine\Common\EventManager; use Doctrine\DBAL\Driver\ServerInfoAwareConnection; use Doctrine\DBAL\Platforms\MySqlPlatform; +use Doctrine\DBAL\Platforms\OraclePlatform; use Doctrine\DBAL\Schema\Schema; use OC\DB\QueryBuilder\QueryBuilder; use OCP\DB\QueryBuilder\IQueryBuilder; @@ -63,6 +64,20 @@ public function connect() { } } + /** + * On Oracle, use a schema manager that introspects the whole schema with a + * constant number of queries instead of four per table. See + * {@see \OC\DB\OracleSchemaManager} for why this is needed. + * + * @return \Doctrine\DBAL\Schema\AbstractSchemaManager + */ + public function getSchemaManager() { + if ($this->_schemaManager === null && $this->getDatabasePlatform() instanceof OraclePlatform) { + $this->_schemaManager = new OracleSchemaManager($this, $this->getDatabasePlatform()); + } + return parent::getSchemaManager(); + } + /** * Returns a QueryBuilder for the connection. * diff --git a/lib/private/DB/OracleSchemaManager.php b/lib/private/DB/OracleSchemaManager.php new file mode 100644 index 000000000000..34a6afb631a4 --- /dev/null +++ b/lib/private/DB/OracleSchemaManager.php @@ -0,0 +1,179 @@ + + * + * @copyright Copyright (c) 2026, ownCloud GmbH + * @license AGPL-3.0 + * + * This code is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License, version 3, + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License, version 3, + * along with this program. If not, see + * + */ + +namespace OC\DB; + +use Doctrine\DBAL\Schema\Identifier; +use Doctrine\DBAL\Schema\OracleSchemaManager as DoctrineOracleSchemaManager; +use Doctrine\DBAL\Schema\Table; + +/** + * Introspects the whole Oracle schema with a constant number of queries. + * + * doctrine/dbal 2.13 implements AbstractSchemaManager::listTables() by looping + * listTableDetails() over every table, which issues four data dictionary queries + * per table (columns, indexes, foreign keys, table comment). Each of those has the + * table name inlined as a string literal, so Oracle cannot share cursors and hard + * parses every single one. Because OC\DB\Migrator::getDiff() introspects the full + * schema once per applied migration, `maintenance:install` ends up running + * thousands of hard-parsed dictionary queries and takes over 20 minutes. + * + * This subclass replaces the per-table loop with one batched query per category + * over the user_* dictionary views, which is what doctrine/dbal does natively from + * 3.4 onwards. The rows are bucketed by table name and handed to the very same + * _getPortableTable*() methods the stock implementation uses, so the resulting + * Table objects are unchanged. + */ +class OracleSchemaManager extends DoctrineOracleSchemaManager { + /** + * {@inheritdoc} + */ + public function listTables() { + // keeps the schema asset filter semantics of the stock implementation + $tableNames = $this->listTableNames(); + if ($tableNames === []) { + return []; + } + + $database = $this->_conn->getDatabase(); + $columnsByTable = $this->fetchGroupedByTable($this->getBatchedTableColumnsSQL()); + $indexesByTable = $this->fetchGroupedByTable($this->getBatchedTableIndexesSQL()); + $foreignKeysByTable = $this->fetchGroupedByTable($this->getBatchedTableForeignKeysSQL()); + $commentsByTable = $this->fetchTableComments(); + + $tables = []; + foreach ($tableNames as $tableName) { + $key = $this->normalizeTableName($tableName); + + $table = new Table( + $tableName, + $this->_getPortableTableColumnList($tableName, $database, $columnsByTable[$key] ?? []), + $this->_getPortableTableIndexesList($indexesByTable[$key] ?? [], $tableName), + $this->_getPortableTableForeignKeysList($foreignKeysByTable[$key] ?? []) + ); + + // mirrors the parent's listTableDetails(): the option is set whenever + // user_tab_comments has a row, even when the comment itself is null + if (\array_key_exists($key, $commentsByTable)) { + $table->addOption('comment', $commentsByTable[$key]); + } + + $tables[] = $table; + } + + return $tables; + } + + /** + * Runs $sql and buckets the rows by their (lower cased) table_name column. + * + * @return array>> + */ + private function fetchGroupedByTable(string $sql): array { + $grouped = []; + foreach ($this->_conn->fetchAllAssociative($sql) as $row) { + $row = \array_change_key_case($row, CASE_LOWER); + $grouped[$row['table_name']][] = $row; + } + return $grouped; + } + + /** + * @return array table name => comment, null comments included + */ + private function fetchTableComments(): array { + $comments = []; + $rows = $this->_conn->fetchAllAssociative('SELECT table_name, comments FROM user_tab_comments'); + foreach ($rows as $row) { + $row = \array_change_key_case($row, CASE_LOWER); + $comments[$row['table_name']] = $row['comments']; + } + return $comments; + } + + /** + * Same shape as OraclePlatform::getListTableColumnsSQL(), minus the table + * predicate and with the comment subquery turned into a join. + */ + private function getBatchedTableColumnsSQL(): string { + return 'SELECT c.*, d.comments AS comments + FROM user_tab_columns c + LEFT JOIN user_col_comments d + ON d.table_name = c.table_name + AND d.column_name = c.column_name + ORDER BY c.table_name, c.column_id'; + } + + /** + * Same shape as OraclePlatform::getListTableIndexesSQL(), minus the table + * predicate and with the correlated subqueries turned into joins. + * + * The is_primary join is restricted to 'P' because the only thing the consumer + * (_getPortableTableIndexesList) does with the value is compare it to 'P'. + */ + private function getBatchedTableIndexesSQL(): string { + return "SELECT uind_col.table_name AS table_name, + uind_col.index_name AS name, + uind.index_type AS type, + DECODE(uind.uniqueness, 'NONUNIQUE', 0, 'UNIQUE', 1) AS is_unique, + uind_col.column_name AS column_name, + uind_col.column_position AS column_pos, + ucon.constraint_type AS is_primary + FROM user_ind_columns uind_col + LEFT JOIN user_indexes uind + ON uind.index_name = uind_col.index_name + LEFT JOIN user_constraints ucon + ON ucon.index_name = uind_col.index_name + AND ucon.constraint_type = 'P' + ORDER BY uind_col.table_name, uind_col.column_position ASC"; + } + + /** + * Same shape as OraclePlatform::getListTableForeignKeysSQL(), minus the table + * predicate and with the correlated subqueries turned into a join. + */ + private function getBatchedTableForeignKeysSQL(): string { + return "SELECT alc.table_name AS table_name, + alc.constraint_name, + alc.delete_rule, + cols.column_name AS local_column, + cols.position, + r_cols.table_name AS references_table, + r_cols.column_name AS foreign_column + FROM user_cons_columns cols + JOIN user_constraints alc + ON alc.constraint_name = cols.constraint_name + AND alc.constraint_type = 'R' + LEFT JOIN user_cons_columns r_cols + ON r_cols.constraint_name = alc.r_constraint_name + AND r_cols.position = cols.position + ORDER BY alc.table_name, cols.constraint_name ASC, cols.position ASC"; + } + + /** + * Reproduces the private OraclePlatform::normalizeIdentifier(): unquoted + * identifiers are upper cased, quoted ones keep their case. + */ + private function normalizeTableName(string $tableName): string { + $identifier = new Identifier($tableName); + return $identifier->isQuoted() ? $identifier->getName() : \strtoupper($identifier->getName()); + } +} diff --git a/lib/private/Files/Cache/Cache.php b/lib/private/Files/Cache/Cache.php index 5dc8a1a9bd01..8da525261499 100644 --- a/lib/private/Files/Cache/Cache.php +++ b/lib/private/Files/Cache/Cache.php @@ -431,7 +431,10 @@ protected function buildParts(array $data) { foreach ($data as $name => $value) { if (\array_search($name, $fields) !== false) { if ($name === 'path') { - $params[] = \md5($value); + // On Oracle an empty path has been turned into null further up, + // so cast back: the hash of the storage root must be md5('') + // on every platform. + $params[] = \md5((string)$value); $queryParts[] = '`path_hash`'; } elseif ($name === 'mimetype') { $params[] = $this->mimetypeLoader->getId(\substr($value, 0, \strpos($value, '/'))); diff --git a/tests/lib/DB/OracleSchemaManagerTest.php b/tests/lib/DB/OracleSchemaManagerTest.php new file mode 100644 index 000000000000..4d2d3abf04dd --- /dev/null +++ b/tests/lib/DB/OracleSchemaManagerTest.php @@ -0,0 +1,159 @@ + + * This file is licensed under the Affero General Public License version 3 or + * later. + * See the COPYING-README file. + */ + +namespace Test\DB; + +use Doctrine\DBAL\Logging\DebugStack; +use Doctrine\DBAL\Platforms\OraclePlatform; +use Doctrine\DBAL\Schema\Comparator; +use Doctrine\DBAL\Schema\OracleSchemaManager as DoctrineOracleSchemaManager; +use Doctrine\DBAL\Schema\Table; + +/** + * Guards the batched Oracle schema introspection in \OC\DB\OracleSchemaManager. + * + * @group DB + */ +class OracleSchemaManagerTest extends \Test\TestCase { + /** @var \OC\DB\Connection */ + private $connection; + + /** @var string[] */ + private $tableNames = []; + + /** @var string */ + private $prefix; + + protected function setUp(): void { + parent::setUp(); + + $this->connection = \OC::$server->getDatabaseConnection(); + if (!$this->connection->getDatabasePlatform() instanceof OraclePlatform) { + $this->markTestSkipped('Test only relevant on Oracle'); + } + + // Lowercase, and well below Oracle's 30 character identifier limit. + // getUniqueID() mixes in uppercase characters, which would end up in a + // differently spelled Oracle identifier than the one created below. + $this->prefix = \strtolower(self::getUniqueID('oc_bt', 8)); + } + + protected function tearDown(): void { + if ($this->connection->getDatabasePlatform() instanceof OraclePlatform) { + $this->connection->getConfiguration()->setFilterSchemaAssetsExpression(null); + foreach ($this->tableNames as $tableName) { + $this->connection->exec('DROP TABLE "' . $tableName . '" CASCADE CONSTRAINTS'); + } + } + parent::tearDown(); + } + + /** + * Creates $count additional throwaway tables and restricts introspection to + * them. Table names continue where a previous call left off, so repeated calls + * add tables instead of colliding. + * + * The identifiers are quoted, so Oracle stores them in lower case just like + * the real ownCloud tables do. Unquoted identifiers would be folded to upper + * case, which is not the code path production uses. + */ + private function createTables(int $count): void { + $schemaManager = $this->connection->getSchemaManager(); + $offset = \count($this->tableNames); + for ($i = $offset; $i < $offset + $count; $i++) { + $tableName = $this->prefix . '_' . $i; + $table = new Table('"' . $tableName . '"'); + $table->addColumn('"id"', 'integer', ['notnull' => true]); + $table->addColumn('"name"', 'string', ['length' => 64, 'notnull' => false]); + $table->setPrimaryKey(['"id"']); + $table->addIndex(['"name"'], '"' . $tableName . '_ix"'); + $schemaManager->createTable($table); + $this->tableNames[] = $tableName; + } + // listTableNames() returns lower case Oracle identifiers quoted, so the + // filter has to allow for the leading quote character + $this->connection->getConfiguration() + ->setFilterSchemaAssetsExpression('/^"?' . \preg_quote($this->prefix, '/') . '/'); + } + + /** + * @return int number of queries the given schema manager needs for a full + * schema introspection + */ + private function countIntrospectionQueries(\Doctrine\DBAL\Schema\AbstractSchemaManager $schemaManager): int { + $stack = new DebugStack(); + $this->connection->getConfiguration()->setSQLLogger($stack); + $schemaManager->createSchema(); + $this->connection->getConfiguration()->setSQLLogger(null); + return \count($stack->queries); + } + + public function testOracleConnectionUsesBatchedSchemaManager() { + $this->assertInstanceOf(\OC\DB\OracleSchemaManager::class, $this->connection->getSchemaManager()); + } + + /** + * The batched introspection must describe the database exactly like the stock + * doctrine/dbal implementation it replaces. + */ + public function testBatchedIntrospectionMatchesStockIntrospection() { + $this->createTables(3); + $platform = $this->connection->getDatabasePlatform(); + + $stockSchema = (new DoctrineOracleSchemaManager($this->connection, $platform))->createSchema(); + $batchedSchema = (new \OC\DB\OracleSchemaManager($this->connection, $platform))->createSchema(); + + // guards against both schemas coming back empty, which would make the + // comparison below pass without comparing anything + foreach ($this->tableNames as $tableName) { + $this->assertTrue($batchedSchema->hasTable($tableName), "table $tableName was not introspected"); + } + + $comparator = new Comparator(); + $this->assertSame( + [], + $comparator->compare($stockSchema, $batchedSchema)->toSql($platform), + 'batched introspection differs from the stock introspection' + ); + $this->assertSame( + [], + $comparator->compare($batchedSchema, $stockSchema)->toSql($platform), + 'stock introspection differs from the batched introspection' + ); + } + + /** + * The whole point of the batched implementation: introspecting twice as many + * tables must not cost more queries. The stock implementation needs four per + * table, which is what made `maintenance:install` on Oracle take over 20 + * minutes. + */ + public function testIntrospectionQueryCountIsIndependentOfTableCount() { + $platform = $this->connection->getDatabasePlatform(); + $batchedSchemaManager = new \OC\DB\OracleSchemaManager($this->connection, $platform); + + $this->createTables(3); + $queriesForThreeTables = $this->countIntrospectionQueries($batchedSchemaManager); + + $this->createTables(6); + $queriesForNineTables = $this->countIntrospectionQueries($batchedSchemaManager); + + $this->assertSame( + $queriesForThreeTables, + $queriesForNineTables, + 'the number of introspection queries must not grow with the number of tables' + ); + + $stockSchemaManager = new DoctrineOracleSchemaManager($this->connection, $platform); + $this->assertLessThan( + $this->countIntrospectionQueries($stockSchemaManager), + $queriesForNineTables, + 'the batched introspection must use fewer queries than the stock one' + ); + } +}