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

[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.

[vharseko]

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:

• on by default, small enough to fit the window — closes JDBC backend: the give-up window of the transaction replay disarms it for the deadlock it was written for #903 on its own, retires the grant on three engines, and hands every deployment a new failure in the contention case that has nothing to do with deadlocks;
• off by default — costs nobody anything, and does not retire [#903] Grant the first replay of a conflict, and bound the rest by its class #904's grant, because the deployments that keep the default keep the unbounded attempt.

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.