From 49d89a117f68ae75dfcbbb3384b8b185ee7e2ee2 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Fri, 14 Aug 2026 09:29:13 +0300 Subject: [PATCH 1/3] Seek the primary key in the SQL Server upsert instead of scanning the table The MSSQL driver binds setString parameters as NVARCHAR. Under the server's 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 no statement could seek the primary key: every read, delete and upsert scanned the whole table. The upsert runs that scan under WITH (HOLDLOCK, UPDLOCK), which range-locks the entire table instead of the single key being written - the lock footprint behind the intermittent "Transaction (Process ID N) was deadlocked on lock resources" failures of MsSqlTestCase#test_issue_496_2. Casting the parameter back to char(128) keeps the comparison seekable. Verified against mssql/server:2019-CU30 with driver 13.4.0: the MERGE plan goes from Clustered Index Scan (no seek predicate, CONVERT_IMPLICIT on the column) to Clustered Index Seek, and eight threads writing distinct keys in a 2000-row table finish in 3.7 s instead of 20.1 s. Other drivers keep the plain "?" placeholder, so their SQL is unchanged. --- .../server/backends/jdbc/JDBCStorage.java | 27 ++++++++++++++----- 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java index fcf3737df4..3291360463 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java @@ -219,6 +219,21 @@ static byte[] db2real(byte[] db) { } }); + /** + * Returns the placeholder to compare against the {@code h} column, casting it where the driver would + * otherwise bind a value of the wrong type. + *

+ * The SQL Server driver sends {@link PreparedStatement#setString} parameters as NVARCHAR, and under a SQL + * collation comparing the {@code char(128)} column against an NVARCHAR value converts the column instead of + * the value: the primary key can no longer be sought, so every statement scans the whole table rather than + * reading one row. The upsert runs that scan under HOLDLOCK, which range-locks the entire table instead of + * the single key being written - the lock footprint that lets concurrent writers deadlock (error 1205). + * Casting the parameter back to char keeps the comparison seekable. + */ + static String hashParam(Connection con) { + return ((CachedConnection) con).parent.getClass().getName().contains("microsoft") ? "cast(? as char(128))" : "?"; + } + private class ReadableTransactionImpl implements ReadableTransaction { final Connection con; boolean isReadOnly=true; @@ -229,7 +244,7 @@ public ReadableTransactionImpl(Connection con) { @Override public ByteString read(TreeName treeName, ByteSequence key) { - try (final PreparedStatement statement=con.prepareStatement("select v from "+getTableName(treeName)+" where h=? and k=?")){ + try (final PreparedStatement statement=con.prepareStatement("select v from "+getTableName(treeName)+" where h="+hashParam(con)+" and k=?")){ statement.setString(1,key2hash.get(ByteBuffer.wrap(key.toByteArray()))); statement.setBytes(2,real2db(key.toByteArray())); try(ResultSet rc=executeResultSet(statement)) { @@ -403,8 +418,8 @@ boolean upsert(TreeName treeName, ByteSequence key, ByteSequence value) throws S statement.setBytes(3, value.toByteArray()); return (execute(statement) == 1 && statement.getUpdateCount() > 0); } - }else if (driverName.contains("microsoft")) { //ANSI MERGE with ; WITH (HOLDLOCK) makes the upsert atomic: without it SQL Server MERGE can race two concurrent NOT MATCHED inserts of the same key into a PRIMARY KEY violation. UPDLOCK is required on top of it: with HOLDLOCK alone the search phase takes a shared lock that the WHEN MATCHED update then has to convert to an exclusive one, so two concurrent upserts of the same key deadlock on the conversion; an update lock is taken right away and makes the second transaction wait instead - try (final PreparedStatement statement = con.prepareStatement("merge into " + getTableName(treeName) + " WITH (HOLDLOCK, UPDLOCK) old using (select ? h,? k,? v) new on (old.h=new.h and old.k=new.k) WHEN MATCHED THEN UPDATE SET old.v=new.v WHEN NOT MATCHED THEN INSERT (h,k,v) VALUES (new.h,new.k,new.v);")) { + }else if (driverName.contains("microsoft")) { //ANSI MERGE with ; WITH (HOLDLOCK) makes the upsert atomic: without it SQL Server MERGE can race two concurrent NOT MATCHED inserts of the same key into a PRIMARY KEY violation. UPDLOCK is required on top of it: with HOLDLOCK alone the search phase takes a shared lock that the WHEN MATCHED update then has to convert to an exclusive one, so two concurrent upserts of the same key deadlock on the conversion; an update lock is taken right away and makes the second transaction wait instead. h is cast back to char so that the join can seek the primary key instead of scanning the whole table under those locks, see hashParam() + try (final PreparedStatement statement = con.prepareStatement("merge into " + getTableName(treeName) + " WITH (HOLDLOCK, UPDLOCK) old using (select cast(? as char(128)) h,? k,? v) new on (old.h=new.h and old.k=new.k) WHEN MATCHED THEN UPDATE SET old.v=new.v WHEN NOT MATCHED THEN INSERT (h,k,v) VALUES (new.h,new.k,new.v);")) { statement.setString(1, key2hash.get(ByteBuffer.wrap(key.toByteArray()))); statement.setBytes(2, real2db(key.toByteArray())); statement.setBytes(3, value.toByteArray()); @@ -453,7 +468,7 @@ public boolean update(TreeName treeName, ByteSequence key, UpdateFunction f) { @Override public boolean delete(TreeName treeName, ByteSequence key) { - try (final PreparedStatement statement=con.prepareStatement("delete from "+getTableName(treeName)+" where h=? and k=?")){ + try (final PreparedStatement statement=con.prepareStatement("delete from "+getTableName(treeName)+" where h="+hashParam(con)+" and k=?")){ statement.setString(1,key2hash.get(ByteBuffer.wrap(key.toByteArray()))); statement.setBytes(2,real2db(key.toByteArray())); return (execute(statement)==1 && statement.getUpdateCount()>0); @@ -572,7 +587,7 @@ public void delete() throws NoSuchElementException, UnsupportedOperationExceptio if (isReadOnly) { throw new UnsupportedOperationException(); } - try (final PreparedStatement statement=con.prepareStatement("delete from "+tableName+" where h=? and k=?")){ + try (final PreparedStatement statement=con.prepareStatement("delete from "+tableName+" where h="+hashParam(con)+" and k=?")){ statement.setString(1,key2hash.get(ByteBuffer.wrap(db2real(currentKeyDb)))); statement.setBytes(2,currentKeyDb); execute(statement); @@ -616,7 +631,7 @@ && compareKeys(target,buffer.peekLast()[0])<=0) { @Override public boolean positionToKey(ByteSequence key) { final byte[] real=key.toByteArray(); - try (final PreparedStatement statement=con.prepareStatement("select v from "+tableName+" where h=? and k=?")){ + try (final PreparedStatement statement=con.prepareStatement("select v from "+tableName+" where h="+hashParam(con)+" and k=?")){ statement.setString(1,key2hash.get(ByteBuffer.wrap(real))); statement.setBytes(2,real2db(real)); try(final ResultSet rc=executeResultSet(statement)) { From ea4a361e783cba4e121f4bec47f582a524345b5f Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Tue, 18 Aug 2026 11:53:24 +0300 Subject: [PATCH 2/3] Retry a JDBC transaction conflict instead of failing the operation Storage.write() requires an implementation to retry a rolled back operation until it succeeds, and WriteOperation is documented as idempotent for exactly that reason; PDBStorage already loops on the conflict exception of its own engine, while JDBCStorage rolled back and rethrew. A SQL Server deadlock (error 1205, "Rerun the transaction") therefore reached the client as a failed operation. read() and write() now replay the operation up to 10 times, with the randomized delay PDBStorage uses, when the failure carries a transaction conflict anywhere in its cause chain. The loop is bounded, unlike PDBStorage: the database may be shared with writers outside this server, so a conflict is not guaranteed to clear. Both the vendor error number and the SQLState are examined - the xopenStates property makes the SQL Server driver report 1205 as 42000 rather than 40001, and the other engines carry the conflict in the standard class 40 states under vendor numbers of their own. Every retry is logged so that the residual conflict rate stays observable. put() now throws StorageRuntimeException, like read() and delete() beside it: EntryContainer passes that type through unchanged, while any other runtime exception became an opaque ERR_UNCHECKED_EXCEPTION before it could be classified as a conflict. The two index config delete listeners of EntryContainer tolerate a replay, their map removal no longer dereferencing a null on the second attempt. --- .../server/backends/jdbc/JDBCStorage.java | 97 ++++++++++++++++--- .../backends/pluggable/EntryContainer.java | 14 ++- .../backends/jdbc/JDBCStorageRetryTest.java | 94 ++++++++++++++++++ 3 files changed, 192 insertions(+), 13 deletions(-) create mode 100644 opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/JDBCStorageRetryTest.java diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java index 3291360463..10ae071f60 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java @@ -47,6 +47,18 @@ public class JDBCStorage implements org.opends.server.backends.pluggable.spi.Sto private static final LocalizedLogger logger = LocalizedLogger.getLoggerForThisClass(); + /** Number of attempts a {@link #read} or {@link #write} makes before it propagates the conflict to the caller. */ + private static final int MAX_RETRIES = 10; + + /** Upper bound of the random delay inserted between two attempts, in milliseconds. */ + private static final double MAX_SLEEP_ON_RETRY_MS = 50.0; + + /** Number of {@link Throwable#getCause()} hops walked when classifying a failure, also a guard against a cycle. */ + private static final int MAX_CAUSE_HOPS = 16; + + /** SQL Server error number of the transaction picked as the deadlock victim: "Rerun the transaction". */ + private static final int ERROR_DEADLOCK_VICTIM = 1205; + private JDBCBackendCfg config; public JDBCStorage(JDBCBackendCfg cfg, ServerContext serverContext) { @@ -172,24 +184,84 @@ public void removeStorageFiles() throws StorageRuntimeException { //operation @Override public T read(ReadOperation readOperation) throws Exception { - try(final Connection con=getConnection()) { - return readOperation.run(new ReadableTransactionImpl(con)); + for (int attempt=1;;attempt++) { + try(final Connection con=getConnection()) { + return readOperation.run(new ReadableTransactionImpl(con)); + } catch (Exception e) { + if (!retryOnConflict(e,attempt)) { + throw e; + } + } } } @Override public void write(WriteOperation writeOperation) throws Exception { - try (final Connection con=getConnection()) { - try { - writeOperation.run(new WriteableTransactionTransactionImpl(con)); - con.commit(); - } catch (Exception e) { + for (int attempt=1;;attempt++) { + try (final Connection con=getConnection()) { try { - con.rollback(); - } catch (SQLException ex) {} - throw e; + writeOperation.run(new WriteableTransactionTransactionImpl(con)); + con.commit(); + return; + } catch (Exception e) { + try { + con.rollback(); + } catch (SQLException ex) {} + throw e; + } + } catch (Exception e) { + if (!retryOnConflict(e,attempt)) { + throw e; + } + } + } + } + + /** + * Waits for a short random delay and returns whether the failed attempt should be replayed. + *

+ * {@link org.opends.server.backends.pluggable.spi.Storage#write(WriteOperation)} requires an implementation to + * retry a rolled back operation until it succeeds, and {@link WriteOperation} is documented as idempotent for + * exactly that reason; {@link org.opends.server.backends.pdb.PDBStorage#write(WriteOperation)} already does so + * on the conflict exception of its own engine. The loop is bounded here, unlike PDBStorage: the database may be + * shared with writers outside this server, so a conflict is not guaranteed to clear and failing the operation is + * better than never returning. The delay is randomized to spread the retries of the transactions that collided. + */ + private boolean retryOnConflict(Exception e, int attempt) throws InterruptedException { + if (attempt>=MAX_RETRIES || !isRetryableConflict(e)) { + return false; + } + //logged rather than silently absorbed, so that a deployment retrying most of its writes stays observable + logger.warn(LocalizableMessage.raw("jdbc: replaying the transaction after a conflict, attempt %d of %d: %s", + attempt, MAX_RETRIES, stackTraceToSingleLineString(e))); + Thread.sleep((long) (Math.random() * MAX_SLEEP_ON_RETRY_MS)); + return true; + } + + /** + * Returns whether the given failure carries a transaction conflict that replaying the operation can resolve. + *

+ * The conflict is looked up along the whole cause chain because it reaches this class wrapped: a deadlock in + * {@code put} arrives as {@code StorageRuntimeException(SQLException)}, and a caller such as + * {@code EntryContainer.addEntry} may wrap it once more. + *

+ * Both the vendor error number and the SQLState are examined, since neither alone covers the drivers in use. + * SQL Server reports the deadlock victim as error 1205 with SQLState 40001, but as 42000 when the connection was + * opened with {@code xopenStates=true}, so the state cannot be relied upon; conversely the standard class 40 + * states carry the conflict of the other engines - 40P01 for PostgreSQL, 40001 for MySQL and H2 - under vendor + * error numbers of their own. Error 1205 of MySQL is a lock wait timeout rather than a deadlock, which the code + * alone cannot tell apart, but that condition is transient as well and is equally resolved by a replay. + */ + static boolean isRetryableConflict(Throwable t) { + for (int hop=0; t!=null && hop + * Runs without a database: the failures the drivers report are reproduced as synthetic + * {@link SQLException}s carrying the same vendor error number and SQLState. + */ +@Test(sequential = true) +@SuppressWarnings("javadoc") +public class JDBCStorageRetryTest extends DirectoryServerTestCase +{ + /** A failure whose cause chain is a cycle, to check that walking it terminates. */ + private static final class SelfCausedException extends RuntimeException + { + private static final long serialVersionUID = 1L; + + @Override + public synchronized Throwable getCause() + { + return this; + } + } + + @DataProvider + public Object[][] failures() + { + return new Object[][] { + // SQL Server picking a transaction as the deadlock victim: the failure this retry exists for + { "mssql deadlock victim", sql(1205, "40001"), true }, + // the xopenStates connection property makes the same driver report a state of no help here + { "mssql deadlock victim, xopenStates", sql(1205, "42000"), true }, + // the conflict of the other engines is carried by the SQLState, under a vendor number of their own + { "postgres serialization failure", sql(0, "40001"), true }, + { "postgres deadlock detected", sql(0, "40P01"), true }, + { "mysql deadlock", sql(1213, "40001"), true }, + // not a deadlock, but transient in the same way and equally resolved by a replay + { "mysql lock wait timeout", sql(1205, "HY000"), true }, + + // the conflict reaches JDBCStorage.write() wrapped, so the whole cause chain has to be walked + { "wrapped once", new StorageRuntimeException(sql(1205, "40001")), true }, + { "wrapped twice", + new DirectoryException(OTHER, raw("unchecked"), new StorageRuntimeException(sql(1205, "40001"))), true }, + + // nothing a replay can resolve + { "primary key violation", sql(2627, "23000"), false }, + { "syntax error", sql(102, "S0001"), false }, + { "no SQLState", sql(0, null), false }, + { "not a SQLException", new IllegalStateException("connection closed"), false }, + { "wrapped, not a conflict", new StorageRuntimeException(sql(2627, "23000")), false }, + { "no failure at all", null, false }, + { "cyclic cause chain", new SelfCausedException(), false }, + }; + } + + @Test(dataProvider = "failures") + public void testIsRetryableConflict(String name, Throwable failure, boolean expected) + { + assertEquals(JDBCStorage.isRetryableConflict(failure), expected, name); + } + + private static SQLException sql(int errorCode, String sqlState) + { + return new SQLException("synthetic failure", sqlState, errorCode); + } +} From bc21fa4cf69a4d52ed9736710b3267941438541f Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Tue, 18 Aug 2026 21:40:43 +0300 Subject: [PATCH 3/3] Address the review: replay only writes, and classify the conflict per driver read() no longer replays a rolled back operation. Storage asks an implementation to retry a read until it succeeds, but two of the read operations of this server are not idempotent: ExportJob runs the whole export inside a single read and its LDIF writer is 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 while the server is online, since an export holds no more than a shared backend lock. A conflict fails the read again, exactly as it did before this branch. The vendor error numbers are now keyed off the driver, the way getTableDialect keys the column types. They cannot be matched driver-independently: 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, which was replayed nine times for nothing, while 1205 is exactly the deadlock victim of SQL Server and the lock wait timeout of MySQL. The two index config delete listeners remove the index from their map before the write rather than inside it: left inside, the second attempt found nothing to delete and committed an empty transaction, reporting success for work that did not happen. Three corrections to the retry itself. An interrupt during the backoff now restores the flag and rethrows the failure being replayed, with the InterruptedException suppressed into it, instead of replacing it. 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. And the delay doubles from 50 ms up to a second instead of staying uniform under 50 ms. --- .../server/backends/jdbc/JDBCStorage.java | 157 ++++++++++++------ .../backends/pluggable/EntryContainer.java | 38 +++-- .../backends/jdbc/JDBCStorageRetryTest.java | 77 ++++++--- 3 files changed, 185 insertions(+), 87 deletions(-) diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java index 10ae071f60..3ead059ad2 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java @@ -47,17 +47,26 @@ public class JDBCStorage implements org.opends.server.backends.pluggable.spi.Sto private static final LocalizedLogger logger = LocalizedLogger.getLoggerForThisClass(); - /** Number of attempts a {@link #read} or {@link #write} makes before it propagates the conflict to the caller. */ + /** Number of attempts a {@link #write} makes before it propagates the conflict to the caller. */ private static final int MAX_RETRIES = 10; - /** Upper bound of the random delay inserted between two attempts, in milliseconds. */ - private static final double MAX_SLEEP_ON_RETRY_MS = 50.0; + /** Upper bound of the random delay before the second attempt, in milliseconds; it doubles with every attempt. */ + private static final double BASE_SLEEP_ON_RETRY_MS = 50.0; + + /** Upper bound the doubled delay is capped at, in milliseconds. */ + private static final double MAX_SLEEP_ON_RETRY_MS = 1000.0; /** Number of {@link Throwable#getCause()} hops walked when classifying a failure, also a guard against a cycle. */ private static final int MAX_CAUSE_HOPS = 16; /** SQL Server error number of the transaction picked as the deadlock victim: "Rerun the transaction". */ - private static final int ERROR_DEADLOCK_VICTIM = 1205; + private static final int MSSQL_DEADLOCK_VICTIM = 1205; + + /** MySQL error number of a lock wait that timed out: ER_LOCK_WAIT_TIMEOUT, reported with SQLState HY000. */ + private static final int MYSQL_LOCK_WAIT_TIMEOUT = 1205; + + /** Oracle error number of a detected deadlock: ORA-00060, reported with SQLState 61000 rather than class 40. */ + private static final int ORACLE_DEADLOCK_DETECTED = 60; private JDBCBackendCfg config; @@ -182,23 +191,46 @@ public void removeStorageFiles() throws StorageRuntimeException { } //operation + /** + * {@inheritDoc} + *

+ * A rolled back read is not replayed, although + * {@link org.opends.server.backends.pluggable.spi.Storage#read(ReadOperation)} asks for it: two of the read + * operations of this server are not idempotent, and replaying them corrupts their result rather than repairing + * it. {@code ExportJob} runs the whole export inside a single read and its LDIF writer is opened once, so a + * replay appends the entries already written instead of truncating the file; {@code 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 while the server is online, since an export holds no more than a shared backend lock. + * A conflict therefore fails the read here, exactly as it did before the retry of {@link #write} was added. + */ @Override public T read(ReadOperation readOperation) throws Exception { - for (int attempt=1;;attempt++) { - try(final Connection con=getConnection()) { - return readOperation.run(new ReadableTransactionImpl(con)); - } catch (Exception e) { - if (!retryOnConflict(e,attempt)) { - throw e; - } - } + try(final Connection con=getConnection()) { + return readOperation.run(new ReadableTransactionImpl(con)); } } + /** + * {@inheritDoc} + *

+ * {@link org.opends.server.backends.pluggable.spi.Storage#write(WriteOperation)} requires an implementation to + * retry a rolled back operation until it succeeds, and {@link WriteOperation} is documented as idempotent for + * exactly that reason; {@link org.opends.server.backends.pdb.PDBStorage#write(WriteOperation)} already does so + * on the conflict exception of its own engine. The loop is bounded here, unlike PDBStorage: the database may be + * shared with writers outside this server, so a conflict is not guaranteed to clear and failing the operation is + * better than never returning. + *

+ * Only the operation itself is replayed: a failure of {@link #getConnection()} or of the implicit + * {@link Connection#close()} - which returns the connection to the pool after a rollback - leaves the loop, so + * that a completed write is never replayed because releasing its connection failed. + */ @Override public void write(WriteOperation writeOperation) throws Exception { for (int attempt=1;;attempt++) { + Exception failure=null; + String driver=null; try (final Connection con=getConnection()) { + driver=getDriverName(con); try { writeOperation.run(new WriteableTransactionTransactionImpl(con)); con.commit(); @@ -207,35 +239,46 @@ public void write(WriteOperation writeOperation) throws Exception { try { con.rollback(); } catch (SQLException ex) {} + //rethrown, so that a failure of the implicit close() is suppressed into the failure being + //replayed rather than replacing it + failure=e; throw e; } } catch (Exception e) { - if (!retryOnConflict(e,attempt)) { + //anything the operation did not throw comes from getConnection() or from the implicit close(), + //which returns the connection to the pool: neither belongs to the replayed region + if (e!=failure) { throw e; } } + if (attempt>=MAX_RETRIES || !isRetryableConflict(failure,driver)) { + throw failure; + } + //logged rather than silently absorbed, so that a deployment retrying most of its writes stays observable + logger.warn(LocalizableMessage.raw("jdbc: replaying the transaction after a conflict, attempt %d of %d: %s", + attempt, MAX_RETRIES, stackTraceToSingleLineString(failure))); + try { + //randomized to spread the retries of the transactions that collided, growing to outlast contention + Thread.sleep(retryDelayMillis(attempt)); + } catch (InterruptedException e) { + //sleep cleared the interrupt flag: restore it, and report the failure being retried rather than the + //interrupt, which would hide from the caller what actually went wrong + Thread.currentThread().interrupt(); + failure.addSuppressed(e); + throw failure; + } } } - /** - * Waits for a short random delay and returns whether the failed attempt should be replayed. - *

- * {@link org.opends.server.backends.pluggable.spi.Storage#write(WriteOperation)} requires an implementation to - * retry a rolled back operation until it succeeds, and {@link WriteOperation} is documented as idempotent for - * exactly that reason; {@link org.opends.server.backends.pdb.PDBStorage#write(WriteOperation)} already does so - * on the conflict exception of its own engine. The loop is bounded here, unlike PDBStorage: the database may be - * shared with writers outside this server, so a conflict is not guaranteed to clear and failing the operation is - * better than never returning. The delay is randomized to spread the retries of the transactions that collided. - */ - private boolean retryOnConflict(Exception e, int attempt) throws InterruptedException { - if (attempt>=MAX_RETRIES || !isRetryableConflict(e)) { - return false; - } - //logged rather than silently absorbed, so that a deployment retrying most of its writes stays observable - logger.warn(LocalizableMessage.raw("jdbc: replaying the transaction after a conflict, attempt %d of %d: %s", - attempt, MAX_RETRIES, stackTraceToSingleLineString(e))); - Thread.sleep((long) (Math.random() * MAX_SLEEP_ON_RETRY_MS)); - return true; + /** Returns the randomized delay before the given attempt is replayed, doubling with each attempt up to a cap. */ + static long retryDelayMillis(int attempt) { + final double bound=Math.min(MAX_SLEEP_ON_RETRY_MS, BASE_SLEEP_ON_RETRY_MS * (1 << Math.min(attempt-1, 5))); + return (long) (Math.random() * bound); + } + + /** Returns the class name of the driver behind the given connection, which names the engine it talks to. */ + static String getDriverName(Connection con) { + return ((con instanceof CachedConnection) ? ((CachedConnection) con).parent : con).getClass().getName(); } /** @@ -245,25 +288,39 @@ private boolean retryOnConflict(Exception e, int attempt) throws InterruptedExce * {@code put} arrives as {@code StorageRuntimeException(SQLException)}, and a caller such as * {@code EntryContainer.addEntry} may wrap it once more. *

- * Both the vendor error number and the SQLState are examined, since neither alone covers the drivers in use. - * SQL Server reports the deadlock victim as error 1205 with SQLState 40001, but as 42000 when the connection was - * opened with {@code xopenStates=true}, so the state cannot be relied upon; conversely the standard class 40 - * states carry the conflict of the other engines - 40P01 for PostgreSQL, 40001 for MySQL and H2 - under vendor - * error numbers of their own. Error 1205 of MySQL is a lock wait timeout rather than a deadlock, which the code - * alone cannot tell apart, but that condition is transient as well and is equally resolved by a replay. + * The standard class 40 states carry the conflict of most engines - 40P01 for PostgreSQL, 40001 for MySQL and + * for SQL Server - but not of all of them, so the vendor error numbers are consulted as well, keyed by the + * driver in the same way {@code getTableDialect} keys the column types. They cannot be matched + * driver-independently: Oracle reports a deadlock as ORA-00060 with SQLState 61000, and gives 1205 to a fatal + * "not a data file" error that no replay can resolve, while 1205 is exactly the deadlock victim of SQL Server + * and the lock wait timeout of MySQL. The MySQL timeout is not a deadlock, but it is transient in the same way + * and is equally resolved by a replay. The SQL Server number is matched beyond its class 40 state because a + * deployment may add {@code xopenStates=true} to its connection URL, which reports the same deadlock as 42000. */ - static boolean isRetryableConflict(Throwable t) { + static boolean isRetryableConflict(Throwable t, String driver) { for (int hop=0; t!=null && hop? order by k" batches: primary key (h,k) cannot serve them - final String driverName=((CachedConnection) con).parent.getClass().getName(); + final String driverName=getDriverName(con); final String tableName=getTableName(treeName); if (driverName.contains("postgres")) { try (final PreparedStatement statement=con.prepareStatement("create index if not exists k_"+tableName.substring("opendj_".length())+" on "+tableName+" (k)")){ @@ -471,7 +528,7 @@ public void put(TreeName treeName, ByteSequence key, ByteSequence value) { } boolean upsert(TreeName treeName, ByteSequence key, ByteSequence value) throws SQLException { - final String driverName=((CachedConnection) con).parent.getClass().getName(); + final String driverName=getDriverName(con); if (driverName.contains("postgres")) { //postgres upsert try (final PreparedStatement statement = con.prepareStatement("insert into " + getTableName(treeName) + " (h,k,v) values (?,?,?) ON CONFLICT (h, k) DO UPDATE set v=excluded.v")) { statement.setString(1, key2hash.get(ByteBuffer.wrap(key.toByteArray()))); diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/EntryContainer.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/EntryContainer.java index b0b1d2a831..8909e56b69 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/EntryContainer.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/EntryContainer.java @@ -239,20 +239,23 @@ public ConfigChangeResult applyConfigurationDelete(final BackendIndexCfg cfg) EntryContainer.this.lock(); try { - storage.write(new WriteOperation() + // The write may be replayed by the storage, so the maps are updated outside of it: left inside, the second + // attempt would find nothing to delete and commit an empty transaction, reporting success for work that + // did not happen. The index may already be gone, since applyConfigurationAdd can fail after the config + // entry was persisted but before the index reached the map. + final AttributeIndex index = attrIndexMap.remove(cfg.getAttribute()); + attrCryptoMap.remove(cfg.getAttribute()); + if (index != null) { - @Override - public void run(WriteableTransaction txn) throws Exception + storage.write(new WriteOperation() { - // The write may be replayed by the storage, so the removal must tolerate having already happened. - final AttributeIndex index = attrIndexMap.remove(cfg.getAttribute()); - if (index != null) + @Override + public void run(WriteableTransaction txn) throws Exception { index.closeAndDelete(txn); } - attrCryptoMap.remove(cfg.getAttribute()); - } - }); + }); + } } catch (Exception de) { @@ -326,19 +329,20 @@ public ConfigChangeResult applyConfigurationDelete(final BackendVLVIndexCfg cfg) EntryContainer.this.lock(); try { - storage.write(new WriteOperation() + // Removed outside the write for the reason given in the index delete listener above: the write may be + // replayed, and a replay must still have the deletion to perform. + final VLVIndex vlvIndex = vlvIndexMap.remove(cfg.getName().toLowerCase()); + if (vlvIndex != null) { - @Override - public void run(WriteableTransaction txn) throws Exception + storage.write(new WriteOperation() { - // The write may be replayed by the storage, so the removal must tolerate having already happened. - final VLVIndex vlvIndex = vlvIndexMap.remove(cfg.getName().toLowerCase()); - if (vlvIndex != null) + @Override + public void run(WriteableTransaction txn) throws Exception { vlvIndex.closeAndDelete(txn); } - } - }); + }); + } } catch (Exception e) { diff --git a/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/JDBCStorageRetryTest.java b/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/JDBCStorageRetryTest.java index a7025254c7..b2d9cb4d5d 100644 --- a/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/JDBCStorageRetryTest.java +++ b/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/JDBCStorageRetryTest.java @@ -26,10 +26,11 @@ import static org.forgerock.i18n.LocalizableMessage.raw; import static org.forgerock.opendj.ldap.ResultCode.OTHER; import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertTrue; /** * Tests how a failure is classified as a transaction conflict, which is what decides whether - * {@link JDBCStorage#write} and {@link JDBCStorage#read} replay the operation. + * {@link JDBCStorage#write} replays the operation, and how long it waits before it does. *

* Runs without a database: the failures the drivers report are reproduced as synthetic * {@link SQLException}s carrying the same vendor error number and SQLState. @@ -38,6 +39,12 @@ @SuppressWarnings("javadoc") public class JDBCStorageRetryTest extends DirectoryServerTestCase { + /** Driver class names, which is what the classification keys the vendor error numbers off. */ + private static final String MSSQL = "com.microsoft.sqlserver.jdbc.SQLServerConnection"; + private static final String MYSQL = "com.mysql.cj.jdbc.ConnectionImpl"; + private static final String ORACLE = "oracle.jdbc.driver.T4CConnection"; + private static final String POSTGRES = "org.postgresql.jdbc.PgConnection"; + /** A failure whose cause chain is a cycle, to check that walking it terminates. */ private static final class SelfCausedException extends RuntimeException { @@ -55,36 +62,66 @@ public Object[][] failures() { return new Object[][] { // SQL Server picking a transaction as the deadlock victim: the failure this retry exists for - { "mssql deadlock victim", sql(1205, "40001"), true }, - // the xopenStates connection property makes the same driver report a state of no help here - { "mssql deadlock victim, xopenStates", sql(1205, "42000"), true }, - // the conflict of the other engines is carried by the SQLState, under a vendor number of their own - { "postgres serialization failure", sql(0, "40001"), true }, - { "postgres deadlock detected", sql(0, "40P01"), true }, - { "mysql deadlock", sql(1213, "40001"), true }, + { "mssql deadlock victim", sql(1205, "40001"), MSSQL, true }, + // a deployment may add xopenStates=true to its connection URL, which reports the same deadlock as 42000 + { "mssql deadlock victim, xopenStates", sql(1205, "42000"), MSSQL, true }, + // the conflict of most other engines is carried by the SQLState, under a vendor number of their own + { "postgres serialization failure", sql(0, "40001"), POSTGRES, true }, + { "postgres deadlock detected", sql(0, "40P01"), POSTGRES, true }, + { "mysql deadlock", sql(1213, "40001"), MYSQL, true }, // not a deadlock, but transient in the same way and equally resolved by a replay - { "mysql lock wait timeout", sql(1205, "HY000"), true }, + { "mysql lock wait timeout", sql(1205, "HY000"), MYSQL, true }, + // Oracle maps ORA-00060 to SQLState 61000, so only its error number identifies the deadlock + { "oracle deadlock detected", sql(60, "61000"), ORACLE, true }, // the conflict reaches JDBCStorage.write() wrapped, so the whole cause chain has to be walked - { "wrapped once", new StorageRuntimeException(sql(1205, "40001")), true }, + { "wrapped once", new StorageRuntimeException(sql(1205, "40001")), MSSQL, true }, { "wrapped twice", - new DirectoryException(OTHER, raw("unchecked"), new StorageRuntimeException(sql(1205, "40001"))), true }, + new DirectoryException(OTHER, raw("unchecked"), new StorageRuntimeException(sql(1205, "40001"))), MSSQL, + true }, + + // the vendor numbers collide across engines, so they must not be matched driver-independently: + // ORA-01205 "not a data file" is fatal, and no replay resolves it + { "oracle not a data file", sql(1205, "64000"), ORACLE, false }, + // and a lock wait timeout is a MySQL number: 1205 means nothing of the kind to PostgreSQL + { "postgres unrelated 1205", sql(1205, "22001"), POSTGRES, false }, // nothing a replay can resolve - { "primary key violation", sql(2627, "23000"), false }, - { "syntax error", sql(102, "S0001"), false }, - { "no SQLState", sql(0, null), false }, - { "not a SQLException", new IllegalStateException("connection closed"), false }, - { "wrapped, not a conflict", new StorageRuntimeException(sql(2627, "23000")), false }, - { "no failure at all", null, false }, - { "cyclic cause chain", new SelfCausedException(), false }, + { "primary key violation", sql(2627, "23000"), MSSQL, false }, + { "syntax error", sql(102, "S0001"), MSSQL, false }, + { "no SQLState", sql(0, null), MSSQL, false }, + { "not a SQLException", new IllegalStateException("connection closed"), MSSQL, false }, + { "wrapped, not a conflict", new StorageRuntimeException(sql(2627, "23000")), MSSQL, false }, + { "no failure at all", null, MSSQL, false }, + { "unknown driver", sql(1205, "HY000"), null, false }, + { "cyclic cause chain", new SelfCausedException(), MSSQL, false }, }; } @Test(dataProvider = "failures") - public void testIsRetryableConflict(String name, Throwable failure, boolean expected) + public void testIsRetryableConflict(String name, Throwable failure, String driver, boolean expected) { - assertEquals(JDBCStorage.isRetryableConflict(failure), expected, name); + assertEquals(JDBCStorage.isRetryableConflict(failure, driver), expected, name); + } + + /** The delay grows with the attempt, so that the replays outlast a contention lasting more than a few ms. */ + @Test + public void testRetryDelayGrowsAndStaysBounded() + { + long previousBound = 0; + for (int attempt = 1; attempt <= 10; attempt++) + { + long bound = 0; + for (int i = 0; i < 100; i++) + { + final long delay = JDBCStorage.retryDelayMillis(attempt); + assertTrue(delay >= 0, "attempt " + attempt + " waited " + delay + " ms"); + assertTrue(delay < 1000, "attempt " + attempt + " waited " + delay + " ms"); + bound = Math.max(bound, delay); + } + assertTrue(bound >= previousBound / 2, "attempt " + attempt + " did not grow past attempt " + (attempt - 1)); + previousBound = bound; + } } private static SQLException sql(int errorCode, String sqlState)