Optimize OracleSchemaManager::listTables #2766
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -418,9 +418,41 @@ protected function _getCreateTableSQL($table, array $columns, array $options = a | |
| */ | ||
| public function getListTableIndexesSQL($table, $currentDatabase = null) | ||
| { | ||
| if (null === $table && !$this->isDatabaseSelected($currentDatabase)) { | ||
| throw new \InvalidArgumentException('Table name must be specified if no database is specified'); | ||
|
There was a problem hiding this comment. This block is not currently covered by tests |
||
| } | ||
|
|
||
| $quotedTableIdentifier = ''; | ||
| $tableWhereClause = ''; | ||
| if (null !== $table) { | ||
| $tableIdentifier = $this->normalizeIdentifier($table); | ||
| $quotedTableIdentifier = $this->quoteStringLiteral($tableIdentifier->getName()); | ||
| $tableWhereClause = "AND ind_col.table_name = " . $quotedTableIdentifier; | ||
| } | ||
|
|
||
| // 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 "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"; | ||
| } | ||
|
|
||
| // If database is not specified, return SQL for the indexes of the | ||
| // specified table from the current database. | ||
|
There was a problem hiding this comment. Do we really need to maintain two branches for the specified and non-specified schema name? There was a problem hiding this comment. See comments above about using ALL* vs USER* views. |
||
| return "SELECT uind_col.index_name AS name, | ||
| ( | ||
| SELECT uind.index_type | ||
|
|
@@ -446,7 +478,7 @@ 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 = " . $quotedTableIdentifier . " | ||
| ORDER BY uind_col.column_position ASC"; | ||
| } | ||
|
|
||
|
|
@@ -610,9 +642,54 @@ private function getAutoincrementIdentifierName(Identifier $table) | |
| */ | ||
| public function getListTableForeignKeysSQL($table) | ||
| { | ||
| return $this->getListForeignKeysSQL($table); | ||
| } | ||
|
|
||
| /** | ||
| * Returns the list of foreign keys for the current database. | ||
| * | ||
| * @param string $table | ||
| * @param string $database | ||
| * | ||
| * @return string | ||
|
There was a problem hiding this comment. this |
||
| */ | ||
| public function getListForeignKeysSQL($table = null, $database = null) | ||
|
There was a problem hiding this comment. Given it's not an API method, should it be There was a problem hiding this comment. There should be two (sets of) methods: There was a problem hiding this comment. 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 There was a problem hiding this comment.
Isn't it true about introducing I think it would be safe for the 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 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'); | ||
|
There was a problem hiding this comment. This block is not currently covered by tests There was a problem hiding this comment. 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. There was a problem hiding this comment. Added tests. See comments above about it needs be public. There was a problem hiding this comment. Could you clarify why the table name is not required when fetching from |
||
| } | ||
|
|
||
| $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 "SELECT cols.table_name, | ||
| alc.constraint_name, | ||
| alc.DELETE_RULE, | ||
| cols.column_name \"local_column\", | ||
|
There was a problem hiding this comment. You can probably use return <<<SQL
SQL; |
||
| 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"; | ||
| } | ||
|
|
||
| // If database is not specified, return SQL for the foreign keys of the | ||
| // specified table from the current database. | ||
| return "SELECT alc.constraint_name, | ||
| alc.DELETE_RULE, | ||
| cols.column_name \"local_column\", | ||
|
|
@@ -633,7 +710,7 @@ public function getListTableForeignKeysSQL($table) | |
| JOIN user_constraints alc | ||
| ON alc.constraint_name = cols.constraint_name | ||
| AND alc.constraint_type = 'R' | ||
| AND alc.table_name = " . $quotedTableIdentifier . " | ||
| ORDER BY cols.constraint_name ASC, cols.position ASC"; | ||
| } | ||
|
|
||
|
|
@@ -653,32 +730,42 @@ public function getListTableConstraintsSQL($table) | |
| */ | ||
| public function getListTableColumnsSQL($table, $database = null) | ||
| { | ||
| if (null === $table && !$this->isDatabaseSelected($database)) { | ||
| throw new \InvalidArgumentException('Table name must be specified if no database is specified'); | ||
| } | ||
|
|
||
| $quotedTableIdentifier = ''; | ||
| $tableWhereClause = ''; | ||
| if (null !== $table) { | ||
| $tableIdentifier = $this->normalizeIdentifier($table); | ||
| $quotedTableIdentifier = $this->quoteStringLiteral($tableIdentifier->getName()); | ||
| $tableWhereClause = "AND c.table_name = " . $quotedTableIdentifier; | ||
| } | ||
|
|
||
| // 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 "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"; | ||
| } | ||
|
|
||
| // If database is not specified, return SQL for the columns of the | ||
| // specified table from the current database. | ||
| return "SELECT c.*, | ||
| ( | ||
| SELECT d.comments | ||
| FROM user_col_comments d | ||
| WHERE d.TABLE_NAME = c.TABLE_NAME | ||
| AND d.COLUMN_NAME = c.COLUMN_NAME | ||
| ) AS comments | ||
| FROM user_tab_columns c | ||
| WHERE c.table_name = $quotedTableIdentifier | ||
| ORDER BY c.column_id"; | ||
| } | ||
|
|
||
|
|
@@ -1176,4 +1263,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($database) | ||
|
There was a problem hiding this comment. Since this is an addition that will land in |
||
| { | ||
| return null !== $database && '/' !== $database; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 = array(); | ||
| foreach ($tableNames as $tableName) { | ||
|
There was a problem hiding this comment. This entire block seems to be an elaborate There was a problem hiding this comment. Actually no, we are iterating through table 'names' and building an array of Table 'objects', AFAIK |
||
| $unquotedTableName = rtrim(ltrim($tableName, '"'), '"'); | ||
|
There was a problem hiding this comment. Can we There was a problem hiding this comment. We need both unquoted table names (to fetch information from arrays returned by the There was a problem hiding this comment. This way, this code depends on the implementation details of the
That's a valid point which brings up the question: if |
||
|
|
||
| // Process columns for this table. | ||
| $tableColumns = array_filter($allColumns, function ($column) use ($unquotedTableName) { | ||
|
There was a problem hiding this comment. 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) : array(); | ||
|
There was a problem hiding this comment. You can use the short array syntax overall |
||
|
|
||
| // 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) : array(); | ||
|
|
||
| $tables[] = new Table($tableName, $columns, $indexes, $foreignKeys, false, array()); | ||
| } | ||
|
|
||
| return $tables; | ||
| } | ||
|
|
||
| /** | ||
| * {@inheritdoc} | ||
| */ | ||
|
|
||
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Connectionis not known withinAbstractPlatformand children classes, so we cannot usegetParamshere to get the schema name. Getting from USERENV would mean a separate SQL call to the DB which IMHO we should avoid if possible. AccessingUSER*views is there to address that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree.