Seek the primary key in the SQL Server upsert instead of scanning the table

[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

The MSSQL driver binds setString parameters as NVARCHAR. Under the server's default SQL_Latin1_General_CP1_CI_AS collation, 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.

That scan is what makes the deadlock possible: the upsert runs it under HOLDLOCK, which range-locks the entire table instead of the single key being written.

Fix

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.

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

mvn -pl opendj-server-legacy -Pprecommit verify -Dit.test=MsSqlTestCase -> Tests run: 37, Failures: 0, Errors: 0, BUILD SUCCESS.

Note: the 1205 deadlock itself could not be reproduced locally — 0 occurrences both before and after the change. It is flaky on CI as well, passing on 4 of the 5 JDK jobs of that same build. The effect on the deadlock is therefore inferred from the measured reduction in lock footprint, from a whole-table range lock down to a single key, rather than directly observed.

A complementary hardening — retrying a transaction on error 1205, which SQL Server documents as transient ("Rerun the transaction") — is deliberately left out of this PR.

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