From 480612bb5502e6d006026e9314c853e02fdeb8fe Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Wed, 9 Sep 2026 16:51:15 +0300 Subject: [PATCH 1/6] [#990] Drop what a previous index left behind instead of adopting it when an index is added The name of an index tree is a pure function of the base DN, the attribute and the index id, and open() creates a tree only where there is none, so an index added for an attribute another index served reopened exactly the trees that one left behind - with their stale content and with the TRUSTED flag their state records carried. Searches then answered out of them and missed every entry written while no configuration named them; where only the state record survived its tree, on JDBC, the index came back empty and trusted. The three paths which open an index the configuration is adding - the index add listener, the VLV index add listener and AttributeIndex.createIndex, reached when an index type is declared again - now drop that tree and its state record first, so the index is created empty and untrusted and asks to be rebuilt, exactly as any other index added to a backend holding entries. A rebuild regenerates everything discarded. The set of trees the storage holds is read before the write which uses it: on JDBC listTrees() borrows a connection of its own, and a snapshot taken before the change is also what a replayed attempt needs. WARN_INDEX_ADD_DISCARDED_LEFTOVER_TREES names the index and the base DN in the change result and in the error log. --- .../backends/pluggable/AttributeIndex.java | 84 ++- .../backends/pluggable/EntryContainer.java | 24 +- .../server/backends/pluggable/VLVIndex.java | 39 ++ .../org/opends/messages/backend.properties | 5 + .../IndexAddedOverLeftoverTreesTest.java | 570 ++++++++++++++++++ 5 files changed, 718 insertions(+), 4 deletions(-) create mode 100644 opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/IndexAddedOverLeftoverTreesTest.java diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/AttributeIndex.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/AttributeIndex.java index 055fdd6240..4147ee9ed4 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/AttributeIndex.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/AttributeIndex.java @@ -43,6 +43,7 @@ import org.forgerock.opendj.ldap.Assertion; import org.forgerock.opendj.ldap.ByteSequence; import org.forgerock.opendj.ldap.ByteString; +import org.forgerock.opendj.ldap.DN; import org.forgerock.opendj.ldap.DecodeException; import org.forgerock.opendj.ldap.schema.AttributeType; import org.forgerock.opendj.ldap.schema.MatchingRule; @@ -448,6 +449,74 @@ void open(WriteableTransaction txn, boolean createOnDemand) throws StorageRuntim config.addChangeListener(this); } + /** + * Opens this index as one the configuration is adding, dropping first whatever an index of the + * same name left behind. + *

+ * The name of an index tree is a pure function of the base DN, the attribute and the index id, and + * {@link #open} creates a tree only where there is none, so an index added for an attribute + * another index served reopens exactly the trees that one left behind - with their content and + * with the TRUSTED flag their {@code state} records carry. Neither is this index's: what those + * trees hold is what the backend was told before the configuration stopped naming them, every + * entry written in between is missing from it, and TRUSTED has searches answer out of it all the + * same. A rebuild regenerates all of it and nothing else is lost with it, so it is dropped here + * rather than adopted, which leaves this index where any other index added to a backend holding + * entries starts: empty, untrusted and asking to be rebuilt (#990). + * + * @param txn a non null transaction + * @param storedTrees the trees the storage holds, read before this transaction was opened + * @return true if anything left behind was dropped + * @throws StorageRuntimeException if an error occurs while opening the index + */ + boolean openAsAdded(WriteableTransaction txn, Set storedTrees) throws StorageRuntimeException + { + boolean dropped = false; + for (Index index : indexIdToIndexes.values()) + { + dropped |= dropLeftoversOf(txn, index, storedTrees); + } + open(txn, true); + return dropped; + } + + /** + * Drops the tree an index about to be opened would adopt, and the {@code state} record which goes + * with it. + *

+ * The record can outlive the tree on its own: on JDBC a tree is dropped by DDL which commits of + * its own accord while the record is deleted by the transaction, so a rollback in between leaves + * the record over a tree which is gone, and the index opened next is created empty and read back + * as trusted. It is therefore taken out whether a tree was found for it or not. + *

+ * No search can be reading what is dropped here, so this does not take the exclusive lock + * {@link #deleteIndex} takes: an index which is only being added is in no map a search reaches, + * and the trees it would have adopted are named by nothing until it opens them. + * + * @return true if a tree or a record was dropped + */ + private boolean dropLeftoversOf(WriteableTransaction txn, Index index, Set storedTrees) + { + if (storedTrees.contains(index.getName())) + { + // Deletes the state record along with the tree. + entryContainer.deleteTree(txn, index); + return true; + } + return state.deleteRecord(txn, index.getName()); + } + + /** + * Tells the operator that trees left behind were discarded rather than adopted, and puts it in the + * error log as well: the session which submitted the change ends, and what a backend was left + * holding has to be findable afterwards. + */ + static void reportDiscardedLeftovers(ConfigChangeResult ccr, Object indexName, DN baseDN) + { + final LocalizableMessage message = WARN_INDEX_ADD_DISCARDED_LEFTOVER_TREES.get(indexName, baseDN); + ccr.addMessage(message); + logger.warn(message); + } + @Override public void close() { @@ -945,6 +1014,12 @@ public synchronized ConfigChangeResult applyConfigurationChange(final BackendInd ccr.addMessage(rebuildMessage); } + // Read outside the write which uses it: listTrees() borrows a connection of its own on JDBC, + // which a transaction already holding one of the same pool must not ask for. A tree an + // attempt of that write creates is not in it either, which is what a replayed attempt needs: + // it must drop what was there before this change, and not what the attempt before it made. + final Set storedTrees = entryContainer.getRootContainer().getStorage().listTrees(); + // Open added indexes *before* adding them to indexIdToIndexes final List addedIndexesToRebuild = new ArrayList<>(); entryContainer.getRootContainer().getStorage().write(new WriteOperation() @@ -957,7 +1032,7 @@ public void run(WriteableTransaction txn) throws Exception addedIndexesToRebuild.clear(); for (MatchingRuleIndex addedIndex : addedIndexes.values()) { - if (createIndex(txn, addedIndex)) + if (createIndex(txn, addedIndex, ccr, storedTrees)) { addedIndexesToRebuild.add(addedIndex.getName()); } @@ -1053,8 +1128,13 @@ public void run(WriteableTransaction txn) throws Exception * Answered to the caller rather than reported from here: this runs inside a {@link WriteOperation} * the storage may replay, and the report belongs to the attempt which commits. */ - private static boolean createIndex(WriteableTransaction txn, MatchingRuleIndex index) + private boolean createIndex(WriteableTransaction txn, MatchingRuleIndex index, ConfigChangeResult ccr, + Set storedTrees) { + if (dropLeftoversOf(txn, index, storedTrees)) + { + reportDiscardedLeftovers(ccr, index.getName(), entryContainer.getBaseDN()); + } index.open(txn, true); return !index.isTrusted(); } diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/EntryContainer.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/EntryContainer.java index 70bf822ca1..42bc847b1f 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/EntryContainer.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/EntryContainer.java @@ -37,6 +37,7 @@ import java.util.Map; import java.util.NoSuchElementException; import java.util.Objects; +import java.util.Set; import java.util.TreeMap; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicReference; @@ -201,7 +202,13 @@ public ConfigChangeResult applyConfigurationAdd(final BackendIndexCfg cfg) { final CryptoSuite cryptoSuite = newCryptoSuite(cfg.isConfidentialityEnabled()); final AttributeIndex index = newAttributeIndex(cfg, cryptoSuite); + // Read outside the write which uses it: listTrees() borrows a connection of its own on JDBC, which a + // transaction already holding one of the same pool must not ask for. A tree an attempt of that write + // creates is not in it either, which is what a replayed attempt needs: it must drop what was there + // before this change, and not what the attempt before it made. + final Set storedTrees = storage.listTrees(); final AtomicBoolean trusted = new AtomicBoolean(); + final AtomicBoolean discarded = new AtomicBoolean(); storage.write(new WriteOperation() { @Override @@ -211,12 +218,17 @@ public void run(WriteableTransaction txn) throws Exception // its configuration. close() removes every registration made for this index, so closing first leaves // one listener behind rather than one per attempt; it is a no-op on the first attempt. index.close(); - index.open(txn, true); + discarded.set(index.openAsAdded(txn, storedTrees)); trusted.set(index.isTrusted()); attrIndexMap.put(cfg.getAttribute(), index); attrCryptoMap.put(cfg.getAttribute(), cryptoSuite); } }); + if (discarded.get()) + { + // Reported outside the write, since a replayed attempt would otherwise repeat the message. + AttributeIndex.reportDiscardedLeftovers(ccr, cfg.getAttribute().getNameOrOID(), getBaseDN()); + } if (!trusted.get()) { // Reported outside the write, since a replayed attempt would otherwise repeat the message. @@ -310,6 +322,9 @@ public ConfigChangeResult applyConfigurationAdd(final BackendVLVIndexCfg cfg) { final AtomicReference built = new AtomicReference<>(); final AtomicBoolean trusted = new AtomicBoolean(); + final AtomicBoolean discarded = new AtomicBoolean(); + // Read outside the write, for the reason given in the index add listener above. + final Set storedTrees = storage.listTrees(); storage.write(new WriteOperation() { @Override @@ -326,11 +341,16 @@ public void run(WriteableTransaction txn) throws Exception } VLVIndex vlvIndex = new VLVIndex(cfg, state, storage, EntryContainer.this, txn); built.set(vlvIndex); - vlvIndex.open(txn, true); + discarded.set(vlvIndex.openAsAdded(txn, storedTrees)); trusted.set(vlvIndex.isTrusted()); vlvIndexMap.put(cfg.getName().toLowerCase(), vlvIndex); } }); + if (discarded.get()) + { + // Reported outside the write, since a replayed attempt would otherwise repeat the message. + AttributeIndex.reportDiscardedLeftovers(ccr, cfg.getName(), getBaseDN()); + } if (!trusted.get()) { // Reported outside the write, since a replayed attempt would otherwise repeat the message. diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/VLVIndex.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/VLVIndex.java index 6288ddc346..500bbdac60 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/VLVIndex.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/VLVIndex.java @@ -31,6 +31,7 @@ import java.util.Iterator; import java.util.LinkedList; import java.util.List; +import java.util.Set; import java.util.TreeSet; import org.forgerock.i18n.LocalizableMessage; @@ -170,6 +171,44 @@ void afterOpen(final WriteableTransaction txn, boolean createOnDemand) throws St } } + /** + * Opens this VLV index as one the configuration is adding, dropping first whatever a VLV index of + * the same name left behind: its tree, the counter which goes with it and the {@code state} record + * which carries their TRUSTED flag. + *

+ * What they hold is what the backend was told before the configuration stopped naming them, and no + * entry written in between is in it; a rebuild regenerates all of it. See + * {@link AttributeIndex#openAsAdded} for why it is dropped rather than adopted (#990). + * + * @param txn a non null transaction + * @param storedTrees the trees the storage holds, read before this transaction was opened + * @return true if anything left behind was dropped + * @throws StorageRuntimeException if an error occurs while opening the index + */ + boolean openAsAdded(WriteableTransaction txn, Set storedTrees) throws StorageRuntimeException + { + boolean dropped = false; + // Each of the two is asked for on its own: deleting a tree which is not there fails on PersistIt, + // and a change which stopped halfway can have left one of them without the other. + if (storedTrees.contains(counter.getName())) + { + counter.delete(txn); + dropped = true; + } + if (storedTrees.contains(getName())) + { + txn.deleteTree(getName()); + dropped = true; + } + // The record can outlive the trees: see AttributeIndex.dropLeftoversOf. + dropped |= state.deleteRecord(txn, getName()); + // The flag was read out of that record when this instance was built, and belongs to the index + // whose trees have just gone. afterOpen() upgrades it again if there is nothing to index. + trusted = false; + open(txn, true); + return dropped; + } + @Override void beforeDelete(WriteableTransaction txn) throws StorageRuntimeException { 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 a897dc4cb0..1f65b63752 100644 --- a/opendj-server-legacy/src/messages/org/opends/messages/backend.properties +++ b/opendj-server-legacy/src/messages/org/opends/messages/backend.properties @@ -1156,3 +1156,8 @@ ERR_CONFIG_BACKEND_DATA_CHANGE_FAILED_627=The compression, encoding and encrypti backend base DN '%s' could not be applied in full: %s. Its entries and its indexes may no longer \ be encoded under the same settings; disabling and enabling this backend builds both from the \ stored configuration again +WARN_INDEX_ADD_DISCARDED_LEFTOVER_TREES_628=Index %s of backend base DN '%s' was created over the trees an \ + index of the same name left behind, which no configuration named while this backend went on taking writes. \ + What they hold is not what this index holds - every entry changed while nothing named them is missing from \ + it - so their content has been discarded rather than adopted, and this index has been created empty. Rebuild \ + it before it is used diff --git a/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/IndexAddedOverLeftoverTreesTest.java b/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/IndexAddedOverLeftoverTreesTest.java new file mode 100644 index 0000000000..812ad379de --- /dev/null +++ b/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/IndexAddedOverLeftoverTreesTest.java @@ -0,0 +1,570 @@ +/* + * 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.forgerock.opendj.config.ConfigurationMock.mockCfg; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; +import static org.opends.messages.BackendMessages.NOTE_INDEX_ADD_REQUIRES_REBUILD; +import static org.opends.server.backends.pluggable.State.IndexFlag.TRUSTED; +import static org.opends.server.backends.pluggable.SuffixContainer.STATE_INDEX_NAME; +import static org.opends.server.util.CollectionUtils.newTreeSet; + +import java.util.EnumSet; +import java.util.HashMap; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; +import java.util.SortedSet; + +import org.forgerock.i18n.LocalizableMessage; +import org.forgerock.opendj.config.server.ConfigChangeResult; +import org.forgerock.opendj.config.server.ConfigException; +import org.forgerock.opendj.config.server.ConfigurationAddListener; +import org.forgerock.opendj.ldap.ByteString; +import org.forgerock.opendj.ldap.DN; +import org.forgerock.opendj.ldap.ResultCode; +import org.forgerock.opendj.ldap.schema.AttributeType; +import org.forgerock.opendj.server.config.meta.BackendIndexCfgDefn.IndexType; +import org.forgerock.opendj.server.config.meta.BackendVLVIndexCfgDefn.Scope; +import org.forgerock.opendj.server.config.server.BackendIndexCfg; +import org.forgerock.opendj.server.config.server.BackendVLVIndexCfg; +import org.forgerock.opendj.server.config.server.PDBBackendCfg; +import org.mockito.ArgumentCaptor; +import org.opends.server.DirectoryServerTestCase; +import org.opends.server.TestCaseUtils; +import org.opends.server.backends.pdb.PDBStorage; +import org.opends.server.backends.pluggable.AttributeIndex.MatchingRuleIndex; +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.backends.pluggable.spi.WriteableTransaction; +import org.opends.server.core.AddOperation; +import org.opends.server.core.ServerContext; +import org.opends.server.types.Entry; +import org.testng.annotations.AfterMethod; +import org.testng.annotations.BeforeClass; +import org.testng.annotations.Test; + +/** + * Tests what a pluggable backend does with the trees of an index which is declared again after the + * configuration naming them is gone. The name of an index tree is a pure function of the base DN, + * the attribute and the index id, and opening an index creates its trees only if they are not + * there, so an index added for an attribute a previous index of the same name served reopens + * exactly the trees that one left behind - with their content and with the TRUSTED flag their + * {@code state} records still carry. Neither belongs to the index being added: what those trees + * hold is whatever the backend was told before the configuration stopped naming them, and every + * entry written in between is missing from it - see OpenDJ issue #990. + *

+ * The trees are left behind here by declaring them, filling them and then reopening the backend + * from a configuration which no longer names them, which is what an index deleted while the + * backend is disabled - or through an offline {@code dsconfig} - leaves behind: the entry + * container registers the listener which would have deleted the trees only while it is open + * ({@code EntryContainer}, its constructor and {@code close()}). A deletion the storage gives up on + * and a stop between the deletion of the trees and the commit of the configuration change leave the + * same state; #962 covers the first of those. + */ +@SuppressWarnings("javadoc") +@Test(groups = { "precommit", "pluggablebackend" }, sequential = true) +public class IndexAddedOverLeftoverTreesTest extends DirectoryServerTestCase +{ + private static final String BACKEND_ID = "IndexAddedOverLeftoverTreesTest"; + private static final DN BASE_DN = DN.valueOf("dc=b990,dc=com"); + private static final String VLV_INDEX_NAME = "b990vlv"; + + private ServerContext serverContext; + private AttributeType cnType; + + @BeforeClass + public void startServer() throws Exception + { + TestCaseUtils.startServer(); + serverContext = TestCaseUtils.getServerContext(); + cnType = serverContext.getSchema().getAttributeType("cn"); + } + + /** + * A test which fails before it finalizes its backend leaves the base DN behind in the server wide + * registry, where it would outlive the test and break the next one to use it. + */ + @AfterMethod + public void deregisterLeftoverBaseDN() + { + try + { + serverContext.getBackendConfigManager().deregisterBaseDN(BASE_DN); + } + catch (Exception alreadyGone) + { + // Which is what a test that finalized its backend leaves behind. + } + } + + /** + * The index the add opens is not the index whose trees are still there: it has indexed none of + * the entries, so it must be untrusted and the operator must be told to rebuild it, exactly as + * for any other index added to a backend which already holds entries. + */ + @Test + public void anIndexAddedOverTheTreesLeftBehindIsNotTrusted() throws Exception + { + final LeftoverBackend backend = leaveTreesBehind(); + try + { + final EntryContainer ec = backend.getRootContainer().getEntryContainer(BASE_DN); + assertThat(backend.getRootContainer().getStorage().listTrees()) + .as("the trees no configuration names any more").containsAll(backend.leftoverIndexTrees); + + final ConfigChangeResult ccr = indexAddListener(backend).applyConfigurationAdd(backend.cnIndexCfg); + + assertThat(ccr.getResultCode()).isEqualTo(ResultCode.SUCCESS); + assertThat(ec.getAttributeIndex(cnType).isTrusted()) + .as("an index trusted over the content of trees it did not fill").isFalse(); + assertThat(ccr.adminActionRequired()).isTrue(); + assertThat(ordinalsOf(ccr)).contains(NOTE_INDEX_ADD_REQUIRES_REBUILD.ordinal()); + } + finally + { + backend.finalizeBackend(); + } + } + + /** + * The flag alone does not make the answers right again: {@code DefaultIndex.get} hands back what + * a key holds whenever it holds anything, whatever the flag says, and only a key with nothing + * behind it answers "undefined" and sends the search to the entries. So the content of the + * adopted trees has to go, or every key they hold goes on answering with the entries of another + * index and misses everything written since. + */ + @Test + public void anIndexAddedOverTheTreesLeftBehindDoesNotAnswerWithTheirContent() throws Exception + { + final LeftoverBackend backend = leaveTreesBehind(); + try + { + final EntryContainer ec = backend.getRootContainer().getEntryContainer(BASE_DN); + final Storage storage = backend.getRootContainer().getStorage(); + final Map keysHeld = keysHeldBy(storage, backend.leftoverIndexTrees); + assertThat(keysHeld).as("the keys the trees left behind hold").isNotEmpty(); + + indexAddListener(backend).applyConfigurationAdd(backend.cnIndexCfg); + + final AttributeIndex index = ec.getAttributeIndex(cnType); + for (final MatchingRuleIndex opened : index.getNameToIndexes().values()) + { + final ByteString keyHeld = keysHeld.get(opened.getName()); + if (keyHeld != null) + { + final Boolean answered = storage.read(txn -> opened.get(txn, keyHeld).isDefined()); + assertThat(answered) + .as("a key of " + opened.getName() + " answered out of the tree left behind").isFalse(); + } + } + } + finally + { + backend.finalizeBackend(); + } + } + + /** + * The trees can be gone while their {@code state} records are not: on JDBC the drop of a table is + * DDL which commits of its own accord, while the {@code state.deleteRecord} of the same + * {@code closeAndDelete} belongs to the transaction, so a rollback after the drop restores the + * records over tables which are already gone. The index added next creates empty trees and reads + * TRUSTED out of those records - an empty index which every search believes. + */ + @Test + public void anIndexAddedOverAStateRecordWhoseTreesAreGoneIsNotTrusted() throws Exception + { + final LeftoverBackend backend = leaveTreesBehind(); + try + { + final EntryContainer ec = backend.getRootContainer().getEntryContainer(BASE_DN); + final Storage storage = backend.getRootContainer().getStorage(); + storage.write(new WriteOperation() + { + @Override + public void run(WriteableTransaction txn) throws Exception + { + for (final TreeName leftover : backend.leftoverIndexTrees) + { + txn.deleteTree(leftover); + } + } + }); + for (final TreeName leftover : backend.leftoverIndexTrees) + { + assertThat(persistedFlags(backend, leftover)) + .as("the state record left over the tree which is gone").contains(TRUSTED); + } + + final ConfigChangeResult ccr = indexAddListener(backend).applyConfigurationAdd(backend.cnIndexCfg); + + assertThat(ccr.getResultCode()).isEqualTo(ResultCode.SUCCESS); + assertThat(ec.getAttributeIndex(cnType).isTrusted()) + .as("an empty index trusted over a state record which outlived its trees").isFalse(); + assertThat(ccr.adminActionRequired()).isTrue(); + assertThat(ordinalsOf(ccr)).contains(NOTE_INDEX_ADD_REQUIRES_REBUILD.ordinal()); + } + finally + { + backend.finalizeBackend(); + } + } + + /** A VLV index adopts what it left behind in the same way, in its tree and in its counter. */ + @Test + public void aVlvIndexAddedOverTheTreesLeftBehindIsNotTrusted() throws Exception + { + final LeftoverBackend backend = leaveTreesBehind(); + try + { + final EntryContainer ec = backend.getRootContainer().getEntryContainer(BASE_DN); + assertThat(backend.getRootContainer().getStorage().listTrees()) + .as("the VLV trees no configuration names any more") + .contains(backend.leftoverVlvTree, backend.leftoverVlvCounterTree); + + final ConfigChangeResult ccr = vlvIndexAddListener(backend).applyConfigurationAdd(backend.vlvIndexCfg); + + assertThat(ccr.getResultCode()).isEqualTo(ResultCode.SUCCESS); + assertThat(ec.getVLVIndex(VLV_INDEX_NAME).isTrusted()) + .as("a VLV index trusted over the content of trees it did not fill").isFalse(); + assertThat(ccr.adminActionRequired()).isTrue(); + assertThat(ordinalsOf(ccr)).contains(NOTE_INDEX_ADD_REQUIRES_REBUILD.ordinal()); + } + finally + { + backend.finalizeBackend(); + } + } + + /** + * An index type declared again for an attribute which is still indexed goes through + * {@code AttributeIndex.applyConfigurationChange} rather than through the add listener, and opens + * its tree the same way. + */ + @Test + public void anIndexTypeAddedOverTheTreeLeftBehindIsNotTrusted() throws Exception + { + final LeftoverBackend backend = leaveTreesBehind(indexCfg(newTreeSet(IndexType.EQUALITY))); + try + { + final EntryContainer ec = backend.getRootContainer().getEntryContainer(BASE_DN); + final AttributeIndex index = ec.getAttributeIndex(cnType); + assertThat(index.isIndexed(IndexType.SUBSTRING)) + .as("the index type this configuration no longer names").isFalse(); + + final ConfigChangeResult ccr = + index.applyConfigurationChange(indexCfg(newTreeSet(IndexType.EQUALITY, IndexType.SUBSTRING))); + + assertThat(ccr.getResultCode()).isEqualTo(ResultCode.SUCCESS); + assertThat(index.isIndexed(IndexType.SUBSTRING)).isTrue(); + assertThat(index.isTrusted()) + .as("an index type trusted over the content of the tree it did not fill").isFalse(); + assertThat(ccr.adminActionRequired()).isTrue(); + assertThat(ordinalsOf(ccr)).contains(NOTE_INDEX_ADD_REQUIRES_REBUILD.ordinal()); + } + finally + { + backend.finalizeBackend(); + } + } + + /** + * An index added to a backend which holds no entry has nothing to index and nothing to adopt, so + * it stays trusted and asks for nothing. Pins the upgrade {@code DefaultIndex.afterOpen} makes. + */ + @Test + public void anIndexAddedToAnEmptyBackendIsTrustedAndAsksForNothing() throws Exception + { + final LeftoverBackend backend = openBackend(true, null, null); + try + { + final ConfigChangeResult ccr = indexAddListener(backend).applyConfigurationAdd(backend.cnIndexCfg); + + assertThat(ccr.getResultCode()).isEqualTo(ResultCode.SUCCESS); + assertThat(backend.getRootContainer().getEntryContainer(BASE_DN).getAttributeIndex(cnType).isTrusted()) + .as("an index of a backend with nothing to index").isTrue(); + assertThat(ccr.adminActionRequired()).isFalse(); + assertThat(ccr.getMessages()).isEmpty(); + } + finally + { + backend.finalizeBackend(); + } + } + + /** + * An index added to a backend which holds entries, with nothing left behind for it to adopt, is + * untrusted over trees it created empty and asks to be rebuilt. Pins what the fix has to leave + * alone: this is the outcome an index added over leftover trees has to reach as well. + */ + @Test + public void anIndexAddedToANonEmptyBackendAsksForARebuild() throws Exception + { + final LeftoverBackend backend = openBackend(true, null, null); + try + { + addEntry(backend, "dn: " + BASE_DN, "objectClass: top", "objectClass: domain", "dc: b990"); + + final ConfigChangeResult ccr = indexAddListener(backend).applyConfigurationAdd(backend.cnIndexCfg); + + assertThat(ccr.getResultCode()).isEqualTo(ResultCode.SUCCESS); + assertThat(backend.getRootContainer().getEntryContainer(BASE_DN).getAttributeIndex(cnType).isTrusted()) + .as("an index which has indexed none of the entries").isFalse(); + assertThat(ccr.adminActionRequired()).isTrue(); + assertThat(ordinalsOf(ccr)).contains(NOTE_INDEX_ADD_REQUIRES_REBUILD.ordinal()); + } + finally + { + backend.finalizeBackend(); + } + } + + private LeftoverBackend leaveTreesBehind() throws Exception + { + return leaveTreesBehind(null); + } + + /** + * Opens a backend whose configuration names a cn index and a VLV index, fills them with an entry, + * and reopens it from a configuration which names {@code reopenedWith} instead - null for a + * configuration which names no index at all. The trees of everything it no longer names are left + * behind, and the entry added afterwards is in none of them. + */ + private LeftoverBackend leaveTreesBehind(BackendIndexCfg reopenedWith) throws Exception + { + final LeftoverBackend indexed = openBackend(true, indexCfg(newTreeSet(IndexType.EQUALITY, IndexType.SUBSTRING)), + vlvIndexCfg()); + final Set indexTrees; + final TreeName vlvTree; + final TreeName vlvCounterTree; + try + { + addEntry(indexed, "dn: " + BASE_DN, "objectClass: top", "objectClass: domain", "dc: b990"); + addEntry(indexed, "dn: cn=stale," + BASE_DN, "objectClass: top", "objectClass: organizationalRole", + "cn: stale"); + final EntryContainer ec = indexed.getRootContainer().getEntryContainer(BASE_DN); + indexTrees = treesOf(ec.getAttributeIndex(cnType)); + vlvTree = new TreeName(ec.getTreePrefix(), "vlv." + VLV_INDEX_NAME); + vlvCounterTree = new TreeName(ec.getTreePrefix(), "counter.vlv." + VLV_INDEX_NAME); + } + finally + { + indexed.finalizeBackend(); + } + + final LeftoverBackend reopened = openBackend(false, reopenedWith, null); + try + { + addEntry(reopened, "dn: cn=fresh," + BASE_DN, "objectClass: top", "objectClass: organizationalRole", + "cn: fresh"); + } + catch (Exception e) + { + reopened.finalizeBackend(); + throw e; + } + reopened.leftoverIndexTrees = indexTrees; + reopened.leftoverVlvTree = vlvTree; + reopened.leftoverVlvCounterTree = vlvCounterTree; + return reopened; + } + + /** The first key each of the given trees holds, for the trees which hold any. */ + private static Map keysHeldBy(Storage storage, Set trees) throws Exception + { + final Map keys = new HashMap<>(); + for (final TreeName tree : trees) + { + final ByteString key = storage.read(txn -> { + try (Cursor cursor = txn.openCursor(tree)) + { + return cursor.next() ? cursor.getKey() : null; + } + }); + if (key != null) + { + keys.put(tree, key); + } + } + return keys; + } + + /** Reads back the flags an index tree carries, as they are stored. */ + private EnumSet persistedFlags(LeftoverBackend backend, TreeName index) throws Exception + { + final EntryContainer ec = backend.getRootContainer().getEntryContainer(BASE_DN); + final State state = new State(new TreeName(ec.getTreePrefix(), STATE_INDEX_NAME)); + return backend.getRootContainer().getStorage().read(txn -> state.getIndexFlags(txn, index)); + } + + /** The messages a change result carries, by identity rather than by their formatted text. */ + private static Set ordinalsOf(ConfigChangeResult ccr) + { + final Set ordinals = new HashSet<>(); + for (final LocalizableMessage message : ccr.getMessages()) + { + ordinals.add(message.ordinal()); + } + return ordinals; + } + + private static Set treesOf(AttributeIndex index) + { + final Set names = new HashSet<>(); + for (final MatchingRuleIndex matchingRuleIndex : index.getNameToIndexes().values()) + { + names.add(matchingRuleIndex.getName()); + } + return names; + } + + private static void addEntry(LeftoverBackend backend, String... ldifLines) throws Exception + { + final Entry entry = TestCaseUtils.makeEntry(ldifLines); + backend.addEntry(entry, mock(AddOperation.class)); + } + + @SuppressWarnings({ "unchecked", "rawtypes" }) + private static ConfigurationAddListener indexAddListener(LeftoverBackend backend) + throws ConfigException + { + final ArgumentCaptor captor = ArgumentCaptor.forClass(ConfigurationAddListener.class); + verify(backend.configuredWith).addBackendIndexAddListener(captor.capture()); + return captor.getValue(); + } + + @SuppressWarnings({ "unchecked", "rawtypes" }) + private static ConfigurationAddListener vlvIndexAddListener(LeftoverBackend backend) + throws ConfigException + { + final ArgumentCaptor captor = ArgumentCaptor.forClass(ConfigurationAddListener.class); + verify(backend.configuredWith).addBackendVLVIndexAddListener(captor.capture()); + return captor.getValue(); + } + + /** + * Opens a backend whose configuration names the given index and VLV index - null for one it does + * not name, whose trees are then left to whatever is already in the storage. + */ + private LeftoverBackend openBackend(boolean pristine, BackendIndexCfg cnIndexCfg, BackendVLVIndexCfg vlvIndexCfg) + throws Exception + { + final LeftoverBackend backend = new LeftoverBackend(); + backend.setBackendID(BACKEND_ID); + backend.cnIndexCfg = cnIndexCfg != null ? cnIndexCfg + : indexCfg(newTreeSet(IndexType.EQUALITY, IndexType.SUBSTRING)); + backend.vlvIndexCfg = vlvIndexCfg != null ? vlvIndexCfg : vlvIndexCfg(); + backend.configuredWith = backendCfg(cnIndexCfg, vlvIndexCfg); + backend.configureBackend(backend.configuredWith, serverContext); + if (pristine) + { + // Start from a pristine on-disk state, so that a previous run cannot mask what this one leaves. + backend.storage.removeStorageFiles(); + } + try + { + backend.openBackend(); + } + catch (Exception e) + { + // openBackend() opens the root container before it registers the base DNs and the monitor, so + // a failure in any of those leaves the volume open and every following test failing here too. + try + { + if (backend.getRootContainer() != null) + { + backend.finalizeBackend(); + } + else + { + backend.storage.close(); + } + } + catch (Exception cleanupFailure) + { + e.addSuppressed(cleanupFailure); + } + throw e; + } + return backend; + } + + private PDBBackendCfg backendCfg(BackendIndexCfg cnIndexCfg, BackendVLVIndexCfg vlvIndexCfg) throws ConfigException + { + final PDBBackendCfg cfg = mockCfg(PDBBackendCfg.class); + when(cfg.dn()).thenReturn(DN.valueOf("ds-cfg-backend-id=" + BACKEND_ID + ",cn=Backends,cn=config")); + when(cfg.getBackendId()).thenReturn(BACKEND_ID); + when(cfg.getDBDirectory()).thenReturn(BACKEND_ID); + when(cfg.getDBDirectoryPermissions()).thenReturn("755"); + when(cfg.getDBCacheSize()).thenReturn(0L); + when(cfg.getDBCachePercent()).thenReturn(20); + when(cfg.getBaseDN()).thenReturn(newTreeSet(BASE_DN)); + when(cfg.listBackendIndexes()).thenReturn(cnIndexCfg != null ? new String[] { "cn" } : new String[0]); + when(cfg.getBackendIndex("cn")).thenReturn(cnIndexCfg); + when(cfg.listBackendVLVIndexes()) + .thenReturn(vlvIndexCfg != null ? new String[] { VLV_INDEX_NAME } : new String[0]); + when(cfg.getBackendVLVIndex(VLV_INDEX_NAME)).thenReturn(vlvIndexCfg); + return cfg; + } + + private BackendIndexCfg indexCfg(SortedSet indexTypes) + { + final BackendIndexCfg cfg = mock(BackendIndexCfg.class); + when(cfg.getIndexType()).thenReturn(indexTypes); + when(cfg.getAttribute()).thenReturn(cnType); + when(cfg.getIndexEntryLimit()).thenReturn(4000); + when(cfg.getSubstringLength()).thenReturn(6); + return cfg; + } + + private BackendVLVIndexCfg vlvIndexCfg() + { + final BackendVLVIndexCfg cfg = mock(BackendVLVIndexCfg.class); + when(cfg.getName()).thenReturn(VLV_INDEX_NAME); + when(cfg.getBaseDN()).thenReturn(BASE_DN); + when(cfg.getScope()).thenReturn(Scope.WHOLE_SUBTREE); + when(cfg.getFilter()).thenReturn("(objectClass=*)"); + when(cfg.getSortOrder()).thenReturn("+cn"); + return cfg; + } + + /** A backend which keeps hold of the configuration its entry container registered with. */ + private static final class LeftoverBackend extends BackendImpl + { + private PDBStorage storage; + /** The configuration the entry container registers its listeners with. */ + private PDBBackendCfg configuredWith; + private BackendIndexCfg cnIndexCfg; + private BackendVLVIndexCfg vlvIndexCfg; + /** The trees of the indexes an earlier configuration named, which nothing names now. */ + private Set leftoverIndexTrees; + private TreeName leftoverVlvTree; + private TreeName leftoverVlvCounterTree; + + @Override + protected Storage configureStorage(PDBBackendCfg cfg, ServerContext serverContext) throws ConfigException + { + storage = new PDBStorage(cfg, serverContext); + return storage; + } + } +} From fa598e9154c644be5f6ec91834d3dcbf677d82f5 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Wed, 16 Sep 2026 17:55:22 +0300 Subject: [PATCH 2/6] [#990] Drop and open in two writes, refuse a second index for an indexed attribute, run the cases over JE On JE, removeDatabase(txn) write-locks the record of the name in _jeNameMap until the transaction commits, and JEStorage opens a tree with openDatabase(null, ...) - a transaction of its own - which asks for a read lock on that record and, at lockTimeout=0, waits for it without limit: a drop and an open sharing one write never returned, and held JEStorage.trees' monitor while parked. The three roads now drop in a write of their own and open in the next; VLVIndex.openAsAdded becomes the static dropLeftovers, and the instance built after it reads no TRUSTED flag out of a record which is gone. The tree is asked for with txn.treeExists() inside the drop write rather than a listTrees() snapshot taken before it: on JDBC that goes through the transaction's own connection, on Cassandra listTrees() is a stub while treeExists and deleteTree are real, and a replayed drop sees what is there when it runs. attrIndexMap is keyed by the attribute type, which every name of the attribute and its OID resolve to, while the configuration entry is named by what was typed: an index declared as commonName over a live cn index named the trees the live index serves and would have dropped them. Refused in isConfigurationAddAcceptable and at the top of applyConfigurationAdd with ERR_CONFIG_INDEX_ATTRIBUTE_ALREADY_INDEXED (629). A record deleted without a tree is no longer reported as discarded content, and the index-type road reports the WARN and the NOTE after its writes rather than inside a replayable one. Tests: every case runs over PDB and JE (JEStorage's constructor is public, as PDBStorage's is); case 4 pins the VLV tree, counter and record, case 5 pins the substring key gone and the equality key kept, the WARN is asserted where it fires and where it must not, a case pins the refusal, and the openBackend() catch closes the root container instead of calling finalizeBackend(), whose deregisterMonitorProvider(null) NPEs before the monitor is registered. --- .../opends/server/backends/jeb/JEStorage.java | 3 +- .../backends/pluggable/AttributeIndex.java | 93 +++-- .../backends/pluggable/EntryContainer.java | 70 +++- .../server/backends/pluggable/VLVIndex.java | 52 +-- .../org/opends/messages/backend.properties | 3 + .../IndexAddedOverLeftoverTreesTest.java | 318 ++++++++++++++---- .../pluggable/ReplayedConfigChangeTest.java | 27 +- 7 files changed, 431 insertions(+), 135 deletions(-) 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 11fa660077..92c909832c 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 @@ -711,7 +711,8 @@ private WriteableTransaction newWriteableTransaction(Transaction txn) * @throws ConfigException * if memory cannot be reserved */ - JEStorage(final JEBackendCfg cfg, ServerContext serverContext) throws ConfigException + // Public as PDBStorage's is: a pluggable backend test which runs the same case over both storages builds them. + public JEStorage(final JEBackendCfg cfg, ServerContext serverContext) throws ConfigException { this.serverContext = serverContext; backendDirectory = getBackendDirectory(cfg); diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/AttributeIndex.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/AttributeIndex.java index 4147ee9ed4..f45a782231 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/AttributeIndex.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/AttributeIndex.java @@ -450,8 +450,8 @@ void open(WriteableTransaction txn, boolean createOnDemand) throws StorageRuntim } /** - * Opens this index as one the configuration is adding, dropping first whatever an index of the - * same name left behind. + * Drops whatever an index of the same name left behind for the trees this index, which the + * configuration is adding, is about to open. *

* The name of an index tree is a pure function of the base DN, the attribute and the index id, and * {@link #open} creates a tree only where there is none, so an index added for an attribute @@ -462,20 +462,28 @@ void open(WriteableTransaction txn, boolean createOnDemand) throws StorageRuntim * same. A rebuild regenerates all of it and nothing else is lost with it, so it is dropped here * rather than adopted, which leaves this index where any other index added to a backend holding * entries starts: empty, untrusted and asking to be rebuilt (#990). + *

+ * Only the trees of the index ids this configuration declares are looked at. A tree of an id it + * does not name is opened by nothing and answers nothing, and the first configuration which + * declares that id again drops it the same way. + *

+ * This must run in a write of its own, committed before the write which opens the index. On JE + * deleting a tree write-locks the record of its name until the transaction commits, while opening + * a tree - which {@code JEStorage} does under a transaction of its own - asks for a read lock on + * that record and waits for it without limit: no cycle, so the deadlock detector is silent, and + * the configuration change never returns. * * @param txn a non null transaction - * @param storedTrees the trees the storage holds, read before this transaction was opened - * @return true if anything left behind was dropped - * @throws StorageRuntimeException if an error occurs while opening the index + * @return true if a tree was dropped; a record deleted on its own discards nothing + * @throws StorageRuntimeException if an error occurs in the storage */ - boolean openAsAdded(WriteableTransaction txn, Set storedTrees) throws StorageRuntimeException + boolean dropLeftovers(WriteableTransaction txn) throws StorageRuntimeException { boolean dropped = false; for (Index index : indexIdToIndexes.values()) { - dropped |= dropLeftoversOf(txn, index, storedTrees); + dropped |= dropLeftoversOf(txn, index); } - open(txn, true); return dropped; } @@ -486,23 +494,34 @@ boolean openAsAdded(WriteableTransaction txn, Set storedTrees) throws * The record can outlive the tree on its own: on JDBC a tree is dropped by DDL which commits of * its own accord while the record is deleted by the transaction, so a rollback in between leaves * the record over a tree which is gone, and the index opened next is created empty and read back - * as trusted. It is therefore taken out whether a tree was found for it or not. + * as trusted. It is therefore taken out whether a tree was found for it or not - but a record + * deleted on its own is not reported as discarded content, since none was. + *

+ * The tree is asked for through the transaction rather than through a list of the trees read + * beforehand: on JDBC that list borrows a connection of its own, which a transaction already + * holding one of the same pool must not ask for, while {@code treeExists} asks the transaction's + * own; on Cassandra the list is not implemented and answers nothing, while {@code treeExists} + * finds the partition; and a replayed attempt then sees what is there when it runs, not what was + * there before the first attempt. *

* No search can be reading what is dropped here, so this does not take the exclusive lock * {@link #deleteIndex} takes: an index which is only being added is in no map a search reaches, - * and the trees it would have adopted are named by nothing until it opens them. + * and the trees it would have adopted are named by nothing until it opens them. The add listener + * refuses an index for an attribute type which is already indexed, so that no live index is + * reached through another of the attribute's names or its OID. * - * @return true if a tree or a record was dropped + * @return true if a tree was dropped */ - private boolean dropLeftoversOf(WriteableTransaction txn, Index index, Set storedTrees) + private boolean dropLeftoversOf(WriteableTransaction txn, Index index) { - if (storedTrees.contains(index.getName())) + if (txn.treeExists(index.getName())) { // Deletes the state record along with the tree. entryContainer.deleteTree(txn, index); return true; } - return state.deleteRecord(txn, index.getName()); + state.deleteRecord(txn, index.getName()); + return false; } /** @@ -532,6 +551,15 @@ AttributeType getAttributeType() return config.getAttribute(); } + /** + * Get the configuration of this attribute index. + * @return The configuration this attribute index is currently applying. + */ + BackendIndexCfg getConfiguration() + { + return config; + } + public CryptoSuite getCryptoSuite() { return cryptoSuite; @@ -1014,11 +1042,29 @@ public synchronized ConfigChangeResult applyConfigurationChange(final BackendInd ccr.addMessage(rebuildMessage); } - // Read outside the write which uses it: listTrees() borrows a connection of its own on JDBC, - // which a transaction already holding one of the same pool must not ask for. A tree an - // attempt of that write creates is not in it either, which is what a replayed attempt needs: - // it must drop what was there before this change, and not what the attempt before it made. - final Set storedTrees = entryContainer.getRootContainer().getStorage().listTrees(); + // Drop what an earlier index left behind for the added ids, in a write of its own: the drop and the open + // must not share a transaction, see dropLeftovers(). Both writes may be replayed by the storage, so what + // they found is reported once they are done, and by the attempt which went through. + final List discarded = new ArrayList<>(); + entryContainer.getRootContainer().getStorage().write(new WriteOperation() + { + @Override + public void run(WriteableTransaction txn) throws Exception + { + discarded.clear(); + for (MatchingRuleIndex addedIndex : addedIndexes.values()) + { + if (dropLeftoversOf(txn, addedIndex)) + { + discarded.add(addedIndex); + } + } + } + }); + for (MatchingRuleIndex index : discarded) + { + reportDiscardedLeftovers(ccr, index.getName(), entryContainer.getBaseDN()); + } // Open added indexes *before* adding them to indexIdToIndexes final List addedIndexesToRebuild = new ArrayList<>(); @@ -1032,7 +1078,7 @@ public void run(WriteableTransaction txn) throws Exception addedIndexesToRebuild.clear(); for (MatchingRuleIndex addedIndex : addedIndexes.values()) { - if (createIndex(txn, addedIndex, ccr, storedTrees)) + if (createIndex(txn, addedIndex)) { addedIndexesToRebuild.add(addedIndex.getName()); } @@ -1128,13 +1174,8 @@ public void run(WriteableTransaction txn) throws Exception * Answered to the caller rather than reported from here: this runs inside a {@link WriteOperation} * the storage may replay, and the report belongs to the attempt which commits. */ - private boolean createIndex(WriteableTransaction txn, MatchingRuleIndex index, ConfigChangeResult ccr, - Set storedTrees) + private static boolean createIndex(WriteableTransaction txn, MatchingRuleIndex index) { - if (dropLeftoversOf(txn, index, storedTrees)) - { - reportDiscardedLeftovers(ccr, index.getName(), entryContainer.getBaseDN()); - } index.open(txn, true); return !index.isTrusted(); } diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/EntryContainer.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/EntryContainer.java index 42bc847b1f..562c88ad8d 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/EntryContainer.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/EntryContainer.java @@ -37,7 +37,6 @@ import java.util.Map; import java.util.NoSuchElementException; import java.util.Objects; -import java.util.Set; import java.util.TreeMap; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicReference; @@ -182,6 +181,12 @@ private class AttributeIndexCfgManager implements @Override public boolean isConfigurationAddAcceptable(final BackendIndexCfg cfg, List unacceptableReasons) { + final LocalizableMessage alreadyIndexed = alreadyIndexedBy(cfg); + if (alreadyIndexed != null) + { + unacceptableReasons.add(alreadyIndexed); + return false; + } try { newAttributeIndex(cfg, null); @@ -194,22 +199,55 @@ public boolean isConfigurationAddAcceptable(final BackendIndexCfg cfg, List + * The map is keyed by the attribute type, which every one of the attribute's names and its OID resolve to, + * while the configuration entry is named by whichever of them was typed. An index declared under another of + * them - commonName or 2.5.4.3 for cn - names the very trees the live index serves, and the add would drop + * them as left behind by an index which is gone. + */ + private LocalizableMessage alreadyIndexedBy(final BackendIndexCfg cfg) + { + final AttributeIndex existing = attrIndexMap.get(cfg.getAttribute()); + if (existing == null) + { + return null; + } + return ERR_CONFIG_INDEX_ATTRIBUTE_ALREADY_INDEXED.get(cfg.getAttribute().getNameOrOID(), getBaseDN(), + existing.getConfiguration().dn()); + } + @Override public ConfigChangeResult applyConfigurationAdd(final BackendIndexCfg cfg) { final ConfigChangeResult ccr = new ConfigChangeResult(); + // Refused by isConfigurationAddAcceptable() before the configuration is written; asked again here since what + // follows drops the trees of the index it would have found. + final LocalizableMessage alreadyIndexed = alreadyIndexedBy(cfg); + if (alreadyIndexed != null) + { + ccr.setResultCode(ResultCode.UNWILLING_TO_PERFORM); + ccr.addMessage(alreadyIndexed); + return ccr; + } try { final CryptoSuite cryptoSuite = newCryptoSuite(cfg.isConfidentialityEnabled()); final AttributeIndex index = newAttributeIndex(cfg, cryptoSuite); - // Read outside the write which uses it: listTrees() borrows a connection of its own on JDBC, which a - // transaction already holding one of the same pool must not ask for. A tree an attempt of that write - // creates is not in it either, which is what a replayed attempt needs: it must drop what was there - // before this change, and not what the attempt before it made. - final Set storedTrees = storage.listTrees(); - final AtomicBoolean trusted = new AtomicBoolean(); + // Dropped in a write of its own, committed before the write which opens the index: the two must not + // share a transaction, see AttributeIndex.dropLeftovers(). final AtomicBoolean discarded = new AtomicBoolean(); storage.write(new WriteOperation() + { + @Override + public void run(WriteableTransaction txn) throws Exception + { + discarded.set(index.dropLeftovers(txn)); + } + }); + final AtomicBoolean trusted = new AtomicBoolean(); + storage.write(new WriteOperation() { @Override public void run(WriteableTransaction txn) throws Exception @@ -218,7 +256,7 @@ public void run(WriteableTransaction txn) throws Exception // its configuration. close() removes every registration made for this index, so closing first leaves // one listener behind rather than one per attempt; it is a no-op on the first attempt. index.close(); - discarded.set(index.openAsAdded(txn, storedTrees)); + index.open(txn, true); trusted.set(index.isTrusted()); attrIndexMap.put(cfg.getAttribute(), index); attrCryptoMap.put(cfg.getAttribute(), cryptoSuite); @@ -320,11 +358,19 @@ public ConfigChangeResult applyConfigurationAdd(final BackendVLVIndexCfg cfg) final ConfigChangeResult ccr = new ConfigChangeResult(); try { + // Dropped in a write of its own, committed before the write which builds and opens the index, for the + // reason given in the index add listener above. + final AtomicBoolean discarded = new AtomicBoolean(); + storage.write(new WriteOperation() + { + @Override + public void run(WriteableTransaction txn) throws Exception + { + discarded.set(VLVIndex.dropLeftovers(txn, EntryContainer.this, state, cfg.getName())); + } + }); final AtomicReference built = new AtomicReference<>(); final AtomicBoolean trusted = new AtomicBoolean(); - final AtomicBoolean discarded = new AtomicBoolean(); - // Read outside the write, for the reason given in the index add listener above. - final Set storedTrees = storage.listTrees(); storage.write(new WriteOperation() { @Override @@ -341,7 +387,7 @@ public void run(WriteableTransaction txn) throws Exception } VLVIndex vlvIndex = new VLVIndex(cfg, state, storage, EntryContainer.this, txn); built.set(vlvIndex); - discarded.set(vlvIndex.openAsAdded(txn, storedTrees)); + vlvIndex.open(txn, true); trusted.set(vlvIndex.isTrusted()); vlvIndexMap.put(cfg.getName().toLowerCase(), vlvIndex); } diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/VLVIndex.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/VLVIndex.java index 500bbdac60..223e4c0b5f 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/VLVIndex.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/VLVIndex.java @@ -31,7 +31,6 @@ import java.util.Iterator; import java.util.LinkedList; import java.util.List; -import java.util.Set; import java.util.TreeSet; import org.forgerock.i18n.LocalizableMessage; @@ -115,8 +114,8 @@ class VLVIndex extends AbstractTree implements ConfigurationChangeListener * What they hold is what the backend was told before the configuration stopped naming them, and no * entry written in between is in it; a rebuild regenerates all of it. See - * {@link AttributeIndex#openAsAdded} for why it is dropped rather than adopted (#990). + * {@link AttributeIndex#dropLeftovers} for why it is dropped rather than adopted (#990), and for + * why this must run in a write of its own, committed before the one which builds and opens the + * index: the constructor then reads its flag out of a record which is gone, and finds none. * * @param txn a non null transaction - * @param storedTrees the trees the storage holds, read before this transaction was opened - * @return true if anything left behind was dropped - * @throws StorageRuntimeException if an error occurs while opening the index + * @param entryContainer the entry container the index is being added to + * @param state the tree holding the index flags + * @param indexName the name of the VLV index being added + * @return true if a tree was dropped; a record deleted on its own discards nothing + * @throws StorageRuntimeException if an error occurs in the storage */ - boolean openAsAdded(WriteableTransaction txn, Set storedTrees) throws StorageRuntimeException + static boolean dropLeftovers(WriteableTransaction txn, EntryContainer entryContainer, State state, String indexName) + throws StorageRuntimeException { + final TreeName name = treeNameOf(entryContainer, indexName); + final TreeName counterName = counterTreeNameOf(entryContainer, indexName); boolean dropped = false; // Each of the two is asked for on its own: deleting a tree which is not there fails on PersistIt, // and a change which stopped halfway can have left one of them without the other. - if (storedTrees.contains(counter.getName())) + if (txn.treeExists(counterName)) { - counter.delete(txn); + txn.deleteTree(counterName); dropped = true; } - if (storedTrees.contains(getName())) + if (txn.treeExists(name)) { - txn.deleteTree(getName()); + txn.deleteTree(name); dropped = true; } // The record can outlive the trees: see AttributeIndex.dropLeftoversOf. - dropped |= state.deleteRecord(txn, getName()); - // The flag was read out of that record when this instance was built, and belongs to the index - // whose trees have just gone. afterOpen() upgrades it again if there is nothing to index. - trusted = false; - open(txn, true); + state.deleteRecord(txn, name); return dropped; } 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 1f65b63752..95e7919003 100644 --- a/opendj-server-legacy/src/messages/org/opends/messages/backend.properties +++ b/opendj-server-legacy/src/messages/org/opends/messages/backend.properties @@ -1161,3 +1161,6 @@ WARN_INDEX_ADD_DISCARDED_LEFTOVER_TREES_628=Index %s of backend base DN '%s' was What they hold is not what this index holds - every entry changed while nothing named them is missing from \ it - so their content has been discarded rather than adopted, and this index has been created empty. Rebuild \ it before it is used +ERR_CONFIG_INDEX_ATTRIBUTE_ALREADY_INDEXED_629=Attribute %s of backend base DN '%s' is already indexed by %s. \ + An attribute type is indexed once, whichever of its names or its OID the index is declared by, so change that \ + index instead of adding another one for the same attribute diff --git a/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/IndexAddedOverLeftoverTreesTest.java b/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/IndexAddedOverLeftoverTreesTest.java index 812ad379de..ca64aaab40 100644 --- a/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/IndexAddedOverLeftoverTreesTest.java +++ b/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/IndexAddedOverLeftoverTreesTest.java @@ -20,14 +20,19 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import static org.opends.messages.BackendMessages.ERR_CONFIG_INDEX_ATTRIBUTE_ALREADY_INDEXED; import static org.opends.messages.BackendMessages.NOTE_INDEX_ADD_REQUIRES_REBUILD; +import static org.opends.messages.BackendMessages.WARN_INDEX_ADD_DISCARDED_LEFTOVER_TREES; import static org.opends.server.backends.pluggable.State.IndexFlag.TRUSTED; import static org.opends.server.backends.pluggable.SuffixContainer.STATE_INDEX_NAME; import static org.opends.server.util.CollectionUtils.newTreeSet; +import java.util.ArrayList; +import java.util.Collection; import java.util.EnumSet; import java.util.HashMap; import java.util.HashSet; +import java.util.List; import java.util.Map; import java.util.Set; import java.util.SortedSet; @@ -44,10 +49,13 @@ import org.forgerock.opendj.server.config.meta.BackendVLVIndexCfgDefn.Scope; import org.forgerock.opendj.server.config.server.BackendIndexCfg; import org.forgerock.opendj.server.config.server.BackendVLVIndexCfg; +import org.forgerock.opendj.server.config.server.JEBackendCfg; import org.forgerock.opendj.server.config.server.PDBBackendCfg; +import org.forgerock.opendj.server.config.server.PluggableBackendCfg; import org.mockito.ArgumentCaptor; import org.opends.server.DirectoryServerTestCase; import org.opends.server.TestCaseUtils; +import org.opends.server.backends.jeb.JEStorage; import org.opends.server.backends.pdb.PDBStorage; import org.opends.server.backends.pluggable.AttributeIndex.MatchingRuleIndex; import org.opends.server.backends.pluggable.spi.Cursor; @@ -60,6 +68,7 @@ import org.opends.server.types.Entry; import org.testng.annotations.AfterMethod; import org.testng.annotations.BeforeClass; +import org.testng.annotations.DataProvider; import org.testng.annotations.Test; /** @@ -79,6 +88,12 @@ * ({@code EntryContainer}, its constructor and {@code close()}). A deletion the storage gives up on * and a stop between the deletion of the trees and the commit of the configuration change leave the * same state; #962 covers the first of those. + *

+ * Every case runs over PDB and over JE. The two lock differently: JE write-locks the name of a + * deleted tree until the transaction commits, and opening a tree of that name from the same + * transaction - which JE does under a transaction of its own - waits for it forever, so a drop and + * an open sharing one write never return there. The time limit is what makes that a failure rather + * than a hang; under Maven {@code TestListener} replaces it with {@code org.opends.test.timeout}. */ @SuppressWarnings("javadoc") @Test(groups = { "precommit", "pluggablebackend" }, sequential = true) @@ -87,6 +102,57 @@ public class IndexAddedOverLeftoverTreesTest extends DirectoryServerTestCase private static final String BACKEND_ID = "IndexAddedOverLeftoverTreesTest"; private static final DN BASE_DN = DN.valueOf("dc=b990,dc=com"); private static final String VLV_INDEX_NAME = "b990vlv"; + private static final long HANG_TIMEOUT_MS = 60_000; + + /** The storages the cases run over, each in a directory of its own. */ + enum StorageKind + { + PDB + { + @Override + PluggableBackendCfg newBackendCfg(String dbDirectory) + { + final PDBBackendCfg cfg = mockCfg(PDBBackendCfg.class); + when(cfg.getDBDirectory()).thenReturn(dbDirectory); + when(cfg.getDBDirectoryPermissions()).thenReturn("755"); + when(cfg.getDBCacheSize()).thenReturn(0L); + when(cfg.getDBCachePercent()).thenReturn(20); + return cfg; + } + + @Override + Storage newStorage(PluggableBackendCfg cfg, ServerContext serverContext) throws ConfigException + { + return new PDBStorage((PDBBackendCfg) cfg, serverContext); + } + }, + JE + { + @Override + PluggableBackendCfg newBackendCfg(String dbDirectory) + { + final JEBackendCfg cfg = mockCfg(JEBackendCfg.class); + when(cfg.getDBDirectory()).thenReturn(dbDirectory); + when(cfg.getDBDirectoryPermissions()).thenReturn("755"); + when(cfg.getDBCacheSize()).thenReturn(0L); + when(cfg.getDBCachePercent()).thenReturn(20); + when(cfg.getDBNumCleanerThreads()).thenReturn(2); + when(cfg.getDBNumLockTables()).thenReturn(63); + return cfg; + } + + @Override + Storage newStorage(PluggableBackendCfg cfg, ServerContext serverContext) throws ConfigException + { + return new JEStorage((JEBackendCfg) cfg, serverContext); + } + }; + + /** A configuration of this storage, with its files under the given directory. */ + abstract PluggableBackendCfg newBackendCfg(String dbDirectory); + + abstract Storage newStorage(PluggableBackendCfg cfg, ServerContext serverContext) throws ConfigException; + } private ServerContext serverContext; private AttributeType cnType; @@ -99,6 +165,12 @@ public void startServer() throws Exception cnType = serverContext.getSchema().getAttributeType("cn"); } + @DataProvider(name = "storages") + public Object[][] storages() + { + return new Object[][] { { StorageKind.PDB }, { StorageKind.JE } }; + } + /** * A test which fails before it finalizes its backend leaves the base DN behind in the server wide * registry, where it would outlive the test and break the next one to use it. @@ -119,12 +191,13 @@ public void deregisterLeftoverBaseDN() /** * The index the add opens is not the index whose trees are still there: it has indexed none of * the entries, so it must be untrusted and the operator must be told to rebuild it, exactly as - * for any other index added to a backend which already holds entries. + * for any other index added to a backend which already holds entries - and told, in the change + * result and in the error log, that content was discarded to get there. */ - @Test - public void anIndexAddedOverTheTreesLeftBehindIsNotTrusted() throws Exception + @Test(dataProvider = "storages", timeOut = HANG_TIMEOUT_MS) + public void anIndexAddedOverTheTreesLeftBehindIsNotTrusted(StorageKind kind) throws Exception { - final LeftoverBackend backend = leaveTreesBehind(); + final LeftoverBackend backend = leaveTreesBehind(kind); try { final EntryContainer ec = backend.getRootContainer().getEntryContainer(BASE_DN); @@ -137,7 +210,8 @@ public void anIndexAddedOverTheTreesLeftBehindIsNotTrusted() throws Exception assertThat(ec.getAttributeIndex(cnType).isTrusted()) .as("an index trusted over the content of trees it did not fill").isFalse(); assertThat(ccr.adminActionRequired()).isTrue(); - assertThat(ordinalsOf(ccr)).contains(NOTE_INDEX_ADD_REQUIRES_REBUILD.ordinal()); + assertThat(ordinalsOf(ccr.getMessages())).contains(NOTE_INDEX_ADD_REQUIRES_REBUILD.ordinal(), + WARN_INDEX_ADD_DISCARDED_LEFTOVER_TREES.ordinal()); } finally { @@ -152,10 +226,10 @@ public void anIndexAddedOverTheTreesLeftBehindIsNotTrusted() throws Exception * adopted trees has to go, or every key they hold goes on answering with the entries of another * index and misses everything written since. */ - @Test - public void anIndexAddedOverTheTreesLeftBehindDoesNotAnswerWithTheirContent() throws Exception + @Test(dataProvider = "storages", timeOut = HANG_TIMEOUT_MS) + public void anIndexAddedOverTheTreesLeftBehindDoesNotAnswerWithTheirContent(StorageKind kind) throws Exception { - final LeftoverBackend backend = leaveTreesBehind(); + final LeftoverBackend backend = leaveTreesBehind(kind); try { final EntryContainer ec = backend.getRootContainer().getEntryContainer(BASE_DN); @@ -163,16 +237,16 @@ public void anIndexAddedOverTheTreesLeftBehindDoesNotAnswerWithTheirContent() th final Map keysHeld = keysHeldBy(storage, backend.leftoverIndexTrees); assertThat(keysHeld).as("the keys the trees left behind hold").isNotEmpty(); - indexAddListener(backend).applyConfigurationAdd(backend.cnIndexCfg); + final ConfigChangeResult ccr = indexAddListener(backend).applyConfigurationAdd(backend.cnIndexCfg); + assertThat(ordinalsOf(ccr.getMessages())).contains(WARN_INDEX_ADD_DISCARDED_LEFTOVER_TREES.ordinal()); final AttributeIndex index = ec.getAttributeIndex(cnType); for (final MatchingRuleIndex opened : index.getNameToIndexes().values()) { final ByteString keyHeld = keysHeld.get(opened.getName()); if (keyHeld != null) { - final Boolean answered = storage.read(txn -> opened.get(txn, keyHeld).isDefined()); - assertThat(answered) + assertThat(answers(storage, opened, keyHeld)) .as("a key of " + opened.getName() + " answered out of the tree left behind").isFalse(); } } @@ -188,12 +262,13 @@ public void anIndexAddedOverTheTreesLeftBehindDoesNotAnswerWithTheirContent() th * DDL which commits of its own accord, while the {@code state.deleteRecord} of the same * {@code closeAndDelete} belongs to the transaction, so a rollback after the drop restores the * records over tables which are already gone. The index added next creates empty trees and reads - * TRUSTED out of those records - an empty index which every search believes. + * TRUSTED out of those records - an empty index which every search believes. Nothing is + * discarded on this road, so nothing says it was: the record goes, and the rebuild is asked for. */ - @Test - public void anIndexAddedOverAStateRecordWhoseTreesAreGoneIsNotTrusted() throws Exception + @Test(dataProvider = "storages", timeOut = HANG_TIMEOUT_MS) + public void anIndexAddedOverAStateRecordWhoseTreesAreGoneIsNotTrusted(StorageKind kind) throws Exception { - final LeftoverBackend backend = leaveTreesBehind(); + final LeftoverBackend backend = leaveTreesBehind(kind); try { final EntryContainer ec = backend.getRootContainer().getEntryContainer(BASE_DN); @@ -221,7 +296,9 @@ public void run(WriteableTransaction txn) throws Exception assertThat(ec.getAttributeIndex(cnType).isTrusted()) .as("an empty index trusted over a state record which outlived its trees").isFalse(); assertThat(ccr.adminActionRequired()).isTrue(); - assertThat(ordinalsOf(ccr)).contains(NOTE_INDEX_ADD_REQUIRES_REBUILD.ordinal()); + assertThat(ordinalsOf(ccr.getMessages())).contains(NOTE_INDEX_ADD_REQUIRES_REBUILD.ordinal()); + assertThat(ordinalsOf(ccr.getMessages())).as("content reported as discarded where no tree was") + .doesNotContain(WARN_INDEX_ADD_DISCARDED_LEFTOVER_TREES.ordinal()); } finally { @@ -229,17 +306,25 @@ public void run(WriteableTransaction txn) throws Exception } } - /** A VLV index adopts what it left behind in the same way, in its tree and in its counter. */ - @Test - public void aVlvIndexAddedOverTheTreesLeftBehindIsNotTrusted() throws Exception + /** + * A VLV index adopts what it left behind in the same way, in its tree and in its counter. Both are + * dropped, and so is the {@code state} record the next open of the backend would read the flag + * out of. + */ + @Test(dataProvider = "storages", timeOut = HANG_TIMEOUT_MS) + public void aVlvIndexAddedOverTheTreesLeftBehindIsNotTrusted(StorageKind kind) throws Exception { - final LeftoverBackend backend = leaveTreesBehind(); + final LeftoverBackend backend = leaveTreesBehind(kind); try { final EntryContainer ec = backend.getRootContainer().getEntryContainer(BASE_DN); - assertThat(backend.getRootContainer().getStorage().listTrees()) + final Storage storage = backend.getRootContainer().getStorage(); + assertThat(storage.listTrees()) .as("the VLV trees no configuration names any more") .contains(backend.leftoverVlvTree, backend.leftoverVlvCounterTree); + assertThat(holdsAnything(storage, backend.leftoverVlvTree)).as("the VLV tree left behind").isTrue(); + assertThat(holdsAnything(storage, backend.leftoverVlvCounterTree)).as("the VLV counter left behind").isTrue(); + assertThat(persistedFlags(backend, backend.leftoverVlvTree)).contains(TRUSTED); final ConfigChangeResult ccr = vlvIndexAddListener(backend).applyConfigurationAdd(backend.vlvIndexCfg); @@ -247,7 +332,14 @@ public void aVlvIndexAddedOverTheTreesLeftBehindIsNotTrusted() throws Exception assertThat(ec.getVLVIndex(VLV_INDEX_NAME).isTrusted()) .as("a VLV index trusted over the content of trees it did not fill").isFalse(); assertThat(ccr.adminActionRequired()).isTrue(); - assertThat(ordinalsOf(ccr)).contains(NOTE_INDEX_ADD_REQUIRES_REBUILD.ordinal()); + assertThat(ordinalsOf(ccr.getMessages())).contains(NOTE_INDEX_ADD_REQUIRES_REBUILD.ordinal(), + WARN_INDEX_ADD_DISCARDED_LEFTOVER_TREES.ordinal()); + assertThat(holdsAnything(storage, backend.leftoverVlvTree)) + .as("the VLV tree left behind still holds its content").isFalse(); + assertThat(holdsAnything(storage, backend.leftoverVlvCounterTree)) + .as("the VLV counter left behind still holds its count").isFalse(); + assertThat(persistedFlags(backend, backend.leftoverVlvTree)) + .as("the TRUSTED record outlived the add").doesNotContain(TRUSTED); } finally { @@ -258,18 +350,27 @@ public void aVlvIndexAddedOverTheTreesLeftBehindIsNotTrusted() throws Exception /** * An index type declared again for an attribute which is still indexed goes through * {@code AttributeIndex.applyConfigurationChange} rather than through the add listener, and opens - * its tree the same way. + * its tree the same way. Only the tree of the id declared again goes: the tree of the id the index + * was serving all along keeps what it holds. */ - @Test - public void anIndexTypeAddedOverTheTreeLeftBehindIsNotTrusted() throws Exception + @Test(dataProvider = "storages", timeOut = HANG_TIMEOUT_MS) + public void anIndexTypeAddedOverTheTreeLeftBehindIsNotTrusted(StorageKind kind) throws Exception { - final LeftoverBackend backend = leaveTreesBehind(indexCfg(newTreeSet(IndexType.EQUALITY))); + final LeftoverBackend backend = leaveTreesBehind(kind, indexCfg(newTreeSet(IndexType.EQUALITY))); try { final EntryContainer ec = backend.getRootContainer().getEntryContainer(BASE_DN); + final Storage storage = backend.getRootContainer().getStorage(); final AttributeIndex index = ec.getAttributeIndex(cnType); assertThat(index.isIndexed(IndexType.SUBSTRING)) .as("the index type this configuration no longer names").isFalse(); + final Map keysHeld = keysHeldBy(storage, backend.leftoverIndexTrees); + // The index ids are the matching rules' - the one the index serves now is the equality one. + final Map servedBefore = new HashMap<>(index.getNameToIndexes()); + assertThat(servedBefore).hasSize(1); + final MatchingRuleIndex equality = servedBefore.values().iterator().next(); + final ByteString equalityKey = keysHeld.get(equality.getName()); + assertThat(equalityKey).as("a key the live equality tree holds").isNotNull(); final ConfigChangeResult ccr = index.applyConfigurationChange(indexCfg(newTreeSet(IndexType.EQUALITY, IndexType.SUBSTRING))); @@ -279,7 +380,17 @@ public void anIndexTypeAddedOverTheTreeLeftBehindIsNotTrusted() throws Exception assertThat(index.isTrusted()) .as("an index type trusted over the content of the tree it did not fill").isFalse(); assertThat(ccr.adminActionRequired()).isTrue(); - assertThat(ordinalsOf(ccr)).contains(NOTE_INDEX_ADD_REQUIRES_REBUILD.ordinal()); + assertThat(ordinalsOf(ccr.getMessages())).contains(NOTE_INDEX_ADD_REQUIRES_REBUILD.ordinal(), + WARN_INDEX_ADD_DISCARDED_LEFTOVER_TREES.ordinal()); + final Map added = new HashMap<>(index.getNameToIndexes()); + added.keySet().removeAll(servedBefore.keySet()); + assertThat(added).hasSize(1); + final MatchingRuleIndex substring = added.values().iterator().next(); + assertThat(keysHeld).as("the substring tree left behind held a key").containsKey(substring.getName()); + assertThat(answers(storage, substring, keysHeld.get(substring.getName()))) + .as("a substring key answered out of the tree left behind").isFalse(); + assertThat(answers(storage, equality, equalityKey)) + .as("the live equality tree went with the leftover").isTrue(); } finally { @@ -291,10 +402,10 @@ public void anIndexTypeAddedOverTheTreeLeftBehindIsNotTrusted() throws Exception * An index added to a backend which holds no entry has nothing to index and nothing to adopt, so * it stays trusted and asks for nothing. Pins the upgrade {@code DefaultIndex.afterOpen} makes. */ - @Test - public void anIndexAddedToAnEmptyBackendIsTrustedAndAsksForNothing() throws Exception + @Test(dataProvider = "storages", timeOut = HANG_TIMEOUT_MS) + public void anIndexAddedToAnEmptyBackendIsTrustedAndAsksForNothing(StorageKind kind) throws Exception { - final LeftoverBackend backend = openBackend(true, null, null); + final LeftoverBackend backend = openBackend(kind, true, null, null); try { final ConfigChangeResult ccr = indexAddListener(backend).applyConfigurationAdd(backend.cnIndexCfg); @@ -314,12 +425,13 @@ public void anIndexAddedToAnEmptyBackendIsTrustedAndAsksForNothing() throws Exce /** * An index added to a backend which holds entries, with nothing left behind for it to adopt, is * untrusted over trees it created empty and asks to be rebuilt. Pins what the fix has to leave - * alone: this is the outcome an index added over leftover trees has to reach as well. + * alone: this is the outcome an index added over leftover trees has to reach as well, and nothing + * was discarded to reach it here. */ - @Test - public void anIndexAddedToANonEmptyBackendAsksForARebuild() throws Exception + @Test(dataProvider = "storages", timeOut = HANG_TIMEOUT_MS) + public void anIndexAddedToANonEmptyBackendAsksForARebuild(StorageKind kind) throws Exception { - final LeftoverBackend backend = openBackend(true, null, null); + final LeftoverBackend backend = openBackend(kind, true, null, null); try { addEntry(backend, "dn: " + BASE_DN, "objectClass: top", "objectClass: domain", "dc: b990"); @@ -330,7 +442,9 @@ public void anIndexAddedToANonEmptyBackendAsksForARebuild() throws Exception assertThat(backend.getRootContainer().getEntryContainer(BASE_DN).getAttributeIndex(cnType).isTrusted()) .as("an index which has indexed none of the entries").isFalse(); assertThat(ccr.adminActionRequired()).isTrue(); - assertThat(ordinalsOf(ccr)).contains(NOTE_INDEX_ADD_REQUIRES_REBUILD.ordinal()); + assertThat(ordinalsOf(ccr.getMessages())).contains(NOTE_INDEX_ADD_REQUIRES_REBUILD.ordinal()); + assertThat(ordinalsOf(ccr.getMessages())).as("content reported as discarded where nothing was left behind") + .doesNotContain(WARN_INDEX_ADD_DISCARDED_LEFTOVER_TREES.ordinal()); } finally { @@ -338,9 +452,61 @@ public void anIndexAddedToANonEmptyBackendAsksForARebuild() throws Exception } } - private LeftoverBackend leaveTreesBehind() throws Exception + /** + * The entry container keys its indexes by the attribute type, which every name of the attribute + * and its OID resolve to, while a configuration entry is named by whichever of them was typed. An + * index declared for cn as commonName or as 2.5.4.3 is therefore a second index for an attribute + * which is already indexed, naming the very trees the live index serves - and those must not go + * the way of trees left behind. The add is refused instead, before the configuration is written + * and again where it would have dropped them. + */ + @Test(dataProvider = "storages", timeOut = HANG_TIMEOUT_MS) + public void aSecondIndexForAnAttributeAlreadyIndexedIsRefused(StorageKind kind) throws Exception + { + final LeftoverBackend backend = + openBackend(kind, true, indexCfg(newTreeSet(IndexType.EQUALITY, IndexType.SUBSTRING)), null); + try + { + addEntry(backend, "dn: " + BASE_DN, "objectClass: top", "objectClass: domain", "dc: b990"); + addEntry(backend, "dn: cn=live," + BASE_DN, "objectClass: top", "objectClass: organizationalRole", "cn: live"); + final EntryContainer ec = backend.getRootContainer().getEntryContainer(BASE_DN); + final Storage storage = backend.getRootContainer().getStorage(); + final AttributeIndex live = ec.getAttributeIndex(cnType); + assertThat(live.isTrusted()).isTrue(); + final Map keysHeld = keysHeldBy(storage, treesOf(live)); + assertThat(keysHeld).as("the keys the live index holds").isNotEmpty(); + + // What was typed names the configuration entry; the attribute type it resolves to is the one already indexed. + final DN typedAs = DN.valueOf("ds-cfg-attribute=commonName,cn=Index," + backend.configuredWith.dn()); + final BackendIndexCfg declaredAgain = indexCfg(newTreeSet(IndexType.EQUALITY)); + when(declaredAgain.dn()).thenReturn(typedAs); + final ConfigurationAddListener listener = indexAddListener(backend); + + final List reasons = new ArrayList<>(); + assertThat(listener.isConfigurationAddAcceptable(declaredAgain, reasons)).isFalse(); + assertThat(ordinalsOf(reasons)).contains(ERR_CONFIG_INDEX_ATTRIBUTE_ALREADY_INDEXED.ordinal()); + + final ConfigChangeResult ccr = listener.applyConfigurationAdd(declaredAgain); + assertThat(ccr.getResultCode()).isNotEqualTo(ResultCode.SUCCESS); + assertThat(ordinalsOf(ccr.getMessages())).contains(ERR_CONFIG_INDEX_ATTRIBUTE_ALREADY_INDEXED.ordinal()); + + assertThat(ec.getAttributeIndex(cnType)).as("the live index was replaced").isSameAs(live); + assertThat(live.isTrusted()).isTrue(); + for (final MatchingRuleIndex opened : live.getNameToIndexes().values()) + { + assertThat(answers(storage, opened, keysHeld.get(opened.getName()))) + .as("a key of " + opened.getName() + " the live index held before").isTrue(); + } + } + finally + { + backend.finalizeBackend(); + } + } + + private LeftoverBackend leaveTreesBehind(StorageKind kind) throws Exception { - return leaveTreesBehind(null); + return leaveTreesBehind(kind, null); } /** @@ -349,10 +515,10 @@ private LeftoverBackend leaveTreesBehind() throws Exception * configuration which names no index at all. The trees of everything it no longer names are left * behind, and the entry added afterwards is in none of them. */ - private LeftoverBackend leaveTreesBehind(BackendIndexCfg reopenedWith) throws Exception + private LeftoverBackend leaveTreesBehind(StorageKind kind, BackendIndexCfg reopenedWith) throws Exception { - final LeftoverBackend indexed = openBackend(true, indexCfg(newTreeSet(IndexType.EQUALITY, IndexType.SUBSTRING)), - vlvIndexCfg()); + final LeftoverBackend indexed = + openBackend(kind, true, indexCfg(newTreeSet(IndexType.EQUALITY, IndexType.SUBSTRING)), vlvIndexCfg()); final Set indexTrees; final TreeName vlvTree; final TreeName vlvCounterTree; @@ -371,7 +537,7 @@ private LeftoverBackend leaveTreesBehind(BackendIndexCfg reopenedWith) throws Ex indexed.finalizeBackend(); } - final LeftoverBackend reopened = openBackend(false, reopenedWith, null); + final LeftoverBackend reopened = openBackend(kind, false, reopenedWith, null); try { addEntry(reopened, "dn: cn=fresh," + BASE_DN, "objectClass: top", "objectClass: organizationalRole", @@ -408,6 +574,22 @@ private static Map keysHeldBy(Storage storage, Set { + try (Cursor cursor = txn.openCursor(tree)) + { + return cursor.next(); + } + }); + } + + /** Whether the index answers the key with an entry ID set, as a search reading it would get one. */ + private static boolean answers(Storage storage, MatchingRuleIndex index, ByteString key) throws Exception + { + return storage.read(txn -> index.get(txn, key).isDefined()); + } + /** Reads back the flags an index tree carries, as they are stored. */ private EnumSet persistedFlags(LeftoverBackend backend, TreeName index) throws Exception { @@ -416,11 +598,11 @@ private EnumSet persistedFlags(LeftoverBackend backend, TreeNam return backend.getRootContainer().getStorage().read(txn -> state.getIndexFlags(txn, index)); } - /** The messages a change result carries, by identity rather than by their formatted text. */ - private static Set ordinalsOf(ConfigChangeResult ccr) + /** The messages a change carries, by identity rather than by their formatted text. */ + private static Set ordinalsOf(Collection messages) { final Set ordinals = new HashSet<>(); - for (final LocalizableMessage message : ccr.getMessages()) + for (final LocalizableMessage message : messages) { ordinals.add(message.ordinal()); } @@ -465,15 +647,15 @@ private static ConfigurationAddListener vlvIndexAddListener( * Opens a backend whose configuration names the given index and VLV index - null for one it does * not name, whose trees are then left to whatever is already in the storage. */ - private LeftoverBackend openBackend(boolean pristine, BackendIndexCfg cnIndexCfg, BackendVLVIndexCfg vlvIndexCfg) - throws Exception + private LeftoverBackend openBackend(StorageKind kind, boolean pristine, BackendIndexCfg cnIndexCfg, + BackendVLVIndexCfg vlvIndexCfg) throws Exception { - final LeftoverBackend backend = new LeftoverBackend(); + final LeftoverBackend backend = new LeftoverBackend(kind); backend.setBackendID(BACKEND_ID); backend.cnIndexCfg = cnIndexCfg != null ? cnIndexCfg : indexCfg(newTreeSet(IndexType.EQUALITY, IndexType.SUBSTRING)); backend.vlvIndexCfg = vlvIndexCfg != null ? vlvIndexCfg : vlvIndexCfg(); - backend.configuredWith = backendCfg(cnIndexCfg, vlvIndexCfg); + backend.configuredWith = backendCfg(kind, cnIndexCfg, vlvIndexCfg); backend.configureBackend(backend.configuredWith, serverContext); if (pristine) { @@ -486,13 +668,16 @@ private LeftoverBackend openBackend(boolean pristine, BackendIndexCfg cnIndexCfg } catch (Exception e) { - // openBackend() opens the root container before it registers the base DNs and the monitor, so - // a failure in any of those leaves the volume open and every following test failing here too. + // openBackend() opens the root container before it registers the base DNs and the monitor, so a failure in + // any of those leaves the volume open and every following test failing here too. finalizeBackend() is no + // use for it: closeBackend() deregisters the monitor before it closes the root container, and NPEs on the + // monitor which was never registered. try { - if (backend.getRootContainer() != null) + final RootContainer root = backend.getRootContainer(); + if (root != null) { - backend.finalizeBackend(); + root.close(); } else { @@ -508,15 +693,12 @@ private LeftoverBackend openBackend(boolean pristine, BackendIndexCfg cnIndexCfg return backend; } - private PDBBackendCfg backendCfg(BackendIndexCfg cnIndexCfg, BackendVLVIndexCfg vlvIndexCfg) throws ConfigException + private PluggableBackendCfg backendCfg(StorageKind kind, BackendIndexCfg cnIndexCfg, BackendVLVIndexCfg vlvIndexCfg) + throws ConfigException { - final PDBBackendCfg cfg = mockCfg(PDBBackendCfg.class); + final PluggableBackendCfg cfg = kind.newBackendCfg(BACKEND_ID + "." + kind); when(cfg.dn()).thenReturn(DN.valueOf("ds-cfg-backend-id=" + BACKEND_ID + ",cn=Backends,cn=config")); when(cfg.getBackendId()).thenReturn(BACKEND_ID); - when(cfg.getDBDirectory()).thenReturn(BACKEND_ID); - when(cfg.getDBDirectoryPermissions()).thenReturn("755"); - when(cfg.getDBCacheSize()).thenReturn(0L); - when(cfg.getDBCachePercent()).thenReturn(20); when(cfg.getBaseDN()).thenReturn(newTreeSet(BASE_DN)); when(cfg.listBackendIndexes()).thenReturn(cnIndexCfg != null ? new String[] { "cn" } : new String[0]); when(cfg.getBackendIndex("cn")).thenReturn(cnIndexCfg); @@ -529,6 +711,8 @@ private PDBBackendCfg backendCfg(BackendIndexCfg cnIndexCfg, BackendVLVIndexCfg private BackendIndexCfg indexCfg(SortedSet indexTypes) { final BackendIndexCfg cfg = mock(BackendIndexCfg.class); + when(cfg.dn()).thenReturn(DN.valueOf("ds-cfg-attribute=cn,cn=Index,ds-cfg-backend-id=" + BACKEND_ID + + ",cn=Backends,cn=config")); when(cfg.getIndexType()).thenReturn(indexTypes); when(cfg.getAttribute()).thenReturn(cnType); when(cfg.getIndexEntryLimit()).thenReturn(4000); @@ -547,12 +731,13 @@ private BackendVLVIndexCfg vlvIndexCfg() return cfg; } - /** A backend which keeps hold of the configuration its entry container registered with. */ - private static final class LeftoverBackend extends BackendImpl + /** A backend over the storage of the given kind, which keeps hold of the configuration its entry container registered with. */ + private static final class LeftoverBackend extends BackendImpl { - private PDBStorage storage; + private final StorageKind kind; + private Storage storage; /** The configuration the entry container registers its listeners with. */ - private PDBBackendCfg configuredWith; + private PluggableBackendCfg configuredWith; private BackendIndexCfg cnIndexCfg; private BackendVLVIndexCfg vlvIndexCfg; /** The trees of the indexes an earlier configuration named, which nothing names now. */ @@ -560,10 +745,15 @@ private static final class LeftoverBackend extends BackendImpl private TreeName leftoverVlvTree; private TreeName leftoverVlvCounterTree; + private LeftoverBackend(StorageKind kind) + { + this.kind = kind; + } + @Override - protected Storage configureStorage(PDBBackendCfg cfg, ServerContext serverContext) throws ConfigException + protected Storage configureStorage(PluggableBackendCfg cfg, ServerContext serverContext) throws ConfigException { - storage = new PDBStorage(cfg, serverContext); + storage = kind.newStorage(cfg, serverContext); return storage; } } diff --git a/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/ReplayedConfigChangeTest.java b/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/ReplayedConfigChangeTest.java index 5c4897a8c2..e84e6d2a18 100644 --- a/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/ReplayedConfigChangeTest.java +++ b/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/ReplayedConfigChangeTest.java @@ -597,8 +597,9 @@ public void anIndexAddedByAChangeIsReportedOnceWhenTheTransactionIsReplayed() th addBaseEntry(backend, KEPT, "b907a"); final AttributeIndex index = backend.getRootContainer().getEntryContainer(KEPT).getAttributeIndex(cnType); - // The first of the three writes a change makes is the one which opens the indexes it adds. - backend.storage.conflictAtCommit(1); + // The second of the four writes a change makes is the one which opens the indexes it adds; + // the first drops what an earlier index left behind for them. + backend.storage.conflictAtCommitOnWrite(2, 1); final ConfigChangeResult ccr = index.applyConfigurationChange(indexCfg(newTreeSet(IndexType.EQUALITY, IndexType.PRESENCE), 4000)); @@ -633,13 +634,14 @@ public void aRaisedEntryLimitUntrustsTheIndexWhenTheTransactionIsReplayed() thro final MatchingRuleIndex cnIndex = index.getNameToIndexes().values().iterator().next(); assertThat(persistedFlags(rootContainer, ec, cnIndex.getName())).contains(TRUSTED); - // The third write is the one which removes the flag: the first two open the indexes the - // change adds and delete the ones it removes, and it neither adds nor removes any. + // The fourth write is the one which removes the flag: the first three drop what an earlier + // index left behind for the indexes the change adds, open them, and delete the ones it + // removes, and it neither adds nor removes any. final int writesBefore = backend.storage.writes(); - backend.storage.conflictAtCommitOnWrite(3, 1); + backend.storage.conflictAtCommitOnWrite(4, 1); final ConfigChangeResult ccr = index.applyConfigurationChange(indexCfg(newTreeSet(IndexType.EQUALITY), 8000)); - assertThat(backend.storage.writes()).as("the armed write was the last of three").isEqualTo(writesBefore + 3); + assertThat(backend.storage.writes()).as("the armed write was the last of four").isEqualTo(writesBefore + 4); assertThat(backend.storage.attempts()).isEqualTo(2); assertThat(ccr.getResultCode()).isEqualTo(ResultCode.SUCCESS); assertThat(ccr.adminActionRequired()).isTrue(); @@ -790,7 +792,7 @@ public void aRaisedEntryLimitIsReportedWhenTheWriteWhichUntrustsTheIndexGivesUp( } /** - * The same instruction survives a give-up on the first of the three writes, the one which opens + * The same instruction survives a give-up on the second of the four writes, the one which opens * the indexes the change adds - opened here with none to add: it is asked for before any of the * writes, not only before the one which untrusts the index, since the configuration entry holds * the raised limit whichever of them fails. @@ -808,11 +810,11 @@ public void aRaisedEntryLimitIsReportedWhenTheWriteWhichAddsIndexesGivesUp() thr assertThat(persistedFlags(rootContainer, ec, cnIndex.getName())).contains(TRUSTED); final int writesBefore = backend.storage.writes(); - backend.storage.failWithoutReplayOnWrite(1); + backend.storage.failWithoutReplayOnWrite(2); final ConfigChangeResult ccr = index.applyConfigurationChange(indexCfg(newTreeSet(IndexType.EQUALITY), 8000)); - assertThat(backend.storage.writes()).as("the armed write was the first of three, and the last one made") - .isEqualTo(writesBefore + 1); + assertThat(backend.storage.writes()).as("the armed write was the second of four, and the last one made") + .isEqualTo(writesBefore + 2); assertThat(ccr.getResultCode()).isEqualTo(serverErrorResultCode()); assertThat(ccr.adminActionRequired()).as("the rebuild the raised limit needs, asked for before the first write") .isTrue(); @@ -850,8 +852,9 @@ public void aLoweredEntryLimitNeedsNoRebuildAndOpensNoTransaction() throws Excep assertThat(ccr.getMessages()).isEmpty(); assertThat(cnIndex.isTrusted()).isTrue(); assertThat(cnIndex.getIndexEntryLimit()).as("the limit the change applied").isEqualTo(2000); - assertThat(backend.storage.writes()).as("the two writes which add and remove indexes, and no third") - .isEqualTo(writesBefore + 2); + assertThat(backend.storage.writes()) + .as("the three writes which drop leftovers, add and remove indexes, and no fourth") + .isEqualTo(writesBefore + 3); } finally { From 1104fb6ba505e5a242446113b2d3dbc8967d1ad7 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Fri, 18 Sep 2026 22:02:23 +0300 Subject: [PATCH 3/6] [#990] Report a leftover discard from a finally, and close the coverage gaps round 2's tests left The discard AttributeIndex.dropLeftovers finds is decided by the write which commits it, but was reported only after the write which opens the index returned - so WARN 628 and the error log line it promises were silently skipped whenever that second write threw. All three roads (the attribute index add listener, the VLV add listener, and applyConfigurationChange) now report it from a finally instead. Five gaps the test suite left for a mutant to walk through are pinned: the answers() helper checked isDefined() alone, which a trusted index also returns for an absent key; isConfigurationAddAcceptable was pinned on its refusing arm only; VLVIndex.dropLeftovers' two treeExists guards were never exercised on their false arm; the logger.warn half of reportDiscardedLeftovers was asserted by no case; and the dropped |= fold in dropLeftovers was indistinguishable from a plain assignment. --- .../backends/pluggable/AttributeIndex.java | 20 ++- .../backends/pluggable/EntryContainer.java | 40 +++-- .../IndexAddedOverLeftoverTreesTest.java | 140 +++++++++++++++++- 3 files changed, 174 insertions(+), 26 deletions(-) diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/AttributeIndex.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/AttributeIndex.java index f45a782231..4124f20fc7 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/AttributeIndex.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/AttributeIndex.java @@ -1004,6 +1004,11 @@ public synchronized ConfigChangeResult applyConfigurationChange(final BackendInd { final ConfigChangeResult ccr = new ConfigChangeResult(); final IndexingOptions newIndexingOptions = new IndexingOptionsImpl(newConfiguration.getSubstringLength()); + // Drop what an earlier index left behind for the added ids, in a write of its own: the drop and the open + // must not share a transaction, see dropLeftovers(). discarded is filled by that committed write, so + // reporting it from the finally below repeats nothing on a replayed attempt, and is not skipped when the + // write which opens the added indexes - or a later write in this change - throws after it. + final List discarded = new ArrayList<>(); try { final Map newIndexIdToIndexes = buildIndexes(entryContainer, state, newConfiguration, @@ -1042,10 +1047,6 @@ public synchronized ConfigChangeResult applyConfigurationChange(final BackendInd ccr.addMessage(rebuildMessage); } - // Drop what an earlier index left behind for the added ids, in a write of its own: the drop and the open - // must not share a transaction, see dropLeftovers(). Both writes may be replayed by the storage, so what - // they found is reported once they are done, and by the attempt which went through. - final List discarded = new ArrayList<>(); entryContainer.getRootContainer().getStorage().write(new WriteOperation() { @Override @@ -1061,10 +1062,6 @@ public void run(WriteableTransaction txn) throws Exception } } }); - for (MatchingRuleIndex index : discarded) - { - reportDiscardedLeftovers(ccr, index.getName(), entryContainer.getBaseDN()); - } // Open added indexes *before* adding them to indexIdToIndexes final List addedIndexesToRebuild = new ArrayList<>(); @@ -1165,6 +1162,13 @@ public void run(WriteableTransaction txn) throws Exception ccr.setAdminActionRequired(true); ccr.addMessage(message); } + finally + { + for (MatchingRuleIndex index : discarded) + { + reportDiscardedLeftovers(ccr, index.getName(), entryContainer.getBaseDN()); + } + } return ccr; } diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/EntryContainer.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/EntryContainer.java index 562c88ad8d..26642bea18 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/EntryContainer.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/EntryContainer.java @@ -231,13 +231,15 @@ public ConfigChangeResult applyConfigurationAdd(final BackendIndexCfg cfg) ccr.addMessage(alreadyIndexed); return ccr; } + // Dropped in a write of its own, committed before the write which opens the index: the two must not + // share a transaction, see AttributeIndex.dropLeftovers(). discarded is filled by that committed write, so + // reporting it from a finally below repeats nothing on a replayed attempt, and is not skipped when the + // write which opens the index throws after it. + final AtomicBoolean discarded = new AtomicBoolean(); try { final CryptoSuite cryptoSuite = newCryptoSuite(cfg.isConfidentialityEnabled()); final AttributeIndex index = newAttributeIndex(cfg, cryptoSuite); - // Dropped in a write of its own, committed before the write which opens the index: the two must not - // share a transaction, see AttributeIndex.dropLeftovers(). - final AtomicBoolean discarded = new AtomicBoolean(); storage.write(new WriteOperation() { @Override @@ -262,11 +264,6 @@ public void run(WriteableTransaction txn) throws Exception attrCryptoMap.put(cfg.getAttribute(), cryptoSuite); } }); - if (discarded.get()) - { - // Reported outside the write, since a replayed attempt would otherwise repeat the message. - AttributeIndex.reportDiscardedLeftovers(ccr, cfg.getAttribute().getNameOrOID(), getBaseDN()); - } if (!trusted.get()) { // Reported outside the write, since a replayed attempt would otherwise repeat the message. @@ -279,6 +276,13 @@ public void run(WriteableTransaction txn) throws Exception ccr.setResultCode(DirectoryServer.getCoreConfigManager().getServerErrorResultCode()); ccr.addMessage(LocalizableMessage.raw(e.getLocalizedMessage())); } + finally + { + if (discarded.get()) + { + AttributeIndex.reportDiscardedLeftovers(ccr, cfg.getAttribute().getNameOrOID(), getBaseDN()); + } + } return ccr; } @@ -356,11 +360,13 @@ public boolean isConfigurationAddAcceptable(BackendVLVIndexCfg cfg, List errorLog = registerErrorLogCapture(); try { final EntryContainer ec = backend.getRootContainer().getEntryContainer(BASE_DN); @@ -212,9 +220,13 @@ public void anIndexAddedOverTheTreesLeftBehindIsNotTrusted(StorageKind kind) thr assertThat(ccr.adminActionRequired()).isTrue(); assertThat(ordinalsOf(ccr.getMessages())).contains(NOTE_INDEX_ADD_REQUIRES_REBUILD.ordinal(), WARN_INDEX_ADD_DISCARDED_LEFTOVER_TREES.ordinal()); + assertThat(warningOrdinalsLoggedTo(errorLog)) + .as("the discard reported in the error log, not only in the change result") + .contains(WARN_INDEX_ADD_DISCARDED_LEFTOVER_TREES.ordinal()); } finally { + ErrorLogger.getInstance().removeLogPublisher(errorLog); backend.finalizeBackend(); } } @@ -306,6 +318,43 @@ public void run(WriteableTransaction txn) throws Exception } } + /** + * {@code dropLeftovers} folds each id's result with {@code dropped |= ...} rather than the last one + * assigned; the two only disagree when the ids themselves do, which every other case avoids by giving + * all of them a leftover or none. Deleting one of the two trees {@code leaveTreesBehind} left - the + * cn index has EQUALITY and SUBSTRING - ahead of the add leaves the other one real, so the fold and + * the assignment can differ on whether the change discarded anything. + */ + @Test(dataProvider = "storages", timeOut = HANG_TIMEOUT_MS) + public void anIndexAddedOverOnlyOneOfTwoLeftoverTreesIsReportedDiscarded(StorageKind kind) throws Exception + { + final LeftoverBackend backend = leaveTreesBehind(kind); + try + { + final Storage storage = backend.getRootContainer().getStorage(); + final TreeName oneOfTwo = backend.leftoverIndexTrees.iterator().next(); + storage.write(new WriteOperation() + { + @Override + public void run(WriteableTransaction txn) throws Exception + { + txn.deleteTree(oneOfTwo); + } + }); + + final ConfigChangeResult ccr = indexAddListener(backend).applyConfigurationAdd(backend.cnIndexCfg); + + assertThat(ccr.getResultCode()).isEqualTo(ResultCode.SUCCESS); + assertThat(ordinalsOf(ccr.getMessages())) + .as("the other of the two ids still had a real tree to discard") + .contains(WARN_INDEX_ADD_DISCARDED_LEFTOVER_TREES.ordinal()); + } + finally + { + backend.finalizeBackend(); + } + } + /** * A VLV index adopts what it left behind in the same way, in its tree and in its counter. Both are * dropped, and so is the {@code state} record the next open of the backend would read the flag @@ -315,6 +364,7 @@ public void run(WriteableTransaction txn) throws Exception public void aVlvIndexAddedOverTheTreesLeftBehindIsNotTrusted(StorageKind kind) throws Exception { final LeftoverBackend backend = leaveTreesBehind(kind); + final ErrorLogPublisher errorLog = registerErrorLogCapture(); try { final EntryContainer ec = backend.getRootContainer().getEntryContainer(BASE_DN); @@ -334,6 +384,9 @@ public void aVlvIndexAddedOverTheTreesLeftBehindIsNotTrusted(StorageKind kind) t assertThat(ccr.adminActionRequired()).isTrue(); assertThat(ordinalsOf(ccr.getMessages())).contains(NOTE_INDEX_ADD_REQUIRES_REBUILD.ordinal(), WARN_INDEX_ADD_DISCARDED_LEFTOVER_TREES.ordinal()); + assertThat(warningOrdinalsLoggedTo(errorLog)) + .as("the discard reported in the error log, not only in the change result") + .contains(WARN_INDEX_ADD_DISCARDED_LEFTOVER_TREES.ordinal()); assertThat(holdsAnything(storage, backend.leftoverVlvTree)) .as("the VLV tree left behind still holds its content").isFalse(); assertThat(holdsAnything(storage, backend.leftoverVlvCounterTree)) @@ -342,6 +395,52 @@ public void aVlvIndexAddedOverTheTreesLeftBehindIsNotTrusted(StorageKind kind) t .as("the TRUSTED record outlived the add").doesNotContain(TRUSTED); } finally + { + ErrorLogger.getInstance().removeLogPublisher(errorLog); + backend.finalizeBackend(); + } + } + + /** + * The state record can outlive the VLV tree and its counter the same way it outlives an + * attribute index's tree + * ({@link #anIndexAddedOverAStateRecordWhoseTreesAreGoneIsNotTrusted(StorageKind)}): deleting + * them by hand exercises the two {@code treeExists} guards in + * {@code VLVIndex.dropLeftovers} on their false arm, which the fixture of the case above never + * reaches. On JE, {@code deleteTree} of a tree which is not there is silent, so a guard removed + * or inverted would say content was discarded where none was. + */ + @Test(dataProvider = "storages", timeOut = HANG_TIMEOUT_MS) + public void aVlvIndexAddedOverAStateRecordWhoseTreesAreGoneIsNotTrusted(StorageKind kind) throws Exception + { + final LeftoverBackend backend = leaveTreesBehind(kind); + try + { + final EntryContainer ec = backend.getRootContainer().getEntryContainer(BASE_DN); + final Storage storage = backend.getRootContainer().getStorage(); + storage.write(new WriteOperation() + { + @Override + public void run(WriteableTransaction txn) throws Exception + { + txn.deleteTree(backend.leftoverVlvTree); + txn.deleteTree(backend.leftoverVlvCounterTree); + } + }); + assertThat(persistedFlags(backend, backend.leftoverVlvTree)) + .as("the state record left over the VLV tree which is gone").contains(TRUSTED); + + final ConfigChangeResult ccr = vlvIndexAddListener(backend).applyConfigurationAdd(backend.vlvIndexCfg); + + assertThat(ccr.getResultCode()).isEqualTo(ResultCode.SUCCESS); + assertThat(ec.getVLVIndex(VLV_INDEX_NAME).isTrusted()) + .as("an empty VLV index trusted over a state record which outlived its trees").isFalse(); + assertThat(ccr.adminActionRequired()).isTrue(); + assertThat(ordinalsOf(ccr.getMessages())).contains(NOTE_INDEX_ADD_REQUIRES_REBUILD.ordinal()); + assertThat(ordinalsOf(ccr.getMessages())).as("content reported as discarded where no tree was") + .doesNotContain(WARN_INDEX_ADD_DISCARDED_LEFTOVER_TREES.ordinal()); + } + finally { backend.finalizeBackend(); } @@ -482,7 +581,14 @@ public void aSecondIndexForAnAttributeAlreadyIndexedIsRefused(StorageKind kind) when(declaredAgain.dn()).thenReturn(typedAs); final ConfigurationAddListener listener = indexAddListener(backend); + // An index for an attribute which is not indexed yet is still accepted - pins the refusing arm above + // against a mutant which refuses every add. + final BackendIndexCfg fresh = indexCfg(newTreeSet(IndexType.EQUALITY)); + when(fresh.getAttribute()).thenReturn(serverContext.getSchema().getAttributeType("sn")); final List reasons = new ArrayList<>(); + assertThat(listener.isConfigurationAddAcceptable(fresh, reasons)).isTrue(); + assertThat(reasons).isEmpty(); + assertThat(listener.isConfigurationAddAcceptable(declaredAgain, reasons)).isFalse(); assertThat(ordinalsOf(reasons)).contains(ERR_CONFIG_INDEX_ATTRIBUTE_ALREADY_INDEXED.ordinal()); @@ -584,10 +690,17 @@ private static boolean holdsAnything(Storage storage, TreeName tree) throws Exce }); } - /** Whether the index answers the key with an entry ID set, as a search reading it would get one. */ + /** + * Whether the index answers the key with a non-empty entry ID set, as a search reading it would get one. + * {@code DefaultIndex.get} answers {@code newDefinedSet()} for a missing key while the index is trusted, so + * {@code isDefined()} alone would pin the trusted flag rather than the content a leftover tree still holds. + */ private static boolean answers(Storage storage, MatchingRuleIndex index, ByteString key) throws Exception { - return storage.read(txn -> index.get(txn, key).isDefined()); + return storage.read(txn -> { + final EntryIDSet ids = index.get(txn, key); + return ids.isDefined() && ids.size() > 0; + }); } /** Reads back the flags an index tree carries, as they are stored. */ @@ -609,6 +722,29 @@ private static Set ordinalsOf(Collection messages) return ordinals; } + /** + * Registers a mock {@code ErrorLogPublisher}, enabled for every category and severity, so that + * {@link #warningOrdinalsLoggedTo} can read back what {@code AttributeIndex.reportDiscardedLeftovers} + * put in the error log - the half of it the change result alone does not exercise. The caller removes + * it once done, from a {@code finally}: it would otherwise go on capturing messages other tests log. + */ + @SuppressWarnings("unchecked") + private static ErrorLogPublisher registerErrorLogCapture() + { + final ErrorLogPublisher errorLog = mock(ErrorLogPublisher.class); + when(errorLog.isEnabledFor(any(), any())).thenReturn(true); + ErrorLogger.getInstance().addLogPublisher(errorLog); + return errorLog; + } + + /** The ordinals of the messages logged at {@link Severity#WARNING} through the given capture. */ + private static Set warningOrdinalsLoggedTo(ErrorLogPublisher errorLog) + { + final ArgumentCaptor logged = ArgumentCaptor.forClass(LocalizableMessage.class); + verify(errorLog, atLeastOnce()).log(any(), eq(Severity.WARNING), logged.capture(), any()); + return ordinalsOf(logged.getAllValues()); + } + private static Set treesOf(AttributeIndex index) { final Set names = new HashSet<>(); From e542d5ba383bc4ab5221055a242c37168abf9231 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Sat, 19 Sep 2026 12:41:19 +0300 Subject: [PATCH 4/6] [#990] Pin the discard report against a failed write, and skip the drop write for a change which adds no index The finally which reports WARN 628 reads a flag the drop write sets inside run(), before its commit. On JE and PDB a failure at that commit rolls the drop back with the flag already set, so the report then says more than happened. Kept that way on purpose, and written down in dropLeftovers()' javadoc and at the three sites which used to promise "filled by that committed write": the configuration entry is already written when the listener runs, so what comes next is not another add but the next open of the backend, which adopts the trees with their TRUSTED flag - and the rebuild the report asks for is what puts that right. On JDBC the same failure comes after a DROP which committed on its own, and the report is the only trace of it; a report made to wait for the commit would be silent there, in the one case the drop is for. Neither engine can be made to fail a write on its own, so the test class now wraps the engine's storage in a delegating RefusingStorage which refuses the one write a case arms, before it runs or once it has run. Four cases use it: the report survives a refused open write on each of the three roads, and survives a drop write which fails after it ran - which pins the choice above. The index name and the base DN 628 carries are asserted as rendered text on each road as well, not by ordinal alone. applyConfigurationChange opens no transaction for the drop when the change adds no index, as #997's write which untrusts an index opens none when there is nothing to untrust; three of the four ReplayedConfigChangeTest pins the drop write had moved are back at master's numbering. --- .../backends/pluggable/AttributeIndex.java | 40 ++- .../backends/pluggable/EntryContainer.java | 13 +- .../IndexAddedOverLeftoverTreesTest.java | 333 +++++++++++++++++- .../pluggable/ReplayedConfigChangeTest.java | 23 +- 4 files changed, 377 insertions(+), 32 deletions(-) diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/AttributeIndex.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/AttributeIndex.java index 4124f20fc7..fa7e27e0a4 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/AttributeIndex.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/AttributeIndex.java @@ -472,6 +472,16 @@ void open(WriteableTransaction txn, boolean createOnDemand) throws StorageRuntim * a tree - which {@code JEStorage} does under a transaction of its own - asks for a read lock on * that record and waits for it without limit: no cycle, so the deadlock detector is silent, and * the configuration change never returns. + *

+ * What this answers, the caller reports once every write of its change is over, whichever way + * they went - and when the write this ran in fails at its commit as well, although on JE and PDB + * that failure rolls the drop back. The report then overstates what happened: the trees are still + * there, the configuration entry is already written ({@code ConfigurationHandler} writes it before + * it notifies any listener), and the next open of the backend adopts them with their TRUSTED + * flag; the rebuild the report asks for is what puts that right. On JDBC the DROP has committed on + * its own before that commit failed, the record is back over a table which is gone, and the report + * is the only trace of it. Reported from a flag copied once the write has returned instead, the + * JE and PDB reports would be exact and the JDBC one silent, in the one case this method is for. * * @param txn a non null transaction * @return true if a tree was dropped; a record deleted on its own discards nothing @@ -1005,9 +1015,10 @@ public synchronized ConfigChangeResult applyConfigurationChange(final BackendInd final ConfigChangeResult ccr = new ConfigChangeResult(); final IndexingOptions newIndexingOptions = new IndexingOptionsImpl(newConfiguration.getSubstringLength()); // Drop what an earlier index left behind for the added ids, in a write of its own: the drop and the open - // must not share a transaction, see dropLeftovers(). discarded is filled by that committed write, so - // reporting it from the finally below repeats nothing on a replayed attempt, and is not skipped when the - // write which opens the added indexes - or a later write in this change - throws after it. + // must not share a transaction, see dropLeftovers(). discarded is filled by that write and reported from + // the finally below: every attempt fills it afresh, so a replayed attempt repeats nothing, and the report + // is not skipped when the write which opens the added indexes - or a later write of this change - throws + // after it, nor when the drop write fails at its own commit, for the reason dropLeftovers() gives. final List discarded = new ArrayList<>(); try { @@ -1047,21 +1058,26 @@ public synchronized ConfigChangeResult applyConfigurationChange(final BackendInd ccr.addMessage(rebuildMessage); } - entryContainer.getRootContainer().getStorage().write(new WriteOperation() + // A change which adds no index has nothing to drop, and opens no transaction for it - as the + // write which untrusts an index below opens none when there is nothing to untrust. + if (!addedIndexes.isEmpty()) { - @Override - public void run(WriteableTransaction txn) throws Exception + entryContainer.getRootContainer().getStorage().write(new WriteOperation() { - discarded.clear(); - for (MatchingRuleIndex addedIndex : addedIndexes.values()) + @Override + public void run(WriteableTransaction txn) throws Exception { - if (dropLeftoversOf(txn, addedIndex)) + discarded.clear(); + for (MatchingRuleIndex addedIndex : addedIndexes.values()) { - discarded.add(addedIndex); + if (dropLeftoversOf(txn, addedIndex)) + { + discarded.add(addedIndex); + } } } - } - }); + }); + } // Open added indexes *before* adding them to indexIdToIndexes final List addedIndexesToRebuild = new ArrayList<>(); diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/EntryContainer.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/EntryContainer.java index 26642bea18..4c6d71b47b 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/EntryContainer.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/EntryContainer.java @@ -232,9 +232,10 @@ public ConfigChangeResult applyConfigurationAdd(final BackendIndexCfg cfg) return ccr; } // Dropped in a write of its own, committed before the write which opens the index: the two must not - // share a transaction, see AttributeIndex.dropLeftovers(). discarded is filled by that committed write, so - // reporting it from a finally below repeats nothing on a replayed attempt, and is not skipped when the - // write which opens the index throws after it. + // share a transaction, see AttributeIndex.dropLeftovers(). discarded is filled by that write and reported + // from a finally below: every attempt fills it afresh, so a replayed attempt repeats nothing, and the + // report is not skipped when the write which opens the index throws after it, nor when the drop write + // fails at its own commit - for the reason AttributeIndex.dropLeftovers() gives. final AtomicBoolean discarded = new AtomicBoolean(); try { @@ -361,9 +362,9 @@ public ConfigChangeResult applyConfigurationAdd(final BackendVLVIndexCfg cfg) { final ConfigChangeResult ccr = new ConfigChangeResult(); // Dropped in a write of its own, committed before the write which builds and opens the index, for the - // reason given in the index add listener above. discarded is filled by that committed write, so reporting - // it from a finally below repeats nothing on a replayed attempt, and is not skipped when the write which - // builds and opens the index throws after it. + // reason given in the index add listener above. discarded is filled by that write and reported from a + // finally below, on the terms given there: not repeated by a replayed attempt, not skipped when the write + // which builds and opens the index throws after it, nor when the drop write fails at its own commit. final AtomicBoolean discarded = new AtomicBoolean(); try { diff --git a/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/IndexAddedOverLeftoverTreesTest.java b/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/IndexAddedOverLeftoverTreesTest.java index c3fffe9849..6082579715 100644 --- a/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/IndexAddedOverLeftoverTreesTest.java +++ b/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/IndexAddedOverLeftoverTreesTest.java @@ -63,8 +63,12 @@ import org.opends.server.backends.jeb.JEStorage; import org.opends.server.backends.pdb.PDBStorage; import org.opends.server.backends.pluggable.AttributeIndex.MatchingRuleIndex; +import org.opends.server.backends.pluggable.spi.AccessMode; import org.opends.server.backends.pluggable.spi.Cursor; +import org.opends.server.backends.pluggable.spi.Importer; +import org.opends.server.backends.pluggable.spi.ReadOperation; import org.opends.server.backends.pluggable.spi.Storage; +import org.opends.server.backends.pluggable.spi.StorageStatus; import org.opends.server.backends.pluggable.spi.TreeName; import org.opends.server.backends.pluggable.spi.WriteOperation; import org.opends.server.backends.pluggable.spi.WriteableTransaction; @@ -72,7 +76,11 @@ import org.opends.server.core.ServerContext; import org.opends.server.loggers.ErrorLogPublisher; import org.opends.server.loggers.ErrorLogger; +import org.opends.server.types.BackupConfig; +import org.opends.server.types.BackupDirectory; +import org.opends.server.types.DirectoryException; import org.opends.server.types.Entry; +import org.opends.server.types.RestoreConfig; import org.testng.annotations.AfterMethod; import org.testng.annotations.BeforeClass; import org.testng.annotations.DataProvider; @@ -220,6 +228,8 @@ public void anIndexAddedOverTheTreesLeftBehindIsNotTrusted(StorageKind kind) thr assertThat(ccr.adminActionRequired()).isTrue(); assertThat(ordinalsOf(ccr.getMessages())).contains(NOTE_INDEX_ADD_REQUIRES_REBUILD.ordinal(), WARN_INDEX_ADD_DISCARDED_LEFTOVER_TREES.ordinal()); + assertThat(renderedOf(ccr.getMessages())).as("the index and the base DN the report names") + .contains(WARN_INDEX_ADD_DISCARDED_LEFTOVER_TREES.get("cn", BASE_DN).toString()); assertThat(warningOrdinalsLoggedTo(errorLog)) .as("the discard reported in the error log, not only in the change result") .contains(WARN_INDEX_ADD_DISCARDED_LEFTOVER_TREES.ordinal()); @@ -384,6 +394,8 @@ public void aVlvIndexAddedOverTheTreesLeftBehindIsNotTrusted(StorageKind kind) t assertThat(ccr.adminActionRequired()).isTrue(); assertThat(ordinalsOf(ccr.getMessages())).contains(NOTE_INDEX_ADD_REQUIRES_REBUILD.ordinal(), WARN_INDEX_ADD_DISCARDED_LEFTOVER_TREES.ordinal()); + assertThat(renderedOf(ccr.getMessages())).as("the index and the base DN the report names") + .contains(WARN_INDEX_ADD_DISCARDED_LEFTOVER_TREES.get(VLV_INDEX_NAME, BASE_DN).toString()); assertThat(warningOrdinalsLoggedTo(errorLog)) .as("the discard reported in the error log, not only in the change result") .contains(WARN_INDEX_ADD_DISCARDED_LEFTOVER_TREES.ordinal()); @@ -485,6 +497,9 @@ public void anIndexTypeAddedOverTheTreeLeftBehindIsNotTrusted(StorageKind kind) added.keySet().removeAll(servedBefore.keySet()); assertThat(added).hasSize(1); final MatchingRuleIndex substring = added.values().iterator().next(); + // Named by its tree on this road, where the add listener names the index by its attribute. + assertThat(renderedOf(ccr.getMessages())).as("the tree and the base DN the report names") + .contains(WARN_INDEX_ADD_DISCARDED_LEFTOVER_TREES.get(substring.getName(), BASE_DN).toString()); assertThat(keysHeld).as("the substring tree left behind held a key").containsKey(substring.getName()); assertThat(answers(storage, substring, keysHeld.get(substring.getName()))) .as("a substring key answered out of the tree left behind").isFalse(); @@ -497,6 +512,165 @@ public void anIndexTypeAddedOverTheTreeLeftBehindIsNotTrusted(StorageKind kind) } } + /** + * What the drop write found is reported from a {@code finally}, so a throw from the write which + * opens the index - after the drop has committed and the content is gone for good - does not + * swallow it: the change fails, and the change result and the error log still say what was + * discarded. Neither storage engine can be made to throw on its own, so the failure is injected + * through {@link RefusingStorage}: the drop write goes through, the open write is refused before + * it runs. + */ + @Test(dataProvider = "storages", timeOut = HANG_TIMEOUT_MS) + public void aDiscardIsStillReportedWhenTheWriteWhichOpensTheIndexFails(StorageKind kind) throws Exception + { + final LeftoverBackend backend = leaveTreesBehind(kind); + final ErrorLogPublisher errorLog = registerErrorLogCapture(); + try + { + final Storage storage = backend.getRootContainer().getStorage(); + final int writesBefore = backend.storage.writes(); + backend.storage.refuseWrite(2); + + final ConfigChangeResult ccr = indexAddListener(backend).applyConfigurationAdd(backend.cnIndexCfg); + + assertThat(backend.storage.writes()).as("the refused write was the second, and the last one made") + .isEqualTo(writesBefore + 2); + assertThat(ccr.getResultCode()).isNotEqualTo(ResultCode.SUCCESS); + assertThat(ordinalsOf(ccr.getMessages())).as("the discard the failed change swallowed") + .contains(WARN_INDEX_ADD_DISCARDED_LEFTOVER_TREES.ordinal()); + assertThat(warningOrdinalsLoggedTo(errorLog)).contains(WARN_INDEX_ADD_DISCARDED_LEFTOVER_TREES.ordinal()); + for (final TreeName leftover : backend.leftoverIndexTrees) + { + assertThat(storage. read(txn -> txn.treeExists(leftover))) + .as("the tree the report says was discarded: " + leftover).isFalse(); + } + } + finally + { + ErrorLogger.getInstance().removeLogPublisher(errorLog); + backend.finalizeBackend(); + } + } + + /** The VLV road reports from a {@code finally} the same way. */ + @Test(dataProvider = "storages", timeOut = HANG_TIMEOUT_MS) + public void aVlvDiscardIsStillReportedWhenTheWriteWhichBuildsTheIndexFails(StorageKind kind) throws Exception + { + final LeftoverBackend backend = leaveTreesBehind(kind); + final ErrorLogPublisher errorLog = registerErrorLogCapture(); + try + { + final Storage storage = backend.getRootContainer().getStorage(); + final int writesBefore = backend.storage.writes(); + backend.storage.refuseWrite(2); + + final ConfigChangeResult ccr = vlvIndexAddListener(backend).applyConfigurationAdd(backend.vlvIndexCfg); + + assertThat(backend.storage.writes()).as("the refused write was the second, and the last one made") + .isEqualTo(writesBefore + 2); + assertThat(ccr.getResultCode()).isNotEqualTo(ResultCode.SUCCESS); + assertThat(ordinalsOf(ccr.getMessages())).as("the discard the failed change swallowed") + .contains(WARN_INDEX_ADD_DISCARDED_LEFTOVER_TREES.ordinal()); + assertThat(warningOrdinalsLoggedTo(errorLog)).contains(WARN_INDEX_ADD_DISCARDED_LEFTOVER_TREES.ordinal()); + assertThat(storage. read(txn -> txn.treeExists(backend.leftoverVlvTree))) + .as("the VLV tree the report says was discarded").isFalse(); + assertThat(storage. read(txn -> txn.treeExists(backend.leftoverVlvCounterTree))) + .as("the VLV counter the report says was discarded").isFalse(); + } + finally + { + ErrorLogger.getInstance().removeLogPublisher(errorLog); + backend.finalizeBackend(); + } + } + + /** + * And so does {@code AttributeIndex.applyConfigurationChange}, whose drop write is the first of + * the writes a change which adds an index type makes, and whose open write is the second. + */ + @Test(dataProvider = "storages", timeOut = HANG_TIMEOUT_MS) + public void anIndexTypeDiscardIsStillReportedWhenTheWriteWhichOpensItFails(StorageKind kind) throws Exception + { + final LeftoverBackend backend = leaveTreesBehind(kind, indexCfg(newTreeSet(IndexType.EQUALITY))); + final ErrorLogPublisher errorLog = registerErrorLogCapture(); + try + { + final EntryContainer ec = backend.getRootContainer().getEntryContainer(BASE_DN); + final Storage storage = backend.getRootContainer().getStorage(); + final AttributeIndex index = ec.getAttributeIndex(cnType); + final Map keysHeld = keysHeldBy(storage, backend.leftoverIndexTrees); + final MatchingRuleIndex equality = index.getNameToIndexes().values().iterator().next(); + final Set substringTrees = new HashSet<>(backend.leftoverIndexTrees); + substringTrees.remove(equality.getName()); + assertThat(substringTrees).hasSize(1); + final int writesBefore = backend.storage.writes(); + backend.storage.refuseWrite(2); + + final ConfigChangeResult ccr = + index.applyConfigurationChange(indexCfg(newTreeSet(IndexType.EQUALITY, IndexType.SUBSTRING))); + + assertThat(backend.storage.writes()).as("the refused write was the second, and the last one made") + .isEqualTo(writesBefore + 2); + assertThat(ccr.getResultCode()).isNotEqualTo(ResultCode.SUCCESS); + assertThat(ordinalsOf(ccr.getMessages())).as("the discard the failed change swallowed") + .contains(WARN_INDEX_ADD_DISCARDED_LEFTOVER_TREES.ordinal()); + assertThat(warningOrdinalsLoggedTo(errorLog)).contains(WARN_INDEX_ADD_DISCARDED_LEFTOVER_TREES.ordinal()); + final TreeName substringTree = substringTrees.iterator().next(); + assertThat(storage. read(txn -> txn.treeExists(substringTree))) + .as("the tree the report says was discarded").isFalse(); + assertThat(index.isIndexed(IndexType.SUBSTRING)).as("an index type the failed change declared").isFalse(); + assertThat(answers(storage, equality, keysHeld.get(equality.getName()))) + .as("the live equality tree went with the leftover").isTrue(); + } + finally + { + ErrorLogger.getInstance().removeLogPublisher(errorLog); + backend.finalizeBackend(); + } + } + + /** + * The drop write can fail at its own commit, once it has run and answered - a disk which is full + * when the engine writes its log. JE and PDB roll the drop back with it: the trees are still + * there, and so is the TRUSTED record the next open of the backend reads. The report stands all + * the same, and says more than happened. It is kept that way on purpose: the configuration entry + * is already written when the listener runs, so what comes next is not another add but that + * open, which adopts the trees - and the rebuild the report asks for is what puts it right. On + * JDBC the same failure comes after a DROP which committed on its own, and the report is the only + * trace of it; a report made to wait for the commit would be silent there, in the one case the + * drop exists for. Pinned so that moving it behind the commit is red here rather than silent + * there. + */ + @Test(dataProvider = "storages", timeOut = HANG_TIMEOUT_MS) + public void aDiscardIsStillReportedWhenTheDropWriteFailsAtItsCommit(StorageKind kind) throws Exception + { + final LeftoverBackend backend = leaveTreesBehind(kind); + try + { + final Storage storage = backend.getRootContainer().getStorage(); + final int writesBefore = backend.storage.writes(); + backend.storage.failWriteAfterItRan(1); + + final ConfigChangeResult ccr = indexAddListener(backend).applyConfigurationAdd(backend.cnIndexCfg); + + assertThat(backend.storage.writes()).as("the failed write was the first, and the last one made") + .isEqualTo(writesBefore + 1); + assertThat(ccr.getResultCode()).isNotEqualTo(ResultCode.SUCCESS); + assertThat(ordinalsOf(ccr.getMessages())).as("the report of a drop the engine rolled back") + .contains(WARN_INDEX_ADD_DISCARDED_LEFTOVER_TREES.ordinal()); + for (final TreeName leftover : backend.leftoverIndexTrees) + { + assertThat(holdsAnything(storage, leftover)).as("the drop of " + leftover + " rolled back with the write") + .isTrue(); + assertThat(persistedFlags(backend, leftover)).as("the record of " + leftover).contains(TRUSTED); + } + } + finally + { + backend.finalizeBackend(); + } + } + /** * An index added to a backend which holds no entry has nothing to index and nothing to adopt, so * it stays trusted and asks for nothing. Pins the upgrade {@code DefaultIndex.afterOpen} makes. @@ -711,6 +885,17 @@ private EnumSet persistedFlags(LeftoverBackend backend, TreeNam return backend.getRootContainer().getStorage().read(txn -> state.getIndexFlags(txn, index)); } + /** The messages a change carries, as the operator reads them: identity and arguments both. */ + private static List renderedOf(Collection messages) + { + final List rendered = new ArrayList<>(); + for (final LocalizableMessage message : messages) + { + rendered.add(message.toString()); + } + return rendered; + } + /** The messages a change carries, by identity rather than by their formatted text. */ private static Set ordinalsOf(Collection messages) { @@ -871,7 +1056,7 @@ private BackendVLVIndexCfg vlvIndexCfg() private static final class LeftoverBackend extends BackendImpl { private final StorageKind kind; - private Storage storage; + private RefusingStorage storage; /** The configuration the entry container registers its listeners with. */ private PluggableBackendCfg configuredWith; private BackendIndexCfg cnIndexCfg; @@ -889,8 +1074,152 @@ private LeftoverBackend(StorageKind kind) @Override protected Storage configureStorage(PluggableBackendCfg cfg, ServerContext serverContext) throws ConfigException { - storage = kind.newStorage(cfg, serverContext); + storage = new RefusingStorage(kind.newStorage(cfg, serverContext)); return storage; } } + + /** A failure no storage engine replays, unlike a conflict. */ + private static final class RefusedWrite extends Exception + { + private static final long serialVersionUID = 1L; + + RefusedWrite() + { + super("write refused by the test"); + } + } + + /** + * Delegates to the storage of the engine, and refuses the one write a case arms: neither engine + * can be made to fail a write on its own, and both are final. The refusal comes at one of two + * points - before the operation runs, which is what a write the engine cannot begin looks like + * to the caller, or once the operation has run and before it is committed, which is what a + * failure at commit looks like: the engine rolls the operation back, and whatever the operation + * noted on the way stands. Unarmed, it is the engine's storage. + */ + private static final class RefusingStorage implements Storage + { + private final Storage delegate; + /** Which write, counted over the life of this storage, is armed; zero for none. */ + private int armedWrite; + private boolean afterItRan; + private int writes; + + RefusingStorage(Storage delegate) + { + this.delegate = delegate; + } + + /** Refuses the {@code nth} write asked for from now on, the next one being the first, before it runs. */ + void refuseWrite(int nth) + { + armedWrite = writes + nth; + afterItRan = false; + } + + /** Fails the {@code nth} write asked for from now on once its operation has run, before it is committed. */ + void failWriteAfterItRan(int nth) + { + armedWrite = writes + nth; + afterItRan = true; + } + + /** How many write operations this storage was asked for, refused or not. */ + int writes() + { + return writes; + } + + @Override + public void write(final WriteOperation operation) throws Exception + { + writes++; + if (writes != armedWrite) + { + delegate.write(operation); + return; + } + armedWrite = 0; + if (!afterItRan) + { + throw new RefusedWrite(); + } + delegate.write(new WriteOperation() + { + @Override + public void run(WriteableTransaction txn) throws Exception + { + operation.run(txn); + throw new RefusedWrite(); + } + }); + } + + @Override + public Importer startImport() throws ConfigException + { + return delegate.startImport(); + } + + @Override + public void open(AccessMode accessMode) throws Exception + { + delegate.open(accessMode); + } + + @Override + public T read(ReadOperation readOperation) throws Exception + { + return delegate.read(readOperation); + } + + @Override + public void removeStorageFiles() + { + delegate.removeStorageFiles(); + } + + @Override + public StorageStatus getStorageStatus() + { + return delegate.getStorageStatus(); + } + + @Override + public boolean supportsBackupAndRestore() + { + return delegate.supportsBackupAndRestore(); + } + + @Override + public void createBackup(BackupConfig backupConfig) throws DirectoryException + { + delegate.createBackup(backupConfig); + } + + @Override + public void removeBackup(BackupDirectory backupDirectory, String backupID) throws DirectoryException + { + delegate.removeBackup(backupDirectory, backupID); + } + + @Override + public void restoreBackup(RestoreConfig restoreConfig) throws DirectoryException + { + delegate.restoreBackup(restoreConfig); + } + + @Override + public Set listTrees() + { + return delegate.listTrees(); + } + + @Override + public void close() + { + delegate.close(); + } + } } diff --git a/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/ReplayedConfigChangeTest.java b/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/ReplayedConfigChangeTest.java index e84e6d2a18..cc030d8519 100644 --- a/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/ReplayedConfigChangeTest.java +++ b/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/ReplayedConfigChangeTest.java @@ -634,14 +634,14 @@ public void aRaisedEntryLimitUntrustsTheIndexWhenTheTransactionIsReplayed() thro final MatchingRuleIndex cnIndex = index.getNameToIndexes().values().iterator().next(); assertThat(persistedFlags(rootContainer, ec, cnIndex.getName())).contains(TRUSTED); - // The fourth write is the one which removes the flag: the first three drop what an earlier - // index left behind for the indexes the change adds, open them, and delete the ones it - // removes, and it neither adds nor removes any. + // The third write is the one which removes the flag: the first two open the indexes the + // change adds and delete the ones it removes, and it neither adds nor removes any - so the + // write which drops what an earlier index left behind for the added ones is not made. final int writesBefore = backend.storage.writes(); - backend.storage.conflictAtCommitOnWrite(4, 1); + backend.storage.conflictAtCommitOnWrite(3, 1); final ConfigChangeResult ccr = index.applyConfigurationChange(indexCfg(newTreeSet(IndexType.EQUALITY), 8000)); - assertThat(backend.storage.writes()).as("the armed write was the last of four").isEqualTo(writesBefore + 4); + assertThat(backend.storage.writes()).as("the armed write was the last of three").isEqualTo(writesBefore + 3); assertThat(backend.storage.attempts()).isEqualTo(2); assertThat(ccr.getResultCode()).isEqualTo(ResultCode.SUCCESS); assertThat(ccr.adminActionRequired()).isTrue(); @@ -792,7 +792,7 @@ public void aRaisedEntryLimitIsReportedWhenTheWriteWhichUntrustsTheIndexGivesUp( } /** - * The same instruction survives a give-up on the second of the four writes, the one which opens + * The same instruction survives a give-up on the first of the three writes, the one which opens * the indexes the change adds - opened here with none to add: it is asked for before any of the * writes, not only before the one which untrusts the index, since the configuration entry holds * the raised limit whichever of them fails. @@ -810,11 +810,11 @@ public void aRaisedEntryLimitIsReportedWhenTheWriteWhichAddsIndexesGivesUp() thr assertThat(persistedFlags(rootContainer, ec, cnIndex.getName())).contains(TRUSTED); final int writesBefore = backend.storage.writes(); - backend.storage.failWithoutReplayOnWrite(2); + backend.storage.failWithoutReplayOnWrite(1); final ConfigChangeResult ccr = index.applyConfigurationChange(indexCfg(newTreeSet(IndexType.EQUALITY), 8000)); - assertThat(backend.storage.writes()).as("the armed write was the second of four, and the last one made") - .isEqualTo(writesBefore + 2); + assertThat(backend.storage.writes()).as("the armed write was the first of three, and the last one made") + .isEqualTo(writesBefore + 1); assertThat(ccr.getResultCode()).isEqualTo(serverErrorResultCode()); assertThat(ccr.adminActionRequired()).as("the rebuild the raised limit needs, asked for before the first write") .isTrue(); @@ -852,9 +852,8 @@ public void aLoweredEntryLimitNeedsNoRebuildAndOpensNoTransaction() throws Excep assertThat(ccr.getMessages()).isEmpty(); assertThat(cnIndex.isTrusted()).isTrue(); assertThat(cnIndex.getIndexEntryLimit()).as("the limit the change applied").isEqualTo(2000); - assertThat(backend.storage.writes()) - .as("the three writes which drop leftovers, add and remove indexes, and no fourth") - .isEqualTo(writesBefore + 3); + assertThat(backend.storage.writes()).as("the two writes which add and remove indexes, and no third") + .isEqualTo(writesBefore + 2); } finally { From ca6d9c03f737b81ca06c47f513c6774842a7b97f Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Sat, 19 Sep 2026 16:42:47 +0300 Subject: [PATCH 5/6] [#990] Pin the discard report against a drop write which fails at its commit on the VLV and index-type roads The report of what the drop write found is made from a finally on all three roads, and the comments at every site promise it stands when that write fails at its own commit; only the attribute-add road had a case arming such a failure. On the VLV and the index-type roads every case had the drop write commit, so a report moved out of the finally to behind that write was green there by construction. aVlvDiscardIsStillReportedWhenTheDropWriteFailsAtItsCommit and anIndexTypeDiscardIsStillReportedWhenTheDropWriteFailsAtItsCommit run the drop write to completion, fail it before its commit, and assert the trees are back with their content and their TRUSTED record, the change is not applied, and WARN 628 is said all the same. Measured red on both storage rows with the report moved behind the drop write on either road, while the class's other cases stay green under that mutant. --- .../IndexAddedOverLeftoverTreesTest.java | 76 +++++++++++++++++++ 1 file changed, 76 insertions(+) diff --git a/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/IndexAddedOverLeftoverTreesTest.java b/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/IndexAddedOverLeftoverTreesTest.java index 6082579715..d83d1c1130 100644 --- a/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/IndexAddedOverLeftoverTreesTest.java +++ b/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/IndexAddedOverLeftoverTreesTest.java @@ -671,6 +671,82 @@ public void aDiscardIsStillReportedWhenTheDropWriteFailsAtItsCommit(StorageKind } } + /** + * The VLV road keeps its report when the drop write fails at its commit, on the terms the case + * above gives: the tree and its counter are back, and so is the TRUSTED record. + */ + @Test(dataProvider = "storages", timeOut = HANG_TIMEOUT_MS) + public void aVlvDiscardIsStillReportedWhenTheDropWriteFailsAtItsCommit(StorageKind kind) throws Exception + { + final LeftoverBackend backend = leaveTreesBehind(kind); + try + { + final Storage storage = backend.getRootContainer().getStorage(); + final int writesBefore = backend.storage.writes(); + backend.storage.failWriteAfterItRan(1); + + final ConfigChangeResult ccr = vlvIndexAddListener(backend).applyConfigurationAdd(backend.vlvIndexCfg); + + assertThat(backend.storage.writes()).as("the failed write was the first, and the last one made") + .isEqualTo(writesBefore + 1); + assertThat(ccr.getResultCode()).isNotEqualTo(ResultCode.SUCCESS); + assertThat(ordinalsOf(ccr.getMessages())).as("the report of a drop the engine rolled back") + .contains(WARN_INDEX_ADD_DISCARDED_LEFTOVER_TREES.ordinal()); + assertThat(holdsAnything(storage, backend.leftoverVlvTree)).as("the VLV tree rolled back with the write") + .isTrue(); + assertThat(holdsAnything(storage, backend.leftoverVlvCounterTree)) + .as("the VLV counter rolled back with the write").isTrue(); + assertThat(persistedFlags(backend, backend.leftoverVlvTree)).as("the record of the VLV tree").contains(TRUSTED); + } + finally + { + backend.finalizeBackend(); + } + } + + /** + * And so does {@code AttributeIndex.applyConfigurationChange}: the tree of the index type + * declared again is back with its TRUSTED record, the change has not been applied, and the + * report stands. + */ + @Test(dataProvider = "storages", timeOut = HANG_TIMEOUT_MS) + public void anIndexTypeDiscardIsStillReportedWhenTheDropWriteFailsAtItsCommit(StorageKind kind) throws Exception + { + final LeftoverBackend backend = leaveTreesBehind(kind, indexCfg(newTreeSet(IndexType.EQUALITY))); + try + { + final EntryContainer ec = backend.getRootContainer().getEntryContainer(BASE_DN); + final Storage storage = backend.getRootContainer().getStorage(); + final AttributeIndex index = ec.getAttributeIndex(cnType); + final Map keysHeld = keysHeldBy(storage, backend.leftoverIndexTrees); + final MatchingRuleIndex equality = index.getNameToIndexes().values().iterator().next(); + final Set substringTrees = new HashSet<>(backend.leftoverIndexTrees); + substringTrees.remove(equality.getName()); + assertThat(substringTrees).hasSize(1); + final TreeName substringTree = substringTrees.iterator().next(); + final int writesBefore = backend.storage.writes(); + backend.storage.failWriteAfterItRan(1); + + final ConfigChangeResult ccr = + index.applyConfigurationChange(indexCfg(newTreeSet(IndexType.EQUALITY, IndexType.SUBSTRING))); + + assertThat(backend.storage.writes()).as("the failed write was the first, and the last one made") + .isEqualTo(writesBefore + 1); + assertThat(ccr.getResultCode()).isNotEqualTo(ResultCode.SUCCESS); + assertThat(ordinalsOf(ccr.getMessages())).as("the report of a drop the engine rolled back") + .contains(WARN_INDEX_ADD_DISCARDED_LEFTOVER_TREES.ordinal()); + assertThat(holdsAnything(storage, substringTree)).as("the substring tree rolled back with the write").isTrue(); + assertThat(persistedFlags(backend, substringTree)).as("the record of the substring tree").contains(TRUSTED); + assertThat(index.isIndexed(IndexType.SUBSTRING)).as("an index type the failed change declared").isFalse(); + assertThat(answers(storage, equality, keysHeld.get(equality.getName()))) + .as("the live equality tree went with the leftover").isTrue(); + } + finally + { + backend.finalizeBackend(); + } + } + /** * An index added to a backend which holds no entry has nothing to index and nothing to adopt, so * it stays trusted and asks for nothing. Pins the upgrade {@code DefaultIndex.afterOpen} makes. From 4006265bda9680a31d95b770f97d398b6b59ae57 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Sat, 19 Sep 2026 17:33:02 +0300 Subject: [PATCH 6/6] [#990] Say in the messages of #994 what an index created again does now, and pin it where #994 pinned the adoption #994 landed first and described the behaviour this change removes: ERR 624, 625 and 626 told the operator that an index, a VLV index or an index type declared again adopts the trees a failed deletion left behind and is trusted over their stale content, and ConfigChangeGivesUpTest.anIndexCreatedAgainAdoptsTheTreesAFailedDeletionLeftBehind pinned exactly that so that the fix would turn it red rather than leave the sentence quietly untrue. The three sentences now say what happens on each side of the line this change draws: an index created again while the backend is open discards those trees and starts empty, asking to be rebuilt, while one created while the backend is disabled still adopts them at the next open and is trusted over their content, which is the follow-up the description names. The pin is inverted into anIndexCreatedAgainDiscardsTheTreesAFailedDeletionLeftBehind - untrusted, admin action, NOTE 535 and WARN 628 in the change result - and anIndexChangeWhichGivesUpUpdatingDoesNotPublishWhatItCouldNotApply arms the fourth write rather than the third, since a change which adds an index type now opens the write which drops its leftovers before the three it always made. --- .../org/opends/messages/backend.properties | 18 +++++--- .../pluggable/ConfigChangeGivesUpTest.java | 44 +++++++++++-------- 2 files changed, 38 insertions(+), 24 deletions(-) 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 95e7919003..f4aa8439e1 100644 --- a/opendj-server-legacy/src/messages/org/opends/messages/backend.properties +++ b/opendj-server-legacy/src/messages/org/opends/messages/backend.properties @@ -1136,22 +1136,28 @@ ERR_CONFIG_INDEX_DELETE_FAILED_624=The index for attribute '%s' of backend base removed from the configuration, but the trees holding its data could not be deleted: %s. Those \ trees may still hold the data of that index, in whole or in part, and no configuration names them \ any more, so nothing will delete them and nothing will keep them up to date. An index created \ - again for that attribute with the same settings adopts those trees and is considered trusted over \ - their stale content, so it must be rebuilt before it is used + again for that attribute while this backend is open discards those trees and starts empty, asking \ + to be rebuilt as any index added to a backend holding entries does; one created while this \ + backend is disabled adopts them at the next open and is considered trusted over their stale \ + content, so it must be rebuilt before it is used ERR_CONFIG_VLV_INDEX_DELETE_FAILED_625=The VLV index '%s' of backend base DN '%s' was removed from \ the configuration, but the trees holding its data could not be deleted: %s. Those trees may still \ hold the data of that index, in whole or in part, and no configuration names them any more, so \ nothing will delete them and nothing will keep them up to date. A VLV index created again under \ - the same name adopts those trees and is considered trusted over their stale content, so it must \ - be rebuilt before it is used + the same name while this backend is open discards those trees and starts empty, asking to be \ + rebuilt as any VLV index added to a backend holding entries does; one created while this backend \ + is disabled adopts them at the next open and is considered trusted over their stale content, so \ + it must be rebuilt before it is used ERR_CONFIG_INDEX_CHANGE_FAILED_626=The configuration of the index for attribute '%s' of backend \ base DN '%s' could not be applied in full: %s. What this index holds and what its configuration \ declares may no longer agree, so this index must be rebuilt before it is used, after this backend \ has been disabled and enabled again so that every tree the stored configuration declares is named \ once more. The trees of the index types this change removed may still hold their data while no \ configuration names them any more, so nothing will delete them and nothing will keep them up to \ - date; an index type declared again for that attribute adopts those trees and is considered trusted \ - over their stale content, and must be rebuilt too + date; an index type declared again for that attribute while this backend is open discards those \ + trees and starts empty, asking to be rebuilt, while one declared again while this backend is \ + disabled adopts them at the next open and is considered trusted over their stale content, and \ + must be rebuilt too ERR_CONFIG_BACKEND_DATA_CHANGE_FAILED_627=The compression, encoding and encryption settings of \ backend base DN '%s' could not be applied in full: %s. Its entries and its indexes may no longer \ be encoded under the same settings; disabling and enabling this backend builds both from the \ diff --git a/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/ConfigChangeGivesUpTest.java b/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/ConfigChangeGivesUpTest.java index 538656333d..17ffcc7a6e 100644 --- a/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/ConfigChangeGivesUpTest.java +++ b/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/ConfigChangeGivesUpTest.java @@ -24,6 +24,8 @@ import static org.opends.messages.BackendMessages.ERR_CONFIG_INDEX_CHANGE_FAILED; import static org.opends.messages.BackendMessages.ERR_CONFIG_INDEX_DELETE_FAILED; import static org.opends.messages.BackendMessages.ERR_CONFIG_VLV_INDEX_DELETE_FAILED; +import static org.opends.messages.BackendMessages.NOTE_INDEX_ADD_REQUIRES_REBUILD; +import static org.opends.messages.BackendMessages.WARN_INDEX_ADD_DISCARDED_LEFTOVER_TREES; import static org.opends.server.util.CollectionUtils.newTreeSet; import java.util.HashSet; @@ -174,10 +176,10 @@ public void aVlvIndexDeletionWhichGivesUpAsksForAnAdministrativeAction() throws } /** - * An index change is applied by three writes, and the second one deletes the trees of the - * indexes the new configuration no longer asks for. When it gives up, those trees are still - * there, so the index has to go on naming them: an index taken out of the map while its trees - * survive is an index nothing maintains and nothing deletes. + * An index change which adds no index type is applied by three writes, and the second one + * deletes the trees of the indexes the new configuration no longer asks for. When it gives up, + * those trees are still there, so the index has to go on naming them: an index taken out of the + * map while its trees survive is an index nothing maintains and nothing deletes. */ @Test public void anIndexChangeWhichGivesUpDeletingGoesOnNamingWhatItCouldNotDelete() throws Exception @@ -210,7 +212,7 @@ public void anIndexChangeWhichGivesUpDeletingGoesOnNamingWhatItCouldNotDelete() } /** - * The third write is what untrusts the indexes which stay when the new configuration raises + * The last write is what untrusts the indexes which stay when the new configuration raises * their entry limit, and the limit itself is applied to them only once it has committed. When it * gives up, the configuration must not be published either, or the index claims settings which * were never applied to it. What it declares is read here through the index types it names, the @@ -227,9 +229,10 @@ public void anIndexChangeWhichGivesUpUpdatingDoesNotPublishWhatItCouldNotApply() final Set indexIdsBefore = new HashSet<>(index.getNameToIndexes().keySet()); // A presence index is added and the entry limit of the ones which stay is raised, so that - // all three writes have work to do and the third is the one which gives up: a lowered limit - // untrusts nothing and opens no third write. - backend.storage.giveUpOnWrite(3); + // all four writes have work to do and the fourth is the one which gives up: an added index + // type is what opens the write which drops its leftovers first (#990), and a lowered limit + // untrusts nothing and opens no last write. + backend.storage.giveUpOnWrite(4); final ConfigChangeResult ccr = index.applyConfigurationChange( indexCfg(newTreeSet(IndexType.EQUALITY, IndexType.SUBSTRING, IndexType.PRESENCE), 5000)); @@ -400,14 +403,16 @@ public void anIndexChangeWhichSucceedsPublishesEveryPartOfIt() throws Exception /** * Pins what {@code ERR_CONFIG_INDEX_DELETE_FAILED} tells the operator: an index created again for - * the same attribute adopts the trees a failed deletion left behind, and is trusted over their - * stale content without a word about rebuilding it. This is not the behaviour being asked for - * here - it is the behaviour that message describes (#990). When it is fixed, this test fails and - * the message has to be rewritten, rather than quietly becoming untrue. Its VLV counterpart - * says the same of {@code VLVIndex.afterOpen}, which nothing here holds. + * the same attribute while the backend is open discards the trees a failed deletion left behind + * rather than adopting them, starts empty and untrusted, and asks to be rebuilt - the way any + * index added to a backend holding entries does - and says that content was discarded to get + * there (#990). Before that fix the index adopted those trees and was trusted over their stale + * content without a word about rebuilding it, which is what the message used to describe. The + * adoption the message still describes, of an index created while the backend is disabled, runs + * through {@code EntryContainer.open}, which nothing here holds. */ @Test - public void anIndexCreatedAgainAdoptsTheTreesAFailedDeletionLeftBehind() throws Exception + public void anIndexCreatedAgainDiscardsTheTreesAFailedDeletionLeftBehind() throws Exception { final GivingUpBackend backend = openBackend(); try @@ -426,11 +431,14 @@ public void anIndexCreatedAgainAdoptsTheTreesAFailedDeletionLeftBehind() throws assertThat(ccr.getResultCode()).isEqualTo(ResultCode.SUCCESS); assertThat(treesOf(ec.getAttributeIndex(cnType))) - .as("the trees the index created again holds").isEqualTo(indexTrees); + .as("the trees the index created again names, under the names the deletion could not remove") + .isEqualTo(indexTrees); assertThat(ec.getAttributeIndex(cnType).isTrusted()) - .as("an index trusted over the content of the trees it adopted").isTrue(); - assertThat(ccr.getMessages()) - .as("nothing tells the operator this index has to be rebuilt").isEmpty(); + .as("an index trusted over the content of trees it did not fill").isFalse(); + assertThat(ccr.adminActionRequired()).isTrue(); + assertThat(ordinalsOf(ccr)) + .as("the rebuild this index asks for, and the content discarded to get there") + .contains(NOTE_INDEX_ADD_REQUIRES_REBUILD.ordinal(), WARN_INDEX_ADD_DISCARDED_LEFTOVER_TREES.ordinal()); } finally {