From 419b9afd6b6b1da0993c15bf9d6088c73809bc0a Mon Sep 17 00:00:00 2001 From: Daniel Standish <15932138+dstandish@users.noreply.github.com> Date: Thu, 16 Jun 2022 07:06:36 -0700 Subject: [PATCH 1/5] Add indexes for CASCADE deletes When we add foreign keys with ON DELETE CASCADE, and we delete rows in the foreign table, the database needs to join back to the referencing table. If there's no suitable index, then it can be slow to perform the deletes. --- ...1_2_3_3_add_indexes_for_cascade_deletes.py | 58 +++++++++++++++++++ airflow/models/taskfail.py | 3 +- airflow/models/taskreschedule.py | 1 + airflow/models/xcom.py | 1 + docs/apache-airflow/migrations-ref.rst | 4 +- 5 files changed, 65 insertions(+), 2 deletions(-) create mode 100644 airflow/migrations/versions/0111_2_3_3_add_indexes_for_cascade_deletes.py diff --git a/airflow/migrations/versions/0111_2_3_3_add_indexes_for_cascade_deletes.py b/airflow/migrations/versions/0111_2_3_3_add_indexes_for_cascade_deletes.py new file mode 100644 index 0000000000000..0bc12b2fce809 --- /dev/null +++ b/airflow/migrations/versions/0111_2_3_3_add_indexes_for_cascade_deletes.py @@ -0,0 +1,58 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you 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. + +"""Add indexes for CASCADE deletes + +Revision ID: f5fcbda3e651 +Revises: 3c94c427fdf6 +Create Date: 2022-06-15 18:04:54.081789 + +""" + +from alembic import op + +# revision identifiers, used by Alembic. +revision = 'f5fcbda3e651' +down_revision = '3c94c427fdf6' +branch_labels = None +depends_on = None +airflow_version = '2.3.3' + + +def upgrade(): + """Apply Add indexes for CASCADE deletes""" + with op.batch_alter_table('task_fail', schema=None) as batch_op: + batch_op.create_index('idx_task_fail_task_instance', ['dag_id', 'task_id', 'run_id', 'map_index']) + + with op.batch_alter_table('task_reschedule', schema=None) as batch_op: + batch_op.create_index('idx_task_reschedule_dag_run', ['dag_id', 'run_id']) + + with op.batch_alter_table('xcom', schema=None) as batch_op: + batch_op.create_index('idx_xcom_task_instance', ['dag_id', 'task_id', 'run_id', 'map_index']) + + +def downgrade(): + """Unapply Add indexes for CASCADE deletes""" + with op.batch_alter_table('xcom', schema=None) as batch_op: + batch_op.drop_index('idx_xcom_task_instance') + + with op.batch_alter_table('task_reschedule', schema=None) as batch_op: + batch_op.drop_index('idx_task_reschedule_dag_run') + + with op.batch_alter_table('task_fail', schema=None) as batch_op: + batch_op.drop_index('idx_task_fail_task_instance') diff --git a/airflow/models/taskfail.py b/airflow/models/taskfail.py index f7de99c308cac..b5f23d8ec56d1 100644 --- a/airflow/models/taskfail.py +++ b/airflow/models/taskfail.py @@ -17,7 +17,7 @@ # under the License. """Taskfail tracks the failed run durations of each task instance""" -from sqlalchemy import Column, ForeignKeyConstraint, Integer +from sqlalchemy import Column, ForeignKeyConstraint, Index, Integer from sqlalchemy.orm import relationship from airflow.models.base import Base, StringID @@ -39,6 +39,7 @@ class TaskFail(Base): duration = Column(Integer) __table_args__ = ( + Index("idx_task_fail_task_instance", dag_id, task_id, run_id, map_index), ForeignKeyConstraint( [dag_id, task_id, run_id, map_index], [ diff --git a/airflow/models/taskreschedule.py b/airflow/models/taskreschedule.py index 518f1e77ff65f..132554d8d1760 100644 --- a/airflow/models/taskreschedule.py +++ b/airflow/models/taskreschedule.py @@ -61,6 +61,7 @@ class TaskReschedule(Base): name="task_reschedule_ti_fkey", ondelete="CASCADE", ), + Index('idx_task_reschedule_dag_run', dag_id, run_id), ForeignKeyConstraint( [dag_id, run_id], ['dag_run.dag_id', 'dag_run.run_id'], diff --git a/airflow/models/xcom.py b/airflow/models/xcom.py index d67160d1fa901..aad720bd8b421 100644 --- a/airflow/models/xcom.py +++ b/airflow/models/xcom.py @@ -72,6 +72,7 @@ class BaseXCom(Base, LoggingMixin): # but it goes over MySQL's index length limit. So we instead index 'key' # separately, and enforce uniqueness with DagRun.id instead. Index("idx_xcom_key", key), + Index("idx_xcom_task_instance", dag_id, task_id, run_id, map_index), ForeignKeyConstraint( [dag_id, task_id, run_id, map_index], [ diff --git a/docs/apache-airflow/migrations-ref.rst b/docs/apache-airflow/migrations-ref.rst index ae765b99dd80f..8de4fb0047da6 100644 --- a/docs/apache-airflow/migrations-ref.rst +++ b/docs/apache-airflow/migrations-ref.rst @@ -27,7 +27,9 @@ Here's the list of all the Database Migrations that are executed via when you ru +---------------------------------+-------------------+-------------------+--------------------------------------------------------------+ | Revision ID | Revises ID | Airflow Version | Description | +=================================+===================+===================+==============================================================+ -| ``3c94c427fdf6`` (head) | ``1de7bc13c950`` | ``2.3.2`` | Add cascade to dag_tag foreign key | +| ``f5fcbda3e651`` (head) | ``3c94c427fdf6`` | ``2.3.3`` | Add indexes for CASCADE deletes | ++---------------------------------+-------------------+-------------------+--------------------------------------------------------------+ +| ``3c94c427fdf6`` | ``1de7bc13c950`` | ``2.3.2`` | Add cascade to dag_tag foreign key | +---------------------------------+-------------------+-------------------+--------------------------------------------------------------+ | ``1de7bc13c950`` | ``b1b348e02d07`` | ``2.3.1`` | Add index for ``event`` column in ``log`` table. | +---------------------------------+-------------------+-------------------+--------------------------------------------------------------+ From 740c462f6c46fc3c69105331a37df9851e3e5ed2 Mon Sep 17 00:00:00 2001 From: Daniel Standish <15932138+dstandish@users.noreply.github.com> Date: Thu, 16 Jun 2022 07:46:16 -0700 Subject: [PATCH 2/5] skip index for mysql --- .../0111_2_3_3_add_indexes_for_cascade_deletes.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/airflow/migrations/versions/0111_2_3_3_add_indexes_for_cascade_deletes.py b/airflow/migrations/versions/0111_2_3_3_add_indexes_for_cascade_deletes.py index 0bc12b2fce809..6813059b8e705 100644 --- a/airflow/migrations/versions/0111_2_3_3_add_indexes_for_cascade_deletes.py +++ b/airflow/migrations/versions/0111_2_3_3_add_indexes_for_cascade_deletes.py @@ -18,6 +18,8 @@ """Add indexes for CASCADE deletes +Some databases don't add indexes on the FK columns so we have to add them for performance on CASCADE deletes. + Revision ID: f5fcbda3e651 Revises: 3c94c427fdf6 Create Date: 2022-06-15 18:04:54.081789 @@ -36,6 +38,12 @@ def upgrade(): """Apply Add indexes for CASCADE deletes""" + conn = op.get_bind() + + # mysql adds indexes for FKs so we don't have to + if conn.dialect.name == 'mysql': + return + with op.batch_alter_table('task_fail', schema=None) as batch_op: batch_op.create_index('idx_task_fail_task_instance', ['dag_id', 'task_id', 'run_id', 'map_index']) @@ -47,6 +55,12 @@ def upgrade(): def downgrade(): + conn = op.get_bind() + + # mysql adds indexes for FKs so we didn't have to + if conn.dialect.name == 'mysql': + return + """Unapply Add indexes for CASCADE deletes""" with op.batch_alter_table('xcom', schema=None) as batch_op: batch_op.drop_index('idx_xcom_task_instance') From 3c8b4b8ab2deb4c6ff2d5238ebdb7384036ed042 Mon Sep 17 00:00:00 2001 From: Daniel Standish <15932138+dstandish@users.noreply.github.com> Date: Thu, 16 Jun 2022 08:18:51 -0700 Subject: [PATCH 3/5] handle mysql --- ...1_2_3_3_add_indexes_for_cascade_deletes.py | 42 ++++++++++++++----- 1 file changed, 32 insertions(+), 10 deletions(-) diff --git a/airflow/migrations/versions/0111_2_3_3_add_indexes_for_cascade_deletes.py b/airflow/migrations/versions/0111_2_3_3_add_indexes_for_cascade_deletes.py index 6813059b8e705..aeabff7d1d54a 100644 --- a/airflow/migrations/versions/0111_2_3_3_add_indexes_for_cascade_deletes.py +++ b/airflow/migrations/versions/0111_2_3_3_add_indexes_for_cascade_deletes.py @@ -36,32 +36,54 @@ airflow_version = '2.3.3' +def _mysql_tables_where_indexes_already_present(conn): + """ + If user downgraded and is upgrading again, we have to check for existing + indexes on mysql because we can't (and don't) drop them as part of the + downgrade. + """ + to_check = [ + ('xcom', 'idx_xcom_task_instance'), + ('task_reschedule', 'idx_task_reschedule_dag_run'), + ('task_fail', 'idx_task_fail_task_instance'), + ] + tables = set() + for tbl, idx in to_check: + if conn.execute(f"show indexes from {tbl} where Key_name = '{idx}'").first(): + tables.add(tbl) + return tables + + def upgrade(): """Apply Add indexes for CASCADE deletes""" conn = op.get_bind() + tables_to_skip = set() - # mysql adds indexes for FKs so we don't have to + # mysql requires indexes for FKs, so adding had the effect of renaming, and we cannot remove. if conn.dialect.name == 'mysql': - return + tables_to_skip.update(_mysql_tables_where_indexes_already_present(conn)) - with op.batch_alter_table('task_fail', schema=None) as batch_op: - batch_op.create_index('idx_task_fail_task_instance', ['dag_id', 'task_id', 'run_id', 'map_index']) + if 'task_fail' not in tables_to_skip: + with op.batch_alter_table('task_fail', schema=None) as batch_op: + batch_op.create_index('idx_task_fail_task_instance', ['dag_id', 'task_id', 'run_id', 'map_index']) - with op.batch_alter_table('task_reschedule', schema=None) as batch_op: - batch_op.create_index('idx_task_reschedule_dag_run', ['dag_id', 'run_id']) + if 'task_reschedule' not in tables_to_skip: + with op.batch_alter_table('task_reschedule', schema=None) as batch_op: + batch_op.create_index('idx_task_reschedule_dag_run', ['dag_id', 'run_id']) - with op.batch_alter_table('xcom', schema=None) as batch_op: - batch_op.create_index('idx_xcom_task_instance', ['dag_id', 'task_id', 'run_id', 'map_index']) + if 'xcom' not in tables_to_skip: + with op.batch_alter_table('xcom', schema=None) as batch_op: + batch_op.create_index('idx_xcom_task_instance', ['dag_id', 'task_id', 'run_id', 'map_index']) def downgrade(): + """Unapply Add indexes for CASCADE deletes""" conn = op.get_bind() - # mysql adds indexes for FKs so we didn't have to + # mysql requires indexes for FKs, so adding had the effect of renaming, and we cannot remove. if conn.dialect.name == 'mysql': return - """Unapply Add indexes for CASCADE deletes""" with op.batch_alter_table('xcom', schema=None) as batch_op: batch_op.drop_index('idx_xcom_task_instance') From 04c02e66916d2904fbf57e3758f5db1f3f1f5596 Mon Sep 17 00:00:00 2001 From: Daniel Standish <15932138+dstandish@users.noreply.github.com> Date: Thu, 16 Jun 2022 09:03:07 -0700 Subject: [PATCH 4/5] fix offline --- .../versions/0111_2_3_3_add_indexes_for_cascade_deletes.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/airflow/migrations/versions/0111_2_3_3_add_indexes_for_cascade_deletes.py b/airflow/migrations/versions/0111_2_3_3_add_indexes_for_cascade_deletes.py index aeabff7d1d54a..2329b394c270b 100644 --- a/airflow/migrations/versions/0111_2_3_3_add_indexes_for_cascade_deletes.py +++ b/airflow/migrations/versions/0111_2_3_3_add_indexes_for_cascade_deletes.py @@ -26,7 +26,7 @@ """ -from alembic import op +from alembic import context, op # revision identifiers, used by Alembic. revision = 'f5fcbda3e651' @@ -60,7 +60,7 @@ def upgrade(): tables_to_skip = set() # mysql requires indexes for FKs, so adding had the effect of renaming, and we cannot remove. - if conn.dialect.name == 'mysql': + if conn.dialect.name == 'mysql' and not context.is_offline_mode(): tables_to_skip.update(_mysql_tables_where_indexes_already_present(conn)) if 'task_fail' not in tables_to_skip: From a350845ab9d5450f45046067140ee1a7bbbd4aa4 Mon Sep 17 00:00:00 2001 From: Daniel Standish <15932138+dstandish@users.noreply.github.com> Date: Thu, 16 Jun 2022 10:37:55 -0700 Subject: [PATCH 5/5] better descriptino --- .../versions/0111_2_3_3_add_indexes_for_cascade_deletes.py | 2 +- docs/apache-airflow/migrations-ref.rst | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/airflow/migrations/versions/0111_2_3_3_add_indexes_for_cascade_deletes.py b/airflow/migrations/versions/0111_2_3_3_add_indexes_for_cascade_deletes.py index 2329b394c270b..07cac9608cfac 100644 --- a/airflow/migrations/versions/0111_2_3_3_add_indexes_for_cascade_deletes.py +++ b/airflow/migrations/versions/0111_2_3_3_add_indexes_for_cascade_deletes.py @@ -16,7 +16,7 @@ # specific language governing permissions and limitations # under the License. -"""Add indexes for CASCADE deletes +"""Add indexes for CASCADE deletes on task_instance Some databases don't add indexes on the FK columns so we have to add them for performance on CASCADE deletes. diff --git a/docs/apache-airflow/migrations-ref.rst b/docs/apache-airflow/migrations-ref.rst index 8de4fb0047da6..75233b1c520b4 100644 --- a/docs/apache-airflow/migrations-ref.rst +++ b/docs/apache-airflow/migrations-ref.rst @@ -27,7 +27,7 @@ Here's the list of all the Database Migrations that are executed via when you ru +---------------------------------+-------------------+-------------------+--------------------------------------------------------------+ | Revision ID | Revises ID | Airflow Version | Description | +=================================+===================+===================+==============================================================+ -| ``f5fcbda3e651`` (head) | ``3c94c427fdf6`` | ``2.3.3`` | Add indexes for CASCADE deletes | +| ``f5fcbda3e651`` (head) | ``3c94c427fdf6`` | ``2.3.3`` | Add indexes for CASCADE deletes on task_instance | +---------------------------------+-------------------+-------------------+--------------------------------------------------------------+ | ``3c94c427fdf6`` | ``1de7bc13c950`` | ``2.3.2`` | Add cascade to dag_tag foreign key | +---------------------------------+-------------------+-------------------+--------------------------------------------------------------+