Seek the primary key in the SQL Server upsert and retry a transaction conflict

[vharseko]

Problem

MsSqlTestCase#test_issue_496_2 fails intermittently on CI with SQL Server error 1205, most recently in run 31627184500 — the only failing job of that build, 1 failure out of 31844 tests:

Transaction (Process ID 57) was deadlocked on lock resources with another process
and has been chosen as the deadlock victim. Rerun the transaction.
  at JDBCStorage$WriteableTransactionTransactionImpl.upsert(JDBCStorage.java:411)
  at JDBCStorage$WriteableTransactionTransactionImpl.put(JDBCStorage.java:377)

The upsert already carries WITH (HOLDLOCK, UPDLOCK) from an earlier deadlock fix, so this is a residual deadlock rather than a missing hint.

Root cause

Two independent causes, both addressed here.

1. The primary key could not be sought. The MSSQL driver binds setString parameters as NVARCHAR. Under a SQL collation such as the SQL_Latin1_General_CP1_CI_AS default of the testcontainers image, comparing the char(128) h column against an NVARCHAR value converts the column rather than the value, so the primary key cannot be sought — every read, delete and upsert scans the whole table. The upsert runs that scan under HOLDLOCK, which range-locks the entire table instead of the single key being written.

2. A conflict was never retried. Storage.write() requires an implementation to "ensure the write operation is retried until it succeeds", and WriteOperation is documented as idempotent for exactly that reason; PDBStorage.write() already loops on the conflict exception of its own engine. JDBCStorage.write() rolled back and rethrew, so a deadlock surfaced as a failed operation — a failed LDAP ADD in production, a failed test on CI — instead of a replay.

Fix

1. Cast the parameter back to char(128) where the h column is compared, so the comparison stays seekable. Other drivers keep the plain ? placeholder — their SQL is byte-for-byte unchanged.
2. Bounded retry in write(): up to 10 attempts inside a 10 s wall-clock window when the failure carries a transaction conflict anywhere in its cause chain, with a randomized delay doubling from 50 ms up to a second. The window is checked between attempts, so an attempt already running is never interrupted; it bounds the conflicts an engine is slow to report, which an attempt count alone does not — MySQL surfaces a lock wait timeout only after innodb_lock_wait_timeout, 50 s by default and overridden nowhere in this tree, so ten attempts would have parked a worker thread for eight minutes where master released it after 50 s. A deadlock, which every engine reports in well under a second, keeps its full attempt budget. Every replay is logged at WARN as one line naming the SQLState and the vendor number, with the stack trace at trace level, so the residual conflict rate stays observable without one add emitting nine stack traces. The loop is bounded, unlike PDBStorage's for(;;): the database may be shared with writers outside this server, so a conflict is not guaranteed to clear. Only the operation is replayed — a failure of getConnection() or of the implicit close(), which rolls back and returns the connection to the pool, leaves the loop, so a completed write is never replayed because releasing its connection failed.
3. read() is deliberately not retried, although Storage.read() asks for it. The contract's premise is an idempotent ReadOperation, and two of this server's read operations are not: ExportJob runs the whole export inside a single read with an LDIF writer opened once, so a replay appends the entries already written instead of truncating the file, and VerifyJob accumulates its counters in instance fields that no attempt resets, so a replay reports twice the entry count of the backend. Both are reachable online, since an export holds no more than a shared backend lock. A conflict fails the read, exactly as it did before this PR. The same exposure has existed in PDBStorage.read() all along and is recorded separately in PDBStorage.read() may replay a read whose body is not idempotent (export, verify, backendstat) #870.
4. The conflict is classified per driver, the way getTableDialect keys the column types, because the vendor numbers collide across engines: Oracle reports a deadlock as ORA-00060 with SQLState 61000, which no class 40 check covers, and gives 1205 to a fatal "not a data file" error, while 1205 is exactly the deadlock victim of SQL Server. So: Oracle 60, SQL Server 1205, and for everything else the standard class 40 states alone. MySQL needs no number of its own — Connector/J replaces the server-side HY000 of both ER_LOCK_WAIT_TIMEOUT (1205) and ER_LOCK_DEADLOCK (1213) with 40001, so the class 40 match already covers them. That timeout is not a deadlock, but it is transient in the same way and equally resolved by a replay. Two class 40 states are excluded from the blanket match: 40003 leaves the outcome of the transaction unknown, so replaying an add that in fact committed would answer the client with ENTRY_ALREADY_EXISTS, and 40002 is an integrity constraint violation a replay only hits again.
5. put() throws StorageRuntimeException, like read() and delete() beside it. With a plain RuntimeException, EntryContainer.addEntry turned every deadlock into an opaque ERR_UNCHECKED_EXCEPTION before it could be classified as a conflict.
6. The two index config delete listeners of EntryContainer remove the index from their map before the write rather than inside it. Inside, the second attempt found nothing to delete and committed an empty transaction, reporting SUCCESS for work that did not happen; before this branch it dereferenced a null instead. The null check stays — applyConfigurationAdd can fail after the config entry was persisted but before the index reached the map.
7. The two index config add listeners no longer register one listener per attempt. AttributeIndex.open() registers the index as a change listener of its configuration and the VLVIndex constructor registers the instance it builds, both inside the write. A replayed attempt registered another while only the last instance reached the map, and nothing ever closed the orphans, so they stayed registered for the lifetime of the process and every later configuration change was applied once per attempt — with dsconfig reporting SUCCESS. The attribute index is now closed before it is opened (deregisterChangeListener removes every adaptor wrapping the listener, so it is a no-op on the first attempt), and the VLV index built by the previous attempt is closed before the next one is built. The message asking for a rebuild moved out of the write for the same reason: it could otherwise be printed ten times.

On the lock footprint

The seek narrows the upsert from a whole-table range lock to the single key being written, which is the point of change 1. It is not free of conflicts: on the NOT MATCHED path the seek still takes RangeS-U on the next existing key, so two writers inserting distinct new keys into the same gap of a small tree can cycle where the whole-table lock would merely have made them queue. That window is bounded — small trees during the initial population of a backend — and it is exactly what change 2 covers: a conflict is now replayed rather than propagated. Neither change stands well alone, which is why they land together.

TestCase#testConcurrentWritersInsertingDistinctKeys exercises that shape directly, and all four engine suites inherit it: it seeds a tree with 10 rows, then has 4 threads write 3 distinct new keys each into that tree per transaction, 25 rounds apiece. Two or more new keys per transaction is what can form a cycle — a single new key takes one gap resource and can only block. It asserts that nothing escapes write() and that no record is lost.

Verification

Against mcr.microsoft.com/mssql/server:2019-CU30-ubuntu-20.04 with mssql-jdbc 13.4.0 (same image and driver as CI), replaying the storage access pattern of update():

	before	after
declared parameter	@P0 nvarchar(4000)	@P0 nvarchar(4000)
MERGE plan	Clustered Index Scan, no seek predicate, CONVERT_IMPLICIT on the column	Clustered Index Seek, seek predicate present
8 threads x 200 iterations on distinct keys, 2000-row table	20 124 ms	3 726 ms

The plan change is collation-conditional: under Windows collations (Latin1_General_BIN2, _UTF8, Japanese_CI_AS, Turkish_CI_AS, Latin1_General_CS_AS) the uncast query already seeks and the cast changes nothing. It matters under SQL collations such as SQL_Latin1_General_CP1_CI_AS, which is the testcontainers and CI default.

That benchmark measures throughput, not deadlocks — it reports 0 occurrences either way, since it writes distinct keys. The evidence for the deadlock itself is the reduction in lock footprint (whole-table range lock down to a single key) plus the review measurement below; the 1205 could not be reproduced locally on the original one-key workload.

Reported in review, on the same image and collation, hammering a single key with 8 threads and recreating the table each round: 101/330 rounds failed on master (all error 1205 / SQLState 40001), 0/630 with the cast; and 19x faster on a many-distinct-keys workload, 98.5 s -> 5.2 s.

The per-driver classification was checked against oracle/jdbc/driver/errorMap.xml of ojdbc8 23.7.0.25.01, the version this build resolves: ORA-00060 falls in the 50-68 range mapped to SQLState 61000, ORA-01205 in the 1100-1250 range mapped to 64000, and ORA-02091/02092 is the only class 40 entry in the whole file.

The MySQL mapping was checked against the compile-scope dependency itself, com.mysql:mysql-connector-j:9.2.0. MysqlErrorNumbers.mysqlToSqlstate maps 1205 and 1213 to 40001, and NativeProtocol applies that mapping whenever the server reports HY000. Of class 40 the driver emits only 40000 (error 3101, the rollback of a group replication conflict) and 40001 — never 40002 or 40003.

JDBCStorageRetryTest covers the classification against synthetic SQLExceptions — the per-driver error numbers and states including Oracle, the collisions that must not be matched driver-independently, the class 40 states that must not be replayed, the wrapped cause chains, a cyclic cause chain, the summary a replay logs, and the growth of the backoff. 26 cases, no container, 0.6 s.

Test runs, all green:

suite	result
JDBCStorageRetryTest	26/26
MsSqlTestCase	38/38
OracleTestCase	38/38
MySqlTestCase	38/38
PgSqlTestCase	38/38

Left out

• RootContainer.open() and BackendImpl.applyConfigurationChange() are genuinely not idempotent under a replay, and are left as they are. Neither is a blocker: a replay of applyConfigurationChange throws at deregisterBaseDN before it issues any SQL, so it destroys nothing master did not, and ERR_ENTRY_CONTAINER_ALREADY_REGISTERED is unreachable with a single base DN, which covers the default userRoot and CI. Both are single-threaded startup/admin paths, and both were already exposed to the retry PDBStorage has always had.
• The retry loop does not consult checkIfCanceled during its backoff. Storage.write() has no cancellation hook to consult — checkIfCanceled belongs to Operation — so wiring one through is its own change. The wall-clock window above bounds the worst case that made it worth raising: applyConfigurationDelete holds the exclusive EntryContainer lock across the whole loop.
• PDBStorage.read() keeps replaying a rolled back read, with the export and verify consequences described in PDBStorage.read() may replay a read whose body is not idempotent (export, verify, backendstat) #870. Changing it is not this PR's business, and nothing here makes it worse.

[maximthomas]

The change is correct and it works: binding h as cast(? as char(128)) turns the SQL Server plan from a clustered-index scan into a seek. Measured on mssql/server:2019-CU30, collation SQL_Latin1_General_CP1_CI_AS, mssql-jdbc 13.4.0:

• Fixes the flake. 8 threads × 1023 iterations on one key, table recreated each round: 101/330 rounds failed on master (all error 1205 / SQLState 40001), 0/630 with the cast.
• 19× faster on a many-distinct-keys workload: 98.5 s → 5.2 s.

No blocker found. key2hash zero-pads (128 chars over 200,004 measured inputs incl. empty and 1 MiB keys), no cast site ever binds a prefix, placeholder/setter counts align, there is no statement cache, and read/write parity holds across six collations in both directions.

Requesting changes for one regression the PR introduces and its fix.

New gap-lock deadlock during online bulk load (major)

The old plan held two lock resources; the new one holds one — that is the fix. But on the NOT MATCHED path the seek still takes RangeS-U on the next existing key, i.e. the gap:

empty table, after SELECT + MERGE in one txn
  no cast:   KEY RangeS-U (ffffffffffff)  +  KEY X (5eaf8eac1132)   <- 2 resources, cycles
  with cast: KEY X (5eaf8eac1132)                                   <- 1 resource, cannot cycle

table with rows, NOT MATCHED path
  with cast: KEY X (new key)  +  KEY RangeS-U (next existing key)   <- shared gap resource

RangeS-U is self-incompatible, so two transactions inserting distinct new keys that land in the same gap block each other. Because h is SHA-512, index order is a random permutation of logical key order, which defeats the ascending-key-order discipline IndexBuffer and EntryContainer deliberately maintain (opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/IndexBuffer.java:69-83).

Measured, 2 writers × 10 new keys per txn into one tree, ascending logical order:

pre-existing rows	1	2	5	20	100	500
with cast	4/10	6/10	6/10	6/10	2/10	0/10
master	0/10	0/10	0/10	0/10	0/10	—

1 new key per tree per txn → 0/30 (a single insert cannot form a same-table cycle).

Exposure is bounded but real: online initial population — parallel ldapmodify -a, or multi-threaded replication replay into an empty backend — hits five or six small trees at once (objectClass.<eq>, and the cn/sn/givenName/mail/telephoneNumber substring trees), so roughly the first ~100 entries are exposed before the trees pass 500 rows. Offline import-ldif and replication total update bypass this (they go through the importer, not addEntry). Permanent residual: member/uniqueMember equality in deployments whose distinct membership stays under a few hundred.

Steady state is unaffected — every permanently-small tree saturates and takes the MATCHED path (singleton X, no gap lock). id2childrenCount sits at ~32 rows forever on a flat DIT and writes 2 keys/txn, but both are MATCHED after each thread's first add.

With no retry in place (below), each of these surfaces as a failed LDAP ADD.

JDBCStorage.write() does not retry, violating its own SPI contract (major)

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:181-193 rolls back and rethrows:

public void write(WriteOperation writeOperation) throws Exception {
    try (final Connection con=getConnection()) {
        try {
            writeOperation.run(new WriteableTransactionTransactionImpl(con));
            con.commit();
        } catch (Exception e) {
            try { con.rollback(); } catch (SQLException ex) {}
            throw e;      // no classification, no retry
        }
    }
}

The SPI already requires otherwise:

• opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/spi/Storage.java:72-74 — "implementations must ensure the write operation is retried until it succeeds"
• opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/spi/WriteOperation.java:25-26 — "Implementation must be idempotent since operation might be retried"

The precedent is in-repo: opendj-server-legacy/src/main/java/org/opends/server/backends/pdb/PDBStorage.java:628-660 loops on the engine's conflict exception with jittered backoff. The hot paths are already replayable by design — addEntry allocates the entry ID outside the lambda (EntryContainer.java:1504), and EntryContainer.java:1529 says so explicitly: "No need to call indexBuffer.reset() since IndexBuffer content will be the same for each retry attempt". The connection survives a 1205: CachedConnection.close() rolls back and returns it to the pool, never closing it.

Suggested shape (~30-40 lines, mirror it in read() too):

for (int attempt = 1; ; attempt++) {
    try { /* existing body */ return; }
    catch (Exception e) {
        if (attempt >= MAX_RETRIES || !isRetryableConflict(e)) throw e;
        logger.warn(...);
        Thread.sleep((long) (Math.random() * MAX_SLEEP_ON_RETRY_MS));
    }
}

// walk the cause chain (~10 hops); BOTH arms are required
static boolean isRetryableConflict(Throwable t) {
    for (; t != null; t = t.getCause())
        if (t instanceof SQLException e)
            return e.getErrorCode() == 1205 || String.valueOf(e.getSQLState()).startsWith("40");
    return false;
}

Both arms matter: the xopenStates connection property makes mssql-jdbc report 1205 as 42000 instead of 40001, while in MySQL error 1205 is lock-wait-timeout — a different condition. startsWith("40") also covers Postgres 40P01 and MySQL/H2 40001. Please bound the loop rather than copying PDB's unbounded for(;;), and log each retry so the residual rate stays observable.

Note three callers that are not replayable — RootContainer.open() (RootContainer.java:135), BackendImpl.applyConfigurationChange() (BackendImpl.java:856, where replay silently skips re-registration), and the index-delete config listeners (EntryContainer.java:242, :324). All are single-threaded admin/startup paths, none is this deadlock class, and all are already exposed to PDB's existing retry — pre-existing, worth a separate ~6-line idempotence fix, not a reason to hold this up.

Deadlocks reach the client as an opaque error (minor)

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:377 wraps in a plain RuntimeException, unlike delete() (:468) and read() (:246) beside it, which both use StorageRuntimeException:

} catch (SQLException e) {
    throw new RuntimeException(e);          // -> should be StorageRuntimeException
}

Because of that, EntryContainer.addEntry's pass-through catch (StorageRuntimeException | ...) at EntryContainer.java:1565-1568 misses it, and every deadlock becomes DirectoryException(ERR_UNCHECKED_EXCEPTION) — opaque to the client and to any future retry predicate. One-word fix.

Nits

• Collation-conditional win: under Windows collations (Latin1_General_BIN2, _UTF8, Japanese_CI_AS, Turkish_CI_AS, Latin1_General_CS_AS) the uncast query already seeks — the cast changes nothing there. It matters under SQL collations such as SQL_Latin1_General_CP1_CI_AS, which is the testcontainers/CI default. The javadoc mentions this in passing; the commit message presents the win unconditionally.
• Cited benchmark doesn't demonstrate the deadlock claim: 8 threads on distinct keys over a 2000-row table measures throughput, and the PR reports 0 deadlocks either way. The deadlock fix is real, but the evidence for it is the lock footprint (2 resources → 1), not that benchmark. On the one-row flake the cast gives no speedup at all — scanning one row costs what seeking it does.
• Driver-agnostic alternative: statement.setObject(1, hash, java.sql.Types.CHAR) gives the same non-Unicode binding without building driver-specific SQL text, and would also cover the Oracle branch (JDBCStorage.java:416), which has the same h char(128) column bound via setString.
• Test coverage: the JDBC suite has no concurrent-writer test, and test_issue_496_2 hammers a single key — the MATCHED path — so it structurally cannot detect the new gap-collision path. A cheap unit test of the retry predicate against synthetic nested SQLException(msg, "40001", 1205) needs no container.
• Leftover bare binds: insert() (:434) and update() (:445) still use h=?. Verified unreachable on SQL Server (only callable from the ANSI else branch at :429), so not a bug — just noting it was checked.

[vharseko]

Thanks — the gap-lock analysis is the part I had missed, and it is what ties the two halves together. Addressed in ea4a361; the PR description is updated accordingly.

New gap-lock deadlock during online bulk load

Accepted. What convinced me is the direction of your measurement: master takes one coarse resource, so a second writer merely queues — a convoy cannot cycle. The seek replaces it with fine-grained resources, and two writers inserting distinct new keys into the same gap can then hold what the other needs. So the cast does not simply shrink the conflict window, it trades a serialization for a cycle on the NOT MATCHED path of a small tree.

That makes it a reason to land the retry with the cast rather than after it, which is what this push does. I have added the trade-off to the PR description explicitly, so it is not implied by the plan change alone.

JDBCStorage.write() does not retry

Fixed, in read() as well. Bounded at 10 attempts, with PDBStorage's randomized delay; every attempt is logged at warn with the attempt number, so a deployment that retries most of its writes stays visible instead of quietly paying for it.

The predicate walks the cause chain and keeps both arms, for the reason you give — errorCode == 1205 catches the SQL Server victim whatever xopenStates does to the state, and SQLState class 40 catches 40P01/40001 under vendor numbers of their own. MySQL's 1205 is a lock wait timeout rather than a deadlock, as you note; it is retried anyway, since that condition is transient in the same way and a replay is the same correct response. The walk is bounded at 16 hops, which also makes a cyclic cause chain terminate.

Bounded rather than PDB's for(;;) on your advice, and for one more reason: a JDBC database can be shared with writers outside this server, so nothing guarantees the conflict clears.

On the three non-replayable callers: I have guarded the two index config delete listeners (EntryContainer.java:242, :324) since it is a null check. RootContainer.open() and BackendImpl.applyConfigurationChange() I left alone and recorded in the PR description — as you say, single-threaded admin/startup paths that this conflict class cannot reach, and already exposed to PDB's retry.

Deadlocks reach the client as an opaque error

Fixed — put() now throws StorageRuntimeException. Worth stressing that it is not cosmetic: without it EntryContainer.addEntry's inner catch (Exception) converts the failure to DirectoryException(ERR_UNCHECKED_EXCEPTION) before it leaves the lambda, so the retry predicate would have had to reach through one more wrapper to classify what it is now handed directly.

Nits

• Collation-conditional win — corrected. The description now says the plan change only applies under SQL collations, and names the Windows collations that already seek.
• Benchmark doesn't demonstrate the deadlock claim — corrected. The benchmark is now presented as throughput only, with the lock footprint and your 101/330 → 0/630 measurement carrying the deadlock claim. Your numbers are cited as coming from review.
• Driver-agnostic setObject(Types.CHAR) — not taken here. mssql-jdbc applies sendStringParametersAsUnicode beyond setString, so I would want to confirm on the wire that the declared parameter actually changes type before swapping a verified fix for it. It is the better shape if it holds, and it would cover the Oracle branch too — I would rather do that as its own change, with the Oracle char(128) binding measured rather than assumed.
• Test coverage — JDBCStorageRetryTest added: the per-driver numbers and states, wrapped cause chains, the states that must not be retried, and the cyclic chain. 15 cases, no container, 0.5 s. The concurrent-writer test on distinct keys is not in this PR and is listed as left out; it needs a container and a repeat count to be meaningful, which is a different kind of test from the rest of the suite.
• Leftover bare binds in insert()/update() — agreed, and thanks for checking they are unreachable on SQL Server. Left as they are so the ANSI branch keeps identical SQL across drivers.

Runs on this push: JDBCStorageRetryTest 15/15, MsSqlTestCase 37/37, MySqlTestCase 37/37, PgSqlTestCase 37/37, jdbc/cassandra EncryptedTestCase 34/34 each, PDBTestCase 34/34 and PDBStorageTest 3/3.

[maximthomas]

The cast(? as char(128)) seek fix looks right: key2hash is always a 128-char SHA-512 hex string, so the cast neither truncates nor pads, and hashParam(con) is only ever handed a CachedConnection. The write() retry is correct and welcome. Three points on the rest.

For the record on read(): Storage.java:58-60 does mandate read retry ("implementations must ensure the read operation is retried until it succeeds"), so the loop itself conforms to the contract — the problem is that two read callers are not idempotent.

Replaying read() duplicates export output (major)

ExportJob runs the whole export inside storage.read(...), and its try/catch sits outside that call, so it cannot stop a replay:

// opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/ExportJob.java:114
rootContainer.getStorage().read(new ReadOperation<Void>() { ... exportContainer(txn, exportContainer); ... });
...
catch (Exception e) { throw new StorageRuntimeException(e); }   // outside read(), cannot stop the replay

In exportContainer, while (cursor.next()) (:177) has no swallowing catch — the two inner catch (Exception) blocks wrap only new EntryID(key) and entryFromDatabase — while entry.toLDIF(exportConfig) (:227) has already written entries 1..N-1. CursorImpl.next() -> fetchBatch throws StorageRuntimeException(SQLException) with the cause intact, so a class-40/1205 failure at row N replays the whole export. LDIFExportConfig creates the writer once, so OVERWRITE truncates only on attempt 1 and the replay appends: a duplicate-bearing LDIF reported as success. Before this commit the same conflict gave TaskState.STOPPED_BY_ERROR and a visibly truncated file.

Reachable online: ExportTask.java:363 takes only acquireSharedLock, the running server itself holds a shared lock, and JDBCStorage has no exclusivity at all — no StorageInUseException, no file lock, unlike PDB/JE. Replication total update exports the same way (LDAPReplicationDomain.java:3460).

VerifyJob is the same shape: keyCount (:75) and attrIndexList (:103) are instance fields never reset per attempt, so a replay yields keyCount ~ 2 x storedEntryCount and a spurious ERR_VERIFY_WRONG_ENTRY_COUNT (:394-397) on a healthy backend — which verify-index --countErrors returns as its exit code.

Simplest fix: retry write() only, or refuse to replay a read once its body has emitted output.

Worth knowing: the online LDAP search path is not affected, but only by accident. EntryContainer.java:1280 and DN2URI.java:639 swallow storage failures, and ID2Entry.java:489 builds a causeless DirectoryException, so isRetryableConflict can never see the SQLException. Giving that exception its cause, or turning either swallow into a rethrow, would immediately open a re-send hole into live searches.

Oracle conflicts are not classified (major)

// opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:259
if (e.getErrorCode()==ERROR_DEADLOCK_VICTIM || String.valueOf(e.getSQLState()).startsWith("40")) {

Oracle is shipped (ojdbc8 is a compile-scope dependency, packaged into lib/) and exercised (OracleTestCase, testcontainers, run under -P precommit). Per the driver's own oracle/jdbc/driver/errorMap.xml:

Oracle error	errorCode	SQLState	matched?
ORA-00060 deadlock detected	60	61000 (range 50-68)	no
ORA-01205 not a data file (fatal)	1205	64000 (range 1100-1250)	yes

The only class-40 entry in the entire file is ORA-02091/02092. So an Oracle deadlock still fails the operation — the bug this commit exists to fix — while a fatal media error gets 9 replays, 9 WARN stack traces and ~225 ms of added latency before the same error surfaces. JDBCStorageRetryTest.failures() has mssql/postgres/mysql rows and no Oracle row.

Suggest keying the vendor error numbers off the driver, as getTableDialect already does. Two side notes: the javadoc's xopenStates justification for the vendor-independent 1205 match describes a property nothing in this repo sets, and it credits H2, which is not a dependency here at all.

Replayed index delete reports SUCCESS (minor)

attrIndexMap.remove(...) is the first statement of the replayed body, so on attempt 2 the guard skips the delete, the write commits empty, and applyConfigurationDelete returns a default ConfigChangeResult — ResultCode.SUCCESS — for work that did not happen. Attempts 2..10 can never complete this operation.

It bites harder on PDB than on JDBC: removeTree is MVCC-versioned, so a rollback leaves the index trees and their TRUSTED state records fully intact; re-adding the index later has DefaultIndex.afterOpen read that surviving record, mark an empty index trusted, and skip NOTE_INDEX_ADD_REQUIRES_REBUILD — silently incomplete search results.

Minor rather than blocking because the old code was also broken here (attempt 2 threw an NPE), and ConfigurationHandler.deleteEntry deletes and persists the config entry before the listener runs either way — so the on-disk state is unchanged and only the error signal is lost.

Keep the null check: it fixes a real pre-existing NPE, since applyConfigurationAdd can fail after the config entry is persisted but before attrIndexMap.put. Just hoist the mutation out of the replayed body:

// opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/EntryContainer.java:242
final AttributeIndex index = attrIndexMap.remove(cfg.getAttribute());
attrCryptoMap.remove(cfg.getAttribute());
if (index != null)
{
  storage.write(new WriteOperation()
  {
    @Override
    public void run(WriteableTransaction txn) throws Exception
    {
      index.closeAndDelete(txn);
    }
  });
}

The replay is then idempotent: deleteTree is isExistsTable-guarded, state.deleteRecord on a missing row returns false without throwing, and close() twice is a no-op. The vlvIndexMap listener just below needs the same treatment.

Nits

• Interrupt during the backoff discards the real failure (opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:237): Thread.sleep clears the interrupt flag and throws, so retryOnConflict propagates InterruptedException in place of the SQL failure being retried, and downstream Thread.interrupted() sees false. Restore the flag and rethrow the original e with the interrupt as a suppressed exception.
• con.close() is inside the retried region (JDBCStorage.java:188): the catch is attached to the try-with-resources, so it also covers the implicit close — and CachedConnection.close() calls rollback(). A class-40 SQLException from that rollback replays an already-completed operation. An inner try around readOperation.run(...) closes this, and keeps getConnection() failures out of the retry budget too.
• Duplicate response controls on replay: addResponseControl and addAdditionalLogItem only append to lists, so a pre-send search replay can emit two ServerSideSortResponseControls or a duplicate unindexed log item.
• Flat backoff: uniform 0-50 ms with no growth, so all 9 replays finish within ~250 ms on average — unlikely to outlast sustained contention.

[vharseko]

All three land, and the read retry goes away rather than gets patched. Pushed in bc21fa4; the PR description is updated accordingly.

Replaying read() duplicates export output

Removed. Only write() replays now, and read() carries a javadoc saying why the contract is deliberately unimplemented here.

I verified both callers rather than take them on trust: LDIFExportConfig.getWriter() (:184-206) opens the stream once and keeps it, so OVERWRITE truncates on attempt 1 only and the replay appends; VerifyJob.keyCount (:75) and attrIndexList (:103) are instance fields no attempt resets, and the whole verifyBackend0 sits inside the read (:130). The online reachability holds too — ExportTask.java:363 takes only acquireSharedLock, and StorageInUseException exists solely in PDBStorage.java:1027, so nothing keeps a JDBC export off a live backend.

You are right that Storage.java:58-60 mandates the retry. It mandates it on a premise — an idempotent ReadOperation — that two in-tree callers do not satisfy, so failing the read is the honest reading. It is also what master did, so nothing regresses.

PDBStorage.read() has replayed in a for(;;) since long before this PR and carries the same exposure. Silently changing it here would be the wrong place for it, so it is filed as #870 with the evidence.

That also settles the response-controls nit: with reads no longer replayed there is nothing to append twice.

Oracle conflicts are not classified

Fixed by keying the vendor numbers off the driver, as you suggested — isConflict now branches the same way getTableDialect does. I confirmed your table against errorMap.xml in ojdbc8 23.7.0.25.01, the version this build resolves: ORA-00060 is in the 50-68 range mapped to 61000, ORA-01205 in the 1100-1250 range mapped to 64000, and 2091/2092 is indeed the only class 40 entry in the file.

• Oracle: error 60, plus class 40 which covers ORA-02091;
• SQL Server: 1205;
• MySQL: 1205, the lock wait timeout;
• everything else: class 40 only.

So ORA-00060 is retried now and ORA-01205 is not. I left ORA-08177 out: CachedConnection sets TRANSACTION_READ_COMMITTED, so it cannot arise, and I would rather add it with a reproduction than on principle.

JDBCStorageRetryTest gained the Oracle rows, including ORA-01205 as an explicit negative and a "PostgreSQL sees 1205" row so the keying itself is covered — 20 cases now. The javadoc no longer credits H2 and no longer implies the repo sets xopenStates; the SQL Server number is still matched beyond its class 40 state, but for the honest reason: a deployment can add that property to its connection URL.

Replayed index delete reports SUCCESS

Taken as written, both listeners. The map removal and the null check now sit before the write, so a replay still has the deletion to perform. I checked the JDBC side is replay-safe as you said it is on PDB: deleteTree is isExistsTable-guarded (JDBCStorage.java:450), State.deleteRecord returns false rather than throwing, and close() twice is a no-op.

Nits

• Interrupt — the flag is restored and the failure being replayed is rethrown with the InterruptedException suppressed into it.
• con.close() inside the retried region — the operation is now rethrown inside the try-with-resources, so a close failure is suppressed into it rather than replacing it, and the outer catch replays only that same exception object. getConnection() and close() failures leave the loop.
• Duplicate response controls — moot, see above.
• Flat backoff — 50 ms doubling per attempt, capped at 1 s, still fully randomized: about 2.8 s of expected total wait across 9 replays instead of 0.25 s. There is a test for the growth and the cap.

Runs on this push: JDBCStorageRetryTest 20/20, MsSqlTestCase 37/37, OracleTestCase 37/37, MySqlTestCase 37/37, PgSqlTestCase 37/37, jdbc/cassandra EncryptedTestCase 34/34 each, PDBTestCase 34/34 and PDBStorageTest 3/3.

[maximthomas]

The cast(? as char(128)) half is solid — key2hash always yields exactly 128 hex chars, all five cast sites bind a complete hash, non-Microsoft SQL text is byte-identical, and the measured plan change (scan → seek) is real. The retry loop's mechanics are also right: the e != failure identity check correctly keeps getConnection()/close() failures out of the replayed region, and the cause-chain walk is cycle-bounded. Splitting the read-replay problem out to #870/#871 is clean.

No blockers or majors. Four minor items below, all cheap.

Retry loop has no wall-clock bound (minor)

MAX_RETRIES bounds attempts, not time. That is fine for a deadlock, which every engine reports in well under a second, but not for a conflict that is slow to report. MySQL surfaces ER_LOCK_WAIT_TIMEOUT only after innodb_lock_wait_timeout (50 s by default; nothing in-tree overrides it), so 10 attempts can park a worker thread for ~505 s where master released it at 50 s.

This is not a merge blocker — MySQL is the only engine affected, and it is also the only one that had a bound at all (SQL Server lockTimeout=-1, Oracle has no row-lock timeout, PostgreSQL lock_timeout=0 all park a worker indefinitely today). For a holder lasting 50–505 s the retry actually wins, turning a hard failure into a success. Only past ~505 s is it a pure loss. But nothing else caps it: there is no setQueryTimeout anywhere under opendj-server-legacy/src/main/java/org/opends/server/backends/, all driver timeouts default to 0/-1, and ds-cfg-max-blocked-write-time-limit governs client socket writes only.

A deadline also discriminates correctly — fast deadlocks keep their full attempt budget, a 50 s timeout blows it on attempt 1:

	/**
	 * Wall-clock budget the retry loop may spend, in nanoseconds. Checked between attempts, so an attempt already
	 * running is never interrupted: the loop returns after at most this window plus one attempt. It bounds the
	 * conflicts that are slow to report - MySQL reports ER_LOCK_WAIT_TIMEOUT only after innodb_lock_wait_timeout,
	 * 50 s by default, so 10 attempts would park a worker thread for eight minutes - while leaving the full attempt
	 * budget to the deadlocks this retry exists for, which every engine reports in well under a second.
	 */
	private static final long MAX_RETRY_WINDOW_NANOS = 10_000L * 1_000_000L;

	public void write(WriteOperation writeOperation) throws Exception {
		final long giveUpAt = System.nanoTime() + MAX_RETRY_WINDOW_NANOS;
		for (int attempt=1;;attempt++) {
			// ... unchanged ...
			if (attempt>=MAX_RETRIES || System.nanoTime()-giveUpAt >= 0 || !isRetryableConflict(failure,driver)) {
				throw failure;
			}

System.nanoTime()-giveUpAt >= 0 is the overflow-safe form; no new imports needed.

The MySQL branch is dead code, and documents a SQLState the driver cannot emit (minor)

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:318-319

Connector/J overrides ER_LOCK_WAIT_TIMEOUT's server-side HY000 to 40001 (a "Manual override" for Bug#16634180, applied unconditionally in NativeProtocol, present since 5.1.49). Verified by reflection against the exact compile-scope dependency, com.mysql:mysql-connector-j:9.2.0:

1205 (ER_LOCK_WAIT_TIMEOUT) -> 40001      1206 (ER_LOCK_TABLE_FULL) -> HY000
1213 (ER_LOCK_DEADLOCK)     -> 40001      1062 (ER_DUP_ENTRY)       -> 23000

So startsWith("40") at :310 matches first and the mysql branch never runs. Two knock-on corrections:

• the javadoc at :65 and :296-297 says MySQL reports 1205 "with SQLState HY000" — it does not;
• opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/JDBCStorageRetryTest.java:73 and :96 assert sql(1205, "HY000"), a shape the driver cannot produce. Those rows pass only because the dead branch catches them. They should be sql(1205, "40001"), which also makes :96 ("unknown driver") flip to true — correctly, since class 40 is driver-independent.

Note this means MySQL lock-wait timeouts are retried via :310 regardless, so deleting the branch is a behavioural no-op — it is the deadline above that changes behaviour.

Replayed index add re-registers config change listeners (minor)

opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/EntryContainer.java — the two index delete listeners were made replay-safe; the two add listeners were not.

VLVIndex's constructor ends with this.config.addChangeListener(this) (VLVIndex.java:144) and is called inside the lambda, so each attempt builds and registers another instance while only the last reaches vlvIndexMap. There is no dedup anywhere: ServerManagedObject.registerChangeListener wraps a fresh adaptor per call, the store is a CopyOnWriteArrayList with plain add(), and neither adaptor overrides equals/hashCode. The orphans are never closed, so they stay registered for the process lifetime and every later VLV config change is applied N times.

Reachability is narrow — in both add paths every deadlock-prone statement (the state-table merge under HOLDLOCK, UPDLOCK, the id2entry scan) runs before the registration, so a duplicate needs the conflict to land in the trailing catalog/DDL window, i.e. concurrent DDL. And the effect is benign rather than corrupting: AttributeIndex.applyConfigurationChange is synchronized and converges after invocation 1 (removedIndexes/addedIndexes are both empty from invocation 2 on), so there is no delete-and-recreate race. What is worth fixing is that on a successful retry dsconfig reports SUCCESS while the orphans leak, where master failed loudly.

For the attribute path one line suffices, since deregisterChangeListener removes every adaptor wrapping the listener (so it is a no-op on attempt 1):

// close() removes every adaptor registered for this index, so a replayed attempt does not add a second one
index.close();
index.open(txn, true);

For VLV the previous attempt's instance has to be closed before the next is built — same shape as the hoisting already done for the delete listeners.

No concurrent-writer test on distinct keys (minor)

This is the one gap the PR's own "On the lock footprint" section leaves open. The new gap-lock path needs two or more new keys inserted into the same table per transaction to form a cycle — a single shared gap is one resource and can only block. Nothing in the suite exercises that:

• PluggableBackendImplTestCase.java:1275-1336 — 8 threads × 1023 iterations, all on the literal key "key", i.e. one resource and the MATCHED path;
• opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java — all three tests single-threaded;
• ID2ChildrenCountTest is concurrent but binds PDBStorage, never JDBC;
• JDBCStorageRetryTest is synthetic and threadless.

A test in jdbc/TestCase.java (so all four engines inherit it) seeding ~10 rows then running N≥2 threads × M rounds, each writing ≥2 distinct new keys per storage.write, asserting nothing escapes — that is the harness that produced the 4–6/10 numbers, and it would regression-guard both halves of this PR.

Nits

• "Left out" overstates reachability: it says these paths are ones "that the conflict class handled here cannot reach". That is unproven, and it sits awkwardly next to the shared-database premise used to justify bounding the loop two paragraphs earlier. BackendImpl.applyConfigurationChange and RootContainer.open() are both genuinely non-idempotent — worth saying so plainly instead. Neither is a blocker: a replay of applyConfigurationChange throws at deregisterBaseDN before issuing any SQL, so it destroys nothing master didn't; and ERR_ENTRY_CONTAINER_ALREADY_REGISTERED is unreachable with a single base DN, which covers the default userRoot and CI.
• ccr messages accumulate per attempt: both add listeners call ccr.addMessage(NOTE_INDEX_ADD_REQUIRES_REBUILD...) inside the lambda, so dsconfig can print it up to 10 times. Moving it out with the map puts fixes it.
• Retry logging volume: JDBCStorage.java:258 logs a full single-line stack trace at WARN per retry. During the initial-population window this PR expects to conflict, one add can emit 9 of them, which reads as failure to monitoring. Attempt count + SQLState/error number at WARN, stack trace at debug.
• Backoff ignores cancellation: the loop never consults checkIfCanceled, and applyConfigurationDelete holds the exclusive EntryContainer.this.lock() — which drains all in-flight shared access — across the whole ~5.5 s of it.
• SQLState 40003: the blanket class-40 test also matches "statement completion unknown". A con.commit() that fails that way is inside the retried try, so it replays, and addEntry's replay then returns ENTRY_ALREADY_EXISTS for an add that succeeded. Narrowing the blanket match to 40001/40P01 closes it.
• Collation-conditional win: worth keeping the note that under Windows collations (Latin1_General_BIN2, _UTF8, Japanese_CI_AS, Turkish_CI_AS, Latin1_General_CS_AS) the uncast query already seeks and the cast is a no-op. The description does say this; the commit message does not.
• Merge interaction: #866 also modifies JDBCStorage.java. Expect a conflict for whichever lands second.

Three pre-existing bugs surfaced while reviewing this. None belong to this PR, but each is worth its own issue:

1. PersistentCompressedSchema.java:59,61 builds static final TreeName("compressed_schema", ...) with a literal in the baseDN slot — no backend qualifier — and getTableName is sha224(TreeName). Every JDBC backend sharing a database URL therefore shares the same two tables.
2. export-ldif, verify-index and backendstat cannot open a JDBC backend at all: BackendImpl.java:952 uses AccessMode.READ_ONLY, RootContainer.open always goes through storage.write(...), and WriteableTransactionTransactionImpl's constructor throws ReadOnlyStorageException when the mode is not writeable (JDBCStorage.java:260-264).
3. CachedConnection.java:99-105 — getConnection recurses unboundedly with a doubling wait when DriverManager.getConnection fails, so pool exhaustion has no bound.

[vharseko]

All four minors are taken, in bfb8c95; the PR description is updated accordingly. Two of the nits I answered differently than suggested, and one of them is a correction to the review — details below.

Retry loop has no wall-clock bound

Taken as written: a 10 s window, checked between attempts with the overflow-safe System.nanoTime()-giveUpAt >= 0.

Worth stating the consequence plainly rather than leaving it implied, because it reverses a decision I made two rounds ago: with a 10 s window a MySQL ER_LOCK_WAIT_TIMEOUT gets zero replays — 50 s blows the whole budget on attempt 1 — so on MySQL that condition now fails the way master failed it. I kept the window at 10 s anyway rather than sizing it to fit one retry after a 50 s wait. A deadlock is what this retry exists for and every engine reports one in milliseconds; a caller that has already waited 50 s is better served by an error than by another 50 s. The javadoc no longer says the timeout "is equally resolved by a replay" without qualification.

The MySQL branch is dead code

Confirmed independently before deleting it, by reflection against the exact compile-scope dependency:

MysqlErrorNumbers.mysqlToSqlstate:  1205 -> 40001   1213 -> 40001   1206 -> HY000   1062 -> 23000
class 40 the driver can emit at all: 40000 <- 3101   40001 <- 1205, 1213

and the override is visible in NativeProtocol itself — if (state.equals("HY000")) state = mysqlToSqlstate(errno). So the branch was unreachable and is gone; the javadoc at :65 and :296-297 and the sql(1205, "HY000") row are corrected. A MySQL lock wait timeout is still replayed, by its state rather than by its number, so nothing changes behaviourally.

One departure: I did not flip :96 ("unknown driver") to sql(1205, "40001"). That row's job is to assert that a vendor number is never matched without a driver to key it off, which is worth keeping as a negative — the engines collide on 1205. Driver-independence of class 40 is now asserted by a separate positive row instead, and there is a new row for 3101/40000.

Replayed index add re-registers config change listeners

Taken, both listeners, plus the ccr nit folded into the same change — the rebuild message and setAdminActionRequired now sit after the write.

I verified the mechanism rather than take it on trust: AttributeIndex.open() ends with config.addChangeListener(this) (AttributeIndex.java:447), VLVIndex's constructor with the same call (:144); ServerManagedObject.registerChangeListener wraps a fresh ConfigChangeListenerAdaptor per call with no dedup, and deregisterChangeListener loops the whole list and removes every adaptor whose listener is that instance — which is what makes index.close() before index.open() a no-op on attempt 1 and exactly right on attempt 2.

One correction to the reachability estimate, in the direction of it mattering more. For VLV the registration is the last statement of the constructor, and the constructor is not the trailing catalog window: it reads state.getIndexFlags(txn, ...) and entryContainer.getHighestEntryID(txn), and may call setTrusted(txn, true), all of which are ordinary statements that can be picked as a deadlock victim; vlvIndex.open(txn, true), which creates the trees, runs after it. So on the VLV path a conflict anywhere from the tree creation to the commit leaves an orphan, not only concurrent DDL.

No concurrent-writer test on distinct keys

Added as TestCase#testConcurrentWritersInsertingDistinctKeys, in jdbc/TestCase.java so all four engines inherit it: seed 10 rows, then 4 threads x 25 rounds x 3 distinct new keys per storage.write into the same tree, asserting that nothing escapes write() and that the record count is exact. Two or more new keys per transaction is the part that matters, as you say — one gap resource can only block.

Nits

• ccr messages per attempt — fixed with the listener change above.
• Retry logging volume — fixed. WARN is now one line: attempt number, SQLState, vendor error number and message. The stack trace moved to trace level.
• SQLState 40003 — the concern is right, the narrowing is not the fix I took. Restricting the blanket match to 40001/40P01 would drop 40000, which Connector/J really does emit (error 3101, the rollback of a group replication conflict — replayable), and would fail any further engine reporting a conflict of its own. 40002 and 40003 are excluded explicitly instead, which closes the same hole and keeps class 40 generic. Neither is reachable with the drivers shipped here — of class 40 Connector/J emits only 40000/40001, Oracle only ORA-02091/02092, mssql-jdbc and PostgreSQL report their deadlock as 40001/40P01 — so this is belt and braces rather than a fix for something observed.
• Backoff ignores cancellation — left out, and now recorded under "Left out" with the reason. Storage.write() has no cancellation hook to consult; checkIfCanceled belongs to Operation, so plumbing one through is its own change. The wall-clock window bounds the case that made it worth raising: the exclusive EntryContainer lock held by applyConfigurationDelete is now capped at ~10 s rather than at 10 attempts of unbounded length.
• "Left out" overstates reachability — corrected; both paths are now described as genuinely non-idempotent, with your two reasons why neither is a blocker.
• Collation-conditional win — the qualification is now in the commit message as well, naming the Windows collations that already seek.
• Merge interaction with Stamp JDBC backend tables with their tree name and refresh optimizer statistics after import #866 — this one does not hold, and it is cheap to check. git merge-tree from the common base b9b857b0b1 between the two branches produces zero conflict markers: JDBCStorage.java is changed in both but merges automatically, since Stamp JDBC backend tables with their tree name and refresh optimizer statistics after import #866's additions live outside write(), on their own connections. Still worth re-verifying after whichever lands first, but no conflict is expected.

The three pre-existing bugs are all real; I confirmed the second down to the code path — BackendImpl.java:952 opens READ_ONLY, RootContainer.open() always goes through storage.write(...) (:130-135), and WriteableTransactionTransactionImpl's constructor throws ReadOnlyStorageException when the mode is not writeable (JDBCStorage.java:406-408), so export-ldif, verify-index and backendstat cannot open a JDBC backend at all. Filing the three separately; none belongs on this branch.

[vharseko]

The three pre-existing bugs are filed: #873 (compressed-schema tables shared by every JDBC backend on one database), #874 (offline export-ldif/verify-index/backendstat cannot open a JDBC backend), #875 (CachedConnection.getConnection retries any SQLException forever).

Two refinements came out of writing them up. For #874 the failure is narrower than "cannot open a JDBC backend at all": getReadOnlyRootContainer() is reached only under mustOpenRootContainer(), which is rootContainer == null, so the online task path reuses the server's READ_WRITE container and works — it is the offline tools that fail. And for #873 the corrupting step is the token allocation: id = mappings.adDecodeMap.size() under the instance's own lock (CompressedSchema.java:296), one instance per RootContainer, so two backends hand out the same token and the second store() overwrites the first. Cassandra is not affected — CASStorage.getTableName() qualifies the table with the backend id.