From 3d69d6719667088d6effb81b2c68c15352f6f69b Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Wed, 9 Sep 2026 20:12:13 +0300 Subject: [PATCH 1/3] [#933] Read a catalog table another session created while this one was creating it createCatalogTable() answers whether it created the table or found it already there, and openCatalog() reads the rows on the second answer instead of raising catalogTableOpened over an empty enrolledTrees. A storage that adopted a table without reading it enrols every tree of that open again - one upsert and one commit each on the catalog connection, against rows that are already there - and goes on doing it for every later write of that open which names a tree. Fixes #933 --- .../server/backends/jdbc/JDBCStorage.java | 31 +++-- .../opends/server/backends/jdbc/TestCase.java | 123 ++++++++++++++++++ 2 files changed, 146 insertions(+), 8 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 4f56aa4eb0..7c15142e5c 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 @@ -4721,8 +4721,9 @@ Connection catalogConnection() { *

* 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,20 @@ 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 + 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 +4863,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 +4889,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 +4913,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..a49eedfd5b 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 @@ -2190,6 +2190,74 @@ 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 catalog stopped naming the tree it records"); + } 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 +3071,59 @@ 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}. + */ + protected static final class AdoptingStorage extends JDBCStorage { + private volatile String tableToHide; + private volatile boolean hidden; + private final AtomicInteger catalogConnects = 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; + } + + /** How many connections this storage has opened its catalog on since it was constructed. */ + int catalogConnects() { + return catalogConnects.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(); + return super.newCatalogConnection(budgetDeadline); + } + } } From 50e367e6a192a1dd6c9311ff44782c0e2fde73c7 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Fri, 18 Sep 2026 23:07:43 +0300 Subject: [PATCH 2/3] [#933] Address round-1 review: pin the create-only read, note the adopt-path reconnect, sharpen an assert Add testAnOpenThatCreatesTheCatalogTableReadsNothing, pinning the "created" answer of createCatalogTable() against the same mistake the "adopted" one already pins: an open that made the table itself has nothing to read back. Note, at the connect-outside-the-lock invariant, that the adopt path's readEnrolledTrees() can re-establish the catalog connection under catalogLock - a create cut by a dead connection, a table another session made meanwhile, and a database refusing logins, bounded by the same borrow deadline as the connect made outside it. Reword the third assert of testAnOpenThatAdoptsTheCatalogTableOfAnotherSessionEnrolsNothingAgain: it pins nothing a revert of the fix wouldn't already fail earlier on, and its old message read as if it did. --- .../server/backends/jdbc/JDBCStorage.java | 6 ++- .../opends/server/backends/jdbc/TestCase.java | 40 ++++++++++++++++++- 2 files changed, 44 insertions(+), 2 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 7c15142e5c..488374d4cb 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 @@ -4767,7 +4767,11 @@ void openCatalog(TreeName catalog) { // 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 + // 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 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 a49eedfd5b..b6d1d3b6cc 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 @@ -2252,12 +2252,35 @@ public void run(WriteableTransaction txn) throws Exception { "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 catalog stopped naming the tree it 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); + } + } + /** * 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 @@ -3089,6 +3112,7 @@ protected static final class AdoptingStorage extends JDBCStorage { private volatile String tableToHide; private volatile boolean hidden; private final AtomicInteger catalogConnects = new AtomicInteger(); + private final AtomicInteger catalogReads = new AtomicInteger(); AdoptingStorage(JDBCBackendCfg cfg) { super(cfg, null); @@ -3109,6 +3133,11 @@ 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; @@ -3125,5 +3154,14 @@ Connection newCatalogConnection(long budgetDeadline) throws SQLException { catalogConnects.incrementAndGet(); return super.newCatalogConnection(budgetDeadline); } + + // 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 readCatalogRows(Connection con, String catalogTable) throws SQLException { + catalogReads.incrementAndGet(); + return super.readCatalogRows(con, catalogTable); + } } } From 924f7922a5f9a085d3dddf3004710b4a77cd744e Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Mon, 21 Sep 2026 17:19:31 +0300 Subject: [PATCH 3/3] [#933] Address round-2 review: pin the read made on a session the failed rollback gave up The adopt road has an arm no case reached: where reset() cannot roll the failed create back it gives the session up, and readEnrolledTrees() has to establish it again - the one connect of this class made under catalogLock. AdoptingStorage now hands the next catalog connection out through a proxy whose first no-argument rollback is refused, which is the state of a session the database has dropped. Only the rollback of a whole transaction: a rollback to a savepoint is what withDdlLockBound() takes back where its own setting failed, so refusing by name alone would arm another road than this one. The case counts the attempts of the adopting write beside the connections it opened, because write() would otherwise answer for this road itself: a session given up without being let go of is a select on a closed connection, which is classified as a dropped connection and replayed, and the replay makes the table usable at the same two connects. Counted, the road has to recover inside the attempt that met the race. --- .../opends/server/backends/jdbc/TestCase.java | 136 +++++++++++++++++- 1 file changed, 135 insertions(+), 1 deletion(-) 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 b6d1d3b6cc..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; @@ -2281,6 +2285,90 @@ public void run(WriteableTransaction txn) throws Exception { } } + /** + * 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 @@ -3107,10 +3195,15 @@ void assertReported(String whatWentUnsaid, String... fragments) { * 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(); @@ -3128,6 +3221,16 @@ 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(); @@ -3152,7 +3255,38 @@ boolean isExistsTable(Connection con, JDBCStorage.TableScope scope, String table @Override Connection newCatalogConnection(long budgetDeadline) throws SQLException { catalogConnects.incrementAndGet(); - return super.newCatalogConnection(budgetDeadline); + 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