Skip to content

JDBC backend: nothing bounds an attempt of the transaction replay, so the retry window has to work around it #915

Closed

Description

@maximthomas

Describe the bug

JDBCStorage.write() replays a rolled-back transaction, and bounds the replays by MAX_RETRIES and by a 10 s
wall-clock window. Neither bounds an attempt, and an attempt is not short: the lock wait that precedes a
conflict is charged to the attempt that hit it, and is unbounded on three of the four engines here.

Dialect.lockTimeoutSql already carries the statement for all four engines, but it is only ever issued on the
stamp session — never on a transaction connection:

// opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java
POSTGRES("set local lock_timeout ...", ...),
MYSQL(...),
ORACLE(...),
MICROSOFT("set lock_timeout ...", ...),

So SQL Server, PostgreSQL and Oracle wait indefinitely for a row lock inside an attempt, and only MySQL bounds
it — with innodb_lock_wait_timeout, 50 s by default.

Why it matters

Everything the retry loop had to grow to work around #903 exists only because the attempt is unbounded:

  • the Conflict classification (PROMPT vs AFTER_LOCK_WAIT), and the per-hop most-specific walk that keeps it;
  • MYSQL_LOCK_WAIT_TIMEOUT, read driver-keyed only to tell a late conflict from a prompt one;
  • grantedPastTheWindow — the first replay of a prompt conflict, taken although the window is already spent,
    because no window survives a wait nothing bounds.

Bound the wait inside the attempt and every one of them goes away: each attempt becomes shorter than the window,
RETRY_WINDOW_NANOS governs both classes on its own, and the grant, the enum and the vendor number can be
deleted. The diff gets smaller, not bigger.

It also settles the two shapes the current design still leaves open, both raised in review of #904:

  1. The grant is keyed to attempt==1, which is a proxy. The invariant behind it — "no clock can bound a wait
    that nothing else bounds" — holds on attempt 2 as well. So the JDBC backend: the give-up window of the transaction replay disarms it for the deadlock it was written for #903 shape survives one attempt later: a
    SQL Server write whose attempt 1 conflicts promptly at 2 s (granted) and whose attempt 2 waits 12 s on a row
    lock before being picked as the victim is refused at 14 s < 10 s, by a clock a lock wait spent. Widening the
    grant to every attempt is the wrong answer — it leaves MAX_RETRIES as the only real cap.
  2. A MySQL lock wait timeout provably never replays at the stock innodb_lock_wait_timeout=50: it is
    reported at ~50 s, past the window on the first check, and gets no grant. Master behaved the same way, and
    one bounded wait does beat two — but it means MySQL deployments get no write retry for the conflict class
    they see most. With a lock bound on the attempt, a tuned-down timeout stops being the exception.

What it needs

Not a one-line change:

  • SQL Server converts the bounded wait into error 1222, "Lock request time out period exceeded", SQLState
    HY000. isConflict does not match it today, so issuing SET LOCK_TIMEOUT without classifying 1222 in the
    same change turns a retryable conflict into a hard failure.
  • PostgreSQL reports lock_timeout as 55P03 (lock_not_available), also unmatched today.
  • The bound has to be a configuration property, not a constant: it changes the failure every deployment sees.
  • Each engine needs container coverage of the new failure, since none of it is reachable without a real lock.

Related

Follow-up to #904 (#903), requested in review:
#904 (review)
The last section of #877 covers the same statement from the query-timeout side.

Activity

  1. added a commit that references this issue on Sep 3, 2026
  2. vharseko commented on Sep 9, 2026

    @vharseko
    Member

    Read against master at 36d4af9bd7, and against the two open PRs this issue sits between — #904, which it is the follow-up to, and #936, which has already built the mechanism it needs and rejected the placement it proposes. Line numbers below are that commit's.

    Three of the premises no longer hold as written, and the shape of the change is narrower in one place and wider in two others than "What it needs" says.

    1. Dialect.lockTimeoutSql bounds a row lock on two engines of the four, not all four

    That field is the session setting a comment statement needs — COMMENT_LOCK_TIMEOUT_SECONDS is 5 s (JDBCStorage.java:1183), and it is issued on the stamp connection at :1340 against the metadata lock a COMMENT ON takes. Reused on a transaction connection, only half of it bounds the wait this issue is about:

    engine statement (JDBCStorage.java:1225-:1236) bounds a row lock?
    PostgreSQL set lock_timeout yes — the docs name table, index and row locks
    SQL Server set lock_timeout yes — every lock wait of the session
    MySQL set session lock_wait_timeout no — metadata locks only; the row lock is innodb_lock_wait_timeout
    Oracle alter session set ddl_lock_timeout no — DDL against DML locks only

    Oracle is the harder half of that: there is no session-level bound for a row-lock enqueue under plain DML at all — SELECT ... FOR UPDATE WAIT n is the only place the wait can be named, and this backend's writes do not go through one. So "bound the wait inside the attempt and every one of them goes away" is true for at most three engines, and the grant (or something in its place) still has to cover Oracle. #936's body states both of these from the DDL side: "MySQL gets lock_wait_timeout and never innodb_lock_wait_timeout", and "Oracle is left alone deliberately".

    2. The attempt is bounded today — at 120 s, twelve times the window

    StatementBound.OPERATION (:277-:293) puts a 120 s query timeout, plus the socket read backstop behind it, on every ordinary statement of a write transaction — ReadableTransactionImpl takes StatementBound.OPERATION unless the importer hands it BULK (:3377). So the accurate statement of the defect is not "nothing bounds an attempt" but "the only bound on an attempt is twelve times the window that is supposed to govern the replay, and the failure it produces is not replayable": timedOut() (:817) carries the state over, and a cancelled statement arrives as 57014 / ORA-01013 / a driver timeout, which isConflict (:3282) does not match.

    Two things do stay genuinely unbounded, and they are worth naming in the issue instead:

    • the BULK class, which ships with no bound by design — clearTree()'s delete from takes row locks and carries neither layer, and so does every statement of the importer;
    • Oracle even in the OPERATION class: a session blocked in a row-lock enqueue does not act on the driver's break (the bounded() javadoc at :390 says so outright), so what ends the wait there is the socket read timeout — i.e. the connection, not the statement.

    3. A session-wide bound is not free: read() never replays

    The pool is shared between reads and writes, CachedConnection.close() only rolls back, and a session setting survives the return to the pool. read() (:2905) replays nothing at all, deliberately — ExportJob and VerifyJob are not idempotent. On SQL Server under READ COMMITTED a reader blocks on a writer, so a session-wide LOCK_TIMEOUT turns that wait into error 1222 in a client operation that has no retry to absorb it. #936 rejected exactly this placement for exactly this reason, and the bounded() javadoc states the rule the file already keeps: a pooled connection cannot carry a session setting.

    That fixes the shape of the change rather than blocking it: the bound has to be set and put back around the transaction of write(), the way #936 does it around a DDL — readback and reset per engine, a savepoint on PostgreSQL, the connection kept out of the pool when the reset fails, the statements issued through executeSessionStatement() so SQL Server does not revert them out of sp_executesql. All of that is written already in #936's withDdlLockBound(). #915 should be built on top of it, which makes it considerably smaller than "not a one-line change" suggests — a second call site, a second property, and one classification.

    4. The bound and the window are one decision, not two

    An attempt that ends at the bound spends that much of the window, so the replay budget is RETRY_WINDOW_NANOS / bound. At the 5 s of COMMENT_LOCK_TIMEOUT_SECONDS a 10 s window leaves exactly one replay; at anything ≥ 10 s #903 comes back in a new form. Whatever value the property defaults to has to be chosen against the window in the same change — 1-3 s, or a wider window — otherwise "RETRY_WINDOW_NANOS governs both classes on its own" does not follow.

    5. A lock timeout is not a deadlock, and the default is the real question

    When the engine picks a deadlock victim it has already rolled someone back, so the replay has somewhere to go. A lock wait timeout means the blocker is still holding the lock: the replay queues behind the same transaction, and the randomized retryDelayMillis softens that without answering it. More importantly, it changes the outcome of ordinary contention — a write blocked behind a long writer succeeds slowly today on PostgreSQL, SQL Server and Oracle, and with a small bound in force it becomes a client-visible failure once the window is spent.

    That is the trade the issue does not state, and it decides the default:

    The issue's argument ("the diff gets smaller, not bigger") holds only under the first, so it is worth saying which one is meant before either PR merges.

    6. What the classification has to be careful about

    isConflict needs 1222 / HY000 and 55P03 — but only where this backend set the bound. A deployment that set its own lock_timeout on the URL or the server gets the same two failures today, and making them replayable unconditionally starts silently retrying a wait an operator deliberately bounded. #936 already extracts isLockTimeout() out of scopeOf() (:1991-:2028, which knows 55P03, MySQL 1205, ORA-00054/04021 and 1222 for the stamp path) — the classification should key off that and off the dialect resolved once, not off a fifth contains(...) cascade over the driver name. That is the same request the review rounds of #904 made twice.

    7. Order with #904

    write() in master is still the plain double bound (:2964, guard at :3060, :58, :69): the Conflict enum, grantedPastTheWindow and the driver-keyed vendor read that this issue promises to delete exist only in #904, which is still open. So the two are alternatives more than a sequence — landing #915 first closes #903 by making the window work as designed and leaves nothing of #904 to retire (Oracle aside, per section 1), while landing #904 first means #915 also has to unwind it. Whichever way round, it is a decision worth taking before #904 merges rather than after.

  3. added 2 commits that reference this issue on Sep 10, 2026
  4. added 4 commits that reference this issue on Sep 21, 2026
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

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions