mikebridge opened a new pull request, #44247:
URL: https://github.com/apache/superset/pull/44247
### SUMMARY
`set_db_default_isolation()` has always *intended* to default the
metadata-database isolation level to READ COMMITTED on MySQL/Postgres when
`SQLALCHEMY_ENGINE_OPTIONS` doesn't set one — and has always logged that it
did. It never has: the method calls SQLAlchemy's `Engine.execution_options()`,
which is **generative** (returns a new engine, leaves `db.engine` untouched),
and discards the result. The whole method is a no-op.
Consequences (sc-120480):
- **Every MySQL deployment** without an explicit `isolation_level` has been
running at InnoDB's default **REPEATABLE READ**, not the READ COMMITTED the
code and log claim.
- **Postgres is unaffected in practice** — its server default is already
READ COMMITTED.
- **CI cannot see it**: `.github/workflows/bashlib.sh` pins the MySQL
service to READ COMMITTED via `SET GLOBAL transaction_isolation`, masking the
no-op end to end.
- This is the root cause of the REPEATABLE-READ snapshot manifestation
cluster (stale entity re-read under conditional writes, stale If-Match
validator read) addressed by #44027 / #44091 — those locking-read fixes are
correct and necessary under either isolation level, so **this PR is explicitly
NOT a GA dependency**.
The fix is the minimal in-place form:
`db.engine.update_execution_options(isolation_level=...)` — the non-generative
sibling that mutates the engine — plus a why-comment naming the trap. (The
alternative shape, injecting `isolation_level` into `SQLALCHEMY_ENGINE_OPTIONS`
at config time so flask-sqlalchemy creates the engine with it, is noted on
sc-120480; happy to pivot if preferred.)
**Committer decision requested on rollout.** Applying the fix changes the
effective isolation level for every query on every MySQL deployment that
doesn't set `isolation_level` explicitly. The no-op has shipped for years, so
some deployments may implicitly rely on REPEATABLE READ semantics. UPDATING.md
documents the change and the escape hatch (`SQLALCHEMY_ENGINE_OPTIONS =
{"isolation_level": "REPEATABLE READ"}`). Open question for reviewers: should
the `bashlib.sh` `SET GLOBAL` override be **removed in this PR** so CI actually
exercises the app-level default? Recommended — otherwise the default remains
untestable end to end — but kept out of this diff pending that decision.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — engine configuration. Before: `SHOW VARIABLES LIKE
'transaction_isolation'` inside a Superset MySQL connection returns
`REPEATABLE-READ` while the log says "Setting database isolation level to READ
COMMITTED". After: `READ-COMMITTED`.
### TESTING INSTRUCTIONS
`pytest tests/unit_tests/initialization_test.py -k Isolation` — three tests
that assert on the **engine's stored execution options**, not on call shape (a
mock-level "was it called" assertion passes for the broken generative form too):
- MySQL URI → the engine carries `isolation_level == "READ COMMITTED"` after
the call. **Control verified**: reverting the fix back to the generative call
fails this test.
- Operator-set `isolation_level` in `SQLALCHEMY_ENGINE_OPTIONS` →
engine-level default not applied (config governs at engine creation).
- Non-MySQL/PG backend → untouched.
### ADDITIONAL INFORMATION
- [x] Has associated issue: sc-120480
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01JRLEJS4mUqKBoPjSjviKUW
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
