fitzee commented on code in PR #44091:
URL: https://github.com/apache/superset/pull/44091#discussion_r3995812463
##########
superset/datasets/api.py:
##########
@@ -703,14 +705,40 @@ def put(self, pk: int) -> Response:
# Serialise conditional saves on this dataset: the guard below reads
# the live version, the command writes, and the two must not interleave
- # with another request's. Only a conditional save pays for the lock; an
+ # with another request's. Only a conditional save pays for the locks;
an
# unconditional PUT behaves exactly as it did before the guard existed.
- if is_conditional_write():
+ conditional = is_conditional_write()
+ if conditional:
lock_entity_for_update(SqlaTable, pk)
Review Comment:
**The entity-row lock acquisition is outside the contention try/except, so
the most common contention case returns 500, not the intended 409.**
`lock_entity_for_update(SqlaTable, pk)` takes a `SELECT … FOR UPDATE` here,
*before* the `try:` that wraps the transaction-id read (719-721). Two
conditional PUTs on the same dataset: writer A holds the `SqlaTable` row lock
through its command; writer B blocks in `lock_entity_for_update` and after
`innodb_lock_wait_timeout` (default 50s) the driver raises `OperationalError`
1205. That error never reaches `is_lock_contention_error` — it propagates as an
uncaught **500**, defeating this PR's own goal of mapping "another save in
progress" to a retryable 409.
Same-entity queuing on this lock is the *primary* serialization point and
the most common contention case — more common than the gap-lock edge the two
new handlers cover. Consider moving this acquisition inside the same contention
handling (or wrapping it) so a lost race here also maps to 409.
##########
superset/datasets/api.py:
##########
@@ -785,6 +813,23 @@ 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:
**Nit (cleanup):** this 409 block (rollback + the identical `_('Another save
is in progress for this dataset. Retry the same request.')` response) is
copy-pasted from the read-point handler (~lines 726-741). The duplicated i18n
string must be kept in sync by hand, and layering these two special-case
branches onto `put()` is what forced the `# noqa: C901` at line 581. A small
helper (e.g. `_lock_contention_response()`) would remove both the duplication
and, potentially, the complexity suppression.
##########
superset/versioning/queries.py:
##########
@@ -224,6 +224,66 @@ def current_version_number(
return version_number
+def current_live_transaction_id_locked(
+ model_cls: type[Model], entity_id: int, entity_uuid: UUID
+) -> int | None:
+ """Return the live row's ``transaction_id`` via an exclusive locking read.
+
+ The conditional-write (``If-Match``) guard must compare the client's
+ token against *committed* state. A plain consistent read is served
+ from the transaction's REPEATABLE READ snapshot on MySQL/InnoDB
+ (pinned by the request's earlier auth queries), so a version row
+ committed by a concurrent writer between this request's first read
+ and its row lock stays invisible -- the stale token then matches and
+ the 412 the guard exists to raise is missed. A locking read is exempt
+ from the snapshot and returns current committed data.
+
+ The lock is exclusive (``with_for_update()``), not shared: this
+ transaction later closes the very row it reads here (Continuum's
+ validity strategy sets ``end_transaction_id`` at commit), and holding
+ a shared lock first invites InnoDB's shared-to-exclusive upgrade
+ deadlock whenever anything else queues for the row in between. Plain
+ MVCC readers are not blocked by either lock strength, and writers to
+ the same entity are already serialised by the entity row lock taken
+ first, so exclusivity here costs nothing.
+
+ Residual, documented rather than removed: on MySQL a locking read
+ over an empty range (an entity with no live version row yet) takes a
+ gap lock, and two concurrent conditional writers whose version rows
+ share a primary-key gap can deadlock. That deadlock has two surfacing
+ points with different outcomes. At THIS read (rare -- gap locks are
+ mutually compatible, so both readers usually succeed) the PUT path
+ maps it to a retryable 409. At Continuum's version-row INSERT inside
+ the update command it surfaces as the command's pre-existing 422
Review Comment:
**This docstring contradicts the handler added in the same PR.** It states
the version-row INSERT deadlock "surfaces as the command's pre-existing 422
error mapping." But `superset/datasets/api.py:824` now intercepts
`is_lock_contention_error(ex.__cause__)` on the `DatasetUpdateFailedError` path
and returns **409** — and a MySQL deadlock (errno 1213) is in
`_MYSQL_LOCK_CONTENTION`, so it *is* detected. The same stale "422" claim also
appears in the api.py read-point comment (the parenthetical near line 719). A
maintainer trusting either comment would believe the INSERT deadlock yields 422
when the code now yields 409. Either the comments are stale and should say 409,
or the 409 mapping for the INSERT-deadlock case is unintended — worth
reconciling, since the docstring even argues "retry the same request" (409
semantics) while naming 422.
##########
superset/versioning/db_errors.py:
##########
@@ -34,6 +34,38 @@
from sqlalchemy.exc import DBAPIError
+#: MySQL/MariaDB deadlock and lock-wait-timeout error codes.
+_MYSQL_LOCK_CONTENTION = (1213, 1205)
+
+#: PostgreSQL SQLSTATEs: serialization_failure, deadlock_detected,
+#: lock_not_available.
+_PG_LOCK_CONTENTION = ("40001", "40P01", "55P03")
+
+
+def is_lock_contention_error(exc: BaseException | None) -> bool:
+ """Whether *exc* is a database deadlock / lock-wait failure.
+
+ A write that loses a lock race has, by definition, interleaved with a
+ concurrent writer. It does NOT prove the caller's ``If-Match`` token
+ stale, so response-mapping callers classify it as a retryable
+ conflict (409, retry the same request) rather than a 500 -- or a 412,
+ whose refetch-the-token guidance would be wrong here. Accepts ``None``
+ (e.g. an exception with no ``__cause__``) and errors with empty
+ driver args without raising.
+ """
+ if exc is None:
+ return False
+ orig = getattr(exc, "orig", None)
+ args = getattr(orig, "args", None)
+ if args and args[0] in _MYSQL_LOCK_CONTENTION:
+ return True
+ sqlstate = getattr(orig, "pgcode", None) or getattr(orig, "sqlstate", None)
+ if sqlstate in _PG_LOCK_CONTENTION:
+ return True
+ text = str(exc).lower()
+ return "deadlock" in text or "lock wait timeout" in text
Review Comment:
**Text fallback misses SQLite lock contention.** The final fallback matches
only `"deadlock"` / `"lock wait timeout"`. SQLite raises
`OperationalError('database is locked')` / `('database table is locked')` under
write concurrency — `orig.args[0]` is a string (not in the MySQL int codes),
`pgcode`/`sqlstate` is None, and neither substring matches, so it returns
`False` and the error is misclassified (500 at the read point, or 422 via the
command handler) instead of 409. Lower severity since production is
MySQL/Postgres, but the integration suite runs on SQLite, so a genuine
lock-contention test there wouldn't get the 409 path. Consider adding
`"database is locked"` / `"database table is locked"` to the text fallback.
##########
superset/datasets/api.py:
##########
@@ -703,14 +705,40 @@ def put(self, pk: int) -> Response:
# Serialise conditional saves on this dataset: the guard below reads
# the live version, the command writes, and the two must not interleave
- # with another request's. Only a conditional save pays for the lock; an
+ # with another request's. Only a conditional save pays for the locks;
an
# unconditional PUT behaves exactly as it did before the guard existed.
- if is_conditional_write():
+ conditional = is_conditional_write()
+ if conditional:
lock_entity_for_update(SqlaTable, pk)
# Live version identifiers before the update (empty + query-free when
- # ``ENABLE_VERSIONING_CAPTURE`` is off).
- old_info = current_entity_version_info(SqlaTable, pk)
+ # ``ENABLE_VERSIONING_CAPTURE`` is off). On the conditional path the
+ # live transaction id is read under an exclusive row lock: a plain
+ # read is served from the request's REPEATABLE READ snapshot on MySQL
+ # and can miss a concurrent commit, letting a stale If-Match token
+ # pass the guard. A lock race lost at that read (deadlock / lock
+ # wait) proves concurrent CONTENTION, not that this request's token
+ # is stale — so it maps to a retryable 409, and the client should
+ # retry the SAME request. (A deadlock at Continuum's version-row
+ # insert inside the command surfaces as the pre-existing 422 via the
+ # command's error mapping.)
+ try:
+ old_info = current_entity_version_info(
+ SqlaTable, pk, lock_for_stale_check=conditional
+ )
+ except OperationalError as ex:
+ if not (conditional and is_lock_contention_error(ex)):
+ raise
+ # Not a unit of work: the transaction is already dead (deadlock
Review Comment:
**This comment is misleading for lock-wait-timeout, where the rollback is
load-bearing, not cleanup.** It says "the transaction is already dead (deadlock
rollback); this clears the aborted session." That's true for a deadlock (1213),
but `is_lock_contention_error` also accepts **lock-wait-timeout (1205)** — and
with `innodb_rollback_on_timeout` OFF (the MySQL default) a 1205 rolls back
only the *failing statement*, leaving the transaction alive and still holding
the entity row lock taken by `lock_entity_for_update`. In that case the
`db.session.rollback()` below is what actually releases the lock. A future
"cleanup" that trusts this comment and drops the rollback would leak the entity
lock for the rest of the request. Suggest rewording to note the rollback is
required to release held locks on the timeout path.
--
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]
