mikebridge commented on code in PR #44091:
URL: https://github.com/apache/superset/pull/44091#discussion_r4007219740
##########
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:
Confirmed and fixed in 9aa4d732c0 — real bug, thanks.
`lock_entity_for_update` is now wrapped with the same classification, so a 1205
at the acquisition returns the retryable 409 instead of an uncaught 500;
unit-pinned (`test_put_dataset_maps_entity_lock_contention_to_retryable_409`).
##########
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:
Fixed in 9aa4d732c0 — the comments were stale, the 409 mapping is the
intended behavior. Both the queries.py residual note and the read-point
parenthetical now say the insert-point deadlock is classified via `__cause__`
to the same retryable 409.
##########
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:
Fixed in 9aa4d732c0 — the shared `_lock_contention_response()` docstring now
states the rollback is LOAD-BEARING for 1205 with `innodb_rollback_on_timeout`
OFF (statement-only rollback; the transaction still holds the entity row lock
and this rollback releases it), with the deadlock case as the secondary framing.
##########
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:
Fixed in 9aa4d732c0 — `"database is locked"` / `"database table is locked"`
added to the text fallback (SQLite carries no code to match), parametrized
positive cases added alongside the MySQL/PG ones.
##########
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:
Done in 9aa4d732c0 — all three lock points now share
`_lock_contention_response()` (one rollback, one i18n string). The `# noqa:
C901` stays: `put` is still 15 > 10 even after the extraction, so removing it
needs a larger decomposition than this round should carry.
--
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]
