From 2194e518558a7b7f3f26fd7e26b366e27e5952d0 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Wed, 19 Aug 2026 14:50:04 +0300 Subject: [PATCH 1/3] [#873] Give each backend its own compressed schema trees The two trees holding the compressed schema definitions were named from a literal, so they carried no backend qualifier while every other tree of a backend carries its base DN. That is harmless where a storage holds one backend - JE and PDB give each its own directory, and the Cassandra backend names its table after the backend id - but the JDBC backend derives its table name from the tree name alone, so two JDBC backends addressing one database mapped to one pair of tables. Each allocated tokens from the size of its own in-memory map under its own lock, so both handed out the same token for different attribute descriptions and overwrote each other's definitions with a blind put. Nothing showed until a restart, after which the entries of the losing backend decoded as the wrong attributes, silently. The trees are now named "/compressed_schema_/...". A backend upgraded from a version that shared them migrates on its first open: load() copies the records the shared trees hold and its own do not, and fails the open if that copy cannot complete - carrying on with no definitions at all would restart token allocation from zero and mis-decode every entry already written. The shared trees are read, never emptied: on a shared database they may still be the only copy another backend has, and leaving them is what makes a downgrade possible. Only keys the backend does not already hold are copied, so the migration is safe to re-run after it was interrupted, which is how a storage without transactions recovers from a copy that stopped halfway. A read-only open - export-ldif, verify-index - cannot migrate, so it reads the shared trees where they lie. Deciding that needs a question no read can answer, since a storage may materialize a tree on first access (JE opens its databases with setAllowCreate) or reject the access outright (JDBC, when no such table exists), so ReadableTransaction gains treeExists(). The JDBC implementation derives the table name without entering it into tree2table, which removeStorageFiles() drops: asking about another backend's tree must not put it up for removal. UpgradeTasks is left alone. Its rename step migrates 2.x local-db backends to pluggable JE by renaming JE databases through a JE Environment directly and never runs for PDB or JDBC; what it produces is the shared pair, which the first open then migrates like any other. --- .../server/backends/cassandra/CASStorage.java | 26 +++ .../server/backends/jdbc/JDBCStorage.java | 97 +++++--- .../opends/server/backends/jeb/JEStorage.java | 28 +++ .../server/backends/pdb/PDBStorage.java | 40 ++++ .../pluggable/OnDiskMergeImporter.java | 12 + .../pluggable/PersistentCompressedSchema.java | 220 +++++++++++++++--- .../backends/pluggable/RootContainer.java | 2 +- .../backends/pluggable/TracedStorage.java | 18 ++ .../pluggable/spi/ReadableTransaction.java | 17 ++ .../org/opends/messages/backend.properties | 6 + .../opends/server/backends/jdbc/TestCase.java | 144 ++++++++++++ .../backends/pluggable/DefaultIndexTest.java | 14 +- .../PersistentCompressedSchemaTest.java | 217 +++++++++++++++++ 13 files changed, 764 insertions(+), 77 deletions(-) create mode 100644 opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/PersistentCompressedSchemaTest.java diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/cassandra/CASStorage.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/cassandra/CASStorage.java index e30c7481f8..82e2d86c75 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/cassandra/CASStorage.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/cassandra/CASStorage.java @@ -63,6 +63,7 @@ import com.datastax.oss.driver.api.core.cql.ResultSet; import com.datastax.oss.driver.api.core.cql.Row; import com.datastax.oss.driver.api.core.cql.Statement; +import com.datastax.oss.driver.api.core.servererrors.InvalidQueryException; import com.github.benmanes.caffeine.cache.Caffeine; import com.github.benmanes.caffeine.cache.LoadingCache; @@ -247,6 +248,31 @@ public long getRecordCount(TreeName treeName) { ).one().getLong(0); } + @Override + public boolean treeExists(TreeName treeName) { + // Every tree of this backend is a partition of the one table named after the backend id, + // so a tree has no existence apart from its rows: "exists" here means "holds at least one + // record". A LIMIT 1 lookup rather than getRecordCount() to avoid the full partition scan + // a count would run. + try { + return execute( + prepared.get("SELECT key FROM "+getTableName()+" WHERE baseDN=:baseDN and indexId=:indexId LIMIT 1").bind() + .setString("baseDN", treeName.getBaseDN()).setString("indexId", treeName.getIndexId()) + ).one()!=null; + }catch (RuntimeException e) { + // The backend's own table has not been created yet - a read-only open of a backend + // that was never written - so none of its trees can exist either. The driver reports + // it from prepare() as much as from execute(), and the statement cache may hand back + // the loader's failure wrapped, so the whole chain is searched. + for (Throwable cause=e; cause!=null; cause=cause.getCause()) { + if (cause instanceof InvalidQueryException) { + return false; + } + } + throw e; + } + } + @Override public void deleteTree(TreeName treeName) { checkReadOnly(); 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 c94088c361..6e2f90f3a0 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 @@ -168,22 +168,33 @@ public void close() { unstampableTrees.clear(); } + // The trees this storage has taken an interest in, and the tables they map to. listTrees() - + // and through it removeStorageFiles() - reads this, so a tree only belongs here once this + // backend uses it: see toTableName() below for the trees that are merely asked about. final LoadingCache tree2table = Caffeine.newBuilder() - .build(treeName -> { - try { - final MessageDigest md = MessageDigest.getInstance("SHA-224"); - final byte[] messageDigest = md.digest(treeName.toString().getBytes()); - final StringBuilder hashtext = new StringBuilder(56); - for (byte b : messageDigest) { - String hex = Integer.toHexString(0xff & b); - if (hex.length() == 1) hashtext.append('0'); - hashtext.append(hex); - } - return "opendj_" + hashtext; - } catch (NoSuchAlgorithmException e) { - throw new RuntimeException(e); + .build(JDBCStorage::toTableName); + + /** + * The table a tree name maps to. A pure function of the name, so that a tree can be asked about + * without being entered into tree2table: treeExists() is asked about trees this backend does not + * own - the compressed schema probes the tree its definitions used to be shared under (#873) - + * and removeStorageFiles() drops every table tree2table names, so a probe must not enrol one. + */ + static String toTableName(TreeName treeName) { + try { + final MessageDigest md = MessageDigest.getInstance("SHA-224"); + final byte[] messageDigest = md.digest(treeName.toString().getBytes()); + final StringBuilder hashtext = new StringBuilder(56); + for (byte b : messageDigest) { + String hex = Integer.toHexString(0xff & b); + if (hex.length() == 1) hashtext.append('0'); + hashtext.append(hex); } - }); + return "opendj_" + hashtext; + } catch (NoSuchAlgorithmException e) { + throw new RuntimeException(e); + } + } String getTableName(TreeName treeName) { return tree2table.get(treeName); @@ -1042,6 +1053,40 @@ public long getRecordCount(TreeName treeName) { throw new StorageRuntimeException(e); } } + + @Override + public boolean treeExists(TreeName treeName) { + return isExistsTable(treeName); + } + + // Readable, not writeable: a read-only open has to be able to tell a tree that was never + // written from one it may not read, since every other statement here fails outright on a + // table that does not exist. + boolean isExistsTable(TreeName treeName) { + // toTableName() rather than getTableName(): asking whether a tree is there must not + // enrol it in tree2table, which is what removeStorageFiles() drops. + final String tableName = toTableName(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; + } } /** * A transaction able to write, unless the storage was opened read-only: then it may open an existing tree and @@ -1075,30 +1120,6 @@ void checkReadOnly() { } } - 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; - } - String getTableDialect() { if (driverNameOf(con).contains("oracle")) { return "h char(128),k raw(2000),v blob,primary key(h,k)"; diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/jeb/JEStorage.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/jeb/JEStorage.java index cc9b8700d3..4db4d9fcaf 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/jeb/JEStorage.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/jeb/JEStorage.java @@ -427,6 +427,21 @@ public long getRecordCount(TreeName treeName) } } + @Override + public boolean treeExists(TreeName treeName) + { + // Deliberately not getOrOpenTree(): dbConfig() sets allowCreate, so asking the tree itself + // would create the very database whose absence is being tested. + try + { + return env.getDatabaseNames().contains(toDatabaseName(treeName)); + } + catch (DatabaseException e) + { + throw new StorageRuntimeException(e); + } + } + @Override public Cursor openCursor(final TreeName treeName) { @@ -550,6 +565,12 @@ public long getRecordCount(TreeName treeName) return delegate.getRecordCount(treeName); } + @Override + public boolean treeExists(TreeName treeName) + { + return delegate.treeExists(treeName); + } + @Override public void openTree(TreeName treeName, boolean createOnDemand) { @@ -638,6 +659,13 @@ public long getRecordCount(TreeName treeName) { return 0; } + + @Override + public boolean treeExists(TreeName treeName) + { + // No environment was ever opened, so nothing is stored. + return false; + } } private WriteableTransaction newWriteableTransaction(Transaction txn) diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/pdb/PDBStorage.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/pdb/PDBStorage.java index ea233952de..e99db989b4 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/pdb/PDBStorage.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/pdb/PDBStorage.java @@ -467,6 +467,12 @@ public long getRecordCount(TreeName treeName) } } + @Override + public boolean treeExists(TreeName treeName) + { + return PDBStorage.this.treeExists(treeName); + } + @Override public Cursor openCursor(final TreeName treeName) { @@ -690,6 +696,12 @@ public long getRecordCount(TreeName treeName) return delegate.getRecordCount(treeName); } + @Override + public boolean treeExists(TreeName treeName) + { + return delegate.treeExists(treeName); + } + @Override public void openTree(TreeName treeName, boolean createOnDemand) { @@ -819,6 +831,13 @@ public long getRecordCount(TreeName treeName) return 0; } + @Override + public boolean treeExists(TreeName treeName) + { + // No volume was ever opened, so nothing is stored. + return false; + } + @Override public T read(ReadOperation operation) throws Exception { @@ -832,6 +851,27 @@ public void write(WriteOperation operation) throws Exception } } + /** + * Tells whether the volume holds a tree of that name. Deliberately not implemented by asking for + * an {@link Exchange}: {@code getExchange(volume, name, true)} would create the tree being tested, + * and with {@code false} Persistit reports its absence by throwing. + */ + boolean treeExists(final TreeName treeName) + { + if (volume == null) + { + return false; + } + try + { + return volume.getTree(treeName.toString(), false) != null; + } + catch (PersistitException e) + { + throw new StorageRuntimeException(e); + } + } + Exchange getNewExchange(final TreeName treeName, final boolean create) throws PersistitException { final Exchange ex = db.getExchange(volume, treeName.toString(), create); diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/OnDiskMergeImporter.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/OnDiskMergeImporter.java index 5b537910e8..efb49c5297 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/OnDiskMergeImporter.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/OnDiskMergeImporter.java @@ -1628,6 +1628,12 @@ public Cursor openCursor(TreeName treeName) throw new UnsupportedOperationException(); } + @Override + public boolean treeExists(TreeName treeName) + { + throw new UnsupportedOperationException(); + } + @Override public long getRecordCount(TreeName treeName) { @@ -4207,6 +4213,12 @@ public long getRecordCount(TreeName treeName) return counter; } + @Override + public boolean treeExists(TreeName treeName) + { + throw new UnsupportedOperationException(); + } + @Override public void openTree(TreeName name, boolean createOnDemand) { diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/PersistentCompressedSchema.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/PersistentCompressedSchema.java index af484abb73..0e2d63c7fb 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/PersistentCompressedSchema.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/PersistentCompressedSchema.java @@ -13,10 +13,12 @@ * * Copyright 2008-2009 Sun Microsystems, Inc. * Portions Copyright 2013-2016 ForgeRock AS. + * Portions Copyright 2026 3A Systems, LLC. */ package org.opends.server.backends.pluggable; import static org.opends.messages.BackendMessages.*; +import static org.opends.server.util.StaticUtils.stackTraceToSingleLineString; import java.io.IOException; import java.util.Collection; @@ -32,6 +34,7 @@ import org.opends.server.api.CompressedSchema; import org.opends.server.backends.pluggable.spi.AccessMode; import org.opends.server.backends.pluggable.spi.Cursor; +import org.opends.server.backends.pluggable.spi.ReadableTransaction; import org.opends.server.backends.pluggable.spi.Storage; import org.opends.server.backends.pluggable.spi.StorageRuntimeException; import org.opends.server.backends.pluggable.spi.TreeName; @@ -55,10 +58,33 @@ final class PersistentCompressedSchema extends CompressedSchema /** The name of the tree used to store compressed object class set definitions. */ private static final String DB_NAME_OC = "compressed_object_classes"; - /** The compressed attribute description schema tree. */ - private static final TreeName adTreeName = new TreeName("compressed_schema", DB_NAME_AD); - /** The compressed object class set schema tree. */ - private static final TreeName ocTreeName = new TreeName("compressed_schema", DB_NAME_OC); + /** + * The tree prefix every backend shared before the definitions were separated per backend. + *

+ * Every other tree of a backend is named from {@link EntryContainer#getTreePrefix()} and so + * carries its base DN, but these two belong to the backend rather than to any one of its base + * DNs and were named from a literal. That is harmless where a storage holds a single backend - + * JE and PDB give each its own directory, and the Cassandra backend names its table after the + * backend id - but the JDBC backend derives its table name from the tree name alone, so two + * JDBC backends addressing one database met in this pair of tables. Each allocated tokens from + * the size of its own in-memory map under its own lock, so both handed out the same token for + * different attribute descriptions and overwrote each other's definitions; after a restart the + * entries of the losing backend decoded as the wrong attributes, silently (issue #873). + *

+ * Kept only to be read: {@link #load} migrates what it finds here into the backend's own trees + * and never writes to it again. + */ + private static final String LEGACY_TREE_PREFIX = "compressed_schema"; + private static final TreeName LEGACY_AD_TREE_NAME = new TreeName(LEGACY_TREE_PREFIX, DB_NAME_AD); + private static final TreeName LEGACY_OC_TREE_NAME = new TreeName(LEGACY_TREE_PREFIX, DB_NAME_OC); + + /** The compressed attribute description schema tree of this backend. */ + private final TreeName adTreeName; + /** The compressed object class set schema tree of this backend. */ + private final TreeName ocTreeName; + + /** The id of the backend these definitions belong to. */ + private final String backendId; /** The storage in which the trees are held. */ private final Storage storage; @@ -73,6 +99,9 @@ final class PersistentCompressedSchema extends CompressedSchema * * @param serverContext * The server context. + * @param backendId + * The id of the backend whose definitions are held, which qualifies the trees so that + * two backends sharing one database do not share one token space. * @param storage * A reference to the storage in which the trees will be held. * @param txn a non null transaction @@ -85,14 +114,29 @@ final class PersistentCompressedSchema extends CompressedSchema * If an error occurs while loading and processing the compressed * schema definitions. */ - PersistentCompressedSchema(ServerContext serverContext, final Storage storage, WriteableTransaction txn, - AccessMode accessMode) throws StorageRuntimeException, InitializationException + PersistentCompressedSchema(ServerContext serverContext, String backendId, final Storage storage, + WriteableTransaction txn, AccessMode accessMode) throws StorageRuntimeException, InitializationException { super(serverContext); this.storage = storage; + this.backendId = backendId; + final String treePrefix = treePrefix(backendId); + this.adTreeName = new TreeName(treePrefix, DB_NAME_AD); + this.ocTreeName = new TreeName(treePrefix, DB_NAME_OC); load(txn, accessMode.isWriteable()); } + /** + * Qualifies the legacy prefix with the backend id. {@link TreeName} splits its string form on + * '/' and states that no component may contain one, and nothing rules a '/' out of a backend id + * - ds-cfg-backend-id is a plain string - so it is escaped rather than passed through. The + * escape is reversible, so two distinct backend ids can never produce one prefix. + */ + private static String treePrefix(String backendId) + { + return LEGACY_TREE_PREFIX + "_" + backendId.replace("%", "%25").replace("/", "%2F"); + } + @Override protected void storeAttribute(final byte[] encodedAttribute, final String attributeName, final Iterable attributeOptions) @@ -143,53 +187,155 @@ private void load(WriteableTransaction txn, boolean shouldCreate) txn.openTree(adTreeName, shouldCreate); txn.openTree(ocTreeName, shouldCreate); + if (needsLegacyDefinitions(txn)) + { + if (shouldCreate) + { + migrateLegacyDefinitions(txn); + } + else + { + // Read-only: nothing may be written, so the legacy definitions are read where they lie. + // Loaded first, so that anything this backend has already migrated and since added under + // its own prefix wins for the same token. + loadTrees(txn, LEGACY_OC_TREE_NAME, LEGACY_AD_TREE_NAME); + } + } + loadTrees(txn, ocTreeName, adTreeName); + } + + /** + * Tells whether the definitions under {@link #LEGACY_TREE_PREFIX} are still needed, either + * because this backend has not been opened since the upgrade that separated them, or because a + * previous migration did not run to completion - on a storage without transactions the copy can + * stop halfway. Once migrated, this backend's trees only ever grow, so they can no longer hold + * fewer records than the legacy ones and the question is settled without reading either tree. + */ + private boolean needsLegacyDefinitions(ReadableTransaction txn) + { + final long legacyAdCount = recordCount(txn, LEGACY_AD_TREE_NAME); + final long legacyOcCount = recordCount(txn, LEGACY_OC_TREE_NAME); + return (legacyAdCount > 0 || legacyOcCount > 0) + && (recordCount(txn, adTreeName) < legacyAdCount || recordCount(txn, ocTreeName) < legacyOcCount); + } + + /** The number of records of a tree, without asking a storage to materialize one that is absent. */ + private long recordCount(ReadableTransaction txn, TreeName treeName) + { + return txn.treeExists(treeName) ? txn.getRecordCount(treeName) : 0; + } + + private void migrateLegacyDefinitions(WriteableTransaction txn) throws InitializationException + { + try + { + final long copied = copyMissingRecords(txn, LEGACY_AD_TREE_NAME, adTreeName) + + copyMissingRecords(txn, LEGACY_OC_TREE_NAME, ocTreeName); + logger.info(NOTE_COMPSCHEMA_MIGRATED, copied, backendId, LEGACY_TREE_PREFIX, adTreeName.getBaseDN()); + } + catch (final Exception e) + { + logger.traceException(e); + // Deliberately fatal to the open. Loading no definitions at all would restart token + // allocation from zero and decode every entry written so far as the wrong attributes, + // without reporting anything. + throw new InitializationException(ERR_COMPSCHEMA_CANNOT_MIGRATE.get( + backendId, LEGACY_TREE_PREFIX, adTreeName.getBaseDN(), stackTraceToSingleLineString(e)), e); + } + } + + /** + * Copies the records of {@code from} that {@code to} does not already hold, and returns how many + * were copied. The legacy tree is left in place: on a shared database it may still be the only + * copy another backend has, and leaving it is what makes a downgrade possible. + *

+ * A key already present in {@code to} is never overwritten, which is what makes this safe to + * re-run after an interrupted migration and safe against a legacy tree that a backend of an + * earlier version is still writing to. + */ + private long copyMissingRecords(WriteableTransaction txn, TreeName from, TreeName to) + { + if (!txn.treeExists(from)) + { + return 0; + } + long copied = 0; + try (Cursor cursor = txn.openCursor(from)) + { + while (cursor.next()) + { + final ByteString key = cursor.getKey(); + if (txn.read(to, key) == null) + { + txn.put(to, key, cursor.getValue()); + copied++; + } + } + } + return copied; + } + + /** + * Loads the object class set definitions and then the attribute description definitions of the + * provided pair of trees into the maps. A tree that does not exist contributes nothing: a + * read-only open creates none, and a storage that keeps one object per tree fails outright when + * asked to read one that was never written. + */ + private void loadTrees(ReadableTransaction txn, TreeName ocTree, TreeName adTree) throws InitializationException + { // Cursor through the object class database and load the object class set // definitions. At the same time, figure out the highest token value and // initialize the object class counter to one greater than that. - try (Cursor ocCursor = txn.openCursor(ocTreeName)) + if (txn.treeExists(ocTree)) { - while (ocCursor.next()) + try (Cursor ocCursor = txn.openCursor(ocTree)) { - final byte[] encodedObjectClasses = ocCursor.getKey().toByteArray(); - final ASN1Reader reader = ASN1.getReader(ocCursor.getValue()); - reader.readStartSequence(); - final List objectClassNames = new LinkedList<>(); - while (reader.hasNextElement()) + while (ocCursor.next()) { - objectClassNames.add(reader.readOctetStringAsString()); + final byte[] encodedObjectClasses = ocCursor.getKey().toByteArray(); + final ASN1Reader reader = ASN1.getReader(ocCursor.getValue()); + reader.readStartSequence(); + final List objectClassNames = new LinkedList<>(); + while (reader.hasNextElement()) + { + objectClassNames.add(reader.readOctetStringAsString()); + } + reader.readEndSequence(); + loadObjectClasses(encodedObjectClasses, objectClassNames); } - reader.readEndSequence(); - loadObjectClasses(encodedObjectClasses, objectClassNames); } - } - catch (final IOException e) - { - logger.traceException(e); - throw new InitializationException(ERR_COMPSCHEMA_CANNOT_DECODE_OC_TOKEN.get(e.getMessage()), e); + catch (final IOException e) + { + logger.traceException(e); + throw new InitializationException(ERR_COMPSCHEMA_CANNOT_DECODE_OC_TOKEN.get(e.getMessage()), e); + } } // Cursor through the attribute description database and load the attribute set definitions. - try (Cursor adCursor = txn.openCursor(adTreeName)) + if (txn.treeExists(adTree)) { - while (adCursor.next()) + try (Cursor adCursor = txn.openCursor(adTree)) { - final byte[] encodedAttribute = adCursor.getKey().toByteArray(); - final ASN1Reader reader = ASN1.getReader(adCursor.getValue()); - reader.readStartSequence(); - final String attributeName = reader.readOctetStringAsString(); - final List attributeOptions = new LinkedList<>(); - while (reader.hasNextElement()) + while (adCursor.next()) { - attributeOptions.add(reader.readOctetStringAsString()); + final byte[] encodedAttribute = adCursor.getKey().toByteArray(); + final ASN1Reader reader = ASN1.getReader(adCursor.getValue()); + reader.readStartSequence(); + final String attributeName = reader.readOctetStringAsString(); + final List attributeOptions = new LinkedList<>(); + while (reader.hasNextElement()) + { + attributeOptions.add(reader.readOctetStringAsString()); + } + reader.readEndSequence(); + loadAttribute(encodedAttribute, attributeName, attributeOptions); } - reader.readEndSequence(); - loadAttribute(encodedAttribute, attributeName, attributeOptions); } - } - catch (final IOException e) - { - logger.traceException(e); - throw new InitializationException(ERR_COMPSCHEMA_CANNOT_DECODE_AD_TOKEN.get(e.getMessage()), e); + catch (final IOException e) + { + logger.traceException(e); + throw new InitializationException(ERR_COMPSCHEMA_CANNOT_DECODE_AD_TOKEN.get(e.getMessage()), e); + } } } diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/RootContainer.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/RootContainer.java index 94cb42d144..6cb74cfc61 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/RootContainer.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/RootContainer.java @@ -137,7 +137,7 @@ void open(final AccessMode accessMode) throws StorageRuntimeException, ConfigExc @Override public void run(WriteableTransaction txn) throws Exception { - compressedSchema = new PersistentCompressedSchema(serverContext, storage, txn, accessMode); + compressedSchema = new PersistentCompressedSchema(serverContext, backendId, storage, txn, accessMode); openAndRegisterEntryContainers(txn, config.getBaseDN(), accessMode); } }); diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/TracedStorage.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/TracedStorage.java index 172388ef62..42ff1dae71 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/TracedStorage.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/TracedStorage.java @@ -287,6 +287,15 @@ public long getRecordCount(TreeName name) return count; } + @Override + public boolean treeExists(TreeName name) + { + traceEnter("treeExists", "name", name); + final boolean exists = txn.treeExists(name); + traceLeave("treeExists", "name", name, "exists", exists); + return exists; + } + @Override public Cursor openCursor(final TreeName name) { @@ -365,6 +374,15 @@ public long getRecordCount(TreeName name) return count; } + @Override + public boolean treeExists(TreeName name) + { + traceEnter("treeExists", "name", name); + final boolean exists = txn.treeExists(name); + traceLeave("treeExists", "name", name, "exists", exists); + return exists; + } + @Override public Cursor openCursor(final TreeName name) { diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/spi/ReadableTransaction.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/spi/ReadableTransaction.java index 1f9ab9e02d..0cb0db18b4 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/spi/ReadableTransaction.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/spi/ReadableTransaction.java @@ -12,6 +12,7 @@ * information: "Portions Copyright [year] [name of copyright owner]". * * Copyright 2014-2015 ForgeRock AS. + * Portions Copyright 2026 3A Systems, LLC. */ package org.opends.server.backends.pluggable.spi; @@ -51,4 +52,20 @@ public interface ReadableTransaction * @return the number of key/value pairs in the provided tree. */ long getRecordCount(TreeName treeName); + + /** + * Returns whether the tree whose name is provided is present in the storage. + *

+ * This is not the same question as whether the tree is empty, and it cannot be answered by + * reading from the tree: a storage is free to materialize a tree on first access - the JE backend + * opens its databases with {@code setAllowCreate(true)} - or to reject the access outright, as the + * JDBC backend does when no table of that name exists. Callers that must distinguish "never + * written" from "written and since emptied", such as the compressed schema deciding whether it + * has anything to migrate, need this instead. + * + * @param treeName + * the tree name + * @return {@code true} if the tree exists, {@code false} otherwise + */ + boolean treeExists(TreeName treeName); } diff --git a/opendj-server-legacy/src/messages/org/opends/messages/backend.properties b/opendj-server-legacy/src/messages/org/opends/messages/backend.properties index a8d16b8795..18a07eb991 100644 --- a/opendj-server-legacy/src/messages/org/opends/messages/backend.properties +++ b/opendj-server-legacy/src/messages/org/opends/messages/backend.properties @@ -1108,3 +1108,9 @@ ERR_SERVICE_DISCOVERY_CONFIG_MANAGER_LISTENER_615=Registering Service Discovery NOTE_IMPORT_MIGRATION_START_616=Migrating %s entries for base DN %s so that they are preserved by the partial import WARN_PSEARCH_BACKEND_UNAVAILABLE_617=The persistent search is being terminated because backend %s is \ no longer available +NOTE_COMPSCHEMA_MIGRATED_618=Migrated %d compressed schema definitions of backend '%s' from the shared tree '%s' \ + to '%s'. The shared tree is left untouched: on a storage where several backends address one database, such as a \ + JDBC backend, it may still hold the definitions of another backend +ERR_COMPSCHEMA_CANNOT_MIGRATE_619=The compressed schema definitions of backend '%s' could not be migrated from \ + the shared tree '%s' to '%s': %s. The backend cannot be opened, because its entries were encoded against the \ + definitions that were not migrated and would decode as the wrong attributes 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 22e8ea1bb9..2aefcdfa31 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 @@ -236,6 +236,150 @@ public void run(WriteableTransaction txn) throws Exception { } } + /** + * treeExists() has to answer for a table that was never created rather than fail: it is how the + * compressed schema tells a backend with nothing to migrate from one whose definitions are still + * under the shared prefix (#873), and every other statement of this storage fails outright on a + * table that does not exist. + */ + @Test + public void testTreeExistsAnswersForAMissingTable() throws Exception { + final JDBCStorage storage = new JDBCStorage(createBackendCfg(), null); + final TreeName present = new TreeName("testTreeExists", "present"); + final TreeName absent = new TreeName("testTreeExists", "absent"); + try { + storage.open(AccessMode.READ_WRITE); + storage.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + txn.openTree(present, true); + assertTrue(txn.treeExists(present)); + assertFalse(txn.treeExists(absent)); + } + }); + // the read path has to answer as well: export-ldif and verify-index open read-only, where + // no tree is created and the question cannot be settled by writing one + storage.read(new ReadOperation() { + @Override + public Void run(ReadableTransaction txn) throws Exception { + assertTrue(txn.treeExists(present)); + assertFalse(txn.treeExists(absent)); + return null; + } + }); + storage.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + txn.deleteTree(present); + assertFalse(txn.treeExists(present)); + } + }); + } finally { + try { + storage.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + txn.deleteTree(present); + } + }); + } catch (Exception ignored) {} + storage.close(); + } + } + + /** + * The compressed schema definitions of this backend must live in a table of its own. The tree + * name they used to carry held no backend qualifier, so its table name was a constant that every + * JDBC backend of every server sharing the database mapped to, and two of them overwrote each + * other's token definitions there (#873). The backend of this suite has been opened and populated + * by PluggableBackendImplTestCase#setUp, so its own table exists by now. + */ + @Test + public void testCompressedSchemaTableIsQualifiedByBackendId() throws Exception { + final JDBCStorage storage = new JDBCStorage(createBackendCfg(), null); + try { + storage.open(AccessMode.READ_WRITE); + final String shared = JDBCStorage.toTableName(new TreeName("compressed_schema", "compressed_attributes")); + final String own = JDBCStorage.toTableName( + new TreeName("compressed_schema_" + getBackendId(), "compressed_attributes")); + assertFalse(shared.equals(own), "the qualified tree name must map to a table of its own"); + try (final Connection con = DriverManager.getConnection(getJdbcUrl())) { + assertTrue(isExistingTable(con, own), own + " (this backend's own definitions) is missing"); + assertFalse(isExistingTable(con, shared), shared + " is the table every backend used to share"); + } + } finally { + storage.close(); + } + } + + /** + * Asking whether a tree is there must not enrol it in the storage's tree map: removeStorageFiles() + * drops every table that map names, and the compressed schema asks about the tree its definitions + * used to be shared under - which on a shared database is another backend's to keep (#873). + */ + @Test + public void testProbingATreeDoesNotPutItUpForRemoval() throws Exception { + final TreeName foreign = new TreeName("testProbe", "foreign"); + final JDBCStorage owner = new JDBCStorage(createBackendCfg(), null); + 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)); + } + }); + owner.close(); + + // a second storage on the same database, which never opened that tree - the shape of two + // backends addressing one database + final JDBCStorage other = new JDBCStorage(createBackendCfg(), null); + try { + other.open(AccessMode.READ_WRITE); + other.read(new ReadOperation() { + @Override + public Void run(ReadableTransaction txn) throws Exception { + assertTrue(txn.treeExists(foreign)); + return null; + } + }); + assertFalse(other.listTrees().contains(foreign), "a probed tree must not be listed for removal"); + + other.removeStorageFiles(); + + try (final Connection con = DriverManager.getConnection(getJdbcUrl())) { + assertTrue(isExistingTable(con, JDBCStorage.toTableName(foreign)), + "clearing one backend dropped a table it had only asked about"); + } + } finally { + other.close(); + final JDBCStorage cleanup = new JDBCStorage(createBackendCfg(), null); + try { + cleanup.open(AccessMode.READ_WRITE); + cleanup.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + txn.deleteTree(foreign); + } + }); + } catch (Exception ignored) { + } finally { + cleanup.close(); + } + } + } + + private static boolean isExistingTable(Connection con, String tableName) throws SQLException { + try (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; + } + /** * Forward repositioning inside the already-fetched batch must be served from the buffer without SQL, * and batch sizes must grow from "fetchsize.initial" to "fetchsize" on sequential reads (#860). diff --git a/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/DefaultIndexTest.java b/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/DefaultIndexTest.java index e3b01b86d8..ebea2d40b4 100644 --- a/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/DefaultIndexTest.java +++ b/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/DefaultIndexTest.java @@ -12,6 +12,7 @@ * information: "Portions Copyright [year] [name of copyright owner]". * * Copyright 2015-2016 ForgeRock AS. + * Portions Copyright 2026 3A Systems, LLC. */ package org.opends.server.backends.pluggable; @@ -248,10 +249,21 @@ public long getRecordCount(TreeName treeName) return getTree(treeName).size(); } + @Override + public boolean treeExists(TreeName treeName) + { + return storage.containsKey(treeName); + } + @Override public void openTree(TreeName name, boolean createOnDemand) { - storage.put(name, new TreeMap()); + // Honours createOnDemand, and leaves an already open tree alone rather than replacing it with + // an empty one, so that a caller opening the same tree twice does not silently lose its records. + if (createOnDemand) + { + storage.putIfAbsent(name, new TreeMap()); + } } @Override diff --git a/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/PersistentCompressedSchemaTest.java b/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/PersistentCompressedSchemaTest.java new file mode 100644 index 0000000000..81a0cdc4f8 --- /dev/null +++ b/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/PersistentCompressedSchemaTest.java @@ -0,0 +1,217 @@ +/* + * The contents of this file are subject to the terms of the Common Development and + * Distribution License (the License). You may not use this file except in compliance with the + * License. + * + * You can obtain a copy of the License at legal/CDDLv1.0.txt. See the License for the + * specific language governing permission and limitations under the License. + * + * When distributing Covered Software, include this CDDL Header Notice in each file and include + * the License file at legal/CDDLv1.0.txt. If applicable, add the following below the CDDL + * Header, with the fields enclosed by brackets [] replaced by your own identifying + * information: "Portions copyright [year] [name of copyright owner]". + * + * Copyright 2026 3A Systems, LLC. + */ +package org.opends.server.backends.pluggable; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Matchers.any; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.mock; + +import org.forgerock.opendj.ldap.ByteString; +import org.forgerock.opendj.ldap.ByteStringBuilder; +import org.opends.server.DirectoryServerTestCase; +import org.opends.server.TestCaseUtils; +import org.opends.server.backends.pluggable.DefaultIndexTest.DummyWriteableTransaction; +import org.opends.server.backends.pluggable.spi.AccessMode; +import org.opends.server.backends.pluggable.spi.Cursor; +import org.opends.server.backends.pluggable.spi.Storage; +import org.opends.server.backends.pluggable.spi.TreeName; +import org.opends.server.backends.pluggable.spi.WriteOperation; +import org.opends.server.core.ServerContext; +import org.opends.server.types.Attributes; +import org.testng.annotations.BeforeClass; +import org.testng.annotations.BeforeMethod; +import org.testng.annotations.Test; + +/** + * The compressed schema of a backend must not be shared with another backend that happens to + * address the same database, and a backend upgraded from a version that did share it must keep + * decoding the entries it wrote before the upgrade (issue #873). + */ +@SuppressWarnings("javadoc") +@Test(groups = { "precommit", "pluggablebackend" }, sequential = true) +public class PersistentCompressedSchemaTest extends DirectoryServerTestCase +{ + private static final String AD = "compressed_attributes"; + private static final String OC = "compressed_object_classes"; + + /** The pair every backend wrote to before the definitions were separated. */ + private static final TreeName LEGACY_AD = new TreeName("compressed_schema", AD); + private static final TreeName LEGACY_OC = new TreeName("compressed_schema", OC); + + private ServerContext serverContext; + private DummyWriteableTransaction txn; + private Storage storage; + + @BeforeClass + public void startServer() throws Exception + { + // Needed for the schema: loading a definition resolves its attribute type against the server's. + TestCaseUtils.startServer(); + serverContext = TestCaseUtils.getServerContext(); + } + + @BeforeMethod + public void setUp() throws Exception + { + // One transaction shared by every backend of a test stands for one database addressed by all of + // them - the JDBC deployment of the issue. On JE or PDB each backend would have its own. + txn = new DummyWriteableTransaction(); + storage = mock(Storage.class); + doAnswer(invocation -> { + ((WriteOperation) invocation.getArguments()[0]).run(txn); + return null; + }).when(storage).write(any(WriteOperation.class)); + } + + /** + * The heart of the issue: both backends allocate the first token of their own map for a different + * attribute description, so if they shared a tree the second would overwrite the definition of + * the first and the entries of the first would decode as the wrong attribute after a restart. + */ + @Test + public void backendsSharingOneDatabaseDoNotShareTheTokenSpace() throws Exception + { + // Both are opened before either writes, as they are at server start: each loads the same state + // and so holds the same next token. Opening the second one after the first had written would + // hide the issue, since it would have loaded the definition the first one just stored. + final PersistentCompressedSchema backendA = open("backendA", AccessMode.READ_WRITE); + final PersistentCompressedSchema backendB = open("backendB", AccessMode.READ_WRITE); + final ByteString encodedByA = encode(backendA, "cn"); + final ByteString encodedByB = encode(backendB, "sn"); + + // Re-opening is what a restart does: the definitions come back from the trees, not from memory. + assertThat(decode(open("backendA", AccessMode.READ_WRITE), encodedByA)).isEqualTo("cn"); + assertThat(decode(open("backendB", AccessMode.READ_WRITE), encodedByB)).isEqualTo("sn"); + + // and nothing was written under the prefix they used to share + assertThat(txn.treeExists(LEGACY_AD)).isFalse(); + assertThat(txn.treeExists(LEGACY_OC)).isFalse(); + } + + /** A backend upgraded from a version that wrote under the shared prefix finds its definitions. */ + @Test + public void definitionsWrittenBeforeTheUpgradeAreMigrated() throws Exception + { + final ByteString encoded = encode(open("backendA", AccessMode.READ_WRITE), "cn"); + makeStoreLookPreUpgrade("backendA"); + + assertThat(decode(open("backendA", AccessMode.READ_WRITE), encoded)).isEqualTo("cn"); + assertThat(txn.getRecordCount(ownTree("backendA", AD))).isEqualTo(txn.getRecordCount(LEGACY_AD)); + assertThat(txn.getRecordCount(ownTree("backendA", OC))).isEqualTo(txn.getRecordCount(LEGACY_OC)); + } + + /** + * The shared trees are read, never emptied: on a database addressed by several backends they may + * still be the only copy another backend has, and leaving them is what makes a downgrade possible. + */ + @Test + public void theSharedTreesSurviveTheMigration() throws Exception + { + encode(open("backendA", AccessMode.READ_WRITE), "cn"); + makeStoreLookPreUpgrade("backendA"); + final long legacyRecords = txn.getRecordCount(LEGACY_AD); + + open("backendA", AccessMode.READ_WRITE); + + assertThat(txn.getRecordCount(LEGACY_AD)).isEqualTo(legacyRecords); + } + + /** + * A migration interrupted halfway - a storage without transactions can stop between two records - + * is finished by the next open rather than leaving the backend with a partial token space. + */ + @Test + public void anInterruptedMigrationIsFinishedByTheNextOpen() throws Exception + { + final ByteString encoded = encode(open("backendA", AccessMode.READ_WRITE), "cn"); + makeStoreLookPreUpgrade("backendA"); + open("backendA", AccessMode.READ_WRITE); + + // undo one record of the completed migration, which is what an interrupted one would have left + txn.delete(ownTree("backendA", AD), firstKeyOf(ownTree("backendA", AD))); + + assertThat(decode(open("backendA", AccessMode.READ_WRITE), encoded)).isEqualTo("cn"); + assertThat(txn.getRecordCount(ownTree("backendA", AD))).isEqualTo(txn.getRecordCount(LEGACY_AD)); + } + + /** + * export-ldif and verify-index open the root container read-only, where the migration cannot run. + * The definitions must still be found, and nothing may be written. + */ + @Test + public void aReadOnlyOpenReadsTheSharedTreesWhereTheyLie() throws Exception + { + final ByteString encoded = encode(open("backendA", AccessMode.READ_WRITE), "cn"); + makeStoreLookPreUpgrade("backendA"); + + assertThat(decode(open("backendA", AccessMode.READ_ONLY), encoded)).isEqualTo("cn"); + assertThat(txn.treeExists(ownTree("backendA", AD))).isFalse(); + assertThat(txn.treeExists(ownTree("backendA", OC))).isFalse(); + } + + private PersistentCompressedSchema open(String backendId, AccessMode accessMode) throws Exception + { + return new PersistentCompressedSchema(serverContext, backendId, storage, txn, accessMode); + } + + private static TreeName ownTree(String backendId, String indexId) + { + return new TreeName("compressed_schema_" + backendId, indexId); + } + + private ByteString encode(PersistentCompressedSchema schema, String attributeName) throws Exception + { + final ByteStringBuilder builder = new ByteStringBuilder(); + schema.encodeAttribute(builder, Attributes.create(attributeName, "a value")); + return builder.toByteString(); + } + + private String decode(PersistentCompressedSchema schema, ByteString encoded) throws Exception + { + return schema.decodeAttribute(encoded.asReader()).getAttributeDescription().getAttributeType().getNameOrOID(); + } + + /** Moves the definitions of a backend back under the shared prefix, as an earlier version left them. */ + private void makeStoreLookPreUpgrade(String backendId) throws Exception + { + copyTree(ownTree(backendId, AD), LEGACY_AD); + copyTree(ownTree(backendId, OC), LEGACY_OC); + txn.deleteTree(ownTree(backendId, AD)); + txn.deleteTree(ownTree(backendId, OC)); + } + + private void copyTree(TreeName from, TreeName to) + { + txn.openTree(to, true); + try (Cursor cursor = txn.openCursor(from)) + { + while (cursor.next()) + { + txn.put(to, cursor.getKey(), cursor.getValue()); + } + } + } + + private ByteString firstKeyOf(TreeName treeName) + { + try (Cursor cursor = txn.openCursor(treeName)) + { + assertThat(cursor.next()).isTrue(); + return cursor.getKey(); + } + } +} From 63e3e7087a660fa46a91d5a37800eba380376f05 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Thu, 20 Aug 2026 11:33:29 +0300 Subject: [PATCH 2/3] [#873] Report an absent Cassandra tree only where no write can follow The Cassandra treeExists() took every InvalidQueryException for an absent tree, and the driver reports the whole INVALID protocol code that way - an absent keyspace, an unknown column, a table a coordinator has not caught up with during a rolling upgrade. Calling a populated tree absent has the compressed schema load nothing, restart its token allocation from zero and overwrite the definitions the entries already written were encoded with, which is the corruption this change exists to prevent. Only a read-only transaction, where no write can follow the answer, may still read a rejected query as an absent table; a writeable one has just created it through openTree(), so a rejection there fails the open as it did before. read(), getRecordCount() and the cursor of the JDBC backend now take the pure toTableName() that the existence probe already used, so reading a tree this backend does not own no longer enrols it in the map removeStorageFiles() drops: the guard covered treeExists() alone, and its test exercised only that while the migration counts the legacy tree and copies it out. The compressed schema no longer asks openTree() of a read-only open, which JEStorage grants regardless of createOnDemand, leaving two empty databases behind an offline export-ldif or verify-index of an unmigrated backend. --- .../server/backends/cassandra/CASStorage.java | 25 +++++--- .../server/backends/jdbc/JDBCStorage.java | 23 ++++--- .../pluggable/PersistentCompressedSchema.java | 23 ++++++- .../pluggable/spi/ReadableTransaction.java | 6 ++ .../server/backends/cassandra/TestCase.java | 63 +++++++++++++++++++ .../opends/server/backends/jdbc/TestCase.java | 18 ++++-- 6 files changed, 134 insertions(+), 24 deletions(-) diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/cassandra/CASStorage.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/cassandra/CASStorage.java index 82e2d86c75..b417a7ee9f 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/cassandra/CASStorage.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/cassandra/CASStorage.java @@ -259,17 +259,24 @@ public boolean treeExists(TreeName treeName) { prepared.get("SELECT key FROM "+getTableName()+" WHERE baseDN=:baseDN and indexId=:indexId LIMIT 1").bind() .setString("baseDN", treeName.getBaseDN()).setString("indexId", treeName.getIndexId()) ).one()!=null; - }catch (RuntimeException e) { + }catch (InvalidQueryException e) { // The backend's own table has not been created yet - a read-only open of a backend - // that was never written - so none of its trees can exist either. The driver reports - // it from prepare() as much as from execute(), and the statement cache may hand back - // the loader's failure wrapped, so the whole chain is searched. - for (Throwable cause=e; cause!=null; cause=cause.getCause()) { - if (cause instanceof InvalidQueryException) { - return false; - } + // that was never written, where openTree() creates nothing - so none of its trees + // can exist either. The driver reports it from prepare() as much as from execute(). + // + // Only a read-only open may answer "absent" here. InvalidQueryException carries the + // whole INVALID protocol code - an absent keyspace, an unknown column, a table a + // coordinator has not caught up with yet, which is what a rolling upgrade produces + // since schema agreement is never reached in a mixed-version cluster - and calling a + // populated tree absent would have the compressed schema restart its token allocation + // from zero and overwrite the definitions the entries already written were encoded + // with (#873). A writeable open has just run CREATE TABLE IF NOT EXISTS through + // openTree(), so a rejection there is a fault and must fail the open, as it did + // before this method existed and loadTrees() opened its cursor unconditionally. + if (accessMode.isWriteable()) { + throw e; } - throw e; + return false; } } 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 6e2f90f3a0..9f476d9b81 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 @@ -175,10 +175,17 @@ public void close() { .build(JDBCStorage::toTableName); /** - * The table a tree name maps to. A pure function of the name, so that a tree can be asked about - * without being entered into tree2table: treeExists() is asked about trees this backend does not - * own - the compressed schema probes the tree its definitions used to be shared under (#873) - - * and removeStorageFiles() drops every table tree2table names, so a probe must not enrol one. + * The table a tree name maps to. A pure function of the name, so that a tree can be read + * without being entered into tree2table: the compressed schema reads the tree its definitions + * used to be shared under (#873), a tree this backend does not own, and removeStorageFiles() + * drops every table tree2table names. + *

+ * Which of the two a statement takes therefore says who owns the tree it names: a path that + * creates or writes one - openTree(), clearTree(), deleteTree(), put(), update(), delete() - + * takes the enrolling {@link #getTableName(TreeName)}, and a read-only path - read(), + * getRecordCount(), isExistsTable() and the cursor - takes this one. Every tree this backend + * owns passes through openTree(name, true) as it is opened, so listTrees() still names the + * complete owned set. */ static String toTableName(TreeName treeName) { try { @@ -1028,7 +1035,7 @@ public ReadableTransactionImpl(Connection con) { @Override public ByteString read(TreeName treeName, ByteSequence key) { - try (final PreparedStatement statement=con.prepareStatement("select v from "+getTableName(treeName)+" where h="+hashParam(con)+" and k=?")){ + try (final PreparedStatement statement=con.prepareStatement("select v from "+toTableName(treeName)+" where h="+hashParam(con)+" and k=?")){ statement.setString(1,key2hash.get(ByteBuffer.wrap(key.toByteArray()))); statement.setBytes(2,real2db(key.toByteArray())); try(ResultSet rc=executeResultSet(statement)) { @@ -1046,7 +1053,7 @@ public Cursor openCursor(TreeName treeName) { @Override public long getRecordCount(TreeName treeName) { - try (final PreparedStatement statement=con.prepareStatement("select count(*) from "+getTableName(treeName)); + try (final PreparedStatement statement=con.prepareStatement("select count(*) from "+toTableName(treeName)); final ResultSet rc=executeResultSet(statement)){ return rc.next() ? rc.getLong(1) : 0; }catch (SQLException e) { @@ -1063,8 +1070,6 @@ public boolean treeExists(TreeName treeName) { // written from one it may not read, since every other statement here fails outright on a // table that does not exist. boolean isExistsTable(TreeName treeName) { - // toTableName() rather than getTableName(): asking whether a tree is there must not - // enrol it in tree2table, which is what removeStorageFiles() drops. final String tableName = toTableName(treeName); try { final DatabaseMetaData metaData = con.getMetaData(); @@ -1350,7 +1355,7 @@ final class CursorImpl implements Cursor { public CursorImpl(boolean isReadOnly, Connection con, TreeName treeName) { this.isReadOnly=isReadOnly; this.con=con; - this.tableName=getTableName(treeName); + this.tableName=toTableName(treeName); this.limitClause=((CachedConnection)con).parent.getClass().getName().contains("mysql") ? " limit ?,?" : " offset ? rows fetch next ? rows only"; } diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/PersistentCompressedSchema.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/PersistentCompressedSchema.java index 0e2d63c7fb..b2b9a28797 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/PersistentCompressedSchema.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/PersistentCompressedSchema.java @@ -184,8 +184,16 @@ protected void storeObjectClasses(final byte[] encodedObjectClasses, private void load(WriteableTransaction txn, boolean shouldCreate) throws StorageRuntimeException, InitializationException { - txn.openTree(adTreeName, shouldCreate); - txn.openTree(ocTreeName, shouldCreate); + if (shouldCreate) + { + // Asked for only where the trees may be created. A read-only open must leave the storage as + // it found it, and passing the flag on would not: JEStorage.openTree ignores createOnDemand + // and reaches env.openDatabase() with setAllowCreate(true), so an offline export-ldif or + // verify-index of a backend that has not migrated yet would leave two empty databases behind. + // Nothing below needs the trees open - every read is guarded by treeExists(). + txn.openTree(adTreeName, true); + txn.openTree(ocTreeName, true); + } if (needsLegacyDefinitions(txn)) { @@ -210,6 +218,17 @@ private void load(WriteableTransaction txn, boolean shouldCreate) * previous migration did not run to completion - on a storage without transactions the copy can * stop halfway. Once migrated, this backend's trees only ever grow, so they can no longer hold * fewer records than the legacy ones and the question is settled without reading either tree. + *

+ * That invariant holds in one direction only. A version from before the separation resumes + * writing to the legacy pair, so after a downgrade and a second upgrade the counts can agree + * while the definitions behind them have diverged, and nothing is migrated. Downgrading across + * the separation is not a supported path. + *

+ * A backend created after the upgrade on a database that already holds legacy definitions copies + * them although it has no entries of its own. Deliberate: what it inherits is a consistent + * token-to-definition mapping and costs one copy, whereas asking instead whether its own trees + * are absent would answer "nothing to migrate" for the half-copied trees an interrupted + * migration leaves behind - the one case this method exists to catch. */ private boolean needsLegacyDefinitions(ReadableTransaction txn) { diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/spi/ReadableTransaction.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/spi/ReadableTransaction.java index 0cb0db18b4..c84668754a 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/spi/ReadableTransaction.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/spi/ReadableTransaction.java @@ -62,6 +62,12 @@ public interface ReadableTransaction * JDBC backend does when no table of that name exists. Callers that must distinguish "never * written" from "written and since emptied", such as the compressed schema deciding whether it * has anything to migrate, need this instead. + *

+ * A storage whose trees have no existence of their own cannot keep that distinction: the + * Cassandra backend holds every tree of a backend as a partition of the one table named after + * the backend id, so it answers whether the partition holds a record and a tree that was emptied + * reports itself absent. Nothing may be inferred from a {@code false} beyond "there is nothing + * to read here". * * @param treeName * the tree name diff --git a/opendj-server-legacy/src/test/java/org/opends/server/backends/cassandra/TestCase.java b/opendj-server-legacy/src/test/java/org/opends/server/backends/cassandra/TestCase.java index 9e784d3f0a..d73da222ae 100644 --- a/opendj-server-legacy/src/test/java/org/opends/server/backends/cassandra/TestCase.java +++ b/opendj-server-legacy/src/test/java/org/opends/server/backends/cassandra/TestCase.java @@ -42,6 +42,7 @@ import com.datastax.oss.driver.api.core.AllNodesFailedException; import com.datastax.oss.driver.api.core.CqlSession; import com.datastax.oss.driver.api.core.config.DriverConfigLoader; +import com.datastax.oss.driver.api.core.servererrors.InvalidQueryException; import java.net.InetSocketAddress; import java.util.NoSuchElementException; @@ -134,6 +135,68 @@ public void run(WriteableTransaction txn) throws Exception { }); } + /** A storage under a backend id of its own, so that its table is one no other test has created. */ + private CASStorage openStorage(String backendId, AccessMode accessMode) throws Exception { + final CASBackendCfg backendCfg = mockCfg(CASBackendCfg.class); + when(backendCfg.getBackendId()).thenReturn(backendId); + when(backendCfg.getDBDirectory()).thenReturn("CASTestCase"); + final CASStorage storage = new CASStorage(backendCfg, null); + storage.open(accessMode); + return storage; + } + + /** + * The compressed schema asks whether a tree is there before anything has been written (#873), so + * treeExists() has to answer for a table that openTree() has not created yet. It may only answer + * where nothing can follow from the answer: a read has nothing to lose, but a writeable + * transaction has just created the table through openTree(), so a query rejected there is a + * fault. Reporting a populated tree absent would have the compressed schema allocate its tokens + * from zero again and overwrite the definitions its entries were encoded with. + */ + @Test + public void testTreeExistsAnswersForAMissingTable() throws Exception { + final CASStorage storage = openStorage("CASTreeExists", AccessMode.READ_WRITE); + final TreeName tree = new TreeName("testTreeExists", "tree"); + try { + // the keyspace is there - open(READ_WRITE) creates it - but the table is not + assertFalse(storage.read(new ReadOperation() { + @Override + public Boolean run(ReadableTransaction txn) throws Exception { + return txn.treeExists(tree); + } + }), "a read of a table that was never created finds no tree"); + + try { + storage.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + txn.treeExists(tree); + } + }); + fail("a writeable transaction must not report a rejected query as an absent tree"); + } catch (InvalidQueryException expected) { + // the table of a writeable open is created by openTree(), so its absence is a fault + } + + // Every tree of this backend is a partition of the one table, so it has no existence of + // its own: it is there once it holds a record, and not before. + storage.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + txn.openTree(tree, true); + assertFalse(txn.treeExists(tree), "an empty partition holds no record"); + txn.put(tree, key(0), value(0)); + assertTrue(txn.treeExists(tree)); + txn.deleteTree(tree); + assertFalse(txn.treeExists(tree)); + } + }); + } finally { + dropTree(storage, tree); + storage.close(); + } + } + private static void dropTree(CASStorage storage, final TreeName tree) { try { storage.write(new WriteOperation() { 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 2aefcdfa31..8ea79e9d61 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 @@ -313,9 +313,11 @@ public void testCompressedSchemaTableIsQualifiedByBackendId() throws Exception { } /** - * Asking whether a tree is there must not enrol it in the storage's tree map: removeStorageFiles() - * drops every table that map names, and the compressed schema asks about the tree its definitions - * used to be shared under - which on a shared database is another backend's to keep (#873). + * Reading a tree must not enrol it in the storage's tree map: removeStorageFiles() drops every + * table that map names, and the compressed schema reads the tree its definitions used to be + * shared under - which on a shared database is another backend's to keep (#873). Asking whether + * the tree is there is only the first of those reads: the migration counts it and copies it out + * too, so one guarded statement would not be enough. */ @Test public void testProbingATreeDoesNotPutItUpForRemoval() throws Exception { @@ -339,11 +341,19 @@ public void run(WriteableTransaction txn) throws Exception { other.read(new ReadOperation() { @Override public Void run(ReadableTransaction txn) throws Exception { + // every read the compressed schema runs against a tree it does not own: it asks + // whether the tree is there, counts it, reads a key of it and walks it (#873) assertTrue(txn.treeExists(foreign)); + assertEquals(txn.getRecordCount(foreign), 1); + assertEquals(txn.read(foreign, key(1)), value(1)); + try (final Cursor cursor = txn.openCursor(foreign)) { + assertTrue(cursor.next()); + assertEquals(cursor.getKey(), key(1)); + } return null; } }); - assertFalse(other.listTrees().contains(foreign), "a probed tree must not be listed for removal"); + assertFalse(other.listTrees().contains(foreign), "a tree only read must not be listed for removal"); other.removeStorageFiles(); From c655eada0fb02472cb7e0d3d15a82e5bb1aac0e2 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Thu, 20 Aug 2026 19:31:12 +0300 Subject: [PATCH 3/3] [#873] Leave a migration conflict for the storage to replay The migration wrapped everything that left copyMissingRecords() in an InitializationException. It runs inside the WriteOperation of RootContainer.open(), and PDBStorage.write() decides by type what to do with what leaves it: a RollbackException arrives there wrapped, misses the retry clause and fails the open permanently, where the same conflict used to be replayed. A StorageRuntimeException now leaves the migration with the type the storage gave it; everything else still fails the open, and ERR_COMPSCHEMA_CANNOT_MIGRATE names the pair of trees that failed rather than a prefix, as NOTE_COMPSCHEMA_MIGRATED now names both pairs. The JDBC read paths take a new readTableName(), answered from the tree2table memo where the tree is in it. Every tree the backend owns is enrolled as it is opened, so read() - the per-entry hot path - is a map lookup again rather than a JCA provider lookup and a SHA-224 digest per call, while a tree this backend does not own is still computed without being enrolled. The cursor reads through that name and deletes through the enrolling one, resolved once per cursor: a delete writes to the tree, so removeStorageFiles() has to be able to name it. The Cassandra treeExists() reads a rejected query as an absent tree only where the message names an absent table or keyspace. The type covers the whole INVALID code, so a table that exists with a shape this backend did not write would otherwise answer absent and have the compressed schema start again from zero. The migration is reported by RootContainer.open() after the transaction commits and only where something was copied. That line is the only evidence the path emits, and one standing for a copy a rollback undid, or repeated once per replay, says less than none. Three guards no test could falsify are covered now: a shared token standing for another attribute must not overwrite this backend's own definition, a migration that cannot complete fails the open, and a storage failure passes through with its own type. The read-only test can fail at last, its fixture materializing a tree whenever one is asked for, as JEStorage does. The javadoc says what TreeName states about '/', what loading the shared trees first settles in a read-only open and what it does not, why isExistsTable() sits on the readable transaction, what probing on every open costs, and what naming these two trees after the backend id costs when a backend is re-created under another id. --- .../server/backends/cassandra/CASStorage.java | 47 +++++-- .../server/backends/jdbc/JDBCStorage.java | 48 +++++-- .../pluggable/PersistentCompressedSchema.java | 83 ++++++++++-- .../backends/pluggable/RootContainer.java | 3 + .../org/opends/messages/backend.properties | 6 +- .../opends/server/backends/jdbc/TestCase.java | 51 +++++++ .../backends/pluggable/DefaultIndexTest.java | 3 +- .../PersistentCompressedSchemaTest.java | 126 +++++++++++++++++- 8 files changed, 328 insertions(+), 39 deletions(-) diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/cassandra/CASStorage.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/cassandra/CASStorage.java index b417a7ee9f..920b866815 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/cassandra/CASStorage.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/cassandra/CASStorage.java @@ -27,6 +27,7 @@ import java.util.NoSuchElementException; import java.util.Objects; import java.util.Set; +import java.util.regex.Pattern; import org.forgerock.i18n.LocalizableMessage; import org.forgerock.i18n.slf4j.LocalizedLogger; @@ -205,6 +206,13 @@ public void write(WriteOperation writeOperation) throws Exception { System.setProperty("datastax-java-driver.profiles."+profile+".basic.request.timeout", "30 seconds"); } } + // The wordings a server uses for a table or a keyspace that is not there. Matched rather than + // the exception type, which covers the whole INVALID protocol code: see namesAnAbsentTable(). + // "Undefined column name ..." deliberately does not match - that table exists. + static final Pattern ABSENT_TABLE=Pattern.compile( + "unconfigured (table|columnfamily)|(table|keyspace)[^,]* does not exist|undefined (table|keyspace)", + Pattern.CASE_INSENSITIVE); + private final class TransactionImpl implements ReadableTransaction,WriteableTransaction { final AccessMode accessMode; @@ -264,22 +272,41 @@ public boolean treeExists(TreeName treeName) { // that was never written, where openTree() creates nothing - so none of its trees // can exist either. The driver reports it from prepare() as much as from execute(). // - // Only a read-only open may answer "absent" here. InvalidQueryException carries the - // whole INVALID protocol code - an absent keyspace, an unknown column, a table a - // coordinator has not caught up with yet, which is what a rolling upgrade produces - // since schema agreement is never reached in a mixed-version cluster - and calling a - // populated tree absent would have the compressed schema restart its token allocation - // from zero and overwrite the definitions the entries already written were encoded - // with (#873). A writeable open has just run CREATE TABLE IF NOT EXISTS through - // openTree(), so a rejection there is a fault and must fail the open, as it did - // before this method existed and loadTrees() opened its cursor unconditionally. - if (accessMode.isWriteable()) { + // Narrowly, twice over, because calling a populated tree absent would have the + // compressed schema restart its token allocation from zero and overwrite the + // definitions the entries already written were encoded with (#873): + // + // - InvalidQueryException carries the whole INVALID protocol code, so the rejection + // has to name an absent table or keyspace. An undefined column, say, means the + // table is there with a shape this backend did not write, and answering "absent" + // for it would be that same corruption; + // - and only a read-only open may answer it at all. A writeable open has just run + // CREATE TABLE IF NOT EXISTS through openTree(), so a rejection there is a fault + // and must fail the open, as it did before this method existed and loadTrees() + // opened its cursor unconditionally. This is also where a table a coordinator has + // not caught up with lands - what a rolling upgrade produces, since schema + // agreement is never reached in a mixed-version cluster - and it fails loudly + // rather than being taken for a table that was never created. + if (accessMode.isWriteable() || !namesAnAbsentTable(e)) { throw e; } return false; } } + /** + * Whether a rejected query says that the table or the keyspace is not there, as opposed to + * anything else the INVALID protocol code covers. The driver offers nothing finer than the + * server's own message - the code is one value for the whole bucket - so the wordings of + * the server are matched, across the versions that changed them ("unconfigured + * columnfamily" became "unconfigured table", and a keyspace is reported as not existing). + * A wording that is not among them is not read as an absent table: it fails the open, which + * is the side to err on. + */ + private boolean namesAnAbsentTable(InvalidQueryException e) { + return e.getMessage()!=null && ABSENT_TABLE.matcher(e.getMessage()).find(); + } + @Override public void deleteTree(TreeName treeName) { checkReadOnly(); 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 9f476d9b81..5deec484ec 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 @@ -183,7 +183,8 @@ public void close() { * Which of the two a statement takes therefore says who owns the tree it names: a path that * creates or writes one - openTree(), clearTree(), deleteTree(), put(), update(), delete() - * takes the enrolling {@link #getTableName(TreeName)}, and a read-only path - read(), - * getRecordCount(), isExistsTable() and the cursor - takes this one. Every tree this backend + * getRecordCount(), isExistsTable() and the cursor - takes {@link #readTableName(TreeName)}, + * which computes this only for a tree that is not enrolled already. Every tree this backend * owns passes through openTree(name, true) as it is opened, so listTrees() still names the * complete owned set. */ @@ -207,6 +208,22 @@ String getTableName(TreeName treeName) { return tree2table.get(treeName); } + /** + * The table a tree name maps to, for a statement that only reads it. Answered from the memo of + * {@link #getTableName(TreeName)} where the tree is in it, and computed without being put there + * otherwise. + *

+ * Every tree this backend owns is enrolled as it is opened, so the per-entry read path stays a + * map lookup: {@link #toTableName(TreeName)} takes a JCA provider lookup and a digest per call, + * which read() would otherwise pay for every entry of every search. Only a tree this backend + * does not own - the shared compressed schema tree the migration of #873 reads - is computed, + * twice per open of the backend. + */ + String readTableName(TreeName treeName) { + final String enrolled=tree2table.getIfPresent(treeName); + return enrolled!=null ? enrolled : toTableName(treeName); + } + /** * 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 @@ -1035,7 +1052,7 @@ public ReadableTransactionImpl(Connection con) { @Override public ByteString read(TreeName treeName, ByteSequence key) { - try (final PreparedStatement statement=con.prepareStatement("select v from "+toTableName(treeName)+" where h="+hashParam(con)+" and k=?")){ + try (final PreparedStatement statement=con.prepareStatement("select v from "+readTableName(treeName)+" where h="+hashParam(con)+" and k=?")){ statement.setString(1,key2hash.get(ByteBuffer.wrap(key.toByteArray()))); statement.setBytes(2,real2db(key.toByteArray())); try(ResultSet rc=executeResultSet(statement)) { @@ -1053,7 +1070,7 @@ public Cursor openCursor(TreeName treeName) { @Override public long getRecordCount(TreeName treeName) { - try (final PreparedStatement statement=con.prepareStatement("select count(*) from "+toTableName(treeName)); + try (final PreparedStatement statement=con.prepareStatement("select count(*) from "+readTableName(treeName)); final ResultSet rc=executeResultSet(statement)){ return rc.next() ? rc.getLong(1) : 0; }catch (SQLException e) { @@ -1066,11 +1083,13 @@ public boolean treeExists(TreeName treeName) { return isExistsTable(treeName); } - // Readable, not writeable: a read-only open has to be able to tell a tree that was never - // written from one it may not read, since every other statement here fails outright on a - // table that does not exist. + // Readable, not writeable: the caller that asks about a tree this backend does not own is + // the compressed schema migration (#873), which probes the shared tree from the writeable + // transaction of RootContainer.open() but must not create or enrol it. Answering that from + // the readable transaction keeps the probe available to every reader, and costs nothing: + // the writeable one inherits it. boolean isExistsTable(TreeName treeName) { - final String tableName = toTableName(treeName); + final String tableName = readTableName(treeName); try { final DatabaseMetaData metaData = con.getMetaData(); // asked of the catalog by name: openTree(createOnDemand) calls this for every tree @@ -1338,7 +1357,10 @@ static int compareKeys(byte[] key1, byte[] key2) { // repositioning transfer "fetchsize" rows over the network (#860). final class CursorImpl implements Cursor { final Connection con; + final TreeName treeName; final String tableName; + // the enrolling name, resolved once and only if this cursor ever deletes + String writeTableName; final boolean isReadOnly; final int batchSize=Math.max(1,Integer.getInteger("org.openidentityplatform.opendj.jdbc.fetchsize",1000)); final int initialBatchSize=Math.min(batchSize,Math.max(1,Integer.getInteger("org.openidentityplatform.opendj.jdbc.fetchsize.initial",32))); @@ -1355,7 +1377,10 @@ final class CursorImpl implements Cursor { public CursorImpl(boolean isReadOnly, Connection con, TreeName treeName) { this.isReadOnly=isReadOnly; this.con=con; - this.tableName=toTableName(treeName); + this.treeName=treeName; + // the read statements below take the non-enrolling name: a cursor is how the migration + // of #873 reads the shared tree, and reading a tree must not put it up for removal + this.tableName=readTableName(treeName); this.limitClause=((CachedConnection)con).parent.getClass().getName().contains("mysql") ? " limit ?,?" : " offset ? rows fetch next ? rows only"; } @@ -1436,7 +1461,12 @@ public void delete() throws NoSuchElementException, UnsupportedOperationExceptio if (isReadOnly) { throw new UnsupportedOperationException(); } - try (final PreparedStatement statement=con.prepareStatement("delete from "+tableName+" where h="+hashParam(con)+" and k=?")){ + if (writeTableName==null) { + // the enrolling name, unlike the read statements above: this writes to the tree, so + // it is one this backend owns, and removeStorageFiles() has to know about it + writeTableName=getTableName(treeName); + } + try (final PreparedStatement statement=con.prepareStatement("delete from "+writeTableName+" where h="+hashParam(con)+" and k=?")){ statement.setString(1,key2hash.get(ByteBuffer.wrap(db2real(currentKeyDb)))); statement.setBytes(2,currentKeyDb); execute(statement); diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/PersistentCompressedSchema.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/PersistentCompressedSchema.java index b2b9a28797..c3a9f42879 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/PersistentCompressedSchema.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/PersistentCompressedSchema.java @@ -89,6 +89,9 @@ final class PersistentCompressedSchema extends CompressedSchema /** The storage in which the trees are held. */ private final Storage storage; + /** How many definitions the migration copied, reported by {@link #reportMigration()} once committed. */ + private long migratedDefinitions; + private final ByteStringBuilder storeAttributeWriterBuffer = new ByteStringBuilder(); private final ASN1Writer storeAttributeWriter = ASN1.getWriter(storeAttributeWriterBuffer); private final ByteStringBuilder storeObjectClassesWriterBuffer = new ByteStringBuilder(); @@ -127,10 +130,22 @@ final class PersistentCompressedSchema extends CompressedSchema } /** - * Qualifies the legacy prefix with the backend id. {@link TreeName} splits its string form on - * '/' and states that no component may contain one, and nothing rules a '/' out of a backend id - * - ds-cfg-backend-id is a plain string - so it is escaped rather than passed through. The - * escape is reversible, so two distinct backend ids can never produce one prefix. + * Qualifies the legacy prefix with the backend id. {@link TreeName} documents that it assumes no + * name component contains a '/', and {@code TreeName.valueOf} splits the string form at the last + * one, so a prefix carrying a '/' would come back as a different name than it went in as. + * Nothing rules a '/' out of a backend id - ds-cfg-backend-id is a plain string - so it is + * escaped rather than passed through. The escape is reversible, so two distinct backend ids can + * never produce one prefix. + *

+ * The qualifier is the backend id rather than a base DN because this object is built once per + * {@link RootContainer}, before any entry container exists, and a backend holding two base DNs + * has no single prefix to borrow. The price is that these two trees, alone among the trees of a + * backend, do not follow the entries when a JDBC backend is deleted and re-created under another + * id over the same database: every other tree is named from its base DN and is found again, + * while these are not, leaving the token allocation to restart at zero over entries encoded with + * the definitions of the old id. Changing a backend id over populated storage needs an export + * and a re-import - as it always has on Cassandra, where the one table of a backend is named + * after the id and the entries do not survive the rename either. */ private static String treePrefix(String backendId) { @@ -204,8 +219,12 @@ private void load(WriteableTransaction txn, boolean shouldCreate) else { // Read-only: nothing may be written, so the legacy definitions are read where they lie. - // Loaded first, so that anything this backend has already migrated and since added under - // its own prefix wins for the same token. + // Loaded first, so that a token this backend has already migrated, and since re-used under + // its own prefix, decodes to its own definition. That settles the decode map only: + // CompressedSchema.loadAttributeToMaps keys the encode map by attribute description, so a + // legacy description displaced from a token stays in it and would encode to a token that + // now decodes to another attribute. Harmless only because nothing encodes during a + // read-only open - export-ldif and verify-index decode. loadTrees(txn, LEGACY_OC_TREE_NAME, LEGACY_AD_TREE_NAME); } } @@ -222,7 +241,13 @@ private void load(WriteableTransaction txn, boolean shouldCreate) * That invariant holds in one direction only. A version from before the separation resumes * writing to the legacy pair, so after a downgrade and a second upgrade the counts can agree * while the definitions behind them have diverged, and nothing is migrated. Downgrading across - * the separation is not a supported path. + * the separation is not a supported path - the legacy trees are left in place so that a backend + * still running an earlier version can go on reading them, not so that this one can go back. + *

+ * The question is settled again on every open, nothing recording that it has been answered + * before: two existence probes, and the record counts only where the legacy trees are still + * there. That is what not writing a marker of this backend's own into a database it may be + * sharing with another one costs. *

* A backend created after the upgrade on a database that already holds legacy definitions copies * them although it has no entries of its own. Deliberate: what it inherits is a consistent @@ -245,12 +270,26 @@ private long recordCount(ReadableTransaction txn, TreeName treeName) } private void migrateLegacyDefinitions(WriteableTransaction txn) throws InitializationException + { + migratedDefinitions = migrateTree(txn, LEGACY_AD_TREE_NAME, adTreeName) + + migrateTree(txn, LEGACY_OC_TREE_NAME, ocTreeName); + } + + private long migrateTree(WriteableTransaction txn, TreeName from, TreeName to) throws InitializationException { try { - final long copied = copyMissingRecords(txn, LEGACY_AD_TREE_NAME, adTreeName) - + copyMissingRecords(txn, LEGACY_OC_TREE_NAME, ocTreeName); - logger.info(NOTE_COMPSCHEMA_MIGRATED, copied, backendId, LEGACY_TREE_PREFIX, adTreeName.getBaseDN()); + return copyMissingRecords(txn, from, to); + } + catch (final StorageRuntimeException e) + { + // Left with the type the storage gave it. The migration runs inside the WriteOperation of + // RootContainer.open(), and PDBStorage.write() decides by type what to do with what leaves + // it: a transaction conflict reaches its retry as a RollbackException only, so wrapping it + // here would turn a conflict that used to be replayed into a permanent failure of the open. + // Nothing is lost - a storage failure that is not a conflict still fails the open, through + // ERR_OPEN_ENV_FAIL, and leaves the trees as they were, since the transaction rolls back. + throw e; } catch (final Exception e) { @@ -258,15 +297,33 @@ private void migrateLegacyDefinitions(WriteableTransaction txn) throws Initializ // Deliberately fatal to the open. Loading no definitions at all would restart token // allocation from zero and decode every entry written so far as the wrong attributes, // without reporting anything. - throw new InitializationException(ERR_COMPSCHEMA_CANNOT_MIGRATE.get( - backendId, LEGACY_TREE_PREFIX, adTreeName.getBaseDN(), stackTraceToSingleLineString(e)), e); + throw new InitializationException( + ERR_COMPSCHEMA_CANNOT_MIGRATE.get(backendId, from, to, stackTraceToSingleLineString(e)), e); + } + } + + /** + * Reports a migration that has been committed, and reports nothing where none was needed. + *

+ * Called by {@link RootContainer#open} once the transaction the migration ran in has committed, + * and only then: a copy is undone by a rollback - a later failure of the same transaction, or a + * conflict PersistIt replays - and this line is the only evidence the migration path emits, so + * one standing for a copy that was rolled back, or repeated once per replay, would be worse than + * none at all. + */ + void reportMigration() + { + if (migratedDefinitions > 0) + { + logger.info(NOTE_COMPSCHEMA_MIGRATED, migratedDefinitions, backendId, + LEGACY_AD_TREE_NAME, LEGACY_OC_TREE_NAME, adTreeName, ocTreeName); } } /** * Copies the records of {@code from} that {@code to} does not already hold, and returns how many * were copied. The legacy tree is left in place: on a shared database it may still be the only - * copy another backend has, and leaving it is what makes a downgrade possible. + * copy a backend that has not been upgraded yet has. *

* A key already present in {@code to} is never overwritten, which is what makes this safe to * re-run after an interrupted migration and safe against a legacy tree that a backend of an diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/RootContainer.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/RootContainer.java index 6cb74cfc61..ad1dcd6380 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/RootContainer.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/RootContainer.java @@ -141,6 +141,9 @@ public void run(WriteableTransaction txn) throws Exception openAndRegisterEntryContainers(txn, config.getBaseDN(), accessMode); } }); + // after the write, never inside it: a compressed schema migration is only worth reporting + // once the transaction that copied it has committed, and a replayed operation runs twice + compressedSchema.reportMigration(); } catch(StorageRuntimeException e) { diff --git a/opendj-server-legacy/src/messages/org/opends/messages/backend.properties b/opendj-server-legacy/src/messages/org/opends/messages/backend.properties index 18a07eb991..eebfe842ef 100644 --- a/opendj-server-legacy/src/messages/org/opends/messages/backend.properties +++ b/opendj-server-legacy/src/messages/org/opends/messages/backend.properties @@ -1108,9 +1108,9 @@ ERR_SERVICE_DISCOVERY_CONFIG_MANAGER_LISTENER_615=Registering Service Discovery NOTE_IMPORT_MIGRATION_START_616=Migrating %s entries for base DN %s so that they are preserved by the partial import WARN_PSEARCH_BACKEND_UNAVAILABLE_617=The persistent search is being terminated because backend %s is \ no longer available -NOTE_COMPSCHEMA_MIGRATED_618=Migrated %d compressed schema definitions of backend '%s' from the shared tree '%s' \ - to '%s'. The shared tree is left untouched: on a storage where several backends address one database, such as a \ - JDBC backend, it may still hold the definitions of another backend +NOTE_COMPSCHEMA_MIGRATED_618=Migrated %d compressed schema definitions of backend '%s' from the shared trees \ + '%s' and '%s' to '%s' and '%s'. The shared trees are left untouched: on a storage where several backends address \ + one database, such as a JDBC backend, they may still hold the definitions of another backend ERR_COMPSCHEMA_CANNOT_MIGRATE_619=The compressed schema definitions of backend '%s' could not be migrated from \ the shared tree '%s' to '%s': %s. The backend cannot be opened, because its entries were encoded against the \ definitions that were not migrated and would decode as the wrong attributes 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 8ea79e9d61..cd8057efef 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 @@ -379,6 +379,57 @@ public void run(WriteableTransaction txn) throws Exception { } } + /** + * The other side of the same rule: a cursor reads through the non-enrolling name, but deleting + * through one writes to the tree, so it is a tree this backend owns and removeStorageFiles() + * has to be able to name it. + */ + @Test + public void testDeletingThroughACursorPutsTheTreeUpForRemoval() throws Exception { + final TreeName tree = new TreeName("testCursorDelete", "tree"); + final JDBCStorage owner = new JDBCStorage(createBackendCfg(), null); + owner.open(AccessMode.READ_WRITE); + owner.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + txn.openTree(tree, true); + txn.put(tree, key(1), value(1)); + } + }); + owner.close(); + + // a storage that never opened that tree, so nothing but the delete can enrol it + final JDBCStorage other = new JDBCStorage(createBackendCfg(), null); + try { + other.open(AccessMode.READ_WRITE); + other.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + try (final Cursor cursor = txn.openCursor(tree)) { + assertTrue(cursor.next()); + cursor.delete(); + } + } + }); + assertTrue(other.listTrees().contains(tree), "a tree written through a cursor must be listed for removal"); + } finally { + other.close(); + final JDBCStorage cleanup = new JDBCStorage(createBackendCfg(), null); + try { + cleanup.open(AccessMode.READ_WRITE); + cleanup.write(new WriteOperation() { + @Override + public void run(WriteableTransaction txn) throws Exception { + txn.deleteTree(tree); + } + }); + } catch (Exception ignored) { + } finally { + cleanup.close(); + } + } + } + private static boolean isExistingTable(Connection con, String tableName) throws SQLException { try (final ResultSet rs = con.getMetaData().getTables(null, null, null, new String[]{"TABLE"})) { while (rs.next()) { diff --git a/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/DefaultIndexTest.java b/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/DefaultIndexTest.java index ebea2d40b4..00cbcec6e3 100644 --- a/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/DefaultIndexTest.java +++ b/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/DefaultIndexTest.java @@ -126,7 +126,8 @@ private static DefaultIndex newIndex(String name, int indexLimit, EnumSet> storage = new HashMap<>(); diff --git a/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/PersistentCompressedSchemaTest.java b/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/PersistentCompressedSchemaTest.java index 81a0cdc4f8..f2b98de18a 100644 --- a/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/PersistentCompressedSchemaTest.java +++ b/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/PersistentCompressedSchemaTest.java @@ -19,7 +19,9 @@ import static org.mockito.Matchers.any; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; +import static org.testng.Assert.fail; +import org.forgerock.opendj.ldap.ByteSequence; import org.forgerock.opendj.ldap.ByteString; import org.forgerock.opendj.ldap.ByteStringBuilder; import org.opends.server.DirectoryServerTestCase; @@ -28,10 +30,12 @@ import org.opends.server.backends.pluggable.spi.AccessMode; import org.opends.server.backends.pluggable.spi.Cursor; import org.opends.server.backends.pluggable.spi.Storage; +import org.opends.server.backends.pluggable.spi.StorageRuntimeException; import org.opends.server.backends.pluggable.spi.TreeName; import org.opends.server.backends.pluggable.spi.WriteOperation; import org.opends.server.core.ServerContext; import org.opends.server.types.Attributes; +import org.opends.server.types.InitializationException; import org.testng.annotations.BeforeClass; import org.testng.annotations.BeforeMethod; import org.testng.annotations.Test; @@ -53,7 +57,7 @@ public class PersistentCompressedSchemaTest extends DirectoryServerTestCase private static final TreeName LEGACY_OC = new TreeName("compressed_schema", OC); private ServerContext serverContext; - private DummyWriteableTransaction txn; + private SharedDatabase txn; private Storage storage; @BeforeClass @@ -69,7 +73,7 @@ public void setUp() throws Exception { // One transaction shared by every backend of a test stands for one database addressed by all of // them - the JDBC deployment of the issue. On JE or PDB each backend would have its own. - txn = new DummyWriteableTransaction(); + txn = new SharedDatabase(); storage = mock(Storage.class); doAnswer(invocation -> { ((WriteOperation) invocation.getArguments()[0]).run(txn); @@ -150,7 +154,9 @@ public void anInterruptedMigrationIsFinishedByTheNextOpen() throws Exception /** * export-ldif and verify-index open the root container read-only, where the migration cannot run. - * The definitions must still be found, and nothing may be written. + * The definitions must still be found, and nothing may be written - not even the empty trees an + * openTree() would leave behind, which is why {@link SharedDatabase} creates one whenever it is + * asked for a tree at all. */ @Test public void aReadOnlyOpenReadsTheSharedTreesWhereTheyLie() throws Exception @@ -163,6 +169,83 @@ public void aReadOnlyOpenReadsTheSharedTreesWhereTheyLie() throws Exception assertThat(txn.treeExists(ownTree("backendA", OC))).isFalse(); } + /** + * The migration fills the gaps of this backend and touches nothing else: where the shared trees + * and this backend disagree about what a token stands for - which is the state issue #873 leaves + * behind, two backends having allocated the same token for different attributes - the definition + * this backend's own entries were encoded against is the one that survives. + */ + @Test + public void aDefinitionOfThisBackendIsNeverOverwrittenByTheSharedOne() throws Exception + { + final ByteString encodedByA = encode(open("backendA", AccessMode.READ_WRITE), "cn"); + + // What another backend left under the shared prefix before the upgrade: the same first token, + // standing for a different attribute, and one definition beyond what backendA holds, so that + // the record counts send the migration on its way. + final PersistentCompressedSchema backendB = open("backendB", AccessMode.READ_WRITE); + encode(backendB, "sn"); + encode(backendB, "description"); + copyTree(ownTree("backendB", AD), LEGACY_AD); + copyTree(ownTree("backendB", OC), LEGACY_OC); + + assertThat(decode(open("backendA", AccessMode.READ_WRITE), encodedByA)).isEqualTo("cn"); + // and the definition backendA did not have was still copied, so the migration did run + assertThat(txn.getRecordCount(ownTree("backendA", AD))).isEqualTo(txn.getRecordCount(LEGACY_AD)); + } + + /** + * A migration that cannot complete is fatal to the open: carrying on with the definitions half + * copied would restart the token allocation part way through the map the entries were encoded + * with, and report nothing. + */ + @Test + public void aMigrationThatCannotCompleteFailsTheOpen() throws Exception + { + encode(open("backendA", AccessMode.READ_WRITE), "cn"); + makeStoreLookPreUpgrade("backendA"); + final RuntimeException failure = new IllegalStateException("no space left on device"); + txn.failEveryWriteWith(failure); + + try + { + open("backendA", AccessMode.READ_WRITE); + fail("the open must not succeed on a migration that failed"); + } + catch (final InitializationException e) + { + assertThat(e.getCause()).isSameAs(failure); + // and the message names the backend and both trees, since which of the two failed is the point + assertThat(e.getMessage()).contains("backendA") + .contains(LEGACY_AD.toString()).contains(ownTree("backendA", AD).toString()); + } + } + + /** + * A failure the storage reports as its own is left with the type it was given. The migration runs + * inside the WriteOperation of {@link RootContainer#open}, and PDBStorage.write() decides by type + * what to do with what leaves it: a transaction conflict it would have replayed must not come out + * as something else, or the open fails where it used to be retried. + */ + @Test + public void aStorageFailureIsLeftForTheStorageToRecognize() throws Exception + { + encode(open("backendA", AccessMode.READ_WRITE), "cn"); + makeStoreLookPreUpgrade("backendA"); + final StorageRuntimeException conflict = new StorageRuntimeException("transaction rolled back"); + txn.failEveryWriteWith(conflict); + + try + { + open("backendA", AccessMode.READ_WRITE); + fail("the open must not succeed on a migration that failed"); + } + catch (final StorageRuntimeException e) + { + assertThat(e).isSameAs(conflict); + } + } + private PersistentCompressedSchema open(String backendId, AccessMode accessMode) throws Exception { return new PersistentCompressedSchema(serverContext, backendId, storage, txn, accessMode); @@ -214,4 +297,41 @@ private ByteString firstKeyOf(TreeName treeName) return cursor.getKey(); } } + + /** + * The one database every backend of a test addresses. Two behaviours of a real storage are + * modelled on purpose, so that the assertions above can fail: + *

    + *
  • a tree is materialized whenever one is asked for, whether or not creation was requested. + * That is what JEStorage does - dbConfig() sets allowCreate, and openTree() reaches + * env.openDatabase() with it, whatever createOnDemand says - so a read-only open that asks at all + * leaves empty databases behind an offline tool run;
    • + *
  • a write can be made to fail, which is the only way to reach the migration's failure path.
    • + *
+ */ + private static final class SharedDatabase extends DummyWriteableTransaction + { + private RuntimeException failure; + + void failEveryWriteWith(RuntimeException failure) + { + this.failure = failure; + } + + @Override + public void openTree(TreeName name, boolean createOnDemand) + { + super.openTree(name, true); + } + + @Override + public void put(TreeName treeName, ByteSequence key, ByteSequence value) + { + if (failure != null) + { + throw failure; + } + super.put(treeName, key, value); + } + } }