Use serializable isolation and handle retries effectively

Allows punishing a victim concurrently with a much greater degree
  of reliability.

bans-core/src/main/java/space/arim/libertybans/core/database/execute/JooqQueryExecutor.java

@@ -21,8 +21,11 @@
 
 import org.jooq.DSLContext;
 import org.jooq.exception.DataAccessException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 import space.arim.libertybans.core.database.jooq.JooqContext;
 import space.arim.omnibus.util.ArraysUtil;
+import space.arim.omnibus.util.ThisClass;
 import space.arim.omnibus.util.concurrent.CentralisedFuture;
 import space.arim.omnibus.util.concurrent.FactoryOfTheFuture;
 
@@ -40,6 +43,8 @@ public final class JooqQueryExecutor implements QueryExecutor {
 	private final FactoryOfTheFuture futuresFactory;
 	private final Executor threadPool;
 
+	private static final Logger logger = LoggerFactory.getLogger(ThisClass.get());
+
 	public JooqQueryExecutor(JooqContext jooqContext, DataSource dataSource,
 							 FactoryOfTheFuture futuresFactory, Executor threadPool) {
 		this.jooqContext = Objects.requireNonNull(jooqContext, "jooqContext");
@@ -57,6 +62,11 @@ private static <E extends Throwable> E rollbackBeforeThrow(Connection connection
 		throw reason;
 	}
 
+	private static DataAccessException unableToCommit(Connection connection, SQLException cause) {
+		throw rollbackBeforeThrow(connection,
+				new DataAccessException("Unable to commit (" + cause.getSQLState() + ')', cause));
+	}
+
 	private <R> R obtainUnfailing(SQLFunction<R> command) {
 		try (Connection connection = dataSource.getConnection()) {
 			if (command.isReadOnly()) {
@@ -70,11 +80,15 @@ private <R> R obtainUnfailing(SQLFunction<R> command) {
 			} catch (RuntimeException ex) {
 				throw rollbackBeforeThrow(connection, ex);
 			}
-			connection.commit();
+			try {
+				connection.commit();
+			} catch (SQLException ex) {
+				throw unableToCommit(connection, ex);
+			}
 			return value;
 
 		} catch (SQLException ex) {
-			throw new DataAccessException("Miscellaneous failure", ex);
+			throw new DataAccessException("Miscellaneous failure (" + ex.getSQLState() + ')', ex);
 		}
 	}
 
@@ -90,7 +104,7 @@ private static boolean isSerializationFailure(SQLException ex) {
 		MariaDB and MySQL - ER_LOCK_DEADLOCK
 		PostgreSQL and CockroachDB - serialization_failure
 		 */
-		return ex.getErrorCode() == 40001;
+		return "40001".equals(ex.getSQLState());
 	}
 
 	private static void exponentialBackoff(int retry) {
@@ -140,7 +154,13 @@ private <R> R obtainWithRetry(int retryCount, SQLTransactionalFunction<R> comman
 							serializationFailures = ArraysUtil.expandAndInsert(serializationFailures, ex, 0);
 							continue;
 						}
-						throw rollbackBeforeThrow(connection, new DataAccessException("Unable to commit", ex));
+						throw unableToCommit(connection, ex);
+					}
+				}
+				if (retry != 0) {
+					logger.trace("Database operation succeeded after {} tries", retry);
+					if (retry > retryCount / 2) {
+						logger.info("Heavy contention detected on the database. Consider upping the retry count.");
 					}
 				}
 				return value;
@@ -172,7 +192,7 @@ public void executeWithExistingConnection(Connection connection,
 			try {
 				connection.commit();
 			} catch (SQLException ex) {
-				throw rollbackBeforeThrow(connection, new DataAccessException("Unable to commit", ex));
+				throw unableToCommit(connection, ex);
 			}
 		}
 	}

bans-core/src/main/java/space/arim/libertybans/core/database/execute/RollbackTrackingTransaction.java

@@ -43,6 +43,15 @@ boolean wasNotRolledBack() {
 		return !rolledBack;
 	}
 
+	@Override
+	public void setIsolation(int level) {
+		try {
+			connection.setTransactionIsolation(level);
+		} catch (SQLException ex) {
+			throw new DataAccessException("Failed to set isolation", ex);
+		}
+	}
+
 	@Override
 	public void rollback() {
 		try {
@@ -88,6 +97,11 @@ private NestedTransaction(Savepoint savepoint) {
 			this.savepoint = Objects.requireNonNull(savepoint, "savepoint");
 		}
 
+		@Override
+		public void setIsolation(int level) {
+			throw new UnsupportedOperationException("Cannot set isolation levels in nested transactions");
+		}
+
 		@Override
 		public void rollback() {
 			try {

bans-core/src/main/java/space/arim/libertybans/core/database/execute/Transaction.java

@@ -26,6 +26,13 @@
  */
 public interface Transaction {
 
+	/**
+	 * Sets the isolation level on this transaction
+	 *
+	 * @param isolationLevel the isolation level
+	 */
+	void setIsolation(int isolationLevel);
+
 	/**
 	 * Rolls back the enclosing scope
 	 *

bans-core/src/main/java/space/arim/libertybans/core/punish/Enactor.java

@@ -106,6 +106,9 @@ CentralisedFuture<Punishment> enactPunishment(DraftPunishment draftPunishment) {
 				creator);
 
 		return database.queryWithRetry((context, transaction) -> {
+			// Make sure concurrent executions do not conflict
+			transaction.setIsolation(Connection.TRANSACTION_SERIALIZABLE);
+
 			if (type != PunishmentType.KICK) {
 				database.clearExpiredPunishments(context, type, start);
 			}
@@ -124,6 +127,8 @@ CentralisedFuture<Punishment> calculatePunishment(CalculablePunishment calculabl
 
 		InternalDatabase database = dbProvider.get();
 		return database.queryWithRetry((context, transaction) -> {
+			// Make sure concurrent executions do not conflict
+			transaction.setIsolation(Connection.TRANSACTION_SERIALIZABLE);
 
 			var calculationResult = calculablePunishment.getCalculator().compute(
 					escalationTrack, victim,

bans-core/src/main/resources/contributors

@@ -34,6 +34,7 @@ Folas1337
 Franciscoyt94
 hawkfalcon
 Healthy
+Kaludi
 LaurenceBarnes
 MattVid
 mcmdev

pom.xml

@@ -186,6 +186,9 @@
 							</goals>
 						</execution>
 					</executions>
+					<configuration>
+						<createDependencyReducedPom>false</createDependencyReducedPom>
+					</configuration>
 				</plugin>
 				<plugin>
 					<groupId>org.apache.maven.plugins</groupId>