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 3da7d5694a..0c577b1df6 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
@@ -635,6 +635,10 @@ long nanoTime() {
private final AtomicBoolean ddlLockBoundNotSetWarned = new AtomicBoolean();
private final AtomicLong ddlLockBoundLeftBehindWarned = new AtomicLong();
private static final long DDL_LOCK_BOUND_WARNING_INTERVAL_MS = 10000;
+ // And a third way, which is no failure of anything: an engine this backend knows no lock setting
+ // for is left unbounded deliberately, and says so once - see reportTheEngineIsNotKnown(). Not
+ // private, so that a case can read what a storage has already said without reading a log.
+ final AtomicBoolean ddlLockBoundEngineUnknownWarned = new AtomicBoolean();
/**
* The socket read timeout of one connection, and the statements running on it. This second
@@ -1301,6 +1305,12 @@ static String storedIdentifier(DatabaseMetaData metaData, String name) throws SQ
* {@code dsconfig create-backend-index} is answered by {@code alter system set ddl_lock_timeout},
* not by this.
*
+ * An engine behind a driver this backend does not know is left alone as well, and is told about
+ * rather than left silent - see {@code reportTheEngineIsNotKnown()}. {@link #dialectOf} reads the
+ * engine off the class name of the driver, so a mariadb, percona or aurora driver against a live
+ * mysql is one of these: the bound stays off there, and the line saying so is what an operator has
+ * to go on.
+ *
* What it costs is the round trips of the statements around each DDL - three on mysql and sql
* server (reading the value back, setting the bound, giving the value back), two on postgres (the
* savepoint a failed setting is taken back to, and the setting), none on oracle - and only on the
@@ -2127,12 +2137,15 @@ public void close() {
* this reachable from a test with no database behind it.
*
* Nothing of ours is set where it could not be taken off again, and a failure of the readback is
- * never the failure of the DDL: this runs on a pooled connection, so a setting left behind reaches
- * every statement of whoever borrows it next - on sql server that is every lock wait of theirs,
- * row locks included, and {@link #isConflict} classifies error 1222 as no replayable conflict. A
- * session this backend could not take its bound off again is kept out of the pool for that reason
- * ({@link CachedConnection#keepOutOfThePool}), since the validation of the next borrow is
- * {@code isValid()} - a liveness check a connection carrying a stale setting passes.
+ * never the failure of the DDL: this runs mostly on a pooled connection, so a setting left behind
+ * reaches every statement of whoever borrows it next - on sql server that is every lock wait of
+ * theirs, row locks included, and {@link #isConflict} classifies error 1222 as no replayable
+ * conflict. A session this backend could not take its bound off again is kept out of the pool for
+ * that reason ({@link CachedConnection#keepOutOfThePool}), since the validation of the next borrow
+ * is {@code isValid()} - a liveness check a connection carrying a stale setting passes. The one
+ * connection here that is not the pool's is the catalog's own, which {@code createCatalogTable()}
+ * creates its table on: there a setting left behind reaches the rest of that write and goes with
+ * the connection, which is closed with it.
*
* The DDL runs whatever any of that did, and it runs under the same rewrite either way: a setting
* can reach the server and fail only as the statement carrying it is closed, which no driver tells
@@ -2141,10 +2154,21 @@ public void close() {
*/
T withDdlLockBound(Connection con, Dialect dialect, Execution action) throws SQLException {
final int seconds=ddlLockBoundSeconds();
- // Asked first with nothing displaced yet, which is what tells an engine this bound is never put
- // on - oracle, and one none of these settings fit - from an engine it is put on. What the session
- // actually carries is read below, and can take the bound off again all by itself.
- if (dialect==null || seconds<=0 || dialect.ddlLockBoundSql(seconds, null)==null) {
+ if (seconds<=0) { // the wait is left exactly as unbounded as it was, and nobody asked otherwise
+ return action.run();
+ }
+ if (dialect==null) {
+ // The one branch where a bound was asked for and none is put on, which is why it is the one
+ // that says so: an engine none of these settings fit is fed none of them - untested SQL is no
+ // thing to send a database on the path a backend opens by - and the silence around that is
+ // what an operator has no way of finding out.
+ reportTheEngineIsNotKnown(con);
+ return action.run();
+ }
+ // Asked with nothing displaced yet, which is what tells an engine this bound is never put on -
+ // oracle - from an engine it is put on. What the session actually carries is read below, and can
+ // take the bound off again all by itself.
+ if (dialect.ddlLockBoundSql(seconds, null)==null) {
return action.run();
}
final String query=dialect.ddlLockBoundQuery();
@@ -2337,12 +2361,41 @@ private void reportTheWaitIsLeftUnbounded(Dialect dialect, String sql, Exception
}
}
+ /**
+ * Said once per storage, where the engine behind a connection is not one this backend knows a lock
+ * setting for. Leaving the bound off such an engine is the conservative reading and stays - untested
+ * SQL is no thing to send a database on the path a backend opens by - but a deployment that asked
+ * for the bound has no way of finding out that it got none, which is the silence this ends. It is
+ * the argument the strict parsing of {@value #DDL_LOCK_TIMEOUT_PROPERTY} is made with, one property
+ * later.
+ *
+ * {@link #dialectOf} reads the engine off the class name of the driver, so this is not the engine
+ * of an exotic database alone: a mariadb, percona or aurora driver against a live mysql answers
+ * null here, and that is a session whose {@code lock_wait_timeout} is a year - the very wait this
+ * bound exists to end. {@link CachedConnection} says the same of a url it knows no connect bound
+ * for and cannot say it for this one: it keys on the url, which such a driver takes as a mysql one.
+ *
+ * The driver is named rather than the url, since the driver is what this reads and what a
+ * deployment would change - and a url carries the password of the account this backend works as.
+ */
+ private void reportTheEngineIsNotKnown(Connection con) {
+ if (ddlLockBoundEngineUnknownWarned.compareAndSet(false, true)) {
+ logger.warn(LocalizableMessage.raw("jdbc: the wait of a DDL for a lock is left unbounded on this"
+ + " database: %s is not a driver this backend knows a lock setting of an engine for, so %s"
+ + " bounds nothing here and a DDL - the create table and create index of an open, the drop"
+ + " table of a clear - waits for a lock another session holds for as long as this engine"
+ + " lets it", driverNameOf(con), DDL_LOCK_TIMEOUT_PROPERTY));
+ }
+ }
+
/**
* Gives the session back the value it carried. Best effort, and never the outcome of the DDL: this
* runs from a {@code finally} while the caller may be being unwound, where a throw would replace
* the failure that brought it there (JLS 14.20.2) - the very one saying what went wrong.
*
- * A connection this failed on does not go back into the pool. Leaving it to the next borrow to
+ * A connection this failed on is handed on to nobody. A pooled one is closed rather than given
+ * back, and the catalog's own - the one connection reaching this that was never in the pool - is
+ * closed with the write that opened it. Leaving it to the next borrow to
* notice does not work: that validation is {@code con.isValid()}, a liveness check which a
* connection whose reset failed for a transient reason passes while still carrying our bound, and
* on sql server it would then cut every lock wait of that borrower at it - row locks included,
@@ -2371,8 +2424,9 @@ private void restoreDdlLockBound(Connection con, Dialect dialect, String bound,
if (now-last >= DDL_LOCK_BOUND_WARNING_INTERVAL_MS && ddlLockBoundLeftBehindWarned.compareAndSet(last, now)) {
logger.warn(LocalizableMessage.raw("jdbc: the lock bound of a DDL could not be taken off a connection"
+ " of this %s database, which may have been left carrying \"%s\" instead of the value it had:"
- + " that connection is closed rather than pooled, so no borrow after this one gives up on a lock"
- + " at a bound of %s it never asked for (%s)", dialect, bound, DDL_LOCK_TIMEOUT_PROPERTY,
+ + " that connection is not handed on - a pooled one is closed rather than given back, and the"
+ + " catalog's own is closed with the write that opened it - so nothing after this gives up on a"
+ + " lock at a bound of %s it never asked for (%s)", dialect, bound, DDL_LOCK_TIMEOUT_PROPERTY,
stackTraceToSingleLineString(e)));
}
}
@@ -4756,17 +4810,31 @@ void readEnrolledTrees(TreeName catalog) {
* An account that may write its rows but not create a table is a configuration this can meet,
* so the failure says which table it was and why the backend wanted it, rather than reaching
* the operator as a bare SQL error inside ERR_OPEN_ENV_FAIL.
+ *
+ * It waits for its lock under {@link JDBCStorage#DDL_LOCK_TIMEOUT_PROPERTY} like every other DDL
+ * of this backend, and a lock it gives up on names that property: this statement is issued on the
+ * catalog's own connection rather than through {@code commitStatement()}, which is the funnel
+ * that bounds the rest, so the bound is put on here.
*/
void createCatalogTable(TreeName catalog) {
final String tableName=getTableName(catalog);
try {
final Connection catalogCon=catalogSession.connection();
- try (final PreparedStatement statement=catalogCon.prepareStatement("create table "+tableName+" ("+getTableDialect()+")")) {
- // bulk like every other create table of this backend (#882): it is DDL nobody waits on,
- // and the class of a client operation is not what a statement of this kind can be given
- execute(statement, StatementBound.BULK);
- }
- catalogCon.commit();
+ // Under the same bound as every other DDL of this backend, although this one reaches no
+ // commitStatement(): it is a create table of an open like the ones openTree() issues, and
+ // it queues for the same kind of lock - another process creating this very table inside a
+ // transaction it has not committed is a wait three engines out of four never end. The
+ // commit is inside the bound because on postgres it is that commit which ends the
+ // transaction a "set local" belongs to.
+ withDdlLockBound(catalogCon, dialectOf(catalogCon), () -> {
+ try (final PreparedStatement statement=catalogCon.prepareStatement("create table "+tableName+" ("+getTableDialect()+")")) {
+ // bulk like every other create table of this backend (#882): it is DDL nobody waits on,
+ // and the class of a client operation is not what a statement of this kind can be given
+ execute(statement, StatementBound.BULK);
+ }
+ catalogCon.commit();
+ return null;
+ });
} catch (SQLException | RuntimeException e) {
// the unchecked one as well, for the reason enrolInCatalog() takes it: what the statement
// left behind has to be rolled back whatever class the failure arrived in, this connection
diff --git a/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/JDBCDdlLockBoundTestCase.java b/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/JDBCDdlLockBoundTestCase.java
index c7e623995f..ad482e0290 100644
--- a/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/JDBCDdlLockBoundTestCase.java
+++ b/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/JDBCDdlLockBoundTestCase.java
@@ -49,6 +49,7 @@
import static java.util.Collections.singletonList;
import static org.forgerock.opendj.config.ConfigurationMock.mockCfg;
import static org.mockito.Mockito.any;
+import static org.mockito.Mockito.anyBoolean;
import static org.mockito.Mockito.anyInt;
import static org.mockito.Mockito.anyString;
import static org.mockito.Mockito.doAnswer;
@@ -83,19 +84,30 @@ public class JDBCDdlLockBoundTestCase extends DirectoryServerTestCase {
/** The backend the storage of a case is configured as, which is what names its tree catalog. */
private static final String BACKEND_ID = "ddlLockBound";
- /** That catalog's table, which the connection of a case answers as not being there: see engine(). */
- private static final String NO_CATALOG_TABLE =
+ /**
+ * That catalog's table, which the connection of a case answers as not being there (see engine()):
+ * a case reaching {@code openTree()} therefore takes the branch that creates it.
+ */
+ private static final String CATALOG_TABLE =
JDBCStorage.toTableName(new TreeName(JDBCStorage.CATALOG_BASE_DN, BACKEND_ID));
/** What the connection of a case was asked to run, in the order it was asked to run it. */
private final List issued = new ArrayList<>();
+ /**
+ * What the catalog's own connection was asked to run. The create table of the catalog is issued
+ * on that connection rather than on the caller's ({@code CatalogSession}), so what is issued
+ * around it is read off a list of its own instead of out of the middle of the caller's.
+ */
+ private final List catalogIssued = new ArrayList<>();
+
private JDBCStorage storage;
@BeforeMethod
public void createStorage() {
storage = new JDBCStorage(backendCfg(), null);
issued.clear();
+ catalogIssued.clear();
}
/** A configuration naming this backend, which is all any case here reads off one. */
@@ -191,6 +203,44 @@ public void testAnEngineThisBackendDoesNotKnowIsLeftAlone() throws Exception {
assertEquals(issued, singletonList(THE_DDL));
}
+ /**
+ * ... and is told so once, rather than left to be found out. {@code dialectOf()} keys on the class
+ * name of the driver, so a mariadb, percona or aurora driver against a live mysql answers null
+ * here - and that is a session whose {@code lock_wait_timeout} is a year, which is the wait this
+ * bound exists to end. The strict parsing of the property exists so that a deployment which asked
+ * for a bound is never quietly left with none, and this is the same silence one property later.
+ */
+ @Test
+ public void testAnEngineThisBackendDoesNotKnowIsReported() throws Exception {
+ storage.withDdlLockBound(recording(mock(Connection.class), "0"), null, theDdl());
+
+ assertTrue(storage.ddlLockBoundEngineUnknownWarned.get(),
+ "a driver this backend knows no lock bound for left the wait of every DDL unbounded and unsaid");
+ }
+
+ /** A deployment that turned the bound off asked for none anywhere, and has nothing to act on. */
+ @Test
+ public void testAWaitNobodyAskedToBoundIsNotReportedAsAnUnknownEngine() throws Exception {
+ System.setProperty(JDBCStorage.DDL_LOCK_TIMEOUT_PROPERTY, "0");
+
+ storage.withDdlLockBound(recording(mock(Connection.class), "0"), null, theDdl());
+
+ assertFalse(storage.ddlLockBoundEngineUnknownWarned.get(),
+ "a bound nobody asked for was reported as an engine this backend does not know");
+ }
+
+ /**
+ * And oracle is an engine this backend knows perfectly well: it is left to its own
+ * {@code ddl_lock_timeout} on purpose, which is a decision rather than a gap to report.
+ */
+ @Test
+ public void testOracleIsNotReportedAsAnEngineThisBackendDoesNotKnow() throws Exception {
+ storage.withDdlLockBound(recording(mock(Connection.class), "0"), Dialect.ORACLE, theDdl());
+
+ assertFalse(storage.ddlLockBoundEngineUnknownWarned.get(),
+ "the engine left alone deliberately was reported as one this backend cannot bound");
+ }
+
/** Turning the bound off costs no round trip either: the DDL waits exactly as it did before. */
@Test
public void testAnUnboundedWaitIssuesNoSessionStatement() throws Exception {
@@ -556,6 +606,95 @@ private void givingUpOnTheLookup(final Connection con, final SQLException failur
when(con.getMetaData()).thenReturn(metaData);
}
+ /**
+ * The create table of the tree catalog is the DDL of this backend that goes through neither
+ * {@code commitStatement()} nor the drop loop: {@code openTree()} creates that table on the
+ * catalog's own connection before it enrols the tree it is opening, and a backend upgraded from a
+ * version that kept no catalog meets it on the open of every one of its trees.
+ */
+ @Test
+ public void testTheCreateOfTheCatalogTableIsBounded() throws Exception {
+ final JDBCStorage bounded = storageHandingOut(engine(postgresConnection.class, "0"),
+ engine(postgresConnection.class, "0", catalogIssued));
+
+ bounded.write(txn -> txn.openTree(TREE, true));
+
+ assertEquals(firstOfTheCatalog(2), asList("set local lock_timeout = 5000",
+ "create table " + CATALOG_TABLE + " (h char(128),k bytea,v bytea,primary key(h,k))"));
+ }
+
+ /**
+ * And it goes through the same helper every other DDL of this backend does, so the session of an
+ * engine whose setting outlives the transaction is read back first and given its value back after
+ * - on a connection which is not the pool's, and which the write that opened it closes.
+ */
+ @Test
+ public void testTheCreateOfTheCatalogTableGivesMysqlItsValueBack() throws Exception {
+ final JDBCStorage bounded = storageHandingOut(engine(mysqlConnection.class, "31536000"),
+ engine(mysqlConnection.class, "31536000", catalogIssued));
+
+ bounded.write(txn -> txn.openTree(TREE, true));
+
+ assertEquals(firstOfTheCatalog(4), asList("select @@session.lock_wait_timeout",
+ "set session lock_wait_timeout=5",
+ "create table " + CATALOG_TABLE + " (h char(128),k varbinary(255),v longblob,primary key(h,k))",
+ "set session lock_wait_timeout=31536000"));
+ }
+
+ /**
+ * A lock that create gave up on names the property that ended the wait, all the way out to the
+ * operator: the failure is wrapped as the backend not being able to create the table which holds
+ * its catalog, and a bare 55P03 inside that says nothing about which wait ended or what to raise.
+ */
+ @Test
+ public void testALockTheCreateOfTheCatalogTableGaveUpOnNamesTheProperty() throws Exception {
+ final JDBCStorage bounded = storageHandingOut(engine(postgresConnection.class, "0"),
+ failingTheCreate(engine(postgresConnection.class, "0", catalogIssued),
+ new SQLException("canceling statement due to lock timeout", "55P03")));
+
+ try {
+ bounded.write(txn -> txn.openTree(TREE, true));
+ fail("a create of the catalog table that gave up on a lock has to reach the caller");
+ }catch (Exception expected) {
+ assertTrue(namesTheProperty(expected),
+ "the lock this bound ended reached the operator unnamed: " + expected);
+ }
+ }
+
+ /** The statements of the catalog's connection a case reads, without running off its end. */
+ private List firstOfTheCatalog(int statements) {
+ return catalogIssued.subList(0, Math.min(statements, catalogIssued.size()));
+ }
+
+ /** Whether the failure, or any link of its chain, names the property that ended the wait. */
+ private static boolean namesTheProperty(Throwable failure) {
+ for (Throwable link = failure; link != null; link = link.getCause()) {
+ if (link.getMessage() != null
+ && link.getMessage().contains(JDBCStorage.DDL_LOCK_TIMEOUT_PROPERTY)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ /** A connection whose create table is the statement the engine refuses, with the given failure. */
+ private Connection failingTheCreate(final Connection con, final SQLException failure) throws SQLException {
+ // doAnswer() rather than when(): the connection has been given a prepareStatement() already, and
+ // calling it inside a when() would run that answer - which stubs a mock of its own - in the
+ // middle of this stubbing, which mockito reads as a stubbing that never named a method
+ doAnswer(invocation -> {
+ final String sql = (String) invocation.getArguments()[0];
+ catalogIssued.add(sql);
+ final PreparedStatement statement = mock(PreparedStatement.class);
+ when(statement.getConnection()).thenReturn(con);
+ if (sql.startsWith("create table ")) {
+ when(statement.executeUpdate()).thenThrow(failure);
+ }
+ return statement;
+ }).when(con).prepareStatement(anyString());
+ return con;
+ }
+
/** A catalog naming each of the given trees at the table its name hashes to. */
private static Map catalogOf(TreeName... trees) {
final Map catalog = new LinkedHashMap<>();
@@ -725,13 +864,19 @@ private JDBCStorage.Execution theDdl() {
* setting with the value a session of that engine carries.
*/
private Connection recording(final Connection con, final String carries) throws SQLException {
+ return recording(con, carries, issued);
+ }
+
+ /** The same, recording into the list the statements of this connection belong in. */
+ private Connection recording(final Connection con, final String carries, final List into)
+ throws SQLException {
final Statement statement = mock(Statement.class);
when(statement.execute(anyString())).thenAnswer(invocation -> {
- issued.add((String) invocation.getArguments()[0]);
+ into.add((String) invocation.getArguments()[0]);
return false;
});
when(statement.executeQuery(anyString())).thenAnswer(invocation -> {
- issued.add((String) invocation.getArguments()[0]);
+ into.add((String) invocation.getArguments()[0]);
final ResultSet carried = mock(ResultSet.class);
when(carried.next()).thenReturn(true, false);
when(carried.getString(1)).thenReturn(carries);
@@ -787,15 +932,25 @@ private Connection refusingToGiveTheValueBack(final Connection con, final String
interface postgresConnection extends Connection {
}
+ /** The same for mysql, whose setting outlives the transaction and is read back and put back. */
+ interface mysqlConnection extends Connection {
+ }
+
/**
* A connection of the given engine, recording the statements it is asked to run - the DDL among the
* session settings around it - over a catalog holding the table of every tree named here.
*/
private Connection engine(Class engine, String carries) throws SQLException {
- final Connection con = recording(mock(engine), carries);
+ return engine(engine, carries, issued);
+ }
+
+ /** The same, recording into the list the statements of this connection belong in. */
+ private Connection engine(Class engine, String carries, List into)
+ throws SQLException {
+ final Connection con = recording(mock(engine), carries, into);
when(con.isValid(anyInt())).thenReturn(true);
when(con.prepareStatement(anyString())).thenAnswer(invocation -> {
- issued.add((String) invocation.getArguments()[0]);
+ into.add((String) invocation.getArguments()[0]);
final PreparedStatement statement = mock(PreparedStatement.class);
when(statement.getConnection()).thenReturn(con);
return statement;
@@ -809,11 +964,21 @@ private Connection engine(Class engine, String carries) th
// backend upgraded from a version that kept no catalog takes the shortest way to the funnel
// carrying them - the catalog would otherwise want a connection of its own, which is not a
// connection this mock hands out. CatalogConnectionTestCase covers that one.
- when(tables.next()).thenReturn(!NO_CATALOG_TABLE.equals(asked), false);
+ when(tables.next()).thenReturn(!CATALOG_TABLE.equals(asked), false);
// the name the catalog was asked about, so that every tree of a case is found to exist
when(tables.getString("TABLE_NAME")).thenReturn(asked);
return tables;
});
+ // The index of a tree is found missing, which is the shortest way through openTree() to the
+ // catalog table it creates on the way: a getIndexInfo() no case answers comes back null, which
+ // is a NullPointerException inside the lookup rather than a case. The create index that then
+ // follows is issued on the caller's connection, where these cases read nothing.
+ when(metaData.getIndexInfo(any(), any(), anyString(), anyBoolean(), anyBoolean()))
+ .thenAnswer(invocation -> {
+ final ResultSet indexes = mock(ResultSet.class);
+ when(indexes.next()).thenReturn(false);
+ return indexes;
+ });
when(con.getMetaData()).thenReturn(metaData);
return con;
}
@@ -824,12 +989,39 @@ private Connection engine(Class engine, String carries) th
* around a DDL, not the pool that produced the connection carrying them.
*/
private JDBCStorage storageHandingOut(final Connection con) {
+ return storageHandingOut(con, null);
+ }
+
+ /**
+ * The same, handing out the given connection for the tree catalog as well: that connection is not
+ * a pooled one - {@code CatalogSession} opens it through {@code newCatalogConnection()} - so it is
+ * given its own seam rather than borrowed through the one above.
+ */
+ private JDBCStorage storageHandingOut(final Connection con, final Connection catalogCon) {
final JDBCStorage handing = new JDBCStorage(backendCfg(), null) {
@Override
Connection getConnection(boolean trusted) {
return new CachedConnection("jdbc:mock", con);
}
+ @Override
+ Connection newCatalogConnection(long budgetDeadline) throws SQLException {
+ if (catalogCon == null) {
+ throw new SQLException("this case opens no catalog connection");
+ }
+ return catalogCon;
+ }
+
+ @Override
+ Connection newStampConnection(Dialect dialect) throws SQLException {
+ // The stamp of an open is no part of any case here, and commentTable() takes this for
+ // what it is - a stamp connection that could not be made, which leaves the table
+ // unstamped and the open unaffected. Answered here rather than left to fail on its own,
+ // because failing on its own means DriverManager: this suite needs no database, and a
+ // mock-only case has no business registering every jdbc driver on the classpath.
+ throw new SQLException("this case stamps nothing");
+ }
+
@Override
public StorageStatus getStorageStatus() {
return StorageStatus.working(); // open already, so an importer borrows and no more