[#933] Read a catalog table another session created while this one was creating it

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

@@ -4721,8 +4721,9 @@ Connection catalogConnection() {
 		 * <p>
 		 * Serialized on the storage, so that two transactions opening trees at the same time cannot both
 		 * find the table absent and both go on to create it. It serializes this storage and nothing
-		 * else, which is why the create tolerates a table that turned up while it was being made: an
-		 * offline tool beside a running server is a pair no lock of one process can order. The stamp -
+		 * else, which is why the create tolerates a table that turned up while it was being made - an
+		 * offline tool beside a running server is a pair no lock of one process can order - and why what
+		 * it tolerated is then read like any other catalog that was already there. The stamp -
 		 * the one thing under it that is nobody's dependency - is issued outside it.
 		 * <p>
 		 * Two things are kept out of the lock because they are the slow ones. The flag is read before
@@ -4757,11 +4758,24 @@ void openCatalog(TreeName catalog) {
 				if (!lostTheRace) {
 					if (isExistsTable(catalog)) {
 						readEnrolledTrees(catalog);
-					} else {
-						createCatalogTable(catalog);
-						// nothing to read from a table that has just been created, and nothing this open
-						// enrols may be skipped as already recorded
+					} else if (!createCatalogTable(catalog)) {
+						// the table was there after all: another session created it while this one was
+						// creating it, and a table this session did not create is a catalog with rows in it
+						// like any other - the branch above is what those rows are for. The flag below is
+						// raised over what this storage knows the catalog records, and raised over none of
+						// it, every tree of this open is enrolled again against a catalog already naming
+						// them: one upsert and one commit each, and the same for every later write of this
+						// open that names a tree (#933). It reads on the session the failed create reset,
+						// which is a connection rolled back or, where even that failed, one given up for
+						// the read to establish again: either is a session a select may be asked of. That
+						// re-establish is the one connect of this class made under the lock: it needs a
+						// create cut by a dead catalog connection, a table another session made meanwhile,
+						// and a database refusing logins, and it is bounded by the deadline of a borrow
+						// like the connect made outside it
+						readEnrolledTrees(catalog);
 					}
+					// and where the create really did create it, there is nothing to read: a table just
+					// made holds no row, and nothing this open enrols may be skipped as already recorded
 					catalogTableOpened=true;
 				}
 			}
@@ -4853,8 +4867,12 @@ void readEnrolledTrees(TreeName catalog) {
 		 * line. What is left is the privilege the account is missing. An engine this backend does not
 		 * know has no number a lock could be told by ({@code isLockTimeout()} answers false on a null
 		 * dialect), and gets the privilege line there as it did.
+		 *
+		 * @return whether this session created the table. {@code false} says another session created it
+		 *         while this one was creating it, which is a table full of rows this storage has not
+		 *         read: what {@link #openCatalog} does with that answer is read them (#933).
 		 */
-		void createCatalogTable(TreeName catalog) {
+		boolean createCatalogTable(TreeName catalog) {
 			final String tableName=getTableName(catalog);
 			Dialect dialect=null; // read inside the try, and asked again by the catch, which tells a lock by the engine's own number
 			try {
@@ -4875,6 +4893,7 @@ void createCatalogTable(TreeName catalog) {
 					catalogCon.commit();
 					return null;
 				});
+				return true;
 			} 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
@@ -4898,7 +4917,7 @@ void createCatalogTable(TreeName catalog) {
 				if (alreadyThere) {
 					logger.debug(LocalizableMessage.raw("jdbc: table %s was created by another session while this one was creating it: %s",
 						tableName, stackTraceToSingleLineString(e)));
-					return;
+					return false; // read by the caller, the rows being another session's and not this one's
 				}
 				// A lock another session holds is not a privilege the account lacks, whichever bound ended
 				// the wait for it: asked of the engine's own number on every road, since gaveUpOnTheLock()

opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java

@@ -37,6 +37,10 @@
 import org.testng.annotations.BeforeClass;
 import org.testng.annotations.Test;
 
+import java.lang.reflect.InvocationHandler;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import java.lang.reflect.Proxy;
 import java.sql.Connection;
 import java.sql.DriverManager;
 import java.sql.PreparedStatement;
@@ -2190,6 +2194,181 @@ public void run(WriteableTransaction txn) throws Exception {
 		}
 	}
 
+	/**
+	 * A catalog table another session created while this one was creating it has to be read like any
+	 * other catalog that is already there. The create tolerates that race - an offline tool beside a
+	 * running server is a pair no lock of one process can order - and what it must not do is leave the
+	 * storage believing that table records nothing (#933): a storage believing that writes the row of
+	 * every tree it opens again, one upsert and one commit each, for the whole of that open.
+	 * <p>
+	 * The tree the case asks for afterwards is one the adopting write never opened, which is what
+	 * tells the two apart: a storage that read the table knows the catalog names it already, while one
+	 * that only wrote its way through the trees of that write knows those and nothing else.
+	 * <p>
+	 * What says which of the two it is, is the connection. The catalog of a transaction is opened at
+	 * the first row that transaction has to write - see {@link JDBCStorage.CatalogSession} - so a
+	 * write whose trees the catalog already names opens none at all, and one that has to record them
+	 * again opens one to write the rows that are already there.
+	 */
+	@Test
+	public void testAnOpenThatAdoptsTheCatalogTableOfAnotherSessionEnrolsNothingAgain() throws Exception {
+		final TreeName opened = new TreeName("testAdoptedCatalog", "opened");
+		final TreeName recorded = new TreeName("testAdoptedCatalog", "recorded");
+		final JDBCStorage owner = new JDBCStorage(createBackendCfg(getBackendId() + "_adopted"), null);
+		// the two are one backend, so the clear below drops what either of them left behind, and it is
+		// constructed here so that a failure of the half that fills the catalog reaches that clear too:
+		// nothing but @BeforeClass ever drops the tables a case leaves standing
+		final AdoptingStorage racing = new AdoptingStorage(createBackendCfg(getBackendId() + "_adopted"));
+		try {
+			try {
+				owner.open(AccessMode.READ_WRITE);
+				owner.write(new WriteOperation() {
+					@Override
+					public void run(WriteableTransaction txn) throws Exception {
+						txn.openTree(opened, true); // the catalog table, and a row naming each of these trees
+						txn.openTree(recorded, true);
+					}
+				});
+			} finally {
+				owner.close();
+			}
+
+			racing.open(AccessMode.READ_WRITE);
+			// armed after the open and not before it: what this models is the lookup openCatalog() makes,
+			// and anything asking about the same table earlier would spend the one answer it has
+			racing.hideOnce(racing.getTableName(racing.getCatalogTree()));
+			racing.write(new WriteOperation() {
+				@Override
+				public void run(WriteableTransaction txn) throws Exception {
+					txn.openTree(opened, true);
+				}
+			});
+			assertTrue(racing.hidTheCatalogTable(), "the case never reached the create this is about");
+			final int connectsOfTheAdoptingWrite = racing.catalogConnects();
+
+			racing.write(new WriteOperation() {
+				@Override
+				public void run(WriteableTransaction txn) throws Exception {
+					txn.openTree(recorded, true); // named by the catalog already, so there is no row to write
+				}
+			});
+			assertEquals(racing.catalogConnects(), connectsOfTheAdoptingWrite,
+				"a transaction whose tree the catalog already names opened a connection to write that row"
+					+ " again: the open which adopted the table another session created read nothing of what"
+					+ " that table records");
+			assertTrue(racing.listTrees().contains(recorded), "the read of the adopted rows rewrote or removed them");
+		} finally {
+			clearQuietly(racing);
+		}
+	}
+
+	/**
+	 * The road above pins the "adopted" answer of {@code createCatalogTable()}; this one pins the
+	 * "created" answer against the same mistake: an open that made the table itself has nothing of
+	 * its own to read back, the table it just made holding no row, and reading it anyway would be
+	 * one select and one commit spent on an empty catalog for every such open.
+	 */
+	@Test
+	public void testAnOpenThatCreatesTheCatalogTableReadsNothing() throws Exception {
+		final AdoptingStorage fresh = new AdoptingStorage(createBackendCfg(getBackendId() + "_created"));
+		try {
+			fresh.open(AccessMode.READ_WRITE);
+			fresh.write(new WriteOperation() {
+				@Override
+				public void run(WriteableTransaction txn) throws Exception {
+					txn.openTree(new TreeName("testCreatedCatalog", "opened"), true);
+				}
+			});
+			assertEquals(fresh.catalogReads(), 0, "the open that created the catalog table read it back");
+		} finally {
+			clearQuietly(fresh);
+		}
+	}
+
+	/**
+	 * The adopt road again, on the arm where the failed create could not even be rolled back: the
+	 * session is given up rather than left holding a connection the database has dropped, and the
+	 * read of the rows that table already holds has to establish it again - the one connect this
+	 * class makes under {@code catalogLock}, which the invariant of {@code openCatalog()} names as
+	 * its exception.
+	 * <p>
+	 * A rollback refused once is the second state no case can reach from outside, beside the hidden
+	 * lookup it is armed with: a create that fails because its own connection is gone fails its
+	 * rollback with it. What says the read was made on a session established for it is the count of
+	 * connections the adopting write opened - the one the create ran on, and the one the read had to
+	 * make - and what says that read filled the memo all the same is the write after it, whose tree
+	 * the catalog already names and which therefore opens none.
+	 * <p>
+	 * The attempt is counted beside them, because {@code write()} would otherwise answer for this
+	 * road itself: a storage that gave the session up without letting go of it reads on a closed
+	 * connection, which is classified as a dropped connection and replayed, and the replay makes the
+	 * very table it failed to make usable. The counts above are then the counts of an attempt that
+	 * never met the race, so the case asks for the one attempt as well.
+	 */
+	@Test
+	public void testAnOpenThatAdoptsTheCatalogTableReadsItOnASessionItHadToEstablishAgain() throws Exception {
+		final TreeName opened = new TreeName("testRefusedRollbackCatalog", "opened");
+		final TreeName recorded = new TreeName("testRefusedRollbackCatalog", "recorded");
+		final JDBCStorage owner = new JDBCStorage(createBackendCfg(getBackendId() + "_refused"), null);
+		// one backend in two storages, as in the case above: the clear at the end drops what either of
+		// them left behind, and constructing it here reaches that clear whichever half failed
+		final AdoptingStorage racing = new AdoptingStorage(createBackendCfg(getBackendId() + "_refused"));
+		try {
+			try {
+				owner.open(AccessMode.READ_WRITE);
+				owner.write(new WriteOperation() {
+					@Override
+					public void run(WriteableTransaction txn) throws Exception {
+						txn.openTree(opened, true); // the catalog table, and a row naming each of these trees
+						txn.openTree(recorded, true);
+					}
+				});
+			} finally {
+				owner.close();
+			}
+
+			racing.open(AccessMode.READ_WRITE);
+			// armed after the open, for the reason the case above arms its lookup there: the catalog of a
+			// transaction is established at the first row it has to write, which is inside the write below
+			racing.hideOnce(racing.getTableName(racing.getCatalogTree()));
+			racing.refuseNextRollback();
+			final int connectsBeforeTheAdoptingWrite = racing.catalogConnects();
+			final AtomicInteger attemptsOfTheAdoptingWrite = new AtomicInteger();
+			racing.write(new WriteOperation() {
+				@Override
+				public void run(WriteableTransaction txn) throws Exception {
+					attemptsOfTheAdoptingWrite.incrementAndGet();
+					txn.openTree(opened, true);
+				}
+			});
+			assertTrue(racing.hidTheCatalogTable(), "the case never reached the create this is about");
+			// a session the storage gave up and did not let go of is a select on a closed connection, which
+			// write() classifies as a dropped connection and replays: the road recovers, and the counts
+			// below are the counts of the attempt that replaced it. This is what says the read was made
+			// inside the attempt that met the race, at the cost of the one connect and no replay at all
+			assertEquals(attemptsOfTheAdoptingWrite.get(), 1,
+				"the write which adopted the table was replayed: the read of the adopted rows has to be"
+					+ " made on a session established inside that attempt, not by spending another one");
+			assertEquals(racing.catalogConnects(), connectsBeforeTheAdoptingWrite + 2,
+				"the session the refused rollback gave up was not established again for the read of the"
+					+ " adopted rows: this road costs one connection for the create and one for that read");
+
+			racing.write(new WriteOperation() {
+				@Override
+				public void run(WriteableTransaction txn) throws Exception {
+					txn.openTree(recorded, true); // named by the catalog already, so there is no row to write
+				}
+			});
+			assertEquals(racing.catalogConnects(), connectsBeforeTheAdoptingWrite + 2,
+				"a transaction whose tree the catalog already names opened a connection to write that row"
+					+ " again: the read made on the re-established session recorded nothing of what the"
+					+ " adopted table holds");
+			assertTrue(racing.listTrees().contains(recorded), "the read of the adopted rows rewrote or removed them");
+		} finally {
+			clearQuietly(racing);
+		}
+	}
+
 	/**
 	 * A row of the catalog whose table is not there any more must not fail the clear, and must not
 	 * stop it dropping the rest. Nothing of the backend leaves such a row behind - deleteTree() takes
@@ -3003,4 +3182,120 @@ void assertReported(String whatWentUnsaid, String... fragments) {
 			fail(whatWentUnsaid + "; the clear reported: " + reported());
 		}
 	}
+
+	/**
+	 * A storage which answers once that its catalog table is not there while it is, and counts the
+	 * connections its catalog is read and written on.
+	 * <p>
+	 * That answer models the one state no case can reach from outside: a session whose lookup ran a
+	 * moment before another session's "create table" committed, which goes on to create a table that
+	 * is already there and has to make what it finds usable all the same (#933). Every lookup after
+	 * it is the database's own answer, the way {@link ReportingStorage} lets its own go.
+	 * <p>
+	 * The count is what says whether the storage remembered what that table records: the catalog of a
+	 * transaction is opened at the first row the transaction has to write, so a write whose trees the
+	 * catalog already names opens no connection at all - see {@link JDBCStorage.CatalogSession}.
+	 * <p>
+	 * It refuses one rollback where a case asks for it, which is the other state no case reaches from
+	 * outside: the create of a session the database has dropped fails its rollback with it, and the
+	 * read of the adopted rows is then made on a session established again for it.
+	 */
+	protected static final class AdoptingStorage extends JDBCStorage {
+		private volatile String tableToHide;
+		private volatile boolean hidden;
+		private volatile boolean refuseNextRollback;
+		private final AtomicInteger catalogConnects = new AtomicInteger();
+		private final AtomicInteger catalogReads = new AtomicInteger();
+
+		AdoptingStorage(JDBCBackendCfg cfg) {
+			super(cfg, null);
+		}
+
+		/** Answers the next lookup of this table with "not there", whatever the database holds. */
+		void hideOnce(String tableName) {
+			tableToHide = tableName;
+		}
+
+		/** Whether that answer was given, so that a case cannot pass without having reached the race. */
+		boolean hidTheCatalogTable() {
+			return hidden;
+		}
+
+		/**
+		 * Hands the next catalog connection out with its first rollback refused: the state of a
+		 * session the database has dropped, whose create fails and whose rollback of that create fails
+		 * with it, which is what leaves {@code reset()} giving the session up instead of rolling it
+		 * back.
+		 */
+		void refuseNextRollback() {
+			refuseNextRollback = true;
+		}
+
+		/** How many connections this storage has opened its catalog on since it was constructed. */
+		int catalogConnects() {
+			return catalogConnects.get();
+		}
+
+		/** How many times this storage has read the catalog into its memo since it was constructed. */
+		int catalogReads() {
+			return catalogReads.get();
+		}
+
+		@Override
+		boolean isExistsTable(Connection con, JDBCStorage.TableScope scope, String tableName) {
+			final String hiding = tableToHide;
+			if (hiding != null && hiding.equalsIgnoreCase(tableName)) {
+				tableToHide = null; // once: the create it sends is answered by the table the database holds
+				hidden = true;
+				return false;
+			}
+			return super.isExistsTable(con, scope, tableName);
+		}
+
+		@Override
+		Connection newCatalogConnection(long budgetDeadline) throws SQLException {
+			catalogConnects.incrementAndGet();
+			final Connection real = super.newCatalogConnection(budgetDeadline);
+			if (!refuseNextRollback) {
+				return real;
+			}
+			refuseNextRollback = false;
+			return refusingItsFirstRollback(real);
+		}
+
+		/**
+		 * The connection above with one rollback of its own refused, and everything else the driver's.
+		 * The rollback of a whole transaction alone: a rollback to a savepoint is what the bound of a
+		 * DDL takes back where its setting failed ({@code withDdlLockBound}), and refusing that one
+		 * would arm a road other than the one this case is about.
+		 */
+		private static Connection refusingItsFirstRollback(final Connection real) {
+			return (Connection) Proxy.newProxyInstance(Connection.class.getClassLoader(),
+				new Class<?>[] { Connection.class }, new InvocationHandler() {
+					private boolean refused;
+
+					@Override
+					public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
+						if (!refused && "rollback".equals(method.getName()) && method.getParameterCount() == 0) {
+							refused = true;
+							throw new SQLException("the case refused the rollback of this catalog connection");
+						}
+						try {
+							return method.invoke(real, args);
+						} catch (InvocationTargetException e) {
+							throw e.getCause(); // the driver's own failure, and not a wrapper of the call
+						}
+					}
+				});
+		}
+
+		// the only caller of the two-argument overload is readEnrolledTrees(): catalogTables() (a
+		// clear, or listTrees()) takes the three-argument one, so this counts a read of the memo and
+		// nothing a clear does
+		@Override
+		Map<TreeName,String> readCatalogRows(Connection con, String catalogTable) throws SQLException {
+			catalogReads.incrementAndGet();
+			return super.readCatalogRows(con, catalogTable);
+		}
+	}
 }