Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize OracleSchemaManager::listTables #2766

Closed
wants to merge 54 commits into from
Closed
Show file tree
Hide file tree
Changes from 33 commits
Commits
Show all changes
54 commits
Select commit Hold shift + click to select a range
95e0a7b
Optimize Oracle SchemaManager #2676
Jul 3, 2017
dec643f
Adding a functional test for Oracle to create schema on a db with lar…
Jul 4, 2017
bf1a170
Fixes for listTables() and adjusted test.
Jul 4, 2017
2ffcec0
Fixes for OraclePlatform::getListTablesColumnsSQL() and ::getListTabl…
Jul 4, 2017
1448b5d
Temporarily removed changes, extended test method to include various …
Jul 5, 2017
ae73a14
Fixing syntax for foreign keys, adding initial checks for schema corr…
Jul 5, 2017
cf51fa3
Adding asserts to check columns, primary keys, indexes and foreign ke…
Jul 5, 2017
2133c82
getColumns and getIndexes return arrays of objects, not names - fixed.
Jul 5, 2017
e03bc4e
Various fixes to test method to align with current state result.
Jul 5, 2017
d0a9cca
Fixed a typo in a helper method name.
Jul 5, 2017
8a789f6
Fixing order of array keys in the test.
Jul 5, 2017
d1bc080
Allow getListTableColumnsSQL to return SQL to fetch all database tabl…
Jul 5, 2017
40e3ef7
Fixes for hardcoded SQL string tests
Jul 5, 2017
9c86b88
Implementation of OracleSchemaManager::listTables override.
Jul 5, 2017
8f2762f
Allow getListTableColumnsSQL to return SQL to fetch all database tabl…
Jul 6, 2017
7947849
Implementation of OracleSchemaManager::listTables override.
Jul 6, 2017
09bdf2d
Use LEFT JOINS instead of subselects in getListTableIndexesSQL
Jul 6, 2017
d4f28bc
Changed order of the tables and joins in the FROM clause of getListTa…
Jul 6, 2017
c6df19c
More changes in getListTableIndexesSQL
Jul 6, 2017
872ca93
Using ALL_* views instead of USER_* views to fetch indexes
Jul 6, 2017
73b5383
Connection object is not available in AbstractPlatform
Jul 6, 2017
844badd
Forgot removing call to listTableIndexes.
Jul 6, 2017
e2b7711
Use LEFT JOINS instead of subselects in getListTableColumnsSQL
Jul 6, 2017
4db79e8
Adjusted OraclePlatformTest::getReturnsGetListTableColumnsSQL for the…
Jul 6, 2017
ed83bec
Implementation of OracleSchemaManager::listTables override.
Jul 7, 2017
7bd9ca6
Stretch test to creating 2150 tables and expecting processing in 3 se…
Jul 7, 2017
8c505e3
Reduced number of test tables to allow test to run without errors on …
Jul 7, 2017
e205f62
Reverted timing check for createSchema to 15 sec.
Jul 7, 2017
e29e546
Implement requested changes.
Jul 10, 2017
d8329c7
Fixed a Scrutinizer warning about the variable $quotedTableIdentifier…
Jul 10, 2017
1090c87
Fix for Scrutinizer still complaining about $quotedTableIdentifier
Jul 10, 2017
8492bf0
Added tests for OracleSchemaManager::getListTableIndexesSQL, ::getLis…
Jul 12, 2017
816d973
Use nullable syntax in OraclePlatform::_getPortableTableDefinition
Jul 18, 2017
e06174d
Revert some of the changes according to latest comments, and introduc…
Sep 6, 2017
554fbd7
Requested changes on SQL syntax and pre-grouping of records by table …
Sep 7, 2017
fab02a7
Adding debugging code
Sep 7, 2017
0bc13ec
More debugging
Sep 7, 2017
fba72c9
Fix for quoted table names
Sep 7, 2017
b7c9c79
Debugging for test on number of SQL queries executed by createSchema
Sep 7, 2017
26d3d02
Adding test for number of queries expected to be run by createSchema
Sep 7, 2017
a4dba40
Simplify OracleSchemaManager::createSchema
Sep 8, 2017
78c8517
Simplified tests and removed some out-of-scope changes.
Sep 8, 2017
760ffb3
Fixed typos.
Sep 8, 2017
a0ef3d8
Unify OraclePlatform SQL for columns/indexes/foreign keys, keeping BC…
Oct 31, 2017
9b94db2
Fixing a missing parameter in method calls
Oct 31, 2017
5308389
Requested changes
Dec 15, 2017
6027ade
Do not linefeed before <<<SQL
Dec 15, 2017
1183a5e
Fixed PHP 7.1 syntax
Dec 15, 2017
fcbf41d
Requested changes.
mondrake Dec 16, 2017
514ab47
Merge branch 'master' into opt_oracle_schema_manager
mondrake Jun 20, 2019
b780a16
Merge branch 'opt_oracle_schema_manager' of https://github.com/mondra…
mondrake Jun 20, 2019
98597c0
PHPCS fixes.
mondrake Jun 20, 2019
706c4c3
PHPCS fixes.
mondrake Jun 20, 2019
b25bfc6
Align name of test protected variables to master.
mondrake Jun 20, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
177 changes: 144 additions & 33 deletions lib/Doctrine/DBAL/Platforms/OraclePlatform.php
Original file line number Diff line number Diff line change
Expand Up @@ -418,10 +418,45 @@ protected function _getCreateTableSQL($table, array $columns, array $options = a
*/
public function getListTableIndexesSQL($table, $currentDatabase = null)
{
$table = $this->normalizeIdentifier($table);
$table = $this->quoteStringLiteral($table->getName());
if (null === $table && !$this->isDatabaseSelected($currentDatabase)) {
Copy link
Member

@morozov morozov Jul 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we get the schema name from connection parameters (username)? Or if it's possible to not specify it (not sure) can we detect it from the environment? Something like:

SELECT SYS_CONTEXT('USERENV', 'CURRENT_SCHEMA') FROM DUAL

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Connection is not known within AbstractPlatform and children classes, so we cannot use getParams here to get the schema name. Getting from USERENV would mean a separate SQL call to the DB which IMHO we should avoid if possible. Accessing USER* views is there to address that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree.

throw new \InvalidArgumentException('Table name must be specified if no database is specified');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block is not currently covered by tests

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added tests.

}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


$quotedTableIdentifier = '';
$tableWhereClause = '';
if (null !== $table) {
$tableIdentifier = $this->normalizeIdentifier($table);
$quotedTableIdentifier = $this->quoteStringLiteral($tableIdentifier->getName());
$tableWhereClause = "AND ind_col.table_name = " . $quotedTableIdentifier;
}

return "SELECT uind_col.index_name AS name,
// If database is specified, return SQL for all indexes in database,
// optionally filtered by table.
if ($this->isDatabaseSelected($currentDatabase)) {
$databaseIdentifier = $this->normalizeIdentifier($currentDatabase);
$quotedDatabaseIdentifier = $this->quoteStringLiteral($databaseIdentifier->getName());
return <<<SQL
SELECT ind_col.table_name as table_name,
ind_col.index_name AS name,
ind.index_type AS type,
decode(ind.uniqueness, 'NONUNIQUE', 0, 'UNIQUE', 1) AS is_unique,
ind_col.column_name AS column_name,
ind_col.column_position AS column_pos,
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
WHERE ind_col.index_owner = $quotedDatabaseIdentifier $tableWhereClause
ORDER BY ind_col.table_name, ind_col.index_name, ind_col.column_position
SQL;
}

// If database is not specified, return SQL for the indexes of the
// specified table from the current database.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need to maintain two branches for the specified and non-specified schema name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments above about using ALL* vs USER* views.

return <<<SQL
SELECT uind_col.index_name AS name,
(
SELECT uind.index_type
FROM user_indexes uind
Expand All @@ -446,8 +481,9 @@ public function getListTableIndexesSQL($table, $currentDatabase = null)
WHERE ucon.index_name = uind_col.index_name
) AS is_primary
FROM user_ind_columns uind_col
WHERE uind_col.table_name = " . $table . "
ORDER BY uind_col.column_position ASC";
WHERE uind_col.table_name = $quotedTableIdentifier
ORDER BY uind_col.column_position ASC
SQL;
}

/**
Expand Down Expand Up @@ -610,31 +646,80 @@ private function getAutoincrementIdentifierName(Identifier $table)
*/
public function getListTableForeignKeysSQL($table)
{
$table = $this->normalizeIdentifier($table);
$table = $this->quoteStringLiteral($table->getName());
return $this->getListForeignKeysSQL($table);
}

/**
* Returns the list of foreign keys for the current database.
*
* @param string $table
* @param string $database
*
* @return string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this @return is also useless

*/
public function getListForeignKeysSQL($table = null, $database = null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given it's not an API method, should it be public?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be two (sets of) methods: getTableSomething($table) and getAllSomething(). Otherwise, the single function has two paths which don't share much, and a mistakenly defined value of $table may change the resulting function logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method needs to be called by OracleSchemaManager, hence the need to have it public. Are you suggesting to increase the public API to add getAllSomething() methods? I'd be fine with that, we would have to add those to the AbstractPlatform class though... need @Ocramius input here I suppose.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be fine with that, we would have to add those to the AbstractPlatform class though...

Isn't it true about introducing getListForeignKeysSQL() only for the OraclePlatform?

I think it would be safe for the OracleSchemaManager to depend on OraclePlatform instead of the AbstractPlatform. In this case, it won't have to be implemented in all platforms. Not sure how backward compatible it is.

The problem with the same method returning both "table something" and "all something" is that if there's a bug in the calling code which for some reason populates $table with NULL and calls the "magic" method to get its "something", instead of getting an empty results (because there's no null table) it will get everything. It's better to have predictable static return type for a method than many of them depending on the values of the arguments.

For example, it can be refactored using the following approach:

function getTableSomething($table)
{
    selectSomething("WHERE table = $table");
}

function getAllSomething($table)
{
        selectSomething('');
}

function selectSomething($filter)
{
}

{
if (null === $table && !$this->isDatabaseSelected($database)) {
throw new \InvalidArgumentException('Table name must be specified if no database is specified');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block is not currently covered by tests

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this method was private, we could rely on the calling code and not make this check at all. Or we could assume the schema name from the environment and omit this check at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added tests. See comments above about it needs be public.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you clarify why the table name is not required when fetching from ALL_* but is required when fetching from USER_*? As far as I understand, the only difference between having and not having the schema name explicitly defined is that in the first case the query looks like SELECT * FROM ALL_* WHERE owner = $schema and in the second it's SELECT * FROM USER_*.

}

$quotedTableIdentifier = '';
$tableWhereClause = '';
if (null !== $table) {
$tableIdentifier = $this->normalizeIdentifier($table);
$quotedTableIdentifier = $this->quoteStringLiteral($tableIdentifier->getName());
$tableWhereClause = "AND cols.table_name = " . $quotedTableIdentifier;
}

// If database is specified, return SQL for all foreign keys in
// database, optionally filtered by table.
if ($this->isDatabaseSelected($database)) {
$databaseIdentifier = $this->normalizeIdentifier($database);
$quotedDatabaseIdentifier = $this->quoteStringLiteral($databaseIdentifier->getName());
return <<<SQL
SELECT cols.table_name,
alc.constraint_name,
alc.DELETE_RULE,
cols.column_name "local_column",
cols.position,
r_cols.table_name "references_table",
r_cols.column_name "foreign_column"
FROM all_cons_columns cols
LEFT JOIN all_constraints alc
ON alc.owner = cols.owner AND alc.constraint_name = cols.constraint_name
LEFT JOIN all_cons_columns r_cols
ON r_cols.owner = alc.r_owner AND r_cols.constraint_name = alc.r_constraint_name AND r_cols.position = cols.position
WHERE cols.owner = $quotedDatabaseIdentifier $tableWhereClause AND alc.constraint_type = 'R'
ORDER BY cols.table_name, cols.constraint_name, cols.position
SQL;
}

return "SELECT alc.constraint_name,
// If database is not specified, return SQL for the foreign keys of the
// specified table from the current database.
return <<<SQL
SELECT alc.constraint_name,
alc.DELETE_RULE,
cols.column_name \"local_column\",
cols.column_name "local_column",
cols.position,
(
SELECT r_cols.table_name
FROM user_cons_columns r_cols
WHERE alc.r_constraint_name = r_cols.constraint_name
AND r_cols.position = cols.position
) AS \"references_table\",
) AS "references_table",
(
SELECT r_cols.column_name
FROM user_cons_columns r_cols
WHERE alc.r_constraint_name = r_cols.constraint_name
AND r_cols.position = cols.position
) AS \"foreign_column\"
) AS "foreign_column"
FROM user_cons_columns cols
JOIN user_constraints alc
ON alc.constraint_name = cols.constraint_name
AND alc.constraint_type = 'R'
AND alc.table_name = " . $table . "
ORDER BY cols.constraint_name ASC, cols.position ASC";
AND alc.table_name = $quotedTableIdentifier
ORDER BY cols.constraint_name ASC, cols.position ASC
SQL;
}

/**
Expand All @@ -653,33 +738,47 @@ public function getListTableConstraintsSQL($table)
*/
public function getListTableColumnsSQL($table, $database = null)
{
$table = $this->normalizeIdentifier($table);
$table = $this->quoteStringLiteral($table->getName());
if (null === $table && !$this->isDatabaseSelected($database)) {
throw new \InvalidArgumentException('Table name must be specified if no database is specified');
}

$tabColumnsTableName = "user_tab_columns";
$colCommentsTableName = "user_col_comments";
$tabColumnsOwnerCondition = '';
$colCommentsOwnerCondition = '';

if (null !== $database && '/' !== $database) {
$database = $this->normalizeIdentifier($database);
$database = $this->quoteStringLiteral($database->getName());
$tabColumnsTableName = "all_tab_columns";
$colCommentsTableName = "all_col_comments";
$tabColumnsOwnerCondition = "AND c.owner = " . $database;
$colCommentsOwnerCondition = "AND d.OWNER = c.OWNER";
$quotedTableIdentifier = '';
$tableWhereClause = '';
if (null !== $table) {
$tableIdentifier = $this->normalizeIdentifier($table);
$quotedTableIdentifier = $this->quoteStringLiteral($tableIdentifier->getName());
$tableWhereClause = "AND c.table_name = " . $quotedTableIdentifier;
}

return "SELECT c.*,
// If database is specified, return SQL for all columns in database,
// optionally filtered by table.
if ($this->isDatabaseSelected($database)) {
$databaseIdentifier = $this->normalizeIdentifier($database);
$quotedDatabaseIdentifier = $this->quoteStringLiteral($databaseIdentifier->getName());
return <<<SQL
SELECT c.*, d.comments AS comments
FROM all_tab_columns c
LEFT JOIN all_col_comments d
ON d.OWNER = c.OWNER AND d.TABLE_NAME = c.TABLE_NAME AND d.COLUMN_NAME = c.COLUMN_NAME
WHERE c.owner = $quotedDatabaseIdentifier $tableWhereClause
ORDER BY c.table_name, c.column_id
SQL;
}

// If database is not specified, return SQL for the columns of the
// specified table from the current database.
return <<<SQL
SELECT c.*,
(
SELECT d.comments
FROM $colCommentsTableName d
WHERE d.TABLE_NAME = c.TABLE_NAME " . $colCommentsOwnerCondition . "
FROM user_col_comments d
WHERE d.TABLE_NAME = c.TABLE_NAME
AND d.COLUMN_NAME = c.COLUMN_NAME
) AS comments
FROM $tabColumnsTableName c
WHERE c.table_name = " . $table . " $tabColumnsOwnerCondition
ORDER BY c.column_id";
FROM user_tab_columns c
WHERE c.table_name = $quotedTableIdentifier
ORDER BY c.column_id
SQL;
}

/**
Expand Down Expand Up @@ -1176,4 +1275,16 @@ public function quoteStringLiteral($str)

return parent::quoteStringLiteral($str);
}

/**
* Determines if the input variable identifies a specific database.
*
* @param string $database
*
* @return bool
*/
private function isDatabaseSelected(?string $database): bool
{
return null !== $database && '/' !== $database;
}
}
46 changes: 46 additions & 0 deletions lib/Doctrine/DBAL/Schema/OracleSchemaManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,52 @@
*/
class OracleSchemaManager extends AbstractSchemaManager
{
/**
* {@inheritdoc}
*/
public function listTables()
{
$currentDatabase = $this->_conn->getDatabase();

$tableNames = $this->listTableNames();

// Get all column definitions in one database call.
$allColumns = $this->_conn->fetchAll($this->_platform->getListTableColumnsSQL(null, $currentDatabase));

// Get all foreign keys definitions in one database call.
$allForeignKeys = $this->_conn->fetchAll($this->_platform->getListForeignKeysSQL(null, $currentDatabase));

// Get all indexes definitions in one database call.
$allIndexes = $this->_conn->fetchAll($this->_platform->getListTableIndexesSQL(null, $currentDatabase));

$tables = [];
foreach ($tableNames as $tableName) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This entire block seems to be an elaborate array_filter()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually no, we are iterating through table 'names' and building an array of Table 'objects', AFAIK arrayFilter would just return the names again.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright 👍

$unquotedTableName = rtrim(ltrim($tableName, '"'), '"');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we fetchAll() from getListTablesSQL() instead of dealing with quoted table names?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need both unquoted table names (to fetch information from arrays returned by the getListWhatever() methods) and quoted table names that are used to create DBAL's objects names. So yes we could avoid the trim here by fetchAll from db, but we would need to re-quote the identifiers later. Since listTableNames() does all the checks/validations upfront, I'd prefer to keep this as is.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This way, this code depends on the implementation details of the OraclePlatform::listTableNames() — only OraclePlatform returns table names quoted (not sure why) while the others don't.

Since listTableNames() does all the checks/validations upfront, I'd prefer to keep this as is.

That's a valid point which brings up the question: if AbstractSchemaManager::_getPortableTableDefinition() returns quoted or non-quoted identifiers based on the platform, how is the behavior of filterSchemaAssetsExpression (I assume this is what was meant by checks/validations above) is expected to be portable? Or it's not? /cc @Ocramius


// Process columns for this table.
$tableColumns = array_filter($allColumns, function ($column) use ($unquotedTableName) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The list of columns is iterated/filtered once for every single table which is suboptimal. Instead, it could be re-grouped by table name once, and then every table could pick its own group if it exists.

return $column['TABLE_NAME'] === $unquotedTableName;
});
$columns = $this->_getPortableTableColumnList($tableName, null, $tableColumns);

// Process foreign keys for this table.
$tableForeignKeys = array_filter($allForeignKeys, function ($foreignKey) use ($unquotedTableName) {
return $foreignKey['TABLE_NAME'] === $unquotedTableName;
});
$foreignKeys = !empty($tableForeignKeys) ? $this->_getPortableTableForeignKeysList($tableForeignKeys) : [];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really worth optimization IMO. Calling _getPortableTableForeignKeysList() with an empty array will produce an empty array which is fine. Especially if called once before grouping by table name.


// Process indexes for this table.
$tableIndexes = array_filter($allIndexes, function ($index) use ($unquotedTableName) {
return $index['TABLE_NAME'] === $unquotedTableName;
});
$indexes = !empty($tableIndexes) ? $this->_getPortableTableIndexesList($tableIndexes, $tableName) : [];

$tables[] = new Table($tableName, $columns, $indexes, $foreignKeys, false, []);
}

return $tables;
}

/**
* {@inheritdoc}
*/
Expand Down
Loading