SEPURI-SAI-KRISHNA commented on issue #59120:
URL: https://github.com/apache/airflow/issues/59120#issuecomment-5077987321
I looked into the savepoint approach mentioned earlier in this thread. The
concern raised above — that `prohibit_commit` won't allow savepoints — is
**correct**, and it's worth recording exactly why, plus what does work.
### Why `session.begin_nested()` is blocked
It isn't an Airflow quirk. `SessionTransaction._prepare_impl` in SQLAlchemy
2.0 dispatches the session-level event for nested transactions too:
```python
if self._parent is None or self.nested:
self.session.dispatch.before_commit(self.session)
```
`CommitProhibitorGuard` listens on `before_commit`, so releasing a
`SAVEPOINT` raises `UNEXPECTED COMMIT - THIS WILL BREAK HA LOCKS!` exactly as a
real commit would. Verified against SQLAlchemy 2.0.51.
### Dropping to the connection doesn't help either
`session.connection().begin_nested()` sidesteps the Session event layer, but
mixing connection-level savepoints with ORM flush leaves the Session in
`PendingRollbackError` after the first failure. Dead end — noting it so nobody
else spends time there.
### What does work: let the guard tolerate savepoint releases
```python
def _validate_commit(self, session):
if session.in_nested_transaction():
return # savepoint release, not a real commit
if self.expected_commit:
self.expected_commit = False
return
raise RuntimeError("UNEXPECTED COMMIT - THIS WILL BREAK HA LOCKS!")
```
With that, wrapping each iteration of the `_create_dag_runs` loop in
`session.begin_nested()` behaves as intended — a failing iteration rolls back
only its own savepoint and the loop continues:
```
created=['x', 'y', 'z'] failed=['dup'] persisted=['dup', 'x', 'y', 'z']
```
and a rogue `session.commit()` inside the guard is **still** rejected, so
the guard keeps doing its job.
The rationale for allowing this: releasing a savepoint neither ends the
outer transaction nor drops row locks, so it cannot break the HA locks the
guard exists to protect. Only the outer commit can, and that remains guarded.
### Caveat
I ran this against SQLite, which does not reproduce Postgres's
aborted-transaction state. So this establishes the guard mechanics and the loop
pattern, **not** that the Postgres failure mode in the issue description is
fixed. `SAVEPOINT` / `ROLLBACK TO SAVEPOINT` is the documented Postgres
recovery path, so I'd expect it to hold, but it needs confirming against a real
Postgres backend before anyone trusts it.
### Question before I put up a PR
This makes the change smaller than "smaller transactions or savepoints"
implies — no restructuring of the scheduler loop — but it does mean touching
`CommitProhibitorGuard`, which is a core primitive rather than something local
to `_create_dag_runs`.
Is widening the guard to permit savepoint releases acceptable in principle?
If so I'm happy to open a PR with the guard change, the loop wrapped in
`begin_nested()`, and a test that a failing iteration no longer poisons the
rest of the batch. If the preference is to keep the guard strict and
restructure the transaction boundaries instead, I'd rather know before writing
it.
---
Drafted-by: Claude Code (Opus 5); reviewed by @SEPURI-SAI-KRISHNA before
posting
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
