From 8ad97160e90f47e5fffe0b8a8c65ee421fa0903c Mon Sep 17 00:00:00 2001
From: Valery Kharseko <vharseko@3a-systems.ru>
Date: Tue, 22 Sep 2026 09:08:37 +0000
Subject: [PATCH] [#933] Read a catalog table another session created while this one was creating it (#1005)
---
opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java | 295 +++++++++++++++++++++++++++++++++++++++++++++++++
opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java | 35 ++++-
2 files changed, 322 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 b323570..55cb13c 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
@@ -4724,8 +4724,9 @@
* <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
@@ -4760,11 +4761,24 @@
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;
}
}
@@ -4856,8 +4870,12 @@
* 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 {
@@ -4878,6 +4896,7 @@
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
@@ -4901,7 +4920,7 @@
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 da5f705..e60e01d 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;
@@ -2200,6 +2204,181 @@
}
/**
+ * 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
* it out in the commit that drops the table - but a table dropped by hand, or a catalog restored
@@ -3012,4 +3191,120 @@
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);
+ }
+ }
}
--
Gitblit v1.10.0