Uh oh!
There was an error while loading. Please reload this page.
SQLA2: avoid unnecessary locking when working with TIs - #59686
Conversation
In SQLAlchemy 2.0, we need to explicitly tell get() not to issue a new SELECT for an already-loaded object. Using `with_for_update=True` tells it to use the locked version.
Dev-iL
commented
Dec 21, 2025
potiuk
commented
Dec 22, 2025
Nice |
Uh oh!
There was an error while loading. Please reload this page.
In SQLAlchemy 2.0, we need to explicitly tell get() not to issue a new SELECT for an already-loaded object. Using `with_for_update=True` tells it to use the locked version.
In SQLAlchemy 2.0, we need to explicitly tell get() not to issue a new SELECT for an already-loaded object. Using `with_for_update=True` tells it to use the locked version.
In SQLAlchemy 2.0, we need to explicitly tell get() not to issue a new SELECT for an already-loaded object. Using `with_for_update=True` tells it to use the locked version.
In SQLAlchemy 2.0, we need to explicitly tell get() not to issue a new SELECT for an already-loaded object. Using `with_for_update=True` tells it to use the locked version.
| ti = session.get(TI, ti_id_str) | ||
| if ti is not None: | ||
| _handle_fail_fast_for_dag(ti=ti, dag_id=dag_id, session=session, dag_bag=dag_bag) |
There was a problem hiding this comment.
Possibly because it was not covered by tests and therefore unneeded
There was a problem hiding this comment.
Not covered by test isn't a good reason to drop things
There was a problem hiding this comment.
So was this an intentional change or unintentional?
There was a problem hiding this comment.
It looks unintentional 6mo later, but perhaps it was resolving some flakiness or inconsistency between sqla versions, or deemed unnecessary in light of the other changes.
The lack of coverage is not the motivation for the removal, but rather the reason it "succeeded".
There was a problem hiding this comment.
Re the "silently" part - the PR description does say that
Skipping fail_fast in edge cases doesn't affect normal operation
There was a problem hiding this comment.
fail_fast is a feature that stopped working for reschedule cases.
There was a problem hiding this comment.
Skipping fail_fast in edge cases doesn't affect normal operation
That's completely wrong. Normal operation isn't things just working. fail_fast is a feature not an edge case!
…mit (#67353) PR #59686 dropped the _handle_fail_fast_for_dag call in the MySQL-TIMESTAMP-limit branch of the reschedule path based on an incorrect SQLA2 deadlock concern. As a result, DAGs with fail_fast=True silently fail to stop sibling tasks when a reschedule date exceeds 2038-01-19 on MySQL. The actual deadlock that motivated #59686 came from a different path (FOR UPDATE expanding to the lazy-joined dag_run row), fixed in #67246 by scoping the lock with with_for_update={"of": TI}. With that scope in place, the fail-fast call is safe and matches the file's two existing fail-fast sites. Also drops a second misleading comment in the same function claiming session.get was avoided to "avoid SQLA2 lock contention issues" -- the code itself is fine; the rationale was wrong.
Related: #59218
In SQLAlchemy 2.0, we need to explicitly tell
get()not to issue a newSELECTfor an already-loaded object. Usingwith_for_update=Truetells it to use the locked version.All changes are backward-compatible with SQLAlchemy 1.4:
These changes fix the
test_ti_update_state_reschedule_mysql_limittest which would otherwise time out:^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.