From c72f5ef8e32c4b036fd8c25da9d502bd16b61fc6 Mon Sep 17 00:00:00 2001 From: Mate Lakat Date: Thu, 23 Feb 2017 11:05:47 +0100 Subject: [PATCH] Create indexes for foreign keys Some database backends (for example PostgreSQL) do not automatically create an index on a foreign key. As a result database queries are slow. Adding the missing indexes and a migration that will only add indexes if they were not already there. In total, 26 foreign keys were identified as missing indexes. 2 of them are already covered by UniqueConstraints, for the rest, new indexes have been created. Closes-Bug: #1666547 Change-Id: I1437c3a1aa13142ee7a7e3e7bf9ff867b9d72652 --- .../versions/100_add_foreign_key_indexes.py | 65 +++++++++++++++++++ cinder/tests/unit/test_migrations.py | 45 ++++++++++++- 2 files changed, 109 insertions(+), 1 deletion(-) create mode 100644 cinder/db/sqlalchemy/migrate_repo/versions/100_add_foreign_key_indexes.py diff --git a/cinder/db/sqlalchemy/migrate_repo/versions/100_add_foreign_key_indexes.py b/cinder/db/sqlalchemy/migrate_repo/versions/100_add_foreign_key_indexes.py new file mode 100644 index 00000000000..f21d2376115 --- /dev/null +++ b/cinder/db/sqlalchemy/migrate_repo/versions/100_add_foreign_key_indexes.py @@ -0,0 +1,65 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +from oslo_db.sqlalchemy import utils +from oslo_log import log as logging +from sqlalchemy import MetaData + +LOG = logging.getLogger(__name__) + + +def ensure_index_exists(migrate_engine, table_name, column): + index_name = table_name + '_' + column + '_idx' + columns = [column] + + if utils.index_exists_on_columns(migrate_engine, table_name, columns): + LOG.info( + 'Skipped adding %s because an equivalent index already exists.', + index_name + ) + else: + utils.add_index(migrate_engine, table_name, index_name, columns) + + +def upgrade(migrate_engine): + meta = MetaData() + meta.bind = migrate_engine + for table_name, column in INDEXES_TO_CREATE: + ensure_index_exists(migrate_engine, table_name, column) + + +INDEXES_TO_CREATE = ( + ('attachment_specs', 'attachment_id'), + ('cgsnapshots', 'consistencygroup_id'), + ('group_snapshots', 'group_id'), + ('group_type_specs', 'group_type_id'), + ('group_volume_type_mapping', 'group_id'), + ('group_volume_type_mapping', 'volume_type_id'), + ('quality_of_service_specs', 'specs_id'), + ('reservations', 'allocated_id'), + ('reservations', 'usage_id'), + ('snapshot_metadata', 'snapshot_id'), + ('snapshots', 'cgsnapshot_id'), + ('snapshots', 'group_snapshot_id'), + ('snapshots', 'volume_id'), + ('transfers', 'volume_id'), + ('volume_admin_metadata', 'volume_id'), + ('volume_attachment', 'volume_id'), + ('volume_glance_metadata', 'snapshot_id'), + ('volume_glance_metadata', 'volume_id'), + ('volume_metadata', 'volume_id'), + ('volume_type_extra_specs', 'volume_type_id'), + ('volume_types', 'qos_specs_id'), + ('volumes', 'consistencygroup_id'), + ('volumes', 'group_id'), + ('workers', 'service_id'), +) diff --git a/cinder/tests/unit/test_migrations.py b/cinder/tests/unit/test_migrations.py index 0411a51f6c1..5514bbfb0e0 100644 --- a/cinder/tests/unit/test_migrations.py +++ b/cinder/tests/unit/test_migrations.py @@ -29,6 +29,7 @@ from oslo_db.sqlalchemy import test_base from oslo_db.sqlalchemy import test_migrations from oslo_db.sqlalchemy import utils as db_utils import sqlalchemy +from sqlalchemy.engine import reflection from cinder.db import migration import cinder.db.sqlalchemy.migrate_repo @@ -1098,13 +1099,55 @@ class MigrationsMixin(test_migrations.WalkVersionsMixin): self.assertIsInstance(workers.c.race_preventer.type, self.INTEGER_TYPE) + def get_table_names(self, engine): + inspector = reflection.Inspector.from_engine(engine) + return inspector.get_table_names() + + def get_foreign_key_columns(self, engine, table_name): + foreign_keys = set() + table = db_utils.get_table(engine, table_name) + inspector = reflection.Inspector.from_engine(engine) + for column_dict in inspector.get_columns(table_name): + column_name = column_dict['name'] + column = getattr(table.c, column_name) + if column.foreign_keys: + foreign_keys.add(column_name) + return foreign_keys + + def get_indexed_columns(self, engine, table_name): + indexed_columns = set() + for index in db_utils.get_indexes(engine, table_name): + for column_name in index['column_names']: + indexed_columns.add(column_name) + return indexed_columns + + def assert_each_foreign_key_is_part_of_an_index(self): + engine = self.migrate_engine + + non_indexed_foreign_keys = set() + + for table_name in self.get_table_names(engine): + indexed_columns = self.get_indexed_columns(engine, table_name) + foreign_key_columns = self.get_foreign_key_columns( + engine, table_name + ) + for column_name in foreign_key_columns - indexed_columns: + non_indexed_foreign_keys.add(table_name + '.' + column_name) + + self.assertSetEqual(set(), non_indexed_foreign_keys) + def test_walk_versions(self): self.walk_versions(False, False) + self.assert_each_foreign_key_is_part_of_an_index() class TestSqliteMigrations(test_base.DbTestCase, MigrationsMixin): - pass + def assert_each_foreign_key_is_part_of_an_index(self): + # Skip the test for SQLite because SQLite does not list + # UniqueConstraints as indexes, which makes this test fail. + # Given that SQLite is only for testing purposes, it is safe to skip + pass class TestMysqlMigrations(test_base.MySQLOpportunisticTestCase,