Skip to content

[DIV-01] Pessimistic locks are acquired at commit time, not when keys are touched #437

Description

@s2x

Risk for enterprise users: Critical
Labels: bug, semantic-divergence, area/txnkv
Reference implementations: client-go txnkv/transaction/txn.go:1376 (LockKeys) and txnkv/transaction/pessimistic.go, client-rust src/transaction/transaction.rs:612

What differs

In this client, set() and delete() only append the key to a pending-lock list held in TransactionState (src/Client/TxnKv/Transaction.php:220, :236). No KvPessimisticLock RPC is issued until commit() runs, where pessimisticLockBatch() is the first thing it does (src/Client/TxnKv/TwoPhaseCommitter.php:104-107). Reads never lock at all — TxnReader::get() issues a plain KvGet at startTs (src/Client/TxnKv/TxnReader.php:79).

In client-go, a pessimistic transaction acquires the lock at the moment LockKeys is called, which for TiDB happens during statement execution, so a conflicting transaction blocks or fails immediately at the point of the conflicting statement.

Why it matters

A developer porting from Go or Rust will assume that once a key has been written inside a pessimistic transaction, the lock is held and no other transaction can modify it. That assumption is wrong here. Between set() and commit() the key is entirely unprotected, so the whole point of choosing pessimistic mode — early conflict detection and a guarantee that the commit will succeed — is lost. Code that relies on pessimistic mode to serialise a read-modify-write sequence will silently allow lost updates or produce commit-time conflicts that the developer explicitly chose pessimistic mode to avoid.

The failure is silent and load-dependent: it looks correct in testing and manifests as rare, hard-to-reproduce anomalies under concurrency.

Workaround available today

None that restores the intended semantics. Treat pessimistic transactions in this client as optimistic transactions with a commit-time locking pass, and design for commit-time conflict handling accordingly.

Implementation sketch

Move the KvPessimisticLock call from commit() into set(), delete() and a new lockKeys() / getForUpdate() (GAP-22). Each locking call needs its own for_update_ts from PD, which makes the TSO cost in GAP-06 more pressing. TransactionState::updateMaxForUpdateTs() already tracks the maximum for the eventual prewrite (src/Client/TxnKv/TransactionState.php:202). Keep the current behaviour available behind an explicit option if backward compatibility matters, but the default should match the official clients.

Effort estimate

Medium — a few days. The locking machinery exists; the work is restructuring when it runs and handling failures at the point of the call rather than at commit.

Acceptance criteria

  • set() and delete() in a pessimistic transaction acquire the lock immediately.
  • Lock conflicts, deadlocks and wait timeouts surface at the point of the call.
  • for_update_ts is obtained per locking pass and the maximum is used at prewrite.
  • Rollback releases all locks acquired so far.
  • A concurrency test demonstrates that two pessimistic transactions writing the same key serialise at write time, not at commit time.
  • The change and any compatibility option are documented in the README and CHANGELOG.

Filed from the Feature gaps vs official clients audit (audit-feature-gaps.md), finding DIV-01.
See the consolidated summary for the root-cause grouping this belongs to.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    auditAdded by audit issue importbugSomething isn't workingfeature-gapAdded by audit issue importseverity:criticalAdded by audit issue import

    Type

    No type

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions