mikebridge commented on PR #41176: URL: https://github.com/apache/superset/pull/41176#issuecomment-4928016862
A multi-model review pass surfaced one correctness gap worth recording. It **doesn't block this PR** — it's only reachable with `ENABLE_VERSIONING_CAPTURE` on, and this ships dark/off — but it should be fixed before the flag is ever flipped: **Parent baseline capture isn't SAVEPOINT-isolated on PostgreSQL.** In `superset/versioning/baseline/insertion.py`, the direct-SQL baseline inserts run under `try/except` + `no_autoflush` but not a `begin_nested()`. On PostgreSQL a failed statement aborts the whole transaction even when the exception is caught, so a baseline-insert failure would poison the user's save — contradicting the "versioning must never break a save" guarantee. The change-record path (`changes/listener.py`) and the child-baseline path (`baseline/collection.py:133`) already wrap their direct SQL in `session.connection().begin_nested()`; the parent-baseline path is the one spot that doesn't. Rather than disturb this PR at the merge gate, I've put the one-line fix in a small follow-up: #41910 (draft, stacked on this). _(Posted via Claude on my behalf.)_ -- 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]
