mikebridge commented on code in PR #44091:
URL: https://github.com/apache/superset/pull/44091#discussion_r4031584088
##########
superset/datasets/api.py:
##########
@@ -796,6 +867,16 @@ def put(self, pk: int) -> Response:
)
response = self.response_422(message=str(ex))
except DatasetUpdateFailedError as ex:
+ # The gap lock the conditional path's locking read takes on a
+ # zero-live-row range can deadlock against another conditional
+ # writer at Continuum's version-row INSERT inside the command;
+ # on_error chains the driver error as __cause__, and the update
+ # transaction has rolled back. (The post-commit override_columns
+ # refresh raises its own exception type and cannot reach this
+ # branch.) Same retryable classification as the read-point
+ # handler above: the token is not proven stale.
+ if conditional and is_lock_contention_error(ex.__cause__):
Review Comment:
Added in c6767b2a487aa300194778c0fd52db170f47d8b6: a positive command
failure whose __cause__ is OperationalError with driver args (1213, ...), plus
an unconditional control that must remain 422 without retry text. Replacing the
conditional cause handler with if False in a process-local mutation made the
positive fail (422 vs 409) while the unconditional control passed. Final
candidate: 59 focused tests and 221 versioning unit tests pass, complete
changed-file pre-commit passes, and independent final-snapshot review approves.
This is unit verification; fresh live MySQL REPEATABLE READ integration remains
a CI gate.
##########
superset/datasets/api.py:
##########
@@ -571,6 +573,25 @@ def post(self) -> Response:
logger.exception("Unexpected error in DatasetRestApi.post")
return self.response_500(message="Fatal error")
+ def _lock_contention_response(self) -> Response:
+ """The shared retryable 409 for a conditional save losing a lock race.
+
+ The rollback is LOAD-BEARING for lock-wait-timeout: with
+ ``innodb_rollback_on_timeout`` OFF (the MySQL default) a 1205
+ rolls back only the failing STATEMENT -- the transaction is still
+ alive and still holds any entity row lock already acquired, and
+ this rollback is what releases it. For a deadlock (1213) InnoDB
+ already rolled the transaction back and this clears the aborted
+ session before responding.
+ """
+ db.session.rollback() # pylint: disable=consider-using-transaction
Review Comment:
Added in c6767b2a487aa300194778c0fd52db170f47d8b6: the 1205 test covers both
direct lock points and asserts rollback exactly once before request teardown.
Replacing the helper rollback with pass in-process made both cases fail with
zero rollback calls. The docstring also distinguishes the required direct-catch
rollback from the decorated command path where rollback already occurred. Final
focused suite (59) and full versioning unit suite (221) pass; changed-file
hooks and independent final-snapshot review pass. No claim of a fresh
live-server lock-release test.
##########
tests/unit_tests/versioning/test_db_errors.py:
##########
@@ -107,3 +111,192 @@ def
test_connection_drop_with_no_code_is_not_missing_table() -> None:
_FakeDriverError("server closed the connection unexpectedly"),
)
assert is_missing_table_error(error) is False
+
+
+# ---------------------------------------------------------------------------
+# is_lock_contention_error (sc-120050)
+# ---------------------------------------------------------------------------
+
+
+def _op_error(orig: object) -> OperationalError:
+ err: OperationalError = OperationalError("stmt", None, Exception("boom"))
+ err.orig = orig
+ return err
+
+
+class _FakeLockDriverError(Exception): # noqa: N818 — a driver error, not ours
+ """A stand-in for a DBAPI driver error.
+
+ A real driver error IS an exception carrying its own diagnostic, so
+ the fake is one too: the classifier reads ``args`` / ``pgcode`` /
+ ``sqlstate`` and, for code-less drivers, ``str()`` of THIS object —
+ never of the SQLAlchemy wrapper around it.
+ """
+
+ def __init__(
+ self,
+ args: tuple[object, ...] = (),
+ pgcode: str | None = None,
+ sqlstate: str | None = None,
+ message: str = "",
+ ) -> None:
+ super().__init__(*args)
+ self.args: tuple[object, ...] = args
+ self._message: str = message
+ if pgcode is not None:
+ self.pgcode: str = pgcode
+ if sqlstate is not None:
+ self.sqlstate: str = sqlstate
+
+ def __str__(self) -> str:
+ return self._message or super().__str__()
+
+
+#: Statement prefix for the contamination fixtures; the lock phrase is
+#: appended as a trailing SQL comment.
+_CONTAMINATED_SQL: str = (
+ "INSERT INTO slices (slice_name, description) "
+ "VALUES (%(slice_name)s, %(description)s) -- "
+)
+
+
+def _contaminated_statement(phrase: str) -> tuple[str, dict[str, str]]:
+ """SQL + bound parameters that merely CONTAIN a contention phrase.
+
+ The realistic shape: a user naming a chart "deadlock analysis", or a
+ column literally called ``lock_wait_timeout``. ``str()`` of a
+ SQLAlchemy wrapper renders both, which is why the classifier must
+ read the driver diagnostic instead.
+ """
+ # The phrase rides BOTH halves a wrapper renders: a trailing SQL
+ # comment (stands in for a table or column whose name contains it)
+ # and a bound parameter (a user-supplied chart name). Inert fixture
+ # text — nothing here is ever executed.
+ statement: str = _CONTAMINATED_SQL + phrase
+ return statement, {"slice_name": f"{phrase} analysis", "description":
phrase}
+
+
[email protected](
+ "orig",
+ [
+ _FakeLockDriverError(args=(1213, "Deadlock found when trying to get
lock")),
+ _FakeLockDriverError(args=(1205, "Lock wait timeout exceeded")),
Review Comment:
Added both combined driver shapes in
c6767b2a487aa300194778c0fd52db170f47d8b6: 1213/40001 and 1205/HY000. A
process-local SQLSTATE-first mutant fails specifically on 1205/HY000 (1 failed,
7 passed in the positive cases), so this pins the decisive errno-first order
rather than relying on 1213 agreeing by coincidence. The production
classification order is unchanged. Final 59 focused tests, 221 versioning unit
tests, complete changed-file hooks and independent final-snapshot review pass;
current-head CI is separate.
--
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]
