* 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. *
* 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() diff --git a/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java b/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java index 75001d37c1..2b42ff406b 100644 --- a/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java +++ b/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. + *
+ * 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. + *
+ * 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. + *
+ * 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. + *
+ * 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. + *
+ * 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. + *
+ * 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}. + *
+ * 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