From 74f6b83623878cf4dd032fb23db2b7e8f7635922 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Thu, 20 Aug 2026 16:36:08 +0300 Subject: [PATCH 1/3] [#888] Name the trees of a JDBC backend from a catalog in the database listTrees() answered from tree2table, a cache a tree enters the first time this process names a table for it. Nothing seeds it - open() takes a connection and sets the storage status - so a process which has opened nothing names no trees. removeStorageFiles() is the one caller running before the root container is open, and it drops exactly what listTrees() names. In the offline import-ldif the backend is configured and never opened, so the set was empty, the drop loop was skipped, and "import-ldif --clearBackend" cleared a JDBC backend of nothing, without a word in the log. Online the same command does drop the tables: ImportTask calls importLDIF on the Backend object it already holds, whose storage has been serving traffic with a fully populated cache. JE and PDB enumerate the environment itself, so both honour the contract whatever the process did earlier. What survived was not only the tables of trees the import does not rebuild. The importer clears an entry container when its first entry arrives, so a base DN configured in the backend but absent from the imported LDIF was cleared by nothing at all: it kept its entries and went on serving them, where the same command on JE removes the whole backend directory. The option is documented as "Remove all entries for all base DNs in the backend before importing". The trees of a backend are recorded in the database now, in a tree of their own: one row per tree, keyed by the tree name, with the table holding it as its value. The catalog is per backend and named after the backend id alone, so its table name follows from the configuration without asking the database anything - which is what a process that has opened nothing needs - and so that backends sharing one database URL (#873) never name each other's trees. Being an ordinary tree it needs no dialect of its own; it is created without the index openTree() gives a tree, which serves cursor batches the catalog never runs, and without a comment, which would only repeat what its rows say in plain text. openTree(createOnDemand) enrols, and nothing else does: naming a tree in order to read it must never put it up for removal. The row is written on every open rather than only when a table is created, so a backend upgraded from a version without a catalog fills it in at its first read-write open. deleteTree() takes the row out together with the table. The compressed schema trees are the exception. Named from a literal, they are the same pair for every backend of a database (#873), so they are never enrolled: a backend must not offer for removal a tree another one may be the only owner of, and that pair is deliberately left where it lies (#881). The trees the fix of #873 names after the backend id are enrolled like any other. removeStorageFiles() drops the catalog last and skips a table that is not there: dropping a table is DDL, which mysql and oracle commit as they go, so an attempt which fails halfway is finished by the next one instead of leaving behind tables nothing names any more. Where no catalog is there yet, the opendj tables the connection can reach are counted and reported rather than dropped - a table is named after the hash of its tree name, so nothing about it says which backend of a shared database it belongs to. Two cases in jdbc/TestCase, inherited by all four engine suites: * testABackendIsClearedByAProcessThatNeverOpenedIt - a storage which never opened the backend names its tree and drops its tables, while the tables of another backend of the same database stay where they are * testADeletedTreeIsNoLongerNamedByTheCatalog - a dropped tree leaves the catalog with its table, and the clear that follows does not stumble over it PgSqlTestCase and MySqlTestCase pass 55/55 with no skips. MsSqlTestCase and OracleTestCase were not run locally. --- .../server/backends/jdbc/JDBCStorage.java | 283 +++++++++++++++--- .../opends/server/backends/jdbc/TestCase.java | 114 ++++++- 2 files changed, 361 insertions(+), 36 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 d1d8055314..6deb1e053a 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 @@ -36,6 +36,7 @@ import java.io.Closeable; import java.nio.ByteBuffer; +import java.nio.charset.StandardCharsets; import java.security.MessageDigest; import java.security.NoSuchAlgorithmException; import java.sql.*; @@ -166,6 +167,9 @@ public void close() { // that it is not reissued for every tree on every open; disabling and re-enabling the // backend is the way to try again once the privilege has been granted unstampableTrees.clear(); + // what this storage knows of its catalog holds no longer than the open it learnt it in: the + // table may well be gone by the next one, dropped by an offline tool run in the meantime + catalogTableOpened=false; } final LoadingCache tree2table = Caffeine.newBuilder() @@ -189,6 +193,49 @@ String getTableName(TreeName treeName) { return tree2table.get(treeName); } + /** + * The pseudo base DN of the tree naming the trees of a backend. Every real tree of a backend is + * named after an entry container, whose prefix is a normalized DN and so always holds a "=", + * which an identifier of this form cannot collide with. + */ + static final String CATALOG_BASE_DN="opendj_catalog"; + + /** + * The base DN under which the compressed schema trees are named by versions naming them from a + * literal. It carries no backend qualifier, so on a database addressed by several backends - + * which nothing forbids (#873) - that pair of trees is the same pair for all of them, and a + * backend must not put a tree another one may be the owner of up for removal. The pair is left + * where it lies on purpose (#881): it may still be the only copy a backend has. Trees named from + * the backend id instead are enrolled like any other. + */ + static final String SHARED_COMPRESSED_SCHEMA_BASE_DN="compressed_schema"; + + /** + * The tree naming the trees this backend owns: one row per tree, the tree name as its key and the + * table holding that tree as its value. + *

+ * A table is named after the hash of its tree name, so the catalog of a database can neither be + * filtered by a per-backend prefix nor read back into a {@link TreeName}. Without a record of its + * own a backend can therefore only name the trees this very process has already touched - which + * is precisely what {@link #removeStorageFiles()} cannot have, running as it does before the root + * container is open. In the offline {@code import-ldif} nothing has touched a tree at all, so + * {@code --clearBackend} used to clear nothing whatsoever (#888). + *

+ * The catalog is per backend and named after the backend id alone: a process that has opened + * nothing can still find its table, and backends sharing one database URL - which nothing + * forbids (#873) - never name each other's trees. + */ + TreeName getCatalogTree() { + return new TreeName(CATALOG_BASE_DN, config.getBackendId()); + } + + /** + * Whether the table of the catalog was created, or found, by this storage. A tree is enrolled on + * every open - about 25 of them for a stock suffix - and asking the catalog whether the table is + * there would cost a metadata round trip per tree. + */ + private volatile boolean catalogTableOpened=false; + /** * The form a catalog pattern has to take to match an identifier this backend created unquoted. * An unquoted identifier is folded when it is stored - to upper case on oracle, to lower case @@ -769,6 +816,31 @@ boolean updateTableStatistics(Connection con, Collection trees) { return allRefreshed; } + /** + * Whether a table of this name is there. Asked of the catalog by name rather than by listing + * every table of the database: openTree(createOnDemand) asks it for every tree of the backend - + * about 25 of them for a stock suffix - on every open, on a database this backend may well be + * sharing with something else. + */ + boolean isExistsTable(Connection con, String tableName) { + try { + final DatabaseMetaData metaData = con.getMetaData(); + try (final ResultSet rs = metaData.getTables(null, null, + storedIdentifier(metaData, tableName), new String[]{"TABLE"})) { + while (rs.next()) { + // the name still has to be compared: "_" is a single-character wildcard in a + // metadata pattern, so "opendj_" also matches a table named "opendjX" + if (tableName.equalsIgnoreCase(rs.getString("TABLE_NAME"))) { + return true; + } + } + } + } catch (Exception e) { + throw new StorageRuntimeException(e); + } + return false; + } + @Override public void removeStorageFiles() throws StorageRuntimeException { final boolean isOpen=getStorageStatus().isWorking(); @@ -779,12 +851,18 @@ public void removeStorageFiles() throws StorageRuntimeException { throw new StorageRuntimeException(e); } } - final Set trees=listTrees(); - if (!trees.isEmpty()) { - try (final Connection con = getConnection()) { + try (final Connection con = getConnection()) { + final Set trees=listTrees(con); + if (trees.isEmpty()) { + reportUncataloguedTables(con); + } else { try { - for (final TreeName treeName : trees) { - try (final PreparedStatement statement = con.prepareStatement("drop table " + getTableName(treeName))) { + for (final TreeName treeName : catalogDroppedLast(trees)) { + final String tableName=getTableName(treeName); + if (!isExistsTable(con, tableName)) { // a row of the catalog outliving its table + continue; + } + try (final PreparedStatement statement = con.prepareStatement("drop table " + tableName)) { execute(statement); } } @@ -795,17 +873,71 @@ public void removeStorageFiles() throws StorageRuntimeException { } catch (SQLException e2) {} throw new StorageRuntimeException(e); } - } catch (Exception e) { - throw new StorageRuntimeException(e); + // all tables are gone: forget the mappings so listTrees() consumers skip the dropped trees + for (final TreeName treeName : trees) { + tree2table.invalidate(treeName); + unstampableTrees.remove(treeName); // a table recreated later deserves a fresh stamp attempt + } } - // all tables are gone: forget the mappings so listTrees() consumers skip the dropped trees - for (final TreeName treeName : trees) { - tree2table.invalidate(treeName); - unstampableTrees.remove(treeName); // a table recreated later deserves a fresh stamp attempt + } catch (StorageRuntimeException e) { + throw e; + } catch (Exception e) { + throw new StorageRuntimeException(e); + } finally { + // the catalog went with the rest: the next tree enrolled creates its table again. The + // online import needs exactly that - the storage which has just dropped its tables is the + // one going on to open a root container and enrol every tree of it anew. + catalogTableOpened=false; + if (!isOpen) { + close(); } } - if (!isOpen) { - close(); + } + + /** + * The trees to remove, the catalog last: it names what is still to be dropped, so a removal that + * fails halfway - dropping a table is DDL, which mysql and oracle commit as they go - is finished + * by the next attempt rather than leaving behind tables nothing names any more. + */ + private List catalogDroppedLast(Set trees) { + final TreeName catalog=getCatalogTree(); + final List ordered=new ArrayList<>(trees.size()); + for (final TreeName treeName : trees) { + if (!catalog.equals(treeName)) { + ordered.add(treeName); + } + } + if (trees.contains(catalog)) { + ordered.add(catalog); + } + return ordered; + } + + /** + * Reports the tables of a backend whose catalog is not there yet. They are reported rather than + * dropped: a table is named after the hash of its tree name, so nothing about a table found under + * the "opendj_" prefix says which backend of a shared database (#873) it belongs to. The catalog + * is written as the trees of a backend are opened, so the first read-write open after an upgrade + * to a version keeping one is enough to make the next clear complete. + */ + private void reportUncataloguedTables(Connection con) { + int count=0; + try { + final DatabaseMetaData metaData=con.getMetaData(); + try (final ResultSet rs=metaData.getTables(null, null, + storedIdentifier(metaData, "opendj%"), new String[]{"TABLE"})) { + while (rs.next()) { + count++; + } + } + } catch (SQLException e) { + logger.trace(LocalizableMessage.raw("jdbc: unable to look for the tables of an uncatalogued backend: %s", + stackTraceToSingleLineString(e))); + return; + } + if (count>0) { + logger.warn(LocalizableMessage.raw("jdbc: backend %s has no tree catalog (table %s), so none of the %d opendj tables reachable on this connection could be attributed to it and nothing was cleared; the catalog is written as the trees are opened, from the first read-write open of the backend on", + config.getBackendId(), getTableName(getCatalogTree()), count)); } } @@ -1059,27 +1191,7 @@ public WriteableTransactionTransactionImpl(Connection con) { } boolean isExistsTable(TreeName treeName) { - final String tableName = getTableName(treeName); - try { - final DatabaseMetaData metaData = con.getMetaData(); - // asked of the catalog by name: openTree(createOnDemand) calls this for every tree - // of the backend - about 25 of them for a stock suffix, on every open - and listing - // every table of the database each time costs the whole catalog once per tree, on a - // database this backend may well be sharing with something else - try (final ResultSet rs = metaData.getTables(null, null, - storedIdentifier(metaData, tableName), new String[]{"TABLE"})) { - while (rs.next()) { - // the name still has to be compared: "_" is a single-character wildcard in a - // metadata pattern, so "opendj_" also matches a table named "opendjX" - if (tableName.equalsIgnoreCase(rs.getString("TABLE_NAME"))) { - return true; - } - } - } - } catch (Exception e) { - throw new StorageRuntimeException(e); - } - return false; + return JDBCStorage.this.isExistsTable(con, getTableName(treeName)); } String getTableDialect() { @@ -1142,6 +1254,71 @@ public void openTree(TreeName treeName, boolean createOnDemand) { // the dialect is taken off this transaction's own connection: finding it out must // not cost a borrow from a pool this thread is already holding a connection of commentTable(treeName, dialectOf(con), stampSession); + // what makes this tree nameable by a process which has opened nothing: see getCatalogTree() + enrolInCatalog(treeName); + } + } + + /** + * Records the tree in the catalog of this backend, creating the catalog itself along the way + * when this is the first tree of the storage. Enrolling is the business of + * openTree(createOnDemand) alone: naming a tree in order to read it must never put it up for + * removal, since the tree read may belong to another backend of the same database - the + * unqualified compressed schema trees such a database may still hold, say (#873). + *

+ * The row is written on every open rather than only when the table is created, so that a + * backend of an installation upgraded to a version keeping a catalog fills it in at its first + * read-write open instead of waiting for its trees to be created again. + */ + void enrolInCatalog(TreeName treeName) { + final TreeName catalog=getCatalogTree(); + if (catalog.equals(treeName)) { + return; // the catalog holds no row of its own: listTrees() adds it when its table is there + } + if (SHARED_COMPRESSED_SCHEMA_BASE_DN.equals(treeName.getBaseDN())) { + return; // a tree this backend may not be the only owner of: see the constant + } + if (!catalogTableOpened) { + openCatalogTable(catalog); + catalogTableOpened=true; + } + try { + upsert(catalog, ByteString.valueOfUtf8(treeName.toString()), + ByteString.valueOfUtf8(getTableName(treeName))); + } catch (SQLException e) { + throw new StorageRuntimeException(e); + } + } + + /** + * Creates the table of the catalog when it is not there yet. It takes neither the index nor + * the comment openTree() gives a tree: the catalog is read whole and written by key, never + * iterated by key range, so the index a cursor needs would serve nothing here, and its rows + * name the trees in plain text, so a comment naming the table would only repeat what reading + * it says. Keeping it out of the comment sweep also keeps the cost of a stamp the database + * rejects where it was - one attempt per tree of the backend, not one more. + */ + void openCatalogTable(TreeName catalog) { + if (isExistsTable(catalog)) { + return; + } + try (final PreparedStatement statement=con.prepareStatement("create table "+getTableName(catalog)+" ("+getTableDialect()+")")) { + execute(statement); + con.commit(); + } catch (SQLException e) { + throw new StorageRuntimeException(e); + } + } + + /** Takes the tree out of the catalog: a row is what puts a table up for removal, and this one is gone. */ + void unenrolFromCatalog(TreeName treeName) { + final TreeName catalog=getCatalogTree(); + if (catalog.equals(treeName)) { + catalogTableOpened=false; // its own table is gone: the next enrolment creates it again + return; + } + if (catalogTableOpened || isExistsTable(catalog)) { + delete(catalog, ByteString.valueOfUtf8(treeName.toString())); } } @@ -1176,6 +1353,7 @@ public void deleteTree(TreeName treeName) { throw new StorageRuntimeException(e); } } + unenrolFromCatalog(treeName); // forget the mapping so listTrees() consumers (updateTableStatistics) skip the dropped table tree2table.invalidate(treeName); unstampableTrees.remove(treeName); // a table recreated later deserves a fresh stamp attempt @@ -1474,9 +1652,44 @@ public boolean positionToIndex(int index) { } } + /** + * {@inheritDoc} + *

+ * Answered from the catalog of the backend rather than from the trees this process happens to + * have touched: {@link #removeStorageFiles()} runs before anything has touched one (#888). + */ @Override public Set listTrees() { - return tree2table.asMap().keySet(); + try (final Connection con=getConnection()) { + return listTrees(con); + } catch (StorageRuntimeException e) { + throw e; + } catch (Exception e) { + throw new StorageRuntimeException(e); + } + } + + Set listTrees(Connection con) throws SQLException { + final TreeName catalog=getCatalogTree(); + final String catalogTable=getTableName(catalog); + if (!isExistsTable(con, catalogTable)) { // a backend which has never been opened read-write + return Collections.emptySet(); + } + final Set trees=new HashSet<>(); + trees.add(catalog); // the catalog names every tree of the backend but itself + try (final PreparedStatement statement=con.prepareStatement("select k from "+catalogTable); + final ResultSet rs=executeResultSet(statement)) { + while (rs.next()) { + final String name=new String(db2real(rs.getBytes("k")), StandardCharsets.UTF_8); + try { + trees.add(TreeName.valueOf(name)); + } catch (RuntimeException e) { // reported rather than passed off as a backend with fewer trees + logger.warn(LocalizableMessage.raw("jdbc: table %s holds \"%s\", which is not the name of a tree: skipped", + catalogTable, name)); + } + } + } + return trees; } private final class ImporterImpl implements Importer { 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 b650301554..8ee419a9d9 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 @@ -45,6 +45,7 @@ import java.util.Collections; import java.util.List; import java.util.NoSuchElementException; +import java.util.Set; import java.util.concurrent.Callable; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; @@ -117,12 +118,33 @@ protected Backend createBackend() { @Override protected JDBCBackendCfg createBackendCfg() { + return createBackendCfg(getBackendId()); + } + + /** + * A configuration of another backend on the database of this suite: backends sharing one database + * URL is a configuration nothing forbids, and what one of them clears must be its own tables. + */ + protected JDBCBackendCfg createBackendCfg(String backendId) { JDBCBackendCfg backendCfg = mockCfg(JDBCBackendCfg.class); - when(backendCfg.getBackendId()).thenReturn(getBackendId()); + when(backendCfg.getBackendId()).thenReturn(backendId); when(backendCfg.getDBDirectory()).thenReturn(getJdbcUrl()); return backendCfg; } + /** Asked of the database itself, by listing its tables, so that no folding rule of the backend is trusted here. */ + private boolean isExistsTable(String tableName) throws SQLException { + try (final Connection con = DriverManager.getConnection(getJdbcUrl()); + final ResultSet rs = con.getMetaData().getTables(null, null, null, new String[]{"TABLE"})) { + while (rs.next()) { + if (tableName.equalsIgnoreCase(rs.getString("TABLE_NAME"))) { + return true; + } + } + } + return false; + } + @AfterClass @Override public void cleanUp() throws Exception { @@ -1316,4 +1338,94 @@ public void run(WriteableTransaction txn) throws Exception { storage.close(); } } + + /** + * removeStorageFiles() has to clear a backend this process has never opened: offline import-ldif + * configures the backend and calls it before anything opens the root container, so answering from + * the trees this process happens to have touched dropped nothing at all - an offline + * "import-ldif --clearBackend" cleared a JDBC backend of nothing (#888). + */ + @Test + public void testABackendIsClearedByAProcessThatNeverOpenedIt() throws Exception { + final TreeName tree = new TreeName("testOfflineClear", "tree"); + final TreeName neighbourTree = new TreeName("testOfflineClearNeighbour", "tree"); + final JDBCStorage storage = new JDBCStorage(createBackendCfg(getBackendId() + "_cleared"), null); + final JDBCStorage neighbour = new JDBCStorage(createBackendCfg(getBackendId() + "_neighbour"), null); + try { + storage.open(AccessMode.READ_WRITE); + storage.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + txn.openTree(tree, true); + txn.put(tree, key(1), value(1)); + } + }); + neighbour.open(AccessMode.READ_WRITE); + neighbour.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + txn.openTree(neighbourTree, true); + txn.put(neighbourTree, key(1), value(1)); + } + }); + } finally { + storage.close(); + neighbour.close(); + } + + // configured and never opened, nothing touched: what BackendImpl.importLDIF holds offline + final JDBCStorage offline = new JDBCStorage(createBackendCfg(getBackendId() + "_cleared"), null); + assertTrue(offline.listTrees().contains(tree), + "the tree of a backend this process never opened has to be named by its catalog"); + + offline.removeStorageFiles(); + + assertFalse(isExistsTable(offline.getTableName(tree)), "the table of the tree survived the clear"); + assertFalse(isExistsTable(offline.getTableName(offline.getCatalogTree())), "the catalog survived the clear"); + assertTrue(offline.listTrees().isEmpty(), "a cleared backend still names trees"); + // the neighbour is named by a catalog of its own: what one backend clears is never another's + assertTrue(isExistsTable(neighbour.getTableName(neighbourTree)), + "the clear of one backend dropped the table of another backend of the same database"); + neighbour.removeStorageFiles(); + } + + /** + * A dropped tree has to leave the catalog together with its table: a row outliving its table + * would make backendstat name a tree that is not there, and would put a table that is already + * gone up for removal (#888). + */ + @Test + public void testADeletedTreeIsNoLongerNamedByTheCatalog() throws Exception { + final TreeName kept = new TreeName("testCatalogDelete", "kept"); + final TreeName dropped = new TreeName("testCatalogDelete", "dropped"); + final JDBCStorage storage = new JDBCStorage(createBackendCfg(getBackendId() + "_deleted"), null); + try { + storage.open(AccessMode.READ_WRITE); + storage.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + txn.openTree(kept, true); + txn.openTree(dropped, true); + } + }); + final Set opened = storage.listTrees(); + assertTrue(opened.contains(kept) && opened.contains(dropped), "an opened tree is not named by the catalog"); + + storage.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + txn.deleteTree(dropped); + } + }); + + final Set remaining = storage.listTrees(); + assertTrue(remaining.contains(kept), "the catalog forgot a tree that is still there"); + assertFalse(remaining.contains(dropped), "the catalog still names a tree that was deleted"); + // and the removal that follows must not stumble over the tree it no longer names + storage.removeStorageFiles(); + assertFalse(isExistsTable(storage.getTableName(kept)), "the table of the tree survived the clear"); + } finally { + storage.close(); + } + } } From e49fce21fb6d5dd5b4cea1c57cb36f41996d3b25 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Fri, 21 Aug 2026 11:09:51 +0300 Subject: [PATCH 2/3] [#888] Take a tree out of the catalog in the commit that drops its table The catalog row of a deleted tree was left to the enclosing transaction while the drop committed at once, so a terminal failure later in that transaction rolled the row back over a table already gone, and nothing ever put it right - a deleted tree is not opened again. It is taken out before the drop now, so the commit of the DDL carries it. openTree writes its row before the create table for the same reason the other way round: a table nothing names is adopted with its stale rows by the next open and is dropped by no clear. The uncatalogued table report was unreachable past the first read-write open, listTrees() having added the catalog itself unconditionally. It runs after every clear now, counting the opendj tables of the connection's own catalog and schema that no catalog of this backend names; the shared compressed schema pair is left out of that count, being kept on purpose. listTrees() no longer answers the removal. catalogTables() does, from the rows alone, taking each table name from its row rather than recomputing it, while listTrees() adds the shared compressed schema trees whose tables are there, so that dbtest names them again as it did before the catalog. A catalog row whose table is gone is logged where it was skipped in silence, and so is a clear which dropped nothing while there was something to drop - that silence was the whole of #888. Three cases cover the guards nothing covered: the skip of a row whose table was dropped behind the catalog's back, openTree(tree, false) enrolling nothing, and the shared pair being named but never cleared. Both existing cases clear their backends in a finally. --- .../server/backends/jdbc/JDBCStorage.java | 250 +++++++++++++----- .../opends/server/backends/jdbc/TestCase.java | 169 +++++++++++- 2 files changed, 336 insertions(+), 83 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 6deb1e053a..eaf72c6729 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 @@ -210,6 +210,16 @@ String getTableName(TreeName treeName) { */ static final String SHARED_COMPRESSED_SCHEMA_BASE_DN="compressed_schema"; + /** + * The pair named under {@link #SHARED_COMPRESSED_SCHEMA_BASE_DN}, spelled out here because the + * names are private to {@code PersistentCompressedSchema}. They are never enrolled, so nothing + * but this constant can name them - and a tool asking a backend what trees it holds has to be + * told about them all the same, which is what {@link #listTrees()} uses this for. + */ + static final List SHARED_COMPRESSED_SCHEMA_TREES=Collections.unmodifiableList(Arrays.asList( + new TreeName(SHARED_COMPRESSED_SCHEMA_BASE_DN, "compressed_attributes"), + new TreeName(SHARED_COMPRESSED_SCHEMA_BASE_DN, "compressed_object_classes"))); + /** * The tree naming the trees this backend owns: one row per tree, the tree name as its key and the * table holding that tree as its value. @@ -852,33 +862,41 @@ public void removeStorageFiles() throws StorageRuntimeException { } } try (final Connection con = getConnection()) { - final Set trees=listTrees(con); - if (trees.isEmpty()) { - reportUncataloguedTables(con); - } else { - try { - for (final TreeName treeName : catalogDroppedLast(trees)) { - final String tableName=getTableName(treeName); - if (!isExistsTable(con, tableName)) { // a row of the catalog outliving its table - continue; - } - try (final PreparedStatement statement = con.prepareStatement("drop table " + tableName)) { - execute(statement); - } + // the catalog names what this backend owns, and only that: listTrees() also names the + // shared compressed schema trees, which another backend of this database may be the only + // owner of and which a clear must therefore leave exactly where they lie (#881) + final Map trees=catalogTables(con); + int dropped=0; + int missing=0; + try { + for (final Map.Entry tree : trees.entrySet()) { + final String tableName=tree.getValue(); + if (!isExistsTable(con, tableName)) { // a row of the catalog outliving its table + logger.warn(LocalizableMessage.raw( + "jdbc: backend %s names tree %s, whose table %s is not there: nothing to drop for it", + config.getBackendId(), tree.getKey(), tableName)); + missing++; + continue; } - con.commit(); - } catch (SQLException e) { - try { - con.rollback(); - } catch (SQLException e2) {} - throw new StorageRuntimeException(e); - } - // all tables are gone: forget the mappings so listTrees() consumers skip the dropped trees - for (final TreeName treeName : trees) { - tree2table.invalidate(treeName); - unstampableTrees.remove(treeName); // a table recreated later deserves a fresh stamp attempt + try (final PreparedStatement statement = con.prepareStatement("drop table " + tableName)) { + execute(statement); + } + dropped++; } + con.commit(); + } catch (SQLException e) { + try { + con.rollback(); + } catch (SQLException e2) {} + throw new StorageRuntimeException(e); + } + // all tables are gone: a table recreated later deserves a fresh stamp attempt, and the + // memoized table name of a tree nothing holds any more is of no use to anyone + for (final TreeName treeName : trees.keySet()) { + tree2table.invalidate(treeName); + unstampableTrees.remove(treeName); } + reportClearOutcome(con, dropped, missing); } catch (StorageRuntimeException e) { throw e; } catch (Exception e) { @@ -895,52 +913,93 @@ public void removeStorageFiles() throws StorageRuntimeException { } /** - * The trees to remove, the catalog last: it names what is still to be dropped, so a removal that - * fails halfway - dropping a table is DDL, which mysql and oracle commit as they go - is finished - * by the next attempt rather than leaving behind tables nothing names any more. - */ - private List catalogDroppedLast(Set trees) { - final TreeName catalog=getCatalogTree(); - final List ordered=new ArrayList<>(trees.size()); - for (final TreeName treeName : trees) { - if (!catalog.equals(treeName)) { - ordered.add(treeName); - } - } - if (trees.contains(catalog)) { - ordered.add(catalog); - } - return ordered; - } - - /** - * Reports the tables of a backend whose catalog is not there yet. They are reported rather than - * dropped: a table is named after the hash of its tree name, so nothing about a table found under - * the "opendj_" prefix says which backend of a shared database (#873) it belongs to. The catalog - * is written as the trees of a backend are opened, so the first read-write open after an upgrade - * to a version keeping one is enough to make the next clear complete. + * Reports what a clear did not remove, once everything the catalog named is gone. + *

+ * An "opendj" table still standing at that point is named by no catalog of this backend. It is + * reported rather than dropped because nothing about it says whose it is: a table is named after + * the hash of its tree name, so it may as easily hold a tree of a backend sharing this database + * (#873) as be a leftover of a version keeping no catalog at all, or of a tree dropped from the + * configuration while this backend was disabled. Enrolment covers the trees the configuration + * opens read-write, and those alone, so a table of neither kind is never adopted by a catalog and + * has to be removed by hand. The shared compressed schema pair is left out of the count: it is + * kept on purpose, and naming it here would be asking for the removal of the one thing this code + * goes out of its way to spare. + *

+ * A clear which dropped nothing at all is called out on top of that: #888 was exactly such a + * clear, and it went by without a word in the log. */ - private void reportUncataloguedTables(Connection con) { - int count=0; + private void reportClearOutcome(Connection con, int dropped, int missing) { + // scoped to the catalog and the schema of this connection: asked with a null catalog, + // Connector/J 8 answers for every database of the server, and a count spanning the server + // says nothing whatsoever about this backend + final String catalog=catalogOf(con); + final String schema=schemaOf(con); + // the shared compressed schema pair is left standing on purpose, so it is no leftover and + // reporting it after every clear would be telling an administrator to remove the one thing + // this code goes out of its way to keep + final Set leftOnPurpose=new HashSet<>(); + for (final TreeName treeName : SHARED_COMPRESSED_SCHEMA_TREES) { + leftOnPurpose.add(getTableName(treeName).toLowerCase()); + } + int leftBehind=0; try { final DatabaseMetaData metaData=con.getMetaData(); - try (final ResultSet rs=metaData.getTables(null, null, + try (final ResultSet rs=metaData.getTables(catalog, schema, storedIdentifier(metaData, "opendj%"), new String[]{"TABLE"})) { while (rs.next()) { - count++; + if (!leftOnPurpose.contains(String.valueOf(rs.getString("TABLE_NAME")).toLowerCase())) { + leftBehind++; + } } } } catch (SQLException e) { - logger.trace(LocalizableMessage.raw("jdbc: unable to look for the tables of an uncatalogued backend: %s", + logger.trace(LocalizableMessage.raw("jdbc: unable to look for the tables a clear left behind: %s", stackTraceToSingleLineString(e))); return; } - if (count>0) { - logger.warn(LocalizableMessage.raw("jdbc: backend %s has no tree catalog (table %s), so none of the %d opendj tables reachable on this connection could be attributed to it and nothing was cleared; the catalog is written as the trees are opened, from the first read-write open of the backend on", - config.getBackendId(), getTableName(getCatalogTree()), count)); + if (leftBehind>0) { + logger.warn(LocalizableMessage.raw("jdbc: backend %s: %d opendj table(s) of %s are named by no catalog of this backend and were left where they are; a table is named after the hash of its tree name, so nothing about one says whether it holds a tree of a backend sharing this database, or was left behind by a version keeping no tree catalog, or by a tree taken out of the configuration while this backend was disabled. A tree is enrolled as it is opened read-write, and no table is ever attributed to a backend by any other means: these have to be removed by hand", + config.getBackendId(), leftBehind, scopeName(catalog, schema))); + } + if (dropped==0 && (leftBehind>0 || missing>0)) { + logger.warn(LocalizableMessage.raw("jdbc: backend %s: the clear dropped no table at all, %d of the trees its catalog names having lost their table already", + config.getBackendId(), missing)); } } - + + /** How the catalog and the schema a table count was taken over are named in a log line. */ + private static String scopeName(String catalog, String schema) { + if (catalog!=null && schema!=null) { + return catalog+"."+schema; + } + if (catalog!=null) { + return catalog; + } + return schema!=null ? schema : "this connection"; + } + + /** + * The catalog this connection works in, or {@code null} where the driver will not say. A metadata + * pattern is narrowed by it, and a driver refusing to name it must not fail what is being + * narrowed - the unnarrowed answer is a count too wide, not a wrong one. + */ + private static String catalogOf(Connection con) { + try { + return con.getCatalog(); + } catch (Exception e) { + return null; + } + } + + /** The schema this connection works in, or {@code null} where the driver will not say; see {@link #catalogOf}. */ + private static String schemaOf(Connection con) { + try { + return con.getSchema(); + } catch (Exception e) { + return null; + } + } + //operation /** * {@inheritDoc} @@ -1208,6 +1267,13 @@ String getTableDialect() { @Override public void openTree(TreeName treeName, boolean createOnDemand) { if (createOnDemand) { + // what makes this tree nameable by a process which has opened nothing: see + // getCatalogTree(). Written before the table and not after it, so that the commit of + // the "create table" below carries the row with it: of the two ways a half-done open + // can end, a catalog naming a table that is not there is the one the removal is ready + // for - it skips such a row and says so - while a table nothing names is adopted with + // its stale rows by the next open of that tree and is dropped by no clear ever after + enrolInCatalog(treeName); if (!isExistsTable(treeName)) { try (final PreparedStatement statement=con.prepareStatement("create table "+getTableName(treeName)+" ("+getTableDialect()+")")){ execute(statement); @@ -1254,8 +1320,6 @@ public void openTree(TreeName treeName, boolean createOnDemand) { // the dialect is taken off this transaction's own connection: finding it out must // not cost a borrow from a pool this thread is already holding a connection of commentTable(treeName, dialectOf(con), stampSession); - // what makes this tree nameable by a process which has opened nothing: see getCatalogTree() - enrolInCatalog(treeName); } } @@ -1273,7 +1337,7 @@ public void openTree(TreeName treeName, boolean createOnDemand) { void enrolInCatalog(TreeName treeName) { final TreeName catalog=getCatalogTree(); if (catalog.equals(treeName)) { - return; // the catalog holds no row of its own: listTrees() adds it when its table is there + return; // the catalog holds no row of its own: catalogTables() adds it when its table is there } if (SHARED_COMPRESSED_SCHEMA_BASE_DN.equals(treeName.getBaseDN())) { return; // a tree this backend may not be the only owner of: see the constant @@ -1345,6 +1409,16 @@ public void clearTree(TreeName treeName) { @Override public void deleteTree(TreeName treeName) { + // Taken out of the catalog before the table is dropped, so that the commit of the "drop + // table" below carries the removal of the row with it. Left until after it, the row would + // be the only part of this still owed to the enclosing transaction, and a terminal failure + // later in that transaction - write() rolls back everything but a class 40 conflict, which + // alone it replays - would roll the row back over a table that is already gone. Nothing + // would ever put it right: a deleted tree is not opened again, so no enrolment and no + // unenrolment reaches it a second time, and the catalog would name a tree that is not + // there for good. Should the drop fail instead, the row stays pending and goes back with + // the transaction, leaving the tree named and its table standing - both still there. + unenrolFromCatalog(treeName); if (isExistsTable(treeName)) { try (final PreparedStatement statement = con.prepareStatement("drop table " + getTableName(treeName))) { execute(statement); @@ -1353,8 +1427,7 @@ public void deleteTree(TreeName treeName) { throw new StorageRuntimeException(e); } } - unenrolFromCatalog(treeName); - // forget the mapping so listTrees() consumers (updateTableStatistics) skip the dropped table + // the memoized table name of a tree nothing holds any more is of no use to anyone tree2table.invalidate(treeName); unstampableTrees.remove(treeName); // a table recreated later deserves a fresh stamp attempt } @@ -1657,6 +1730,11 @@ public boolean positionToIndex(int index) { *

* Answered from the catalog of the backend rather than from the trees this process happens to * have touched: {@link #removeStorageFiles()} runs before anything has touched one (#888). + *

+ * What a tool has to be shown is not what a clear may drop: the shared compressed schema trees + * are deliberately not enrolled - a backend must not offer a tree another one may own for removal + * - and would go unnamed by {@code dbtest} for it, so they are added here when their tables are + * there. {@link #catalogTables(Connection)} is what the removal reads, and it names them not. */ @Override public Set listTrees() { @@ -1670,25 +1748,57 @@ public Set listTrees() { } Set listTrees(Connection con) throws SQLException { + final Set trees=new HashSet<>(catalogTables(con).keySet()); + for (final TreeName treeName : SHARED_COMPRESSED_SCHEMA_TREES) { + // asked of the database, not assumed: the pair belongs to no backend in particular, and + // once #881 gives each backend a pair of its own an installation may hold neither table + if (isExistsTable(con, getTableName(treeName))) { + trees.add(treeName); + } + } + return trees; + } + + /** + * The trees the catalog of this backend names, each with the table recorded as holding it, the + * catalog itself among them. Empty when the catalog table is not there - a backend which has + * never been opened read-write - which is what tells {@link #removeStorageFiles()} it has nothing + * it may drop. + *

+ * The table name is taken from the row rather than recomputed from the tree name, so that a + * removal drops what was enrolled even if the naming of tables were ever to change. + */ + Map catalogTables(Connection con) throws SQLException { final TreeName catalog=getCatalogTree(); final String catalogTable=getTableName(catalog); - if (!isExistsTable(con, catalogTable)) { // a backend which has never been opened read-write - return Collections.emptySet(); + if (!isExistsTable(con, catalogTable)) { + return Collections.emptyMap(); } - final Set trees=new HashSet<>(); - trees.add(catalog); // the catalog names every tree of the backend but itself - try (final PreparedStatement statement=con.prepareStatement("select k from "+catalogTable); + final Map trees=new LinkedHashMap<>(); + try (final PreparedStatement statement=con.prepareStatement("select k,v from "+catalogTable); final ResultSet rs=executeResultSet(statement)) { while (rs.next()) { final String name=new String(db2real(rs.getBytes("k")), StandardCharsets.UTF_8); + final TreeName treeName; try { - trees.add(TreeName.valueOf(name)); + treeName=TreeName.valueOf(name); } catch (RuntimeException e) { // reported rather than passed off as a backend with fewer trees logger.warn(LocalizableMessage.raw("jdbc: table %s holds \"%s\", which is not the name of a tree: skipped", catalogTable, name)); + continue; } - } - } + final byte[] table=rs.getBytes("v"); + trees.put(treeName, table==null || table.length==0 + ? getTableName(treeName) // a row of a version which recorded the name and not the table + : new String(table, StandardCharsets.UTF_8)); + } + } + // The catalog names every tree of the backend but itself, and is put last on purpose: the + // removal drops the trees in this order, and what names them has to outlive them. Dropping a + // table is DDL, which mysql and oracle commit as they go, so a removal that fails halfway is + // finished by the next attempt rather than leaving behind tables nothing names any more. + trees.remove(catalog); // no row should name it; one that does must not hold back the order + trees.put(catalog, catalogTable); return trees; } 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 8ee419a9d9..c39e04c63e 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 @@ -145,6 +145,24 @@ private boolean isExistsTable(String tableName) throws SQLException { return false; } + /** Drops a table behind the back of the storage that owns it, which no code path of the backend does. */ + private void dropTableBehindTheBackend(String tableName) throws SQLException { + try (final Connection con = DriverManager.getConnection(getJdbcUrl()); + final Statement st = con.createStatement()) { + st.execute("drop table " + tableName); + } + } + + /** Clears a backend of a test without letting the failure of the clear replace the failure being reported. */ + private static void clearQuietly(JDBCStorage storage) { + try { + storage.removeStorageFiles(); + } catch (Exception ignored) { + } finally { + storage.close(); + } + } + @AfterClass @Override public void cleanUp() throws Exception { @@ -1375,18 +1393,26 @@ public void run(WriteableTransaction txn) throws Exception { // configured and never opened, nothing touched: what BackendImpl.importLDIF holds offline final JDBCStorage offline = new JDBCStorage(createBackendCfg(getBackendId() + "_cleared"), null); - assertTrue(offline.listTrees().contains(tree), - "the tree of a backend this process never opened has to be named by its catalog"); - - offline.removeStorageFiles(); - - assertFalse(isExistsTable(offline.getTableName(tree)), "the table of the tree survived the clear"); - assertFalse(isExistsTable(offline.getTableName(offline.getCatalogTree())), "the catalog survived the clear"); - assertTrue(offline.listTrees().isEmpty(), "a cleared backend still names trees"); - // the neighbour is named by a catalog of its own: what one backend clears is never another's - assertTrue(isExistsTable(neighbour.getTableName(neighbourTree)), - "the clear of one backend dropped the table of another backend of the same database"); - neighbour.removeStorageFiles(); + try { + assertTrue(offline.listTrees().contains(tree), + "the tree of a backend this process never opened has to be named by its catalog"); + + offline.removeStorageFiles(); + + assertFalse(isExistsTable(offline.getTableName(tree)), "the table of the tree survived the clear"); + assertFalse(isExistsTable(offline.getTableName(offline.getCatalogTree())), "the catalog survived the clear"); + final Set cleared = offline.listTrees(); + assertFalse(cleared.contains(tree), "a cleared backend still names its tree"); + assertFalse(cleared.contains(offline.getCatalogTree()), "a cleared backend still names its catalog"); + // the neighbour is named by a catalog of its own: what one backend clears is never another's + assertTrue(isExistsTable(neighbour.getTableName(neighbourTree)), + "the clear of one backend dropped the table of another backend of the same database"); + } finally { + // in a finally of their own: a failed assertion above must not leave the tables of either + // backend behind for the rest of the class, which nothing but @BeforeClass ever drops + clearQuietly(neighbour); + clearQuietly(offline); + } } /** @@ -1425,7 +1451,124 @@ public void run(WriteableTransaction txn) throws Exception { storage.removeStorageFiles(); assertFalse(isExistsTable(storage.getTableName(kept)), "the table of the tree survived the clear"); } finally { - storage.close(); + clearQuietly(storage); + } + } + + /** + * 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 + * from a backup older than the database, leaves exactly this (#888). + */ + @Test + public void testAClearSkipsACatalogRowWhoseTableIsGone() throws Exception { + final TreeName kept = new TreeName("testStaleCatalogRow", "kept"); + final TreeName vanished = new TreeName("testStaleCatalogRow", "vanished"); + final JDBCStorage storage = new JDBCStorage(createBackendCfg(getBackendId() + "_stale"), null); + try { + storage.open(AccessMode.READ_WRITE); + storage.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + txn.openTree(kept, true); + txn.openTree(vanished, true); + } + }); + dropTableBehindTheBackend(storage.getTableName(vanished)); + assertTrue(storage.listTrees().contains(vanished), + "the catalog was expected to go on naming the tree whose table was dropped behind its back"); + + storage.removeStorageFiles(); + + assertFalse(isExistsTable(storage.getTableName(kept)), + "a row of the catalog whose table is gone stopped the clear dropping the rest"); + assertFalse(isExistsTable(storage.getTableName(storage.getCatalogTree())), "the catalog survived the clear"); + } finally { + clearQuietly(storage); + } + } + + /** + * Naming a tree in order to read it must never put it up for removal: the tree read may be held + * by another backend of the same database, which nothing forbids (#873). Only + * openTree(createOnDemand) enrols. + */ + @Test + public void testReadingATreeDoesNotPutItUpForRemoval() throws Exception { + final TreeName owned = new TreeName("testReadDoesNotEnrol", "owned"); + final TreeName foreign = new TreeName("testReadDoesNotEnrolForeign", "tree"); + final JDBCStorage storage = new JDBCStorage(createBackendCfg(getBackendId() + "_reader"), null); + final JDBCStorage owner = new JDBCStorage(createBackendCfg(getBackendId() + "_owner"), null); + try { + owner.open(AccessMode.READ_WRITE); + owner.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + txn.openTree(foreign, true); + txn.put(foreign, key(1), value(1)); + } + }); + + storage.open(AccessMode.READ_WRITE); + storage.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + txn.openTree(owned, true); // the catalog of this backend comes into being here + txn.openTree(foreign, false); // read, not owned + } + }); + assertEquals(storage.read(new ReadOperation() { + @Override + public ByteString run(ReadableTransaction txn) throws Exception { + return txn.read(foreign, key(1)); + } + }), value(1), "the tree of the other backend could not be read"); + + assertFalse(storage.listTrees().contains(foreign), "reading a tree enrolled it in the catalog"); + storage.removeStorageFiles(); + assertTrue(isExistsTable(owner.getTableName(foreign)), + "the clear dropped a tree this backend had only read"); + assertFalse(isExistsTable(storage.getTableName(owned)), "the table of the backend's own tree survived the clear"); + } finally { + clearQuietly(owner); + clearQuietly(storage); + } + } + + /** + * The compressed schema trees named from a literal carry no backend qualifier, so on a database + * addressed by several backends they are the same pair for all of them: a clear must leave them + * where they lie (#881). A tool asking a backend what trees it holds has to be shown them all the + * same, which is what keeps them out of the catalog and inside listTrees(). + */ + @Test + public void testTheSharedCompressedSchemaTreesAreNamedButNeverCleared() throws Exception { + final TreeName shared = JDBCStorage.SHARED_COMPRESSED_SCHEMA_TREES.get(0); + final TreeName owned = new TreeName("testSharedCompressedSchema", "owned"); + final JDBCStorage storage = new JDBCStorage(createBackendCfg(getBackendId() + "_schema"), null); + try { + storage.open(AccessMode.READ_WRITE); + storage.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + txn.openTree(owned, true); + // created, never written to: the pair holds the compressed schema of this very + // database, and a row of a test in it would be read back as a schema definition + txn.openTree(shared, true); + } + }); + assertTrue(storage.listTrees().contains(shared), + "a tool asking this backend for its trees was not shown the compressed schema tree"); + + storage.removeStorageFiles(); + + assertTrue(isExistsTable(storage.getTableName(shared)), + "the clear dropped a compressed schema tree another backend of this database may be the only owner of"); + assertFalse(isExistsTable(storage.getTableName(owned)), "the table of the backend's own tree survived the clear"); + } finally { + // the shared pair is left where it lies, exactly as the backend leaves it + clearQuietly(storage); } } } From 87d3bb5f4c799ac6d4730e874e2af704a2d09394 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Fri, 21 Aug 2026 15:46:55 +0300 Subject: [PATCH 3/3] [#888] Tell what a clear left standing apart by its tree stamp, and unenrol after the drop deleteTree() took a tree out of the catalog before dropping its table, so that the commit of the DDL would carry the row with it. On mysql and oracle DDL commits the transaction it finds open before it executes, so there the delete landed first and a drop that then failed - ORA-00054 on a tree another session holds, which write() rethrows unreplayed, it being neither a class 40 state nor ORA-00060 - left a table nothing names, adopted with its stale rows by the next open of that tree and dropped by no clear ever after. The drop goes first now and the row is taken out after it, carrying a commit of its own so that the enclosing transaction can no longer roll it back over a table already gone; the invariant holds on all four engines that way. openTree() goes on writing its row before its table, which is the same rule read the other way round, and the comments say so instead of claiming a symmetry that would be wrong. The report of what a clear did not drop counted every opendj table of the connection and asked for all of them to be removed by hand - the live tables of a backend sharing the database (#873) included, which the case of an offline clear in this very suite reproduces. It reads the tree stamp of #866 instead: a table stamped with a tree of a base DN this backend does not serve is passed over in silence, one stamped with a tree of this backend is named as its own and so as removable by hand, and one carrying no stamp at all is reported as attributable to nobody. The catalog table is stamped like any other for that reason - a neighbouring backend's catalog is otherwise the one table such a report can attribute to no one - and the line for a clear that dropped nothing prints all three counts, where it used to print the one number that had not fired. It also says what that clear means on a backend upgraded in place: the first offline clear of one finds no catalog and drops nothing, nothing enrolling a tree before removeStorageFiles() runs. The existence lookups the removal makes are narrowed to the database and the schema of the connection. Asked with a null catalog, Connector/J answers for every database of the server, and there the answer decides between skipping a row and dropping the table it names: a table of the same name next door would turn a skip that keeps the clear going into an unqualified drop of a table that is not here, failing the whole clear on this attempt and on every one after it. Three cases added - a write() that throws after deleteTree, the table name a catalog row records and the fallback for a row recording none, and the attribution of what a clear left standing - the shared compressed schema pair is asserted for both of its trees, the setup half of the offline clear case clears what it created when it fails, and unenrolFromCatalog() got the guard enrolInCatalog() has. --- .../server/backends/jdbc/JDBCStorage.java | 275 ++++++++++++++---- .../opends/server/backends/jdbc/TestCase.java | 226 +++++++++++++- 2 files changed, 432 insertions(+), 69 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 eaf72c6729..bef15379f4 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 @@ -25,6 +25,7 @@ import org.forgerock.opendj.config.server.ConfigurationChangeListener; import org.forgerock.opendj.ldap.ByteSequence; import org.forgerock.opendj.ldap.ByteString; +import org.forgerock.opendj.ldap.DN; import org.forgerock.opendj.server.config.server.JDBCBackendCfg; import org.opends.server.backends.pluggable.spi.*; import org.opends.server.core.ServerContext; @@ -833,9 +834,26 @@ boolean updateTableStatistics(Connection con, Collection trees) { * sharing with something else. */ boolean isExistsTable(Connection con, String tableName) { + return isExistsTable(con, null, null, tableName); + } + + /** + * Whether a table of this name is there in the catalog and the schema given, which + * {@link #removeStorageFiles()} takes off the connection it works on. + *

+ * Asked with a null catalog the question spans the whole server on some drivers - Connector/J + * reads a null catalog as "any database" since 8.0, and its databaseTerm being CATALOG it + * ignores the schema pattern besides. Where the answer only decides whether a table has to be + * created that is a hazard older than this catalog, but the removal decides something else by + * it: a row whose table is gone is skipped so that the clear goes on, and a table of the same + * name in another database of the server would turn that skip into an unqualified "drop table" + * of a table that is not in this one, failing the whole clear on this attempt and on every + * attempt after it. + */ + boolean isExistsTable(Connection con, String catalog, String schema, String tableName) { try { final DatabaseMetaData metaData = con.getMetaData(); - try (final ResultSet rs = metaData.getTables(null, null, + try (final ResultSet rs = metaData.getTables(catalog, schema, storedIdentifier(metaData, tableName), new String[]{"TABLE"})) { while (rs.next()) { // the name still has to be compared: "_" is a single-character wildcard in a @@ -862,6 +880,12 @@ public void removeStorageFiles() throws StorageRuntimeException { } } try (final Connection con = getConnection()) { + // the database and the schema this connection works in, which every lookup below is + // narrowed to: the skip in the loop decides between leaving a row where it is and dropping + // the table it names, and a table of that name in another database of the server must not + // be allowed to answer for this one + final String catalog=catalogOf(con); + final String schema=schemaOf(con); // the catalog names what this backend owns, and only that: listTrees() also names the // shared compressed schema trees, which another backend of this database may be the only // owner of and which a clear must therefore leave exactly where they lie (#881) @@ -871,7 +895,7 @@ public void removeStorageFiles() throws StorageRuntimeException { try { for (final Map.Entry tree : trees.entrySet()) { final String tableName=tree.getValue(); - if (!isExistsTable(con, tableName)) { // a row of the catalog outliving its table + if (!isExistsTable(con, catalog, schema, tableName)) { // a row of the catalog outliving its table logger.warn(LocalizableMessage.raw( "jdbc: backend %s names tree %s, whose table %s is not there: nothing to drop for it", config.getBackendId(), tree.getKey(), tableName)); @@ -896,7 +920,7 @@ public void removeStorageFiles() throws StorageRuntimeException { tree2table.invalidate(treeName); unstampableTrees.remove(treeName); } - reportClearOutcome(con, dropped, missing); + reportClearOutcome(con, catalog, schema, dropped, missing); } catch (StorageRuntimeException e) { throw e; } catch (Exception e) { @@ -915,56 +939,140 @@ public void removeStorageFiles() throws StorageRuntimeException { /** * Reports what a clear did not remove, once everything the catalog named is gone. *

- * An "opendj" table still standing at that point is named by no catalog of this backend. It is - * reported rather than dropped because nothing about it says whose it is: a table is named after - * the hash of its tree name, so it may as easily hold a tree of a backend sharing this database - * (#873) as be a leftover of a version keeping no catalog at all, or of a tree dropped from the - * configuration while this backend was disabled. Enrolment covers the trees the configuration - * opens read-write, and those alone, so a table of neither kind is never adopted by a catalog and - * has to be removed by hand. The shared compressed schema pair is left out of the count: it is - * kept on purpose, and naming it here would be asking for the removal of the one thing this code - * goes out of its way to spare. + * An "opendj" table still standing at that point is named by no catalog of this backend, and its + * name says nothing about whose it is - a table is named after the hash of its tree name. What + * does say so is the comment a table is stamped with as it is opened (#866): the tree name in + * plain text. A table whose stamp names a tree of a base DN this backend does not serve belongs to + * a backend sharing this database (#873) and is passed over in silence; one whose stamp names a + * tree of this backend is reported as its own, and so as removable by hand; one carrying no stamp + * at all - left by a version stamping no table, or by a database that refused the comment - can be + * attributed to nobody and is reported as exactly that. + *

+ * The shared compressed schema pair is left out of all of it: it is kept on purpose (#881), so it + * is no leftover of anything, and naming it here would be asking for the removal of the one thing + * this code goes out of its way to spare. *

* A clear which dropped nothing at all is called out on top of that: #888 was exactly such a - * clear, and it went by without a word in the log. + * clear, and it went by without a word in the log. A backend upgraded in place is the one case + * where a clear drops nothing while there is something to drop - nothing enrols a tree before + * {@link #removeStorageFiles()} runs, so the first offline clear of such a backend finds an empty + * catalog - and the line says so rather than leaving it to be found out. */ - private void reportClearOutcome(Connection con, int dropped, int missing) { - // scoped to the catalog and the schema of this connection: asked with a null catalog, - // Connector/J 8 answers for every database of the server, and a count spanning the server - // says nothing whatsoever about this backend - final String catalog=catalogOf(con); - final String schema=schemaOf(con); - // the shared compressed schema pair is left standing on purpose, so it is no leftover and - // reporting it after every clear would be telling an administrator to remove the one thing - // this code goes out of its way to keep + void reportClearOutcome(Connection con, String catalog, String schema, int dropped, int missing) { + final ClearLeftovers leftovers=leftoverTables(con, catalog, schema); + if (leftovers==null) { // the database would not say what is still standing: nothing to report + return; + } + if (!leftovers.ours.isEmpty()) { + logger.warn(LocalizableMessage.raw("jdbc: backend %s: %d table(s) of %s hold trees of this backend that its catalog does not name, and the clear left them where they are: %s. A tree is enrolled as it is opened read-write and by no other means, so such a table is one of a tree taken out of the configuration while the backend was disabled, or one left by a version keeping no catalog: it is this backend's own and can be removed by hand, and re-adding the base DN or the index it belongs to adopts it with the rows it still holds", + config.getBackendId(), leftovers.ours.size(), scopeName(catalog, schema), leftovers.ours)); + } + if (!leftovers.unattributed.isEmpty()) { + logger.warn(LocalizableMessage.raw("jdbc: backend %s: %d opendj table(s) of %s are named by no catalog of this backend and carry no tree stamp, so nothing says whose they are: %s. They may hold the trees of a backend sharing this database, which nothing forbids, or be leftovers of a version stamping no table at all - a table is named after the hash of its tree name and can be attributed by no other means. They were left exactly where they are", + config.getBackendId(), leftovers.unattributed.size(), scopeName(catalog, schema), leftovers.unattributed)); + } + if (dropped==0 && (missing>0 || !leftovers.ours.isEmpty() || !leftovers.unattributed.isEmpty())) { + logger.warn(LocalizableMessage.raw("jdbc: backend %s: the clear dropped no table at all: %d of the trees its catalog names had lost their table already, %d table(s) of this backend were named by no catalog, and %d could not be attributed to anyone. A backend upgraded from a version keeping no catalog has to be started once before its first offline \"import-ldif --clearBackend\": nothing enrols a tree before the clear runs, so that first clear finds a catalog that is not there and names nothing", + config.getBackendId(), missing, leftovers.ours.size(), leftovers.unattributed.size())); + } + } + + /** What a clear left standing, told apart by the tree stamp of each table; see {@link #reportClearOutcome}. */ + static final class ClearLeftovers { + /** Tables whose stamp names a tree of this backend: its own, and removable by hand. */ + final List ours=new ArrayList<>(); + /** Tables carrying no stamp naming a tree: they can be attributed to nobody. */ + final List unattributed=new ArrayList<>(); + } + + /** + * The "opendj" tables of the given catalog and schema that this backend can say something about, + * or {@code null} where the database would not list them. A table stamped with a tree of another + * backend is in neither list: it is that backend's business and no part of this clear's outcome. + */ + ClearLeftovers leftoverTables(Connection con, String catalog, String schema) { + // the shared compressed schema pair is left standing on purpose, so it is no leftover of + // anything and reporting it would be pointing at the one thing this code goes out of its way + // to keep. Taken out by name and not by stamp: an installation may hold the pair unstamped, + // from a version that commented no table at all. final Set leftOnPurpose=new HashSet<>(); for (final TreeName treeName : SHARED_COMPRESSED_SCHEMA_TREES) { leftOnPurpose.add(getTableName(treeName).toLowerCase()); } - int leftBehind=0; + final ClearLeftovers leftovers=new ClearLeftovers(); try { + final List standing=new ArrayList<>(); final DatabaseMetaData metaData=con.getMetaData(); try (final ResultSet rs=metaData.getTables(catalog, schema, storedIdentifier(metaData, "opendj%"), new String[]{"TABLE"})) { while (rs.next()) { - if (!leftOnPurpose.contains(String.valueOf(rs.getString("TABLE_NAME")).toLowerCase())) { - leftBehind++; + final String tableName=String.valueOf(rs.getString("TABLE_NAME")); + if (!leftOnPurpose.contains(tableName.toLowerCase())) { + standing.add(tableName); } } } + // the stamps are read once the metadata result set is closed: they are queries of this very + // connection, and a driver may hold it for the whole of that result set + final Dialect dialect=dialectOf(con); + for (final String tableName : standing) { + final TreeName stamp=stampedTree(con, dialect, tableName); + if (stamp==null) { + leftovers.unattributed.add(tableName); + } else if (isOwnTree(stamp)) { + leftovers.ours.add(tableName+" ("+stamp+")"); + } + } } catch (SQLException e) { logger.trace(LocalizableMessage.raw("jdbc: unable to look for the tables a clear left behind: %s", stackTraceToSingleLineString(e))); - return; + return null; + } + return leftovers; + } + + /** + * The tree named by the comment this table carries (#866), or {@code null} where it carries none, + * where what it carries is not the name of a tree, or where the engine has no comment readback of + * its own here. The stamp is the only thing that attributes a table to a backend at all - a table + * name is a bare hash - and stamping is best-effort, so the absence of one states nothing. + */ + private TreeName stampedTree(Connection con, Dialect dialect, String tableName) { + if (dialect==null) { // no readback known for this engine: no table of it can be attributed + return null; + } + try { + final String comment=readStoredComment(con, dialect, tableName); + if (comment==null || comment.isEmpty()) { + return null; + } + return TreeName.valueOf(comment); + } catch (SQLException | RuntimeException e) { // a comment of somebody else's making, or a read that failed + return null; + } + } + + /** + * Whether this tree is one of this backend's own: a tree of a base DN it serves, or its own + * catalog. The catalog counts because a clear drops it last, so one still standing is a clear of + * this backend that did not get to the end, and never anything of anybody else's. + */ + private boolean isOwnTree(TreeName treeName) { + if (getCatalogTree().equals(treeName)) { + return true; } - if (leftBehind>0) { - logger.warn(LocalizableMessage.raw("jdbc: backend %s: %d opendj table(s) of %s are named by no catalog of this backend and were left where they are; a table is named after the hash of its tree name, so nothing about one says whether it holds a tree of a backend sharing this database, or was left behind by a version keeping no tree catalog, or by a tree taken out of the configuration while this backend was disabled. A tree is enrolled as it is opened read-write, and no table is ever attributed to a backend by any other means: these have to be removed by hand", - config.getBackendId(), leftBehind, scopeName(catalog, schema))); + final SortedSet baseDNs=config.getBaseDN(); + if (baseDNs==null) { + return false; } - if (dropped==0 && (leftBehind>0 || missing>0)) { - logger.warn(LocalizableMessage.raw("jdbc: backend %s: the clear dropped no table at all, %d of the trees its catalog names having lost their table already", - config.getBackendId(), missing)); + for (final DN baseDN : baseDNs) { + // every tree of an entry container is named after the normalized form of its base DN, + // which is what EntryContainer builds its tree names from + if (treeName.getBaseDN().equals(baseDN.toNormalizedUrlSafeString())) { + return true; + } } + return false; } /** How the catalog and the schema a table count was taken over are named in a log line. */ @@ -985,7 +1093,10 @@ private static String scopeName(String catalog, String schema) { */ private static String catalogOf(Connection con) { try { - return con.getCatalog(); + // an empty name is not the name of a catalog but a driver's way of saying it has none, and + // passed to a metadata pattern it means "tables that belong to no catalog" - which is not + // the same question and would answer nothing + return emptyToNull(con.getCatalog()); } catch (Exception e) { return null; } @@ -994,12 +1105,17 @@ private static String catalogOf(Connection con) { /** The schema this connection works in, or {@code null} where the driver will not say; see {@link #catalogOf}. */ private static String schemaOf(Connection con) { try { - return con.getSchema(); + return emptyToNull(con.getSchema()); } catch (Exception e) { return null; } } + /** An empty name is the name of nothing: see {@link #catalogOf}. */ + private static String emptyToNull(String name) { + return name==null || name.isEmpty() ? null : name; + } + //operation /** * {@inheritDoc} @@ -1268,11 +1384,15 @@ String getTableDialect() { public void openTree(TreeName treeName, boolean createOnDemand) { if (createOnDemand) { // what makes this tree nameable by a process which has opened nothing: see - // getCatalogTree(). Written before the table and not after it, so that the commit of - // the "create table" below carries the row with it: of the two ways a half-done open - // can end, a catalog naming a table that is not there is the one the removal is ready - // for - it skips such a row and says so - while a table nothing names is adopted with - // its stale rows by the next open of that tree and is dropped by no clear ever after + // getCatalogTree(). Written before the table and not after it, so that the table is never + // there without a row naming it: on postgres and sql server the commit below carries row + // and table together, and on mysql and oracle the "create table" commits the row before + // it creates anything, DDL there committing the transaction it finds open. Of the two ways + // a half-done open can end, a catalog naming a table that is not there is the one the + // removal is ready for - it skips such a row and says so - while a table nothing names is + // adopted with its stale rows by the next open of that tree and is dropped by no clear + // ever after. deleteTree() takes the row out after the drop for that same reason, which is + // why it is not the mirror of this enrolInCatalog(treeName); if (!isExistsTable(treeName)) { try (final PreparedStatement statement=con.prepareStatement("create table "+getTableName(treeName)+" ("+getTableDialect()+")")){ @@ -1344,6 +1464,12 @@ void enrolInCatalog(TreeName treeName) { } if (!catalogTableOpened) { openCatalogTable(catalog); + // stamped with its tree name like any table of a tree (#866), and for a reason of its + // own: a clear reports what it did not drop, and the catalog of a backend sharing this + // database (#873) is the one table such a report could otherwise attribute to nobody. + // It costs one stamp per open of the storage, not one per tree: this runs behind the + // very flag that keeps the catalog from being opened again + commentTable(catalog, dialectOf(con), stampSession); catalogTableOpened=true; } try { @@ -1355,12 +1481,10 @@ void enrolInCatalog(TreeName treeName) { } /** - * Creates the table of the catalog when it is not there yet. It takes neither the index nor - * the comment openTree() gives a tree: the catalog is read whole and written by key, never - * iterated by key range, so the index a cursor needs would serve nothing here, and its rows - * name the trees in plain text, so a comment naming the table would only repeat what reading - * it says. Keeping it out of the comment sweep also keeps the cost of a stamp the database - * rejects where it was - one attempt per tree of the backend, not one more. + * Creates the table of the catalog when it is not there yet. It takes no index of the kind + * openTree() gives a tree: the catalog is read whole and written by key, never iterated by key + * range, so the index a cursor needs would serve nothing here. The stamp it does take is given + * by the caller, on every open rather than on creation alone; see {@link #enrolInCatalog}. */ void openCatalogTable(TreeName catalog) { if (isExistsTable(catalog)) { @@ -1374,16 +1498,27 @@ void openCatalogTable(TreeName catalog) { } } - /** Takes the tree out of the catalog: a row is what puts a table up for removal, and this one is gone. */ - void unenrolFromCatalog(TreeName treeName) { + /** + * Takes the tree out of the catalog: a row is what puts a table up for removal, and this one is + * gone. Returns whether the delete was issued, which is what {@link #deleteTree} commits. + */ + boolean unenrolFromCatalog(TreeName treeName) { final TreeName catalog=getCatalogTree(); if (catalog.equals(treeName)) { catalogTableOpened=false; // its own table is gone: the next enrolment creates it again - return; + return false; + } + if (SHARED_COMPRESSED_SCHEMA_BASE_DN.equals(treeName.getBaseDN())) { + // the symmetry of enrolInCatalog() and nothing more: no row of this pair was ever written, + // so the delete would find none. What keeps the pair out of a clear is that a clear drops + // what the catalog names and the catalog does not name them; see the constant + return false; } if (catalogTableOpened || isExistsTable(catalog)) { delete(catalog, ByteString.valueOfUtf8(treeName.toString())); + return true; } + return false; } boolean isExistsIndex(String tableName, String indexName) throws SQLException { @@ -1409,16 +1544,16 @@ public void clearTree(TreeName treeName) { @Override public void deleteTree(TreeName treeName) { - // Taken out of the catalog before the table is dropped, so that the commit of the "drop - // table" below carries the removal of the row with it. Left until after it, the row would - // be the only part of this still owed to the enclosing transaction, and a terminal failure - // later in that transaction - write() rolls back everything but a class 40 conflict, which - // alone it replays - would roll the row back over a table that is already gone. Nothing - // would ever put it right: a deleted tree is not opened again, so no enrolment and no - // unenrolment reaches it a second time, and the catalog would name a tree that is not - // there for good. Should the drop fail instead, the row stays pending and goes back with - // the transaction, leaving the tree named and its table standing - both still there. - unenrolFromCatalog(treeName); + // A row is written before its table is created and taken out after its table is dropped, + // never the other way round: of the two ways a half-done change can end, a catalog naming a + // table that is not there is the one the removal is ready for - it skips such a row and says + // so - while a table nothing names is adopted with its stale rows by the next open of that + // tree and is dropped by no clear ever after. So this is deliberately not the mirror of + // openTree(): an unenrolment left pending before the drop would be committed by the drop + // itself on mysql and oracle, where DDL commits the transaction it finds open before it + // executes, and would then stand even where the drop goes on to fail - ORA-00054 on a tree + // another session holds, say, which write() does not replay, it being neither a class 40 + // state nor ORA-00060. if (isExistsTable(treeName)) { try (final PreparedStatement statement = con.prepareStatement("drop table " + getTableName(treeName))) { execute(statement); @@ -1427,6 +1562,18 @@ public void deleteTree(TreeName treeName) { throw new StorageRuntimeException(e); } } + // The row carries a commit of its own rather than being left to the enclosing transaction: + // that transaction is the last thing this delete could still be rolled back by - write() + // replays a class 40 conflict and rethrows everything else unreplayed - and the row would be + // rolled back over a table that is already gone, with nothing ever to put it right: a deleted + // tree is not opened again, so no enrolment and no unenrolment reaches it a second time. + if (unenrolFromCatalog(treeName)) { + try { + con.commit(); + } catch (SQLException e) { + throw new StorageRuntimeException(e); + } + } // the memoized table name of a tree nothing holds any more is of no use to anyone tree2table.invalidate(treeName); unstampableTrees.remove(treeName); // a table recreated later deserves a fresh stamp attempt @@ -1749,10 +1896,14 @@ public Set listTrees() { Set listTrees(Connection con) throws SQLException { final Set trees=new HashSet<>(catalogTables(con).keySet()); + final String catalog=catalogOf(con); + final String schema=schemaOf(con); for (final TreeName treeName : SHARED_COMPRESSED_SCHEMA_TREES) { // asked of the database, not assumed: the pair belongs to no backend in particular, and - // once #881 gives each backend a pair of its own an installation may hold neither table - if (isExistsTable(con, getTableName(treeName))) { + // once #881 gives each backend a pair of its own an installation may hold neither table. + // Narrowed to this database: a pair of the same name in another database of the server + // would otherwise have this backend name two trees it does not hold + if (isExistsTable(con, catalog, schema, getTableName(treeName))) { trees.add(treeName); } } @@ -1771,7 +1922,9 @@ Set listTrees(Connection con) throws SQLException { Map catalogTables(Connection con) throws SQLException { final TreeName catalog=getCatalogTree(); final String catalogTable=getTableName(catalog); - if (!isExistsTable(con, catalogTable)) { + // narrowed to this database: a catalog of the same name in another database of the server + // would send the select below at a table that is not here, failing the clear it answers + if (!isExistsTable(con, catalogOf(con), schemaOf(con), catalogTable)) { return Collections.emptyMap(); } final Map trees=new LinkedHashMap<>(); 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 c39e04c63e..3fe69eab31 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 @@ -17,6 +17,7 @@ import org.forgerock.opendj.ldap.ByteString; import org.forgerock.opendj.ldap.ByteStringBuilder; +import org.forgerock.opendj.ldap.DN; import org.forgerock.opendj.server.config.server.JDBCBackendCfg; import org.opends.server.backends.pluggable.PluggableBackendImplTestCase; import org.opends.server.backends.pluggable.spi.AccessMode; @@ -44,8 +45,10 @@ import java.util.Collection; import java.util.Collections; import java.util.List; +import java.util.Map; import java.util.NoSuchElementException; import java.util.Set; +import java.util.TreeSet; import java.util.concurrent.Callable; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; @@ -57,6 +60,7 @@ import static org.testng.Assert.assertEquals; import static org.testng.Assert.assertFalse; import static org.testng.Assert.assertNotEquals; +import static org.testng.Assert.assertNotNull; import static org.testng.Assert.assertNull; import static org.testng.Assert.assertTrue; import static org.testng.Assert.fail; @@ -132,6 +136,18 @@ protected JDBCBackendCfg createBackendCfg(String backendId) { return backendCfg; } + /** + * The same, serving the given base DN: what a clear compares the tree stamp of a table against + * when it says whether the table is this backend's own or another's (#866). + */ + protected JDBCBackendCfg createBackendCfg(String backendId, DN baseDN) { + final JDBCBackendCfg backendCfg = createBackendCfg(backendId); + final TreeSet baseDNs = new TreeSet<>(); + baseDNs.add(baseDN); + when(backendCfg.getBaseDN()).thenReturn(baseDNs); + return backendCfg; + } + /** Asked of the database itself, by listing its tables, so that no folding rule of the backend is trusted here. */ private boolean isExistsTable(String tableName) throws SQLException { try (final Connection con = DriverManager.getConnection(getJdbcUrl()); @@ -644,9 +660,12 @@ public Void run(ReadableTransaction txn) throws Exception { return null; } }); - // the failure is remembered: an unstampable table is not asked again while this backend is open + // the failure is remembered: an unstampable table is not asked again while this backend is open. + // Counted from what the open itself attempted rather than from one: the open stamps the tree and + // the catalog of the backend, and how many tables an open has to stamp is not what this is about + final int attemptsOfTheOpen = stampAttempts.get(); assertEquals(storage.commentTable(stamped, dialect()), JDBCStorage.CommentResult.FAILED); - assertEquals(stampAttempts.get(), 1, "a failed stamp was reissued"); + assertEquals(stampAttempts.get(), attemptsOfTheOpen, "a failed stamp was reissued"); } finally { try { storage.write(new WriteOperation() { @@ -1386,6 +1405,12 @@ public void run(WriteableTransaction txn) throws Exception { txn.put(neighbourTree, key(1), value(1)); } }); + } catch (Exception e) { + // the clears of the case below are reached by no failure of this half, and nothing but + // @BeforeClass ever drops what it leaves behind + clearQuietly(storage); + clearQuietly(neighbour); + throw e; } finally { storage.close(); neighbour.close(); @@ -1407,6 +1432,10 @@ public void run(WriteableTransaction txn) throws Exception { // the neighbour is named by a catalog of its own: what one backend clears is never another's assertTrue(isExistsTable(neighbour.getTableName(neighbourTree)), "the clear of one backend dropped the table of another backend of the same database"); + // nor does it report another backend's tables as tables of its own: a table is named after + // the hash of its tree name and says nothing about whose it is, but it is stamped with that + // tree name (#866), and the neighbour's trees are trees of no base DN this backend serves + assertReportsNothingOf(offline, neighbour, neighbourTree); } finally { // in a finally of their own: a failed assertion above must not leave the tables of either // backend behind for the rest of the class, which nothing but @BeforeClass ever drops @@ -1544,7 +1573,6 @@ public ByteString run(ReadableTransaction txn) throws Exception { */ @Test public void testTheSharedCompressedSchemaTreesAreNamedButNeverCleared() throws Exception { - final TreeName shared = JDBCStorage.SHARED_COMPRESSED_SCHEMA_TREES.get(0); final TreeName owned = new TreeName("testSharedCompressedSchema", "owned"); final JDBCStorage storage = new JDBCStorage(createBackendCfg(getBackendId() + "_schema"), null); try { @@ -1555,20 +1583,202 @@ public void run(WriteableTransaction txn) throws Exception { txn.openTree(owned, true); // created, never written to: the pair holds the compressed schema of this very // database, and a row of a test in it would be read back as a schema definition - txn.openTree(shared, true); + for (final TreeName shared : JDBCStorage.SHARED_COMPRESSED_SCHEMA_TREES) { + txn.openTree(shared, true); + } } }); - assertTrue(storage.listTrees().contains(shared), - "a tool asking this backend for its trees was not shown the compressed schema tree"); + // both of them: the pair is a hand-copy of two privates of PersistentCompressedSchema, and + // a literal naming a tree that does not exist would go unseen if one of them were never asked + // for - the tree it names would be neither shown by listTrees() nor spared by a clear + final Set named = storage.listTrees(); + for (final TreeName shared : JDBCStorage.SHARED_COMPRESSED_SCHEMA_TREES) { + assertTrue(named.contains(shared), + "a tool asking this backend for its trees was not shown " + shared); + } storage.removeStorageFiles(); - assertTrue(isExistsTable(storage.getTableName(shared)), - "the clear dropped a compressed schema tree another backend of this database may be the only owner of"); + for (final TreeName shared : JDBCStorage.SHARED_COMPRESSED_SCHEMA_TREES) { + assertTrue(isExistsTable(storage.getTableName(shared)), + "the clear dropped " + shared + ", which another backend of this database may be the only owner of"); + } assertFalse(isExistsTable(storage.getTableName(owned)), "the table of the backend's own tree survived the clear"); } finally { // the shared pair is left where it lies, exactly as the backend leaves it clearQuietly(storage); } } + + /** + * The row of a deleted tree must not be left to the enclosing transaction: a terminal failure + * later in it - write() replays a class 40 conflict and rethrows everything else - would roll the + * row back over a table that is already gone, and nothing would put it right, a deleted tree not + * being opened again (#888). + */ + @Test + public void testADeletedTreeStaysOutOfTheCatalogWhenItsTransactionFails() throws Exception { + final TreeName kept = new TreeName("testCatalogDeleteRollback", "kept"); + final TreeName deleted = new TreeName("testCatalogDeleteRollback", "deleted"); + final JDBCStorage storage = new JDBCStorage(createBackendCfg(getBackendId() + "_rollback"), null); + try { + storage.open(AccessMode.READ_WRITE); + storage.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + txn.openTree(kept, true); + txn.openTree(deleted, true); + } + }); + + try { + storage.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + txn.deleteTree(deleted); + // terminal, and no conflict for write() to replay: everything this transaction + // still owes goes back, and the row of the deleted tree must not be part of it + throw new IllegalStateException("the transaction of a deleteTree failed"); + } + }); + fail("the write was expected to fail"); + } catch (Exception expected) { + // what the case is about is what the failure left behind + } + + assertFalse(isExistsTable(storage.getTableName(deleted)), "the failed transaction brought a dropped table back"); + final Set remaining = storage.listTrees(); + assertFalse(remaining.contains(deleted), + "the catalog names a tree whose table the failed transaction left dropped"); + assertTrue(remaining.contains(kept), "the catalog forgot a tree that is still there"); + } finally { + clearQuietly(storage); + } + } + + /** + * A clear drops the table its catalog records for a tree, not one it derives again from the tree + * name, so that a removal drops what was enrolled even if the naming of tables were ever to + * change. A row recording no table at all - all a version recording the name alone would have + * left - falls back to the derived name rather than naming nothing. + */ + @Test + public void testAClearDropsTheTableTheCatalogRecords() throws Exception { + final TreeName tree = new TreeName("testCatalogValue", "tree"); + final JDBCStorage storage = new JDBCStorage(createBackendCfg(getBackendId() + "_value"), null); + try { + storage.open(AccessMode.READ_WRITE); + storage.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + txn.openTree(tree, true); + } + }); + try (final Connection con = DriverManager.getConnection(getJdbcUrl())) { + final Map recorded = storage.catalogTables(con); + assertEquals(recorded.get(tree), storage.getTableName(tree), + "the catalog does not record the table holding the tree its row names"); + assertTrue(recorded.containsKey(storage.getCatalogTree()), "a clear was not shown the catalog itself"); + + emptyTheRecordedTableNames(storage.getTableName(storage.getCatalogTree())); + assertEquals(storage.catalogTables(con).get(tree), storage.getTableName(tree), + "a row recording no table name did not fall back to the name derived from the tree"); + } + + storage.removeStorageFiles(); + + assertFalse(isExistsTable(storage.getTableName(tree)), "the table the catalog named survived the clear"); + } finally { + clearQuietly(storage); + } + } + + /** + * What a clear leaves standing it reports, and it reports it as what it is: a table stamped with a + * tree of a base DN this backend serves is its own and can be removed by hand, while a table of a + * backend sharing this database (#873) is that backend's business and no part of this outcome. + * Told apart by the stamp of #866 and by nothing else - a table name is a bare hash. + */ + @Test + public void testAClearReportsTheTablesItCanAttributeToThisBackend() throws Exception { + final DN baseDN = DN.valueOf("dc=clear-report,dc=com"); + final TreeName owned = new TreeName(baseDN.toNormalizedUrlSafeString(), "id2entry"); + final TreeName neighbourTree = new TreeName("testClearReportNeighbour", "tree"); + final JDBCStorage storage = new JDBCStorage(createBackendCfg(getBackendId() + "_reported", baseDN), null); + final JDBCStorage neighbour = new JDBCStorage(createBackendCfg(getBackendId() + "_reportedNeighbour"), null); + try { + storage.open(AccessMode.READ_WRITE); + storage.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + txn.openTree(owned, true); + } + }); + neighbour.open(AccessMode.READ_WRITE); + neighbour.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + txn.openTree(neighbourTree, true); + } + }); + // the state of a backend upgraded from a version keeping no catalog: its tables are there + // and nothing names them, so the clear that follows drops nothing at all + dropTableBehindTheBackend(storage.getTableName(storage.getCatalogTree())); + storage.close(); + + storage.removeStorageFiles(); + + assertTrue(isExistsTable(storage.getTableName(owned)), + "a table named by no catalog was dropped: nothing may be dropped that cannot be attributed"); + try (final Connection con = DriverManager.getConnection(getJdbcUrl())) { + final JDBCStorage.ClearLeftovers leftovers = + storage.leftoverTables(con, con.getCatalog(), con.getSchema()); + assertNotNull(leftovers, "the database would not say which tables the clear left standing"); + assertTrue(leftovers.ours.toString().toLowerCase().contains(storage.getTableName(owned).toLowerCase()), + "a table of a base DN this backend serves was not reported as its own: " + leftovers.ours); + assertFalse(leftovers.unattributed.toString().toLowerCase().contains(storage.getTableName(owned).toLowerCase()), + "a table this backend can name was reported as attributable to nobody: " + leftovers.unattributed); + } + assertReportsNothingOf(storage, neighbour, neighbourTree); + } finally { + clearQuietly(neighbour); + // the catalog of this one is gone, so its clear names nothing: the table it left standing on + // purpose is dropped here by hand, as the report says such a table has to be + clearQuietly(storage); + dropTableIfExists(storage.getTableName(owned)); + } + } + + /** Asserts that the clear of one backend says nothing whatsoever about the tables of another. */ + private void assertReportsNothingOf(JDBCStorage cleared, JDBCStorage other, TreeName otherTree) throws SQLException { + try (final Connection con = DriverManager.getConnection(getJdbcUrl())) { + final JDBCStorage.ClearLeftovers leftovers = + cleared.leftoverTables(con, con.getCatalog(), con.getSchema()); + assertNotNull(leftovers, "the database would not say which tables the clear left standing"); + final String reported = (leftovers.ours + " " + leftovers.unattributed).toLowerCase(); + assertFalse(reported.contains(other.getTableName(otherTree).toLowerCase()), + "the clear of one backend reported the table of another: " + reported); + assertFalse(reported.contains(other.getTableName(other.getCatalogTree()).toLowerCase()), + "the clear of one backend reported the catalog of another: " + reported); + } + } + + /** Empties the recorded table name of every row of a catalog, as a version recording none would have left it. */ + private void emptyTheRecordedTableNames(String catalogTable) throws SQLException { + try (final Connection con = DriverManager.getConnection(getJdbcUrl()); + final PreparedStatement statement = con.prepareStatement("update " + catalogTable + " set v=?")) { + statement.setBytes(1, new byte[0]); + statement.executeUpdate(); + } + } + + /** Drops a table a clear left standing on purpose, so that it is not left behind for the rest of the class. */ + private void dropTableIfExists(String tableName) { + try { + if (isExistsTable(tableName)) { + dropTableBehindTheBackend(tableName); + } + } catch (SQLException ignored) { + } + } }