From 026d538931910375205c7d64081873109011e495 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Wed, 9 Sep 2026 16:46:02 +0300 Subject: [PATCH 1/3] [#991] Decide and report an index configuration change outside the write which is replayed AttributeIndex.applyConfigurationChange called two helpers from inside a WriteOperation, and both wrote into the caller's ConfigChangeResult and mutated in-memory state no rollback undoes. Storage.write() may replay a rolled back operation - PDBStorage and JDBCStorage both do - so the operator was told to rebuild an index once per attempt, and setIndexEntryLimit(), which answers "the limit went up" by comparing the new limit with a field it then assigns, left the replay comparing the raised limit with itself: it found nothing to rebuild and committed an index whose entry limit was raised while the TRUSTED flag the rollback had put back was still stored. DefaultIndex.afterOpen reads that flag back after a restart and the index is used as if it were complete. The decision is now taken before the write, the write only removes the flag, and the entry limit and the messages are applied once it has committed - the shape EntryContainer.applyConfigurationAdd already uses for the index it adds. VLVIndex.applyConfigurationChange had the same shape, and the only thing keeping its replay from committing a trusted vlvIndex was the administrative action the rolled back attempt had left in the shared result - the same accident that repeated the message. It is decided before the write and published after it as well, and a conflict raised by the flag removal now reaches the retry loop instead of being caught and reported. --- .../backends/pluggable/AttributeIndex.java | 98 +++++--- .../server/backends/pluggable/VLVIndex.java | 117 ++++++---- .../pluggable/ReplayedConfigChangeTest.java | 217 ++++++++++++++++-- 3 files changed, 343 insertions(+), 89 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 a63026c8a9..b0b7c77b71 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 @@ -14,6 +14,7 @@ * Copyright 2006-2010 Sun Microsystems, Inc. * Portions Copyright 2011-2016 ForgeRock AS. * Portions Copyright 2014 Manuel Gaupp + * Portions Copyright 2026 3A Systems, LLC. */ package org.opends.server.backends.pluggable; @@ -925,17 +926,32 @@ public synchronized ConfigChangeResult applyConfigurationChange(final BackendInd newIndexIdToIndexes.putAll(updatedIndexes); // Open added indexes *before* adding them to indexIdToIndexes + final List addedIndexesToRebuild = new ArrayList<>(); entryContainer.getRootContainer().getStorage().write(new WriteOperation() { @Override public void run(WriteableTransaction txn) throws Exception { + // Emptied at the start of every attempt: the storage may replay this operation, and what + // has to be reported is what the attempt which commits found, not what every attempt did. + addedIndexesToRebuild.clear(); for (MatchingRuleIndex addedIndex : addedIndexes.values()) { - createIndex(txn, addedIndex, ccr); + if (createIndex(txn, addedIndex)) + { + addedIndexesToRebuild.add(addedIndex.getName()); + } } } }); + // Reported once that write has committed, since a message an attempt which rolls back added + // to the result stays there, and the operator would be told once per attempt. + // EntryContainer.applyConfigurationAdd reports the index it adds the same way. + for (TreeName addedIndex : addedIndexesToRebuild) + { + ccr.setAdminActionRequired(true); + ccr.addMessage(NOTE_INDEX_ADD_REQUIRES_REBUILD.get(addedIndex)); + } config = newConfiguration; indexingOptions = newIndexingOptions; @@ -962,17 +978,36 @@ public void run(WriteableTransaction txn) throws Exception entryContainer.unlock(); } + // Decided before the write which applies it, and applied and reported after it has + // committed. Neither the entry limit an index holds nor its in-memory trusted flag is rolled + // back with the transaction, while the removal of the persisted TRUSTED flag is: an attempt + // which rolls back leaves the raised limit in place, so a replay of it would compare that + // limit against itself, find nothing to rebuild, and commit an index whose entry limit was + // raised and which the storage still records as trusted. + final List indexesToUntrust = new ArrayList<>(); + final List rebuildMessages = new ArrayList<>(); + planIndexUpdates(updatedIndexes.values(), newConfiguration, indexesToUntrust, rebuildMessages); + entryContainer.getRootContainer().getStorage().write(new WriteOperation() { @Override public void run(WriteableTransaction txn) throws Exception { - for (final Index updatedIndex : updatedIndexes.values()) + for (final Index updatedIndex : indexesToUntrust) { - updateIndex(updatedIndex, newConfiguration, ccr, txn); + updatedIndex.setTrusted(txn, false); } } }); + for (final Index updatedIndex : updatedIndexes.values()) + { + updatedIndex.setIndexEntryLimit(newConfiguration.getIndexEntryLimit()); + } + for (LocalizableMessage rebuildMessage : rebuildMessages) + { + ccr.setAdminActionRequired(true); + ccr.addMessage(rebuildMessage); + } } catch (Exception e) { @@ -983,36 +1018,45 @@ public void run(WriteableTransaction txn) throws Exception return ccr; } - private static void createIndex(WriteableTransaction txn, MatchingRuleIndex index, ConfigChangeResult ccr) + /** + * Opens an index this change adds, and answers whether it has to be rebuilt before it is used. + * 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) { index.open(txn, true); - if (!index.isTrusted()) - { - ccr.setAdminActionRequired(true); - ccr.addMessage(NOTE_INDEX_ADD_REQUIRES_REBUILD.get(index.getName())); - } + return !index.isTrusted(); } - private static void updateIndex(Index updatedIndex, BackendIndexCfg newConfig, ConfigChangeResult ccr, - WriteableTransaction txn) + /** + * Works out what the new configuration asks of the indexes which stay: which of them may no longer + * be trusted, and what the operator has to be told about each of them. Decided from the state the + * indexes are in before anything is applied to them, so that a write the storage replays reaches + * the same answer on every attempt. + */ + private static void planIndexUpdates(Collection updatedIndexes, BackendIndexCfg newConfig, + List indexesToUntrust, List rebuildMessages) { - // This index could still be used since a new smaller index size limit doesn't impact validity of the results. - boolean newLimitRequiresRebuild = updatedIndex.setIndexEntryLimit(newConfig.getIndexEntryLimit()); - if (newLimitRequiresRebuild) - { - ccr.setAdminActionRequired(true); - ccr.addMessage(NOTE_CONFIG_INDEX_ENTRY_LIMIT_REQUIRES_REBUILD.get(updatedIndex.getName())); - } - // This index could still be used when disabling confidentiality. - boolean newConfidentialityRequiresRebuild = updatedIndex.setConfidential(newConfig.isConfidentialityEnabled()); - if (newConfidentialityRequiresRebuild) - { - ccr.setAdminActionRequired(true); - ccr.addMessage(NOTE_CONFIG_INDEX_CONFIDENTIALITY_REQUIRES_REBUILD.get(updatedIndex.getName())); - } - if (newLimitRequiresRebuild || newConfidentialityRequiresRebuild) + for (Index updatedIndex : updatedIndexes) { - updatedIndex.setTrusted(txn, false); + // This index could still be used since a new smaller index size limit doesn't impact validity of the results. + boolean newLimitRequiresRebuild = updatedIndex.getIndexEntryLimit() < newConfig.getIndexEntryLimit(); + if (newLimitRequiresRebuild) + { + rebuildMessages.add(NOTE_CONFIG_INDEX_ENTRY_LIMIT_REQUIRES_REBUILD.get(updatedIndex.getName())); + } + // This index could still be used when disabling confidentiality. Asked rather than told: for an + // index this only compares the configuration with the parameters its crypto suite holds. + boolean newConfidentialityRequiresRebuild = updatedIndex.setConfidential(newConfig.isConfidentialityEnabled()); + if (newConfidentialityRequiresRebuild) + { + rebuildMessages.add(NOTE_CONFIG_INDEX_CONFIDENTIALITY_REQUIRES_REBUILD.get(updatedIndex.getName())); + } + if (newLimitRequiresRebuild || newConfidentialityRequiresRebuild) + { + indexesToUntrust.add(updatedIndex); + } } } 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 ca84641b19..262cb67747 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 @@ -65,7 +65,6 @@ import org.opends.server.controls.ServerSideSortRequestControl; import org.opends.server.controls.VLVRequestControl; import org.opends.server.controls.VLVResponseControl; -import org.opends.server.core.DirectoryServer; import org.opends.server.core.SearchOperation; import org.opends.server.protocols.ldap.LDAPResultCode; import org.opends.server.types.Attribute; @@ -73,7 +72,6 @@ import org.opends.server.types.Entry; import org.opends.server.types.Modification; import org.opends.server.types.SearchFilter; -import org.opends.server.util.StaticUtils; /** * This class represents a VLV index. @@ -229,71 +227,96 @@ private static SearchFilter parseSearchFilter(final BackendVLVIndexCfg cfg, Stri @Override public synchronized ConfigChangeResult applyConfigurationChange(final BackendVLVIndexCfg cfg) { - try - { - final ConfigChangeResult ccr = new ConfigChangeResult(); - storage.write(new WriteOperation() - { - @Override - public void run(final WriteableTransaction txn) throws Exception - { - applyConfigurationChange0(txn, cfg, ccr); - } - }); - return ccr; - } - catch (final Exception e) + final ConfigChangeResult ccr = new ConfigChangeResult(); + /* + * What this change asks for is worked out here, before the write which applies it, and what it + * changes is published after that write has committed. Asked and answered from within a + * WriteOperation the storage may replay, every question below is asked of the configuration + * this vlvIndex holds and answered into the result of the change, and neither is rolled back + * with the transaction: an attempt which rolls back leaves this vlvIndex already holding the + * new definition, so the replay of it finds nothing changed, and it leaves the result already + * asking for the rebuild, which is the only thing that keeps the replay removing the TRUSTED + * flag the rollback put back. The operator is told to rebuild the index once per attempt, and + * what stops the storage from committing a vlvIndex it still records as trusted - answering a + * sorted search after a restart out of a tree built for the definition it no longer has - is + * that repetition. See OpenDJ issue #991, which reports this of AttributeIndex, where the + * index itself holds the answer and the replay does commit a stale index as trusted. + */ + final boolean baseDNChanged = !config.getBaseDN().equals(cfg.getBaseDN()); + if (baseDNChanged) { - throw new StorageRuntimeException(e); + ccr.setAdminActionRequired(true); } - } - - private synchronized void applyConfigurationChange0( - final WriteableTransaction txn, final BackendVLVIndexCfg cfg, final ConfigChangeResult ccr) - { - // Update base DN only if changed - if (!config.getBaseDN().equals(cfg.getBaseDN())) + final boolean scopeChanged = !config.getScope().equals(cfg.getScope()); + if (scopeChanged) { - this.baseDN = cfg.getBaseDN(); ccr.setAdminActionRequired(true); } - - // Update scope only if changed - if (!config.getScope().equals(cfg.getScope())) + // parseSearchFilter() asks for the administrative action itself, and only once it has parsed. + final boolean filterChanged = !config.getFilter().equals(cfg.getFilter()); + final SearchFilter newFilter = filterChanged ? parseSearchFilter(cfg, getName().toString(), ccr) : filter; + final boolean sortOrderChanged = !config.getSortOrder().equals(cfg.getSortOrder()); + final List newSortKeys; + if (sortOrderChanged) { - this.scope = convertScope(cfg.getScope()); + newSortKeys = parseSortKeys(cfg.getSortOrder(), ccr); ccr.setAdminActionRequired(true); } - - // Update the filter only if changed - if (!config.getFilter().equals(cfg.getFilter())) + else { - this.filter = parseSearchFilter(cfg, getName().toString(), ccr); + newSortKeys = sortKeys; } - // Update the sort order only if changed - if (!config.getSortOrder().equals(cfg.getSortOrder())) + final boolean requiresRebuild = ccr.adminActionRequired(); + if (requiresRebuild) { - this.sortKeys = parseSortKeys(cfg.getSortOrder(), ccr); - ccr.setAdminActionRequired(true); - } - - if (ccr.adminActionRequired()) - { - trusted = false; - ccr.addMessage(NOTE_INDEX_ADD_REQUIRES_REBUILD.get(getName())); + // The only part of this change which is written down. A change asking for nothing this + // vlvIndex has to be rebuilt for opens no transaction, rather than one a bounded storage + // could give up on with nothing to give up. A failure to remove the flag is raised rather + // than reported: caught inside the operation, as it used to be, it is a conflict swallowed + // where the storage was waiting to be told to replay, and the attempt commits having done + // nothing. Reported the way every other storage failure of this method already is. try { - state.removeFlagsFromIndex(txn, getName(), IndexFlag.TRUSTED); + storage.write(new WriteOperation() + { + @Override + public void run(final WriteableTransaction txn) throws Exception + { + setTrusted(txn, false); + } + }); } - catch (final StorageRuntimeException de) + catch (final Exception e) { - ccr.addMessage(LocalizableMessage.raw(StaticUtils.stackTraceToSingleLineString(de))); - ccr.setResultCodeIfSuccess(DirectoryServer.getCoreConfigManager().getServerErrorResultCode()); + throw new StorageRuntimeException(e); } } + if (baseDNChanged) + { + this.baseDN = cfg.getBaseDN(); + } + if (scopeChanged) + { + this.scope = convertScope(cfg.getScope()); + } + if (filterChanged) + { + this.filter = newFilter; + } + if (sortOrderChanged) + { + this.sortKeys = newSortKeys; + } + if (requiresRebuild) + { + // Reported here rather than from within the write, since a message an attempt which rolls + // back added to the result stays there, and the operator would be told once per attempt. + ccr.addMessage(NOTE_INDEX_ADD_REQUIRES_REBUILD.get(getName())); + } this.config = cfg; + return ccr; } private List parseSortKeys(final String sortOrder, ConfigChangeResult ccr) 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 19c1aaae26..ebc2b2e2dc 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 @@ -19,6 +19,8 @@ import static org.opends.messages.BackendMessages.ERR_BACKEND_BASEDN_NO_LONGER_HELD; import static org.opends.messages.BackendMessages.ERR_BACKEND_CANNOT_LIST_TREES_AFTER_BASEDN_CHANGE; import static org.opends.messages.BackendMessages.ERR_BACKEND_CANNOT_REGISTER_BASEDN; +import static org.opends.messages.BackendMessages.NOTE_CONFIG_INDEX_ENTRY_LIMIT_REQUIRES_REBUILD; +import static org.opends.messages.BackendMessages.NOTE_INDEX_ADD_REQUIRES_REBUILD; import static org.forgerock.opendj.config.ConfigurationMock.mockCfg; import static org.mockito.Mockito.any; import static org.mockito.Mockito.mock; @@ -46,11 +48,14 @@ 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.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.State.IndexFlag; import org.opends.server.backends.pluggable.spi.AccessMode; import org.opends.server.backends.pluggable.spi.Cursor; @@ -63,6 +68,7 @@ import org.opends.server.backends.pluggable.spi.UpdateFunction; 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.BackupConfig; import org.opends.server.types.BackupDirectory; @@ -95,6 +101,8 @@ public class ReplayedConfigChangeTest extends DirectoryServerTestCase private static final DN ADDED = DN.valueOf("dc=b907c,dc=com"); /** Hierarchically related to {@link #KEPT}, which one backend is not allowed to serve as well. */ private static final DN UNREGISTRABLE = DN.valueOf("dc=b907d,dc=b907a,dc=com"); + /** Held in lower case, which is how an entry container keys the vlvIndexes it holds. */ + private static final String VLV_INDEX_NAME = "b907vlv"; private ServerContext serverContext; private AttributeType cnType; @@ -545,6 +553,16 @@ public void aChangeWhichLeavesTheBaseDNsAloneOpensNoTransaction() throws Excepti } } + private static Set treesOf(EntryContainer ec) + { + final Set names = new HashSet<>(); + for (Tree tree : ec.listTrees()) + { + names.add(tree.getName()); + } + return names; + } + /** The messages a change result carries, by identity rather than by their formatted text. */ private static Set ordinalsOf(ConfigChangeResult ccr) { @@ -556,14 +574,118 @@ private static Set ordinalsOf(ConfigChangeResult ccr) return ordinals; } - private static Set treesOf(EntryContainer ec) + /** + * An index a change adds is opened untrusted while the backend holds entries, and the operator is + * told to rebuild it. That message belongs to the attempt which commits: added to the result from + * inside the operation, a replay repeats it once per attempt. + */ + @Test + public void anIndexAddedByAChangeIsReportedOnceWhenTheTransactionIsReplayed() throws Exception { - final Set names = new HashSet<>(); - for (Tree tree : ec.listTrees()) + final ReplayingBackend backend = openBackend(newTreeSet(KEPT)); + try { - names.add(tree.getName()); + 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); + final ConfigChangeResult ccr = + index.applyConfigurationChange(indexCfg(newTreeSet(IndexType.EQUALITY, IndexType.PRESENCE), 4000)); + + assertThat(backend.storage.attempts()).isEqualTo(2); + assertThat(ccr.getResultCode()).isEqualTo(ResultCode.SUCCESS); + assertThat(ccr.adminActionRequired()).isTrue(); + assertThat(ccr.getMessages()).as("the rebuild the added index needs, asked for once").hasSize(1); + assertThat(ordinalsOf(ccr)).containsOnly(NOTE_INDEX_ADD_REQUIRES_REBUILD.ordinal()); + assertThat(index.isIndexed(IndexType.PRESENCE)).as("the index type the change added").isTrue(); + } + finally + { + backend.finalizeBackend(); + } + } + + /** + * Raising the entry limit of an index leaves it holding the keys it gave up under the lower limit, + * so it has to be rebuilt and may not be trusted until it is. The limit an index holds is not + * rolled back with the transaction while the removal of its persisted TRUSTED flag is, so an + * attempt which rolls back must not be what decides that the limit went up. + */ + @Test + public void aRaisedEntryLimitUntrustsTheIndexWhenTheTransactionIsReplayed() throws Exception + { + final ReplayingBackend backend = openBackend(newTreeSet(KEPT)); + try + { + final RootContainer rootContainer = backend.getRootContainer(); + final EntryContainer ec = rootContainer.getEntryContainer(KEPT); + final AttributeIndex index = ec.getAttributeIndex(cnType); + final MatchingRuleIndex cnIndex = index.getNameToIndexes().values().iterator().next(); + assertThat(persistedFlags(rootContainer, ec, cnIndex.getName())).contains(TRUSTED); + + // The third write is the one which applies the entry limit: the first two open the indexes + // the change adds and delete the ones it removes, and it neither adds nor removes any. + backend.storage.conflictAtCommitOnWrite(3, 1); + final ConfigChangeResult ccr = index.applyConfigurationChange(indexCfg(newTreeSet(IndexType.EQUALITY), 8000)); + + assertThat(backend.storage.attempts()).isEqualTo(2); + assertThat(ccr.getResultCode()).isEqualTo(ResultCode.SUCCESS); + assertThat(ccr.adminActionRequired()).isTrue(); + assertThat(ccr.getMessages()).as("the rebuild the raised limit needs, asked for once").hasSize(1); + assertThat(ordinalsOf(ccr)).containsOnly(NOTE_CONFIG_INDEX_ENTRY_LIMIT_REQUIRES_REBUILD.ordinal()); + assertThat(cnIndex.getIndexEntryLimit()).as("the limit the change applied").isEqualTo(8000); + assertThat(cnIndex.isTrusted()).isFalse(); + assertThat(persistedFlags(rootContainer, ec, cnIndex.getName())) + .as("the flag the attempt which committed had to remove").doesNotContain(TRUSTED); + } + finally + { + backend.finalizeBackend(); + } + } + + /** + * A vlvIndex whose sort order changes holds a tree sorted under the order it no longer has, so it + * may not be trusted until it is rebuilt. Every question it asks is asked of the configuration it + * holds, and no rollback takes that configuration back, so the replay of an attempt which rolled + * back finds nothing changed: the rebuild it goes on asking for is the administrative action the + * rolled back attempt left in the result, told to the operator once per attempt. + */ + @Test + public void aChangedSortOrderUntrustsTheVlvIndexWhenTheTransactionIsReplayed() throws Exception + { + final ReplayingBackend backend = openBackendWithVlvIndex(); + try + { + final RootContainer rootContainer = backend.getRootContainer(); + final EntryContainer ec = rootContainer.getEntryContainer(KEPT); + final VLVIndex vlvIndex = ec.getVLVIndex(VLV_INDEX_NAME); + assertThat(persistedFlags(rootContainer, ec, vlvIndex.getName())).contains(TRUSTED); + + backend.storage.conflictAtCommit(1); + final ConfigChangeResult ccr = vlvIndex.applyConfigurationChange(vlvIndexCfg("+sn")); + + assertThat(backend.storage.attempts()).isEqualTo(2); + assertThat(ccr.getResultCode()).isEqualTo(ResultCode.SUCCESS); + assertThat(ccr.adminActionRequired()).isTrue(); + assertThat(ccr.getMessages()).as("the rebuild the new sort order needs, asked for once").hasSize(1); + assertThat(ordinalsOf(ccr)).containsOnly(NOTE_INDEX_ADD_REQUIRES_REBUILD.ordinal()); + assertThat(vlvIndex.isTrusted()).isFalse(); + assertThat(persistedFlags(rootContainer, ec, vlvIndex.getName())) + .as("the flag the attempt which committed had to remove").doesNotContain(TRUSTED); + + // The definition the change applied is the one this vlvIndex holds from now on, so asking for + // it a second time asks for nothing. + final ConfigChangeResult again = vlvIndex.applyConfigurationChange(vlvIndexCfg("+sn")); + assertThat(again.getResultCode()).isEqualTo(ResultCode.SUCCESS); + assertThat(again.adminActionRequired()).as("a change which changes nothing").isFalse(); + assertThat(again.getMessages()).isEmpty(); + } + finally + { + backend.finalizeBackend(); } - return names; } /** Reads back the flags an index was given when it was opened, as they are stored. */ @@ -575,10 +697,21 @@ private static EnumSet persistedFlags(RootContainer rootContainer, En } private ReplayingBackend openBackend(SortedSet baseDNs) throws Exception + { + return openBackend(baseDNs, false); + } + + /** The vlvIndex is opened only where a test is about one, since every base DN gets a copy of it. */ + private ReplayingBackend openBackendWithVlvIndex() throws Exception + { + return openBackend(newTreeSet(KEPT), true); + } + + private ReplayingBackend openBackend(SortedSet baseDNs, boolean withVlvIndex) throws Exception { final ReplayingBackend backend = new ReplayingBackend(); backend.setBackendID(BACKEND_ID); - backend.configuredWith = backendCfg(baseDNs); + backend.configuredWith = backendCfg(baseDNs, withVlvIndex); backend.configureBackend(backend.configuredWith, serverContext); // Start from a pristine on-disk state so that a previous run cannot mask the defect. backend.storage.removeStorageFiles(); @@ -617,6 +750,11 @@ private ReplayingBackend openBackend(SortedSet baseDNs) throws Exception } private PDBBackendCfg backendCfg(SortedSet baseDNs) throws ConfigException + { + return backendCfg(baseDNs, false); + } + + private PDBBackendCfg backendCfg(SortedSet baseDNs, boolean withVlvIndex) throws ConfigException { final PDBBackendCfg cfg = mockCfg(PDBBackendCfg.class); when(cfg.dn()).thenReturn(DN.valueOf("ds-cfg-backend-id=" + BACKEND_ID + ",cn=Backends,cn=config")); @@ -627,17 +765,52 @@ private PDBBackendCfg backendCfg(SortedSet baseDNs) throws ConfigException when(cfg.getDBCachePercent()).thenReturn(20); when(cfg.getBaseDN()).thenReturn(baseDNs); when(cfg.listBackendIndexes()).thenReturn(new String[] { "cn" }); - when(cfg.listBackendVLVIndexes()).thenReturn(new String[0]); - - final BackendIndexCfg indexCfg = mock(BackendIndexCfg.class); - when(indexCfg.getIndexType()).thenReturn(newTreeSet(IndexType.EQUALITY)); - when(indexCfg.getAttribute()).thenReturn(cnType); - when(indexCfg.getIndexEntryLimit()).thenReturn(4000); - when(indexCfg.getSubstringLength()).thenReturn(6); - when(cfg.getBackendIndex("cn")).thenReturn(indexCfg); + // Built before it is handed over: stubbing a mock from inside a when() of another mock leaves + // that when() unfinished, and Mockito fails the next test to touch either of them. + final BackendIndexCfg cnIndexCfg = indexCfg(newTreeSet(IndexType.EQUALITY), 4000); + when(cfg.getBackendIndex("cn")).thenReturn(cnIndexCfg); + if (withVlvIndex) + { + final BackendVLVIndexCfg vlvCfg = vlvIndexCfg("+cn"); + when(cfg.listBackendVLVIndexes()).thenReturn(new String[] { VLV_INDEX_NAME }); + when(cfg.getBackendVLVIndex(VLV_INDEX_NAME)).thenReturn(vlvCfg); + } + else + { + when(cfg.listBackendVLVIndexes()).thenReturn(new String[0]); + } + return cfg; + } + + private BackendIndexCfg indexCfg(SortedSet indexTypes, int indexEntryLimit) + { + final BackendIndexCfg cfg = mock(BackendIndexCfg.class); + when(cfg.getIndexType()).thenReturn(indexTypes); + when(cfg.getAttribute()).thenReturn(cnType); + when(cfg.getIndexEntryLimit()).thenReturn(indexEntryLimit); + when(cfg.getSubstringLength()).thenReturn(6); + return cfg; + } + + private BackendVLVIndexCfg vlvIndexCfg(String sortOrder) + { + final BackendVLVIndexCfg cfg = mock(BackendVLVIndexCfg.class); + when(cfg.getName()).thenReturn(VLV_INDEX_NAME); + when(cfg.getBaseDN()).thenReturn(KEPT); + when(cfg.getScope()).thenReturn(Scope.WHOLE_SUBTREE); + when(cfg.getFilter()).thenReturn("(objectClass=*)"); + when(cfg.getSortOrder()).thenReturn(sortOrder); return cfg; } + /** An index of an empty backend is trusted when it is opened, whatever its tree holds. */ + private static void addBaseEntry(ReplayingBackend backend, DN baseDN, String domainComponent) throws Exception + { + backend.addEntry( + TestCaseUtils.makeEntry("dn: " + baseDN, "objectClass: top", "objectClass: domain", "dc: " + domainComponent), + mock(AddOperation.class)); + } + /** A backend whose storage makes the next write operation conflict, and so be replayed. */ private static final class ReplayingBackend extends BackendImpl { @@ -723,6 +896,8 @@ private enum ConflictPoint private final Storage delegate; private Runnable onListTrees; private ConflictPoint conflictPoint; + /** Which write, counted over the life of this storage, is armed; zero for the next one. */ + private int armedWrite; private int conflictsLeft; private int attempts; private int writes; @@ -742,6 +917,16 @@ void conflictAtCommit(int conflicts) arm(ConflictPoint.COMMIT, conflicts); } + /** + * Conflicts at commit time on the {@code nth} write asked for from now on, the next one being + * the first: a configuration change which makes several writes has to arm the one it is about. + */ + void conflictAtCommitOnWrite(int nth, int conflicts) + { + arm(ConflictPoint.COMMIT, conflicts); + armedWrite = writes + nth; + } + void failWithoutReplay() { arm(ConflictPoint.NO_REPLAY, 1); @@ -756,6 +941,7 @@ private void arm(ConflictPoint where, int conflicts) { conflictPoint = where; conflictsLeft = conflicts; + armedWrite = 0; attempts = 0; } @@ -776,12 +962,13 @@ public void write(final WriteOperation writeOperation) throws Exception { writes++; final ConflictPoint armed = conflictPoint; - if (armed == null) + if (armed == null || (armedWrite != 0 && writes != armedWrite)) { delegate.write(writeOperation); return; } conflictPoint = null; + armedWrite = 0; if (armed == ConflictPoint.NO_REPLAY_AFTER_COMMIT) { // Committed, then reported as a failure: the operation's work outlives the failure, as it From b7094cd5de7b745a255762334ab702390f83577b Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Wed, 16 Sep 2026 17:52:46 +0300 Subject: [PATCH 2/3] [#991] Report the rebuild before the writes, and report a give-up on the vlvIndex flag removal The first round reported the rebuild a raised entry limit needs only once the write which untrusts the index had committed. A write the storage gives up on - the bound of its retry loop spent, or a failure it does not replay at all - dropped that instruction together with the limit it could not apply, while config.ldif already held the raised limit: ConfigurationHandler .replaceEntry writes it before the listeners run and nothing rolls it back. The next open of the index applied the limit to a tree whose keys were given up under the lower one, with TRUSTED still stored, and the operator had been told nothing but a stack trace: the #991 end state, on a road where the base delivered the message by the same accident which repeated it. What the indexes which stay are asked for is now decided and reported before any of the three writes, since the instruction holds whichever way they go, and only the limit itself waits for the untrust write to commit. That write is made only when there is something to untrust, as VLVIndex already did: a lowered limit opens no transaction a bounded storage could give up on. VLVIndex reported a give-up by throwing, which discarded the result carrying the rebuild it had asked for - ConfigurationHandler catches nothing a listener throws. It now reports it the way AttributeIndex does, with the message asked for before the write as well. Pinned: both give-up roads; a lowered limit, which needs no rebuild and opens no transaction; the write ordinals the replay cases arm; and the conflict the flag removal itself raises, in the form the PDB transaction raises it - a StorageRuntimeException wrapping the RollbackException, which the inner catch the first round removed used to swallow, and which the bare form the harness raised never reached. --- .../backends/pluggable/AttributeIndex.java | 56 +++-- .../server/backends/pluggable/VLVIndex.java | 29 ++- .../pluggable/ReplayedConfigChangeTest.java | 201 +++++++++++++++++- 3 files changed, 247 insertions(+), 39 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 b0b7c77b71..86ddbea13e 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 @@ -925,6 +925,26 @@ public synchronized ConfigChangeResult applyConfigurationChange(final BackendInd // indexIdToIndexes newIndexIdToIndexes.putAll(updatedIndexes); + // What the new configuration asks of the indexes which stay is decided here, before any of + // the three writes below, and reported here as well. Decided before the write which applies + // it: neither the entry limit an index holds nor its in-memory trusted flag is rolled back + // with the transaction, while the removal of the persisted TRUSTED flag is, so an attempt + // which rolls back would leave the raised limit in place, and a replay of it would compare + // that limit against itself, find nothing to rebuild, and commit an index whose entry limit + // was raised and which the storage still records as trusted. Reported before the writes + // rather than once they have committed, because the instruction holds whichever way they go: + // the configuration entry already holds the raised limit when this listener runs, and the + // next open of the index applies it to a tree whose keys were given up under the lower one. + // Only the limit itself waits for the write which untrusts the index to commit. + final List indexesToUntrust = new ArrayList<>(); + final List rebuildMessages = new ArrayList<>(); + planIndexUpdates(updatedIndexes.values(), newConfiguration, indexesToUntrust, rebuildMessages); + for (LocalizableMessage rebuildMessage : rebuildMessages) + { + ccr.setAdminActionRequired(true); + ccr.addMessage(rebuildMessage); + } + // Open added indexes *before* adding them to indexIdToIndexes final List addedIndexesToRebuild = new ArrayList<>(); entryContainer.getRootContainer().getStorage().write(new WriteOperation() @@ -978,36 +998,28 @@ public void run(WriteableTransaction txn) throws Exception entryContainer.unlock(); } - // Decided before the write which applies it, and applied and reported after it has - // committed. Neither the entry limit an index holds nor its in-memory trusted flag is rolled - // back with the transaction, while the removal of the persisted TRUSTED flag is: an attempt - // which rolls back leaves the raised limit in place, so a replay of it would compare that - // limit against itself, find nothing to rebuild, and commit an index whose entry limit was - // raised and which the storage still records as trusted. - final List indexesToUntrust = new ArrayList<>(); - final List rebuildMessages = new ArrayList<>(); - planIndexUpdates(updatedIndexes.values(), newConfiguration, indexesToUntrust, rebuildMessages); - - entryContainer.getRootContainer().getStorage().write(new WriteOperation() + // The only part of what the indexes which stay are asked for that is written down. A change + // which untrusts none of them - a lowered limit - opens no transaction, rather than one a + // bounded storage could give up on with nothing to give up; VLVIndex guards its write the + // same way. + if (!indexesToUntrust.isEmpty()) { - @Override - public void run(WriteableTransaction txn) throws Exception + entryContainer.getRootContainer().getStorage().write(new WriteOperation() { - for (final Index updatedIndex : indexesToUntrust) + @Override + public void run(WriteableTransaction txn) throws Exception { - updatedIndex.setTrusted(txn, false); + for (final Index updatedIndex : indexesToUntrust) + { + updatedIndex.setTrusted(txn, false); + } } - } - }); + }); + } for (final Index updatedIndex : updatedIndexes.values()) { updatedIndex.setIndexEntryLimit(newConfiguration.getIndexEntryLimit()); } - for (LocalizableMessage rebuildMessage : rebuildMessages) - { - ccr.setAdminActionRequired(true); - ccr.addMessage(rebuildMessage); - } } catch (Exception e) { 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 262cb67747..ba19cb18ad 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 @@ -270,12 +270,23 @@ public synchronized ConfigChangeResult applyConfigurationChange(final BackendVLV final boolean requiresRebuild = ccr.adminActionRequired(); if (requiresRebuild) { + // Reported outside the write rather than from within it, since a message an attempt which + // rolls back added to the result stays there and the operator would be told once per + // attempt; and before the write rather than once it has committed, because the instruction + // holds whichever way the write goes: the configuration entry already holds the new + // definition when this listener runs, and the next open of this vlvIndex applies it to a + // tree built for the definition it no longer has. + ccr.addMessage(NOTE_INDEX_ADD_REQUIRES_REBUILD.get(getName())); + // The only part of this change which is written down. A change asking for nothing this // vlvIndex has to be rebuilt for opens no transaction, rather than one a bounded storage - // could give up on with nothing to give up. A failure to remove the flag is raised rather - // than reported: caught inside the operation, as it used to be, it is a conflict swallowed - // where the storage was waiting to be told to replay, and the attempt commits having done - // nothing. Reported the way every other storage failure of this method already is. + // could give up on with nothing to give up. A conflict raised by the flag removal is left to + // the storage, whose retry loop replays the operation: caught inside the operation, as it + // used to be, it was swallowed where the storage was waiting to be told to replay, and the + // attempt committed having done nothing. What the storage gives up on is reported the way + // AttributeIndex reports it, with the result built so far - the rebuild asked for above + // holds on that road too - rather than thrown past ConfigurationHandler, which catches + // nothing a listener throws and would discard that result whole. try { storage.write(new WriteOperation() @@ -289,7 +300,9 @@ public void run(final WriteableTransaction txn) throws Exception } catch (final Exception e) { - throw new StorageRuntimeException(e); + ccr.setResultCode(getCoreConfigManager().getServerErrorResultCode()); + ccr.addMessage(LocalizableMessage.raw(stackTraceToSingleLineString(e))); + return ccr; } } @@ -309,12 +322,6 @@ public void run(final WriteableTransaction txn) throws Exception { this.sortKeys = newSortKeys; } - if (requiresRebuild) - { - // Reported here rather than from within the write, since a message an attempt which rolls - // back added to the result stays there, and the operator would be told once per attempt. - ccr.addMessage(NOTE_INDEX_ADD_REQUIRES_REBUILD.get(getName())); - } this.config = cfg; return ccr; } 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 ebc2b2e2dc..e76671930e 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 @@ -624,11 +624,13 @@ 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 applies the entry limit: the first two open the indexes - // the change adds 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. + final int writesBefore = backend.storage.writes(); 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 three").isEqualTo(writesBefore + 3); assertThat(backend.storage.attempts()).isEqualTo(2); assertThat(ccr.getResultCode()).isEqualTo(ResultCode.SUCCESS); assertThat(ccr.adminActionRequired()).isTrue(); @@ -676,11 +678,154 @@ public void aChangedSortOrderUntrustsTheVlvIndexWhenTheTransactionIsReplayed() t .as("the flag the attempt which committed had to remove").doesNotContain(TRUSTED); // The definition the change applied is the one this vlvIndex holds from now on, so asking for - // it a second time asks for nothing. + // it a second time asks for nothing, and opens no transaction to commit nothing. + final int writesBefore = backend.storage.writes(); final ConfigChangeResult again = vlvIndex.applyConfigurationChange(vlvIndexCfg("+sn")); assertThat(again.getResultCode()).isEqualTo(ResultCode.SUCCESS); assertThat(again.adminActionRequired()).as("a change which changes nothing").isFalse(); assertThat(again.getMessages()).isEmpty(); + assertThat(backend.storage.writes()).as("a change which changes nothing opens no transaction") + .isEqualTo(writesBefore); + } + finally + { + backend.finalizeBackend(); + } + } + + /** + * A conflict the flag removal itself raises is reported to the operation by the transaction, as + * a {@link StorageRuntimeException} wrapping it, and belongs to the storage's retry loop. Caught + * inside the operation and turned into a message, it was a conflict swallowed where the storage + * was waiting to be told to replay: the attempt committed having removed nothing, and the change + * reported a failure against a vlvIndex the storage still recorded as trusted. + */ + @Test + public void aConflictRaisedByTheRemovalOfTheVlvIndexFlagIsReplayed() throws Exception + { + final ReplayingBackend backend = openBackendWithVlvIndex(); + try + { + final RootContainer rootContainer = backend.getRootContainer(); + final EntryContainer ec = rootContainer.getEntryContainer(KEPT); + final VLVIndex vlvIndex = ec.getVLVIndex(VLV_INDEX_NAME); + assertThat(persistedFlags(rootContainer, ec, vlvIndex.getName())).contains(TRUSTED); + + backend.storage.conflictAsTheTransactionReportsIt(1); + final ConfigChangeResult ccr = vlvIndex.applyConfigurationChange(vlvIndexCfg("+sn")); + + assertThat(backend.storage.attempts()).as("replayed by the storage, not answered from inside").isEqualTo(2); + assertThat(ccr.getResultCode()).isEqualTo(ResultCode.SUCCESS); + assertThat(ccr.adminActionRequired()).isTrue(); + assertThat(ccr.getMessages()).as("the rebuild the new sort order needs, asked for once").hasSize(1); + assertThat(ordinalsOf(ccr)).containsOnly(NOTE_INDEX_ADD_REQUIRES_REBUILD.ordinal()); + assertThat(vlvIndex.isTrusted()).isFalse(); + assertThat(persistedFlags(rootContainer, ec, vlvIndex.getName())) + .as("the flag the replayed attempt removed").doesNotContain(TRUSTED); + } + finally + { + backend.finalizeBackend(); + } + } + + /** + * A raised entry limit has to be rebuilt for whether or not the write which untrusts the index is + * applied: the configuration entry holds the raised limit before the listener runs, and the next + * open of the index applies it to a tree whose keys were given up under the lower one. So the + * instruction is given before that write, and a write the storage gives up on - the last attempt + * of its retry loop, or a failure it does not replay at all - leaves it in the result, next to + * the failure, rather than dropping it together with the limit it could not apply. + */ + @Test + public void aRaisedEntryLimitIsReportedWhenTheWriteWhichUntrustsTheIndexGivesUp() throws Exception + { + final ReplayingBackend backend = openBackend(newTreeSet(KEPT)); + try + { + final RootContainer rootContainer = backend.getRootContainer(); + final EntryContainer ec = rootContainer.getEntryContainer(KEPT); + final AttributeIndex index = ec.getAttributeIndex(cnType); + final MatchingRuleIndex cnIndex = index.getNameToIndexes().values().iterator().next(); + assertThat(persistedFlags(rootContainer, ec, cnIndex.getName())).contains(TRUSTED); + + backend.storage.failWithoutReplayOnWrite(3); + final ConfigChangeResult ccr = index.applyConfigurationChange(indexCfg(newTreeSet(IndexType.EQUALITY), 8000)); + + assertThat(ccr.getResultCode()).isNotEqualTo(ResultCode.SUCCESS); + assertThat(ccr.adminActionRequired()).as("the rebuild the raised limit needs, on the road which failed").isTrue(); + assertThat(ordinalsOf(ccr)).contains(NOTE_CONFIG_INDEX_ENTRY_LIMIT_REQUIRES_REBUILD.ordinal()); + assertThat(cnIndex.getIndexEntryLimit()).as("the limit the failed write did not apply").isEqualTo(4000); + assertThat(persistedFlags(rootContainer, ec, cnIndex.getName())) + .as("what the restart will read").contains(TRUSTED); + } + finally + { + backend.finalizeBackend(); + } + } + + /** + * A lowered entry limit leaves every key the index holds valid, so it asks for no rebuild, and it + * untrusts nothing, so it opens no transaction: nothing for a bounded storage to give up on. + */ + @Test + public void aLoweredEntryLimitNeedsNoRebuildAndOpensNoTransaction() throws Exception + { + final ReplayingBackend backend = openBackend(newTreeSet(KEPT)); + try + { + final AttributeIndex index = backend.getRootContainer().getEntryContainer(KEPT).getAttributeIndex(cnType); + final MatchingRuleIndex cnIndex = index.getNameToIndexes().values().iterator().next(); + + final int writesBefore = backend.storage.writes(); + final ConfigChangeResult ccr = index.applyConfigurationChange(indexCfg(newTreeSet(IndexType.EQUALITY), 2000)); + + assertThat(ccr.getResultCode()).isEqualTo(ResultCode.SUCCESS); + assertThat(ccr.adminActionRequired()).as("a lowered limit needs no rebuild").isFalse(); + 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); + } + finally + { + backend.finalizeBackend(); + } + } + + /** + * The same road for a vlvIndex: the write which untrusts it is the only one the change makes, and + * a failure of it is reported with the rebuild the change asked for, rather than thrown out of + * the listener with that result discarded. + */ + @Test + public void aChangedSortOrderIsReportedWhenTheWriteWhichUntrustsTheVlvIndexGivesUp() throws Exception + { + final ReplayingBackend backend = openBackendWithVlvIndex(); + try + { + final RootContainer rootContainer = backend.getRootContainer(); + final EntryContainer ec = rootContainer.getEntryContainer(KEPT); + final VLVIndex vlvIndex = ec.getVLVIndex(VLV_INDEX_NAME); + assertThat(persistedFlags(rootContainer, ec, vlvIndex.getName())).contains(TRUSTED); + + backend.storage.failWithoutReplay(); + final ConfigChangeResult ccr = vlvIndex.applyConfigurationChange(vlvIndexCfg("+sn")); + + assertThat(ccr.getResultCode()).isNotEqualTo(ResultCode.SUCCESS); + assertThat(ccr.adminActionRequired()) + .as("the rebuild the new sort order needs, on the road which failed").isTrue(); + assertThat(ordinalsOf(ccr)).contains(NOTE_INDEX_ADD_REQUIRES_REBUILD.ordinal()); + assertThat(persistedFlags(rootContainer, ec, vlvIndex.getName())) + .as("what the restart will read").contains(TRUSTED); + + // The definition the failed change did not publish is still a change when asked for again. + final ConfigChangeResult again = vlvIndex.applyConfigurationChange(vlvIndexCfg("+sn")); + assertThat(again.getResultCode()).isEqualTo(ResultCode.SUCCESS); + assertThat(again.adminActionRequired()).as("the definition the failed write did not publish").isTrue(); + assertThat(persistedFlags(rootContainer, ec, vlvIndex.getName())).doesNotContain(TRUSTED); } finally { @@ -882,6 +1027,13 @@ private enum ConflictPoint { /** As soon as the operation first touches the transaction, before it has changed anything. */ FIRST_STORAGE_ACCESS, + /** + * As soon as the operation first touches the transaction, in the form the transaction of + * {@code PDBStorage} reports a conflict: a {@link StorageRuntimeException} wrapping the + * {@link RollbackException}. What an operation catching the former from inside sees, where + * {@link #FIRST_STORAGE_ACCESS} raises the bare conflict such a catch never meets. + */ + FIRST_STORAGE_ACCESS_AS_THE_TRANSACTION_REPORTS_IT, /** Once the operation has run to completion, as a conflict reported by {@code commit()}. */ COMMIT, /** Once the operation has run to completion, as a failure which is not replayed at all. */ @@ -912,6 +1064,16 @@ void conflictAtFirstStorageAccess(int conflicts) arm(ConflictPoint.FIRST_STORAGE_ACCESS, conflicts); } + /** + * Conflicts at the first storage access of the operation, in the form the transaction itself + * reports one: what the operation sees where it asks the transaction for something, rather than + * what the retry loop sees once the operation has let it through. + */ + void conflictAsTheTransactionReportsIt(int conflicts) + { + arm(ConflictPoint.FIRST_STORAGE_ACCESS_AS_THE_TRANSACTION_REPORTS_IT, conflicts); + } + void conflictAtCommit(int conflicts) { arm(ConflictPoint.COMMIT, conflicts); @@ -932,6 +1094,16 @@ void failWithoutReplay() arm(ConflictPoint.NO_REPLAY, 1); } + /** + * Fails without a replay on the {@code nth} write asked for from now on, the next one being + * the first: what the last attempt of a retry loop which gave up leaves the caller holding. + */ + void failWithoutReplayOnWrite(int nth) + { + arm(ConflictPoint.NO_REPLAY, 1); + armedWrite = writes + nth; + } + void failAfterCommit() { arm(ConflictPoint.NO_REPLAY_AFTER_COMMIT, 1); @@ -999,7 +1171,12 @@ public void run(WriteableTransaction txn) throws Exception } if (armed == ConflictPoint.FIRST_STORAGE_ACCESS) { - writeOperation.run(new ConflictingTransaction()); + writeOperation.run(new ConflictingTransaction(false)); + return; + } + if (armed == ConflictPoint.FIRST_STORAGE_ACCESS_AS_THE_TRANSACTION_REPORTS_IT) + { + writeOperation.run(new ConflictingTransaction(true)); return; } writeOperation.run(txn); @@ -1092,9 +1269,21 @@ public void close() /** A transaction which conflicts as soon as it is used, without ever reaching the storage. */ private static final class ConflictingTransaction implements WriteableTransaction { - private static RollbackException conflict() + /** + * Whether the conflict is raised as {@code PDBStorage}'s transaction raises it - wrapped in a + * {@link StorageRuntimeException}, which that storage's retry loop unwraps - or bare. + */ + private final boolean asTheTransactionReportsIt; + + ConflictingTransaction(boolean asTheTransactionReportsIt) + { + this.asTheTransactionReportsIt = asTheTransactionReportsIt; + } + + private RuntimeException conflict() { - return new RollbackException(); + final RollbackException conflict = new RollbackException(); + return asTheTransactionReportsIt ? new StorageRuntimeException(conflict) : conflict; } @Override From 97c8773023dd43c31f2740fa2a9baf820ee456a5 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Thu, 17 Sep 2026 18:09:52 +0300 Subject: [PATCH 3/3] [#991] Pin the report before the first write, the re-apply after a give-up, and what a give-up answers The second round moved the rebuild instruction before the first of the three writes, and pinned it on the road where the third gives up. Moved back to just before the third, every case stayed green: none combined a raised limit with a give-up on the write which opens the indexes the change adds. A give-up there - the write this change opens with nothing to add - is now pinned too: the instruction in the result, the limit not applied, TRUSTED still stored. AttributeIndex replaces the configuration it holds before the write which untrusts the index, so after a give-up that configuration already carries the raised limit while the index does not. The plan asks the index, which is what makes a re-apply of the same change a change again; asked of the configuration instead, it would apply the limit to a trusted index with no report and nothing was red. The give-up case now re-applies, as its vlvIndex twin already did. VLVIndex publishes its four definition fields after the write, and no case read them: the sort keys deleted, or published before the write as on the base, was green. A package-private getSortKeys() lets the replayed case pin the keys the committed write published and the give-up case pin the ones the failed write did not. Both give-up catches answered any non-SUCCESS code with any message and stayed green; they now have to answer the server error result code with the failure named next to the rebuild. --- .../server/backends/pluggable/VLVIndex.java | 6 ++ .../pluggable/ReplayedConfigChangeTest.java | 73 ++++++++++++++++++- 2 files changed, 77 insertions(+), 2 deletions(-) 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 ba19cb18ad..0664efcc93 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 @@ -384,6 +384,12 @@ boolean isTrusted() return trusted; } + /** The sort keys this vlvIndex encodes its keys with: the definition the last committed change published. */ + List getSortKeys() + { + return sortKeys; + } + synchronized void setTrusted(final WriteableTransaction txn, final boolean trusted) throws StorageRuntimeException { this.trusted = trusted; 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 e76671930e..d703faab5e 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 @@ -46,6 +46,7 @@ import org.forgerock.opendj.ldap.ByteString; import org.forgerock.opendj.ldap.DN; import org.forgerock.opendj.ldap.ResultCode; +import org.forgerock.opendj.ldap.SortKey; 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; @@ -69,6 +70,7 @@ 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.DirectoryServer; import org.opends.server.core.ServerContext; import org.opends.server.types.BackupConfig; import org.opends.server.types.BackupDirectory; @@ -574,6 +576,12 @@ private static Set ordinalsOf(ConfigChangeResult ccr) return ordinals; } + /** The result code a configuration listener answers a failure it caught with. */ + private static ResultCode serverErrorResultCode() + { + return DirectoryServer.getCoreConfigManager().getServerErrorResultCode(); + } + /** * An index a change adds is opened untrusted while the backend holds entries, and the operator is * told to rebuild it. That message belongs to the attempt which commits: added to the result from @@ -676,6 +684,8 @@ public void aChangedSortOrderUntrustsTheVlvIndexWhenTheTransactionIsReplayed() t assertThat(vlvIndex.isTrusted()).isFalse(); assertThat(persistedFlags(rootContainer, ec, vlvIndex.getName())) .as("the flag the attempt which committed had to remove").doesNotContain(TRUSTED); + assertThat(vlvIndex.getSortKeys()).as("the definition the committed write published") + .containsExactly(new SortKey("sn", false)); // The definition the change applied is the one this vlvIndex holds from now on, so asking for // it a second time asks for nothing, and opens no transaction to commit nothing. @@ -752,12 +762,65 @@ public void aRaisedEntryLimitIsReportedWhenTheWriteWhichUntrustsTheIndexGivesUp( backend.storage.failWithoutReplayOnWrite(3); final ConfigChangeResult ccr = index.applyConfigurationChange(indexCfg(newTreeSet(IndexType.EQUALITY), 8000)); - assertThat(ccr.getResultCode()).isNotEqualTo(ResultCode.SUCCESS); + assertThat(ccr.getResultCode()).isEqualTo(serverErrorResultCode()); assertThat(ccr.adminActionRequired()).as("the rebuild the raised limit needs, on the road which failed").isTrue(); assertThat(ordinalsOf(ccr)).contains(NOTE_CONFIG_INDEX_ENTRY_LIMIT_REQUIRES_REBUILD.ordinal()); + assertThat(ccr.getMessages().toString()).as("the failure, next to the rebuild") + .contains(UnreplayableFailure.class.getSimpleName()); assertThat(cnIndex.getIndexEntryLimit()).as("the limit the failed write did not apply").isEqualTo(4000); assertThat(persistedFlags(rootContainer, ec, cnIndex.getName())) .as("what the restart will read").contains(TRUSTED); + + // The limit the failed write did not apply is still a change when asked for again: the + // configuration this attribute index holds was replaced before the write which gave up, and + // what the new one asks of the index is asked of the index, not of that configuration. + final ConfigChangeResult again = index.applyConfigurationChange(indexCfg(newTreeSet(IndexType.EQUALITY), 8000)); + assertThat(again.getResultCode()).isEqualTo(ResultCode.SUCCESS); + assertThat(again.adminActionRequired()).as("the limit the failed write did not apply is still a change").isTrue(); + assertThat(ordinalsOf(again)).containsOnly(NOTE_CONFIG_INDEX_ENTRY_LIMIT_REQUIRES_REBUILD.ordinal()); + assertThat(cnIndex.getIndexEntryLimit()).as("the limit the change applied").isEqualTo(8000); + assertThat(cnIndex.isTrusted()).isFalse(); + assertThat(persistedFlags(rootContainer, ec, cnIndex.getName())).doesNotContain(TRUSTED); + } + finally + { + backend.finalizeBackend(); + } + } + + /** + * 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. + */ + @Test + public void aRaisedEntryLimitIsReportedWhenTheWriteWhichAddsIndexesGivesUp() throws Exception + { + final ReplayingBackend backend = openBackend(newTreeSet(KEPT)); + try + { + final RootContainer rootContainer = backend.getRootContainer(); + final EntryContainer ec = rootContainer.getEntryContainer(KEPT); + final AttributeIndex index = ec.getAttributeIndex(cnType); + final MatchingRuleIndex cnIndex = index.getNameToIndexes().values().iterator().next(); + assertThat(persistedFlags(rootContainer, ec, cnIndex.getName())).contains(TRUSTED); + + final int writesBefore = backend.storage.writes(); + backend.storage.failWithoutReplayOnWrite(1); + 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(ccr.getResultCode()).isEqualTo(serverErrorResultCode()); + assertThat(ccr.adminActionRequired()).as("the rebuild the raised limit needs, asked for before the first write") + .isTrue(); + assertThat(ordinalsOf(ccr)).contains(NOTE_CONFIG_INDEX_ENTRY_LIMIT_REQUIRES_REBUILD.ordinal()); + assertThat(ccr.getMessages().toString()).as("the failure, next to the rebuild") + .contains(UnreplayableFailure.class.getSimpleName()); + assertThat(cnIndex.getIndexEntryLimit()).as("the limit the failed change did not apply").isEqualTo(4000); + assertThat(persistedFlags(rootContainer, ec, cnIndex.getName())) + .as("what the restart will read").contains(TRUSTED); } finally { @@ -814,18 +877,24 @@ public void aChangedSortOrderIsReportedWhenTheWriteWhichUntrustsTheVlvIndexGives backend.storage.failWithoutReplay(); final ConfigChangeResult ccr = vlvIndex.applyConfigurationChange(vlvIndexCfg("+sn")); - assertThat(ccr.getResultCode()).isNotEqualTo(ResultCode.SUCCESS); + assertThat(ccr.getResultCode()).isEqualTo(serverErrorResultCode()); assertThat(ccr.adminActionRequired()) .as("the rebuild the new sort order needs, on the road which failed").isTrue(); assertThat(ordinalsOf(ccr)).contains(NOTE_INDEX_ADD_REQUIRES_REBUILD.ordinal()); + assertThat(ccr.getMessages().toString()).as("the failure, next to the rebuild") + .contains(UnreplayableFailure.class.getSimpleName()); assertThat(persistedFlags(rootContainer, ec, vlvIndex.getName())) .as("what the restart will read").contains(TRUSTED); + assertThat(vlvIndex.getSortKeys()).as("the definition the failed write did not publish") + .containsExactly(new SortKey("cn", false)); // The definition the failed change did not publish is still a change when asked for again. final ConfigChangeResult again = vlvIndex.applyConfigurationChange(vlvIndexCfg("+sn")); assertThat(again.getResultCode()).isEqualTo(ResultCode.SUCCESS); assertThat(again.adminActionRequired()).as("the definition the failed write did not publish").isTrue(); assertThat(persistedFlags(rootContainer, ec, vlvIndex.getName())).doesNotContain(TRUSTED); + assertThat(vlvIndex.getSortKeys()).as("the definition the write which committed published") + .containsExactly(new SortKey("sn", false)); } finally {