Optimize OracleSchemaManager::listTables #2766

mondrake · 2017-07-03T13:31:29Z

A PR replicating the patch included in #2676 by @mathieubouchard .

Tests executed on continuousphp show that running createSchema on an Oracle db with about 170 tables takes over a minute. See #2767.

Time: 4.18 minutes, Memory: 80.00MB 
 
There was 1 failure: 
 
1) Doctrine\Tests\DBAL\Functional\Schema\OracleSchemaManagerTest::testCreateSchemaOnLargeNumberOfTables 
createSchema() executed in less than 15 sec. 
Failed asserting that 64.17334699630737 is less than 15. 
 
tests/Doctrine/Tests/DBAL/Functional/Schema/OracleSchemaManagerTest.php:292 
 
FAILURES! 
Tests: 4613, Assertions: 8763, Failures: 1, Skipped: 519, Incomplete: 11.

Ocramius · 2017-07-03T13:34:07Z

That's a lot of additional code with no additional tests covering all the newly introduced decision paths: if this patch is not a functional change, I'm inclined to rejecting it, sorry.

mondrake · 2017-07-03T13:40:47Z

@Ocramius can you please leave it open for a while and see if I or anyone else can add appropriate tests. The underlying issue is relevant, you can't really use productively schema manager's createSchema() over an Oracle database that has some tables in it.

Ocramius · 2017-07-03T13:44:01Z

That's fine by me: I realise that you put a lot of effort in it, but any code path that isn't tested is a possible bug if not a security issue, so it won't be merged until carefully tested. Since this code runs oracledb specific queries, the tests should be integration ones.

…

On 3 Jul 2017 15:40, "mondrake" ***@***.***> wrote: @Ocramius <https://github.com/ocramius> can you please leave it open for a while and see if I or anyone else can add appropriate tests, The underlying issue is relevant, you can't really use productively schema manager's createSchema() over an Oracle database that has some tables in it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2766 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJakMJL65BfXWZkp7UQSOyZ2kAeKTbNks5sKO9ggaJpZM4OMSnL> .

…ge number of tables.

mondrake · 2017-07-04T12:27:17Z

Added a functional test to create 150 test tables, then run createSchema on the database. The test fails, apparently there are some issues with the original patch. I will create a separate PR just with the test, which is relevant anyway in order to measure the time createSchema needs to run.

mathieubouchard · 2017-07-04T13:15:56Z

Looking at the code of the Oracle test file, it seems there are some tables created for other tests that are not deleted before your new method create it's 150 new tables. So the total number of tables in the database will be greater than 150.

mondrake · 2017-07-04T13:26:15Z

@mathieubouchard yes, I changed the test in #2767 to test for more than 150 tables, but also most importantly to measure the execution time.

Any idea why we have this failure in your original code

Caused by 
PHPUnit_Framework_Error_Notice: Undefined index: table_name 
 
lib/Doctrine/DBAL/Schema/OracleSchemaManager.php:438 
lib/Doctrine/DBAL/Schema/OracleSchemaManager.php:379 
tests/Doctrine/Tests/DBAL/Functional/Schema/OracleSchemaManagerTest.php:36

which affects also other tests?

…esForeignKeysSQL().

mathieubouchard · 2017-07-04T16:51:32Z

The problem is with the use of array_key_exists($foreignKeyRow['table_name'], $tablesForeignKeys) in the method listTablesForeignKeys. the table_name column is not in foreignKeyRow the first time a column is parsed for a given table.

    private function listTablesForeignKeys($database = null)
    {
        $sql = $this->_platform->getListTablesForeignKeysSQL($database);
        $tablesForeignKeysRows = $this->_conn->fetchAll($sql);

        $tablesForeignKeys = array();
        foreach ($tablesForeignKeysRows as $foreignKeyRow) {
            $foreignKeyRow = \array_change_key_case($foreignKeyRow, CASE_LOWER);
            
            if (!array_key_exists($foreignKeyRow['table_name'], $tablesForeignKeys)) {
                $tablesForeignKeys[$foreignKeyRow['table_name']] = array($foreignKeyRow);
            } else {
                $tablesForeignKeys[$foreignKeyRow['table_name']][] = $foreignKeyRow;
            }
        }

There should be an additional check for array_key_exists('table_name', $foreignKeyRow) before the assignment.

Per the PHP documentation, this comportement is :

Attempting to access an array key which has not been defined is the same as accessing any other undefined variable: an E_NOTICE-level error message will be issued, and the result will be NULL.

I suppose that the PHPUnit test considers an E_NOTICE as an error and throw the PHPUnit_Framework_Error_Notice exception in that case.

mondrake · 2017-07-04T16:56:02Z

Thanks @mathieubouchard , is your comment already based on the latest commit 2ffcec0?

mathieubouchard · 2017-07-04T17:05:17Z

Based on the error detailed in the following build: https://app.continuousphp.com/git-hub/doctrine/dbal/build/41d54cad-99e4-4e4c-be18-4c06fb7fa26c

mondrake · 2017-07-05T08:40:49Z

OK, so I think that this comment from #2676

... optimize the creation of a Schema from an Oracle database by overriding the OracleSchemaManager->listTables method to get the list of columns, foreign keys and indexes one time for all the tables in the current database. That information is then used to create the table schemas.

is the way to go - that means 4 SQL queries regardless of the number of tables in the database, instead of [1 (get the tables) + #tables * 3 (to get for each table its columns, indexes and foreign keys separately)]. For 150 tables that means 4 queries instead of 451.

However, the changes so far need further work. My suggestion for now is to focus on the test - make it more robust by adding indexes and foreign keys to the test case, and make it pass on currrent code, with the current performance. With that done, then turn to the approach above.

…db assets.

…ectness.

…ys of test tables.

mondrake · 2017-07-05T14:30:59Z

The test now fails only on the last assert, that measures the performance of createSchema. We have tables, indexes, foreign keys created in a large number (150), including a table with quoted identifiers. The test checks the consistency of the objects returned by createSchema, so that now we can go back adding the code of the original patch and override the ::listTables method with one specific for Oracle, and see that it keeps returning same results.

mondrake · 2017-07-05T15:02:52Z

Maybe we do not need to add separate methods to OraclePlatform to return SQL for all tables' columns / indexes / foreign keys.

If we accept a convention that passing null as $table parameter to getListTableColumnsSQL, getListTableIndexesSQL, getListTableForeignKeysSQL, we return the SQL for enquiring the entire database, we can fit in the current methods.

Doing that for getListTableColumnsSQL in next commit. Just checking that we do not break anything, no implementation yet.

…es' columns in a single query.

mathieubouchard · 2017-07-05T15:35:45Z

The first approach I had was to have only one method to support both use cases. This will reduce the number of lines of code changed and eliminate duplicate SQL code.

But this is a breaking change on the AbstractPlatform API. That's why I created other public methods, specific to Oracle.

mondrake · 2017-07-05T15:38:26Z

Thanks @mathieubouchard

this is a breaking change on the AbstractPlatform API

Why?

mathieubouchard · 2017-07-05T15:45:03Z

The method signatures doesn't accept null for the table parameter, for example :
http://www.doctrine-project.org/api/dbal/2.0/class-Doctrine.DBAL.Platforms.AbstractPlatform.html#_getListTableColumnsSQL

mondrake · 2017-07-05T15:51:14Z

Ah OK, but there's doesn't seem to be any code checking or preventing passing null there, neither in the abstract class nor in its implementations. Would it be a major problem to allow that?

Maybe this is a question for @Ocramius .

morozov · 2017-10-09T18:46:41Z

It would solve the issue I mentioned above but still would leave the possibility for other similar issues to exist. To me, it would make more sense to remove code duplication instead of adding more tests which cover the same cases.

Why do we need two implementations per use case?

… methods

mondrake · 2017-10-31T16:09:26Z

@morozov this is now taking away the code duplication. getListTableColumnsSQL, getListTableForeignKeysSQL, and getListTableIndexesSQL remain as one-liners calling the new methods that can all manage to fetch info for either the full db or a single table.

ContinuousPHP that runs Oracle tests is green.

morozov

Structure-wise looks good to me.

@mondrake please see some more comments.

Someone from the team, please take a look at the usage of quoted/unquoted table names here. I'm not really familiar with the APIs which produce the quoted ones.

morozov · 2017-11-03T20:18:54Z

lib/Doctrine/DBAL/Platforms/OraclePlatform.php

+ }
+ else {
+ $quotedDatabaseIdentifier = "(SELECT SYS_CONTEXT('userenv', 'current_schema') FROM DUAL)";
+ }


Could the block from the beginning of the method until here moved into a method? Looks like it's the same everywhere.

morozov · 2017-11-03T20:19:35Z

lib/Doctrine/DBAL/Platforms/OraclePlatform.php

+ $quotedDatabaseIdentifier = $this->quoteStringLiteral($databaseIdentifier->getName());
+ }
+ else {
+ $quotedDatabaseIdentifier = "(SELECT SYS_CONTEXT('userenv', 'current_schema') FROM DUAL)";


It's not a quoted identifier, it's an expression. The variable name is confusing.

Do we really want to execute this sub-query for every row in the query? I believe the current DB name should be obtained in one query and then used as a value in the second.

The point here is that we do not have a connection to query against in the Platform object.

Then we should use ALL_* objects for the non-current schema and USER_* ones for the current one.

Because in the new concept we use the ALL_* views that provide information about Oracle DBMS objects, instead of the slower USER_* views that do a lot of more checking about user being authorised to access an object (checked with EXPLAIN PLAN).

Is using USER_* slower than using a sub-query on each row of ALL_*?

morozov · 2017-11-03T20:30:00Z

lib/Doctrine/DBAL/Platforms/OraclePlatform.php

+ *
+ * @return string
+ */
+ public function getListIndexesSQL(?string $database, ?string $table): string


This function should be made private since it implements two different behaviors (all indices and table indices). To retrieve all indices, the caller should use another method like getListAllIndexesSQL() which would call this one with the $table = NULL. Thus, we'll have a cleaner public API.

morozov · 2017-11-03T20:31:21Z

lib/Doctrine/DBAL/Platforms/OraclePlatform.php

+ else {
+ $quotedDatabaseIdentifier = "(SELECT SYS_CONTEXT('userenv', 'current_schema') FROM DUAL)";
+ }
+ $tableCondition = NULL;


Technically, it's an empty string, not NULL.

morozov · 2017-11-03T20:33:32Z

lib/Doctrine/DBAL/Platforms/OraclePlatform.php

+ FROM all_ind_columns ind_col
+ LEFT JOIN all_indexes ind ON ind.owner = ind_col.index_owner AND ind.index_name = ind_col.index_name
+ LEFT JOIN all_constraints con ON con.owner = ind_col.index_owner AND con.index_name = ind_col.index_name
+ WHERE ind_col.index_owner = $quotedDatabaseIdentifier $tableCondition


The space between $quotedDatabaseIdentifier and $tableCondition should be part of the non-empty value of $tableCondition. Otherwise, we'll have an unneeded trailing space in case of empty $tableCondition.

lib/Doctrine/DBAL/Platforms/OraclePlatform.php

morozov · 2017-11-03T21:17:30Z

lib/Doctrine/DBAL/Schema/OracleSchemaManager.php

+
+ $tables = [];
+ foreach ($tableNames as $tableName) {
+ $unquotedTableName = trim($tableName, '"');


The quote should be replaced with $this->_platform->getIdentifierQuoteCharacter(). It's not an unquoted table name, it's just a table name. And the original $tableName is the quoted table name.

morozov · 2017-11-03T21:21:15Z

lib/Doctrine/DBAL/Platforms/OraclePlatform.php

+ con.constraint_type AS is_primary
+ FROM all_ind_columns ind_col
+ LEFT JOIN all_indexes ind ON ind.owner = ind_col.index_owner AND ind.index_name = ind_col.index_name
+ LEFT JOIN all_constraints con ON con.owner = ind_col.index_owner AND con.index_name = ind_col.index_name


Extra whitespace in ON con.owner.

morozov · 2017-11-03T21:26:55Z

lib/Doctrine/DBAL/Platforms/OraclePlatform.php

+ * Returns the SQL for a list of indexes in the database.
+ *
+ * @param string $database
+ * @param string $table


It would be nice to have some explanation of the meaning of the parameters given they are nullable.

morozov · 2017-11-03T21:30:45Z

lib/Doctrine/DBAL/Schema/OracleSchemaManager.php

+
+ $indexes = [];
+ if (isset($indexesByTable[$unquotedTableName])) {
+ $indexes = $this->_getPortableTableIndexesList($indexesByTable[$unquotedTableName], $tableName);


Does _getPortableTableIndexesList() expect the quoted table name as the 2nd parameter? I cannot see it used anywhere in the implementation of _getPortableTableIndexesList() besides dispatching some event. Could you please explain what it is for here?

Just passing over to honour the arguments and the chain or parent calls. The code here does not do anything with that.

morozov · 2017-11-03T21:34:08Z

The type of OracleSchemaManager#$_platform should be changed to OraclePlatform since it uses the newly introduced Oracle-specific methods.

mondrake · 2017-12-15T13:19:13Z

@morozov made the changes you suggested. Some comments from my side added to yours that now show having been outdated.

morozov · 2017-12-16T02:35:07Z

lib/Doctrine/DBAL/Platforms/OraclePlatform.php

+ *
+ * @return string
+ */
+ public function getListAllIndexesSQL(?string $database = null): string


It should be either nullable or optional, not both. I think the former is preferable.

I'll strongly disagree, it should be either defaulting to null AND nullable ~~or neither~~ or just nullable. Optionality and nullablility are two things (and for null, the former implies the latter, but not the other way). Missing nullability sign for parameters defaulting to null is PHP's design flaw introduced only because 7.0 had no nullables.

Majkl578

Just some CS nitpicking, mostly.

Majkl578 · 2017-12-16T03:12:09Z

lib/Doctrine/DBAL/Platforms/OraclePlatform.php

+ *
+ * @param string $database The database schema. If NULL or '/', the currently active schema will be queried.
+ *
+ * @return string


Please remove useless (duplicated) @return, it doesn't add any extra information.

Majkl578 · 2017-12-16T03:12:26Z

lib/Doctrine/DBAL/Platforms/OraclePlatform.php

+ $databaseIdentifier = $this->normalizeIdentifier($database);
+ return $this->quoteStringLiteral($databaseIdentifier->getName());
+ }
+ else {


no need for else, we have return above

Majkl578 · 2017-12-16T03:13:06Z

lib/Doctrine/DBAL/Platforms/OraclePlatform.php

+ $tableCondition = " AND ind_col.table_name = $quotedTableIdentifier";
+ }
+ return
+<<<SQL


Should be on the same line as return, but I'm not sure ATM how this is handled by CS.

Majkl578 · 2017-12-16T03:13:40Z

lib/Doctrine/DBAL/Platforms/OraclePlatform.php

+ * @param string $database The database schema. If NULL or '/', the currently active schema will be queried.
+ * @param string $table The table. If left NULL, the SQL will return all the indexes in the database.
+ *
+ * @return string


this @return is also useless

Majkl578 · 2017-12-16T03:13:49Z

lib/Doctrine/DBAL/Platforms/OraclePlatform.php

+ *
+ * @param string $database The database schema. If NULL or '/', the currently active schema will be queried.
+ *
+ * @return string


this @return is also useless

Majkl578 · 2017-12-16T03:14:21Z

lib/Doctrine/DBAL/Platforms/OraclePlatform.php

+ *
+ * @param string $database The database schema. If NULL or '/', the currently active schema will be queried.
+ *
+ * @return string


this @return is also useless

Majkl578 · 2017-12-16T03:14:29Z

lib/Doctrine/DBAL/Platforms/OraclePlatform.php

+ * @param string $database The database schema. If NULL or '/', the currently active schema will be queried.
+ * @param string $table The table. If left NULL, the SQL will return all the indexes in the database.
+ *
+ * @return string


this @return is also useless

Majkl578 · 2017-12-16T03:14:33Z

lib/Doctrine/DBAL/Platforms/OraclePlatform.php

+ *
+ * @param string $database The database schema. If NULL or '/', the currently active schema will be queried.
+ *
+ * @return string


this @return is also useless

Majkl578 · 2017-12-16T03:14:41Z

lib/Doctrine/DBAL/Platforms/OraclePlatform.php

+ * @param string $database The database schema. If NULL or '/', the currently active schema will be queried.
+ * @param string $table The table. If left NULL, the SQL will return all the indexes in the database.
+ *
+ * @return string


this @return is also useless

Majkl578 · 2017-12-16T03:15:05Z

lib/Doctrine/DBAL/Platforms/OraclePlatform.php

+ {
+ $databaseCondition = $this->getDatabaseCondition($database);
+ $tableCondition = '';
+ if ($table !== null) {


please add newlines around if to improve readability

adressed

mondrake · 2017-12-21T13:55:13Z

@Ocramius can we remove the Missing Tests tag? That's been addressed based on @morozov directions.

# Conflicts: # lib/Doctrine/DBAL/Platforms/OraclePlatform.php # tests/Doctrine/Tests/DBAL/Platforms/OraclePlatformTest.php

…ke/dbal into opt_oracle_schema_manager

mondrake · 2021-01-22T09:25:00Z

The problem is still there, schema introspection is practically unusable on an Oracle database. If anyone interested, https://github.com/mondrake/dbal/tree/oracle-schema-list-tables has the fix for DBAL 3.

greg0ire · 2021-01-22T12:10:40Z

Sorry, I didn't mean to close this, I just meant to rename master to 4.0.x. Why does it say unknown repository as source repository?

mondrake · 2021-01-22T16:44:42Z

@greg0ire because in the meantime I actually dropped and re-forked the repo, so the originally referenced fork branch is gone. Probably the best course is to keep this PR closed now, and open a new one instead.

Optimize Oracle SchemaManager #2676

95e0a7b

Adding a functional test for Oracle to create schema on a db with lar…

dec643f

…ge number of tables.

mondrake mentioned this pull request Jul 4, 2017

Test only for Optimize Oracle SchemaManager #2767

Closed

mondrake added 2 commits July 4, 2017 15:45

Fixes for listTables() and adjusted test.

bf1a170

Fixes for OraclePlatform::getListTablesColumnsSQL() and ::getListTabl…

2ffcec0

…esForeignKeysSQL().

mondrake added 7 commits July 5, 2017 11:11

Temporarily removed changes, extended test method to include various …

1448b5d

…db assets.

Fixing syntax for foreign keys, adding initial checks for schema corr…

ae73a14

…ectness.

Adding asserts to check columns, primary keys, indexes and foreign ke…

cf51fa3

…ys of test tables.

getColumns and getIndexes return arrays of objects, not names - fixed.

2133c82

Various fixes to test method to align with current state result.

e03bc4e

Fixed a typo in a helper method name.

d0a9cca

Fixing order of array keys in the test.

8a789f6

Allow getListTableColumnsSQL to return SQL to fetch all database tabl…

d1bc080

…es' columns in a single query.

Fixes for hardcoded SQL string tests

40e3ef7

mondrake added 2 commits October 31, 2017 16:48

Unify OraclePlatform SQL for columns/indexes/foreign keys, keeping BC…

a0ef3d8

… methods

Fixing a missing parameter in method calls

9b94db2

morozov reviewed Nov 3, 2017

View reviewed changes

mondrake added 3 commits December 15, 2017 12:46

Requested changes

5308389

Do not linefeed before <<<SQL

6027ade

Fixed PHP 7.1 syntax

1183a5e

morozov reviewed Dec 16, 2017

View reviewed changes

Majkl578 previously requested changes Dec 16, 2017

View reviewed changes

Requested changes.

fcbf41d

Majkl578 removed the Missing Tests label Dec 21, 2017

mondrake added 5 commits June 20, 2019 18:19

Merge branch 'master' into opt_oracle_schema_manager

514ab47

# Conflicts: # lib/Doctrine/DBAL/Platforms/OraclePlatform.php # tests/Doctrine/Tests/DBAL/Platforms/OraclePlatformTest.php

Merge branch 'opt_oracle_schema_manager' of https://github.com/mondra…

b780a16

…ke/dbal into opt_oracle_schema_manager

PHPCS fixes.

98597c0

PHPCS fixes.

706c4c3

Align name of test protected variables to master.

b25bfc6

mondrake removed their assignment Oct 25, 2020

greg0ire closed this Jan 22, 2021

greg0ire deleted the branch doctrine:master January 22, 2021 07:44

mondrake mentioned this pull request Oct 16, 2021

Optimize schema managers' ::listTables() methods #4882

Closed

6 tasks

morozov mentioned this pull request Feb 12, 2022

Optimize schema managers' ::listTables() methods #5268

Merged

2 tasks

github-actions bot locked as resolved and limited conversation to collaborators Jul 24, 2022

Optimize OracleSchemaManager::listTables #2766

Optimize OracleSchemaManager::listTables #2766

Conversation

mondrake commented Jul 3, 2017 • edited Loading

Ocramius commented Jul 3, 2017

mondrake commented Jul 3, 2017 • edited Loading

Ocramius commented Jul 3, 2017 via email

mondrake commented Jul 4, 2017

mathieubouchard commented Jul 4, 2017

mondrake commented Jul 4, 2017

mathieubouchard commented Jul 4, 2017

mondrake commented Jul 4, 2017

mathieubouchard commented Jul 4, 2017

mondrake commented Jul 5, 2017

mondrake commented Jul 5, 2017

mondrake commented Jul 5, 2017 • edited Loading

mathieubouchard commented Jul 5, 2017

mondrake commented Jul 5, 2017

mathieubouchard commented Jul 5, 2017

mondrake commented Jul 5, 2017

morozov commented Oct 9, 2017

mondrake commented Oct 31, 2017 • edited Loading

morozov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morozov Nov 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morozov commented Nov 3, 2017

mondrake commented Dec 15, 2017

Choose a reason for hiding this comment

Majkl578 Dec 16, 2017 • edited Loading

Choose a reason for hiding this comment

Majkl578 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mondrake commented Dec 21, 2017

mondrake commented Jan 22, 2021

greg0ire commented Jan 22, 2021

mondrake commented Jan 22, 2021

mondrake commented Jul 3, 2017 •

edited

Loading

mondrake commented Jul 3, 2017 •

edited

Loading

mondrake commented Jul 5, 2017 •

edited

Loading

mondrake commented Oct 31, 2017 •

edited

Loading

morozov Nov 3, 2017 •

edited

Loading

Majkl578 Dec 16, 2017 •

edited

Loading