[#990] Drop what a previous index left behind instead of adopting it when an index is added

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);

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,103 @@ void open(WriteableTransaction txn, boolean createOnDemand) throws StorageRuntim
     config.addChangeListener(this);
   }
 
+  /**
+   * Drops whatever an index of the same name left behind for the trees this index, which the
+   * configuration is adding, is about to open.
+   * <p>
+   * 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).
+   * <p>
+   * 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.
+   * <p>
+   * 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.
+   * <p>
+   * 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
+   * @throws StorageRuntimeException if an error occurs in the storage
+   */
+  boolean dropLeftovers(WriteableTransaction txn) throws StorageRuntimeException
+  {
+    boolean dropped = false;
+    for (Index index : indexIdToIndexes.values())
+    {
+      dropped |= dropLeftoversOf(txn, index);
+    }
+    return dropped;
+  }
+
+  /**
+   * Drops the tree an index about to be opened would adopt, and the {@code state} record which goes
+   * with it.
+   * <p>
+   * 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 - but a record
+   * deleted on its own is not reported as discarded content, since none was.
+   * <p>
+   * 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.
+   * <p>
+   * 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. 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 was dropped
+   */
+  private boolean dropLeftoversOf(WriteableTransaction txn, Index index)
+  {
+    if (txn.treeExists(index.getName()))
+    {
+      // Deletes the state record along with the tree.
+      entryContainer.deleteTree(txn, index);
+      return true;
+    }
+    state.deleteRecord(txn, index.getName());
+    return false;
+  }
+
+  /**
+   * 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()
   {
@@ -463,6 +561,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;
@@ -907,6 +1014,12 @@ 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 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<MatchingRuleIndex> discarded = new ArrayList<>();
     try
     {
       final Map<String, MatchingRuleIndex> newIndexIdToIndexes = buildIndexes(entryContainer, state, newConfiguration,
@@ -945,6 +1058,27 @@ public synchronized ConfigChangeResult applyConfigurationChange(final BackendInd
         ccr.addMessage(rebuildMessage);
       }
 
+      // 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())
+      {
+        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);
+              }
+            }
+          }
+        });
+      }
+
       // Open added indexes *before* adding them to indexIdToIndexes
       final List<TreeName> addedIndexesToRebuild = new ArrayList<>();
       entryContainer.getRootContainer().getStorage().write(new WriteOperation()
@@ -1044,6 +1178,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;
   }

opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/EntryContainer.java

@@ -181,6 +181,12 @@ private class AttributeIndexCfgManager implements
     @Override
     public boolean isConfigurationAddAcceptable(final BackendIndexCfg cfg, List<LocalizableMessage> unacceptableReasons)
     {
+      final LocalizableMessage alreadyIndexed = alreadyIndexedBy(cfg);
+      if (alreadyIndexed != null)
+      {
+        unacceptableReasons.add(alreadyIndexed);
+        return false;
+      }
       try
       {
         newAttributeIndex(cfg, null);
@@ -193,14 +199,56 @@ public boolean isConfigurationAddAcceptable(final BackendIndexCfg cfg, List<Loca
       }
     }
 
+    /**
+     * Why an index for the attribute of this configuration must not be added, or null if it may be.
+     * <p>
+     * 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;
+      }
+      // 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 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
       {
         final CryptoSuite cryptoSuite = newCryptoSuite(cfg.isConfidentialityEnabled());
         final AttributeIndex index = newAttributeIndex(cfg, cryptoSuite);
+        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()
         {
@@ -229,6 +277,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;
     }
 
@@ -306,8 +361,21 @@ public boolean isConfigurationAddAcceptable(BackendVLVIndexCfg cfg, List<Localiz
     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 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
       {
+        storage.write(new WriteOperation()
+        {
+          @Override
+          public void run(WriteableTransaction txn) throws Exception
+          {
+            discarded.set(VLVIndex.dropLeftovers(txn, EntryContainer.this, state, cfg.getName()));
+          }
+        });
         final AtomicReference<VLVIndex> built = new AtomicReference<>();
         final AtomicBoolean trusted = new AtomicBoolean();
         storage.write(new WriteOperation()
@@ -343,6 +411,13 @@ public void run(WriteableTransaction txn) throws Exception
         ccr.setResultCode(DirectoryServer.getCoreConfigManager().getServerErrorResultCode());
         ccr.addMessage(LocalizableMessage.raw(StaticUtils.stackTraceToSingleLineString(e)));
       }
+      finally
+      {
+        if (discarded.get())
+        {
+          AttributeIndex.reportDiscardedLeftovers(ccr, cfg.getName(), getBaseDN());
+        }
+      }
       return ccr;
     }
 

opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/VLVIndex.java

@@ -114,8 +114,8 @@ class VLVIndex extends AbstractTree implements ConfigurationChangeListener<Backe
       final EntryContainer entryContainer, final WriteableTransaction txn) throws StorageRuntimeException,
       ConfigException
   {
-    super(new TreeName(entryContainer.getTreePrefix(), "vlv." + config.getName()));
-    this.counter = new ShardedCounter(new TreeName(entryContainer.getTreePrefix(), "counter.vlv." + config.getName()));
+    super(treeNameOf(entryContainer, config.getName()));
+    this.counter = new ShardedCounter(counterTreeNameOf(entryContainer, config.getName()));
     this.config = config;
     this.baseDN = config.getBaseDN();
     this.scope = convertScope(config.getScope());
@@ -170,6 +170,57 @@ void afterOpen(final WriteableTransaction txn, boolean createOnDemand) throws St
     }
   }
 
+  private static TreeName treeNameOf(EntryContainer entryContainer, String indexName)
+  {
+    return new TreeName(entryContainer.getTreePrefix(), "vlv." + indexName);
+  }
+
+  private static TreeName counterTreeNameOf(EntryContainer entryContainer, String indexName)
+  {
+    return new TreeName(entryContainer.getTreePrefix(), "counter.vlv." + indexName);
+  }
+
+  /**
+   * Drops whatever a VLV index of the same name left behind for the VLV index the configuration is
+   * adding: its tree, the counter which goes with it and the {@code state} record which carries
+   * their TRUSTED flag.
+   * <p>
+   * 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#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 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
+   */
+  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 (txn.treeExists(counterName))
+    {
+      txn.deleteTree(counterName);
+      dropped = true;
+    }
+    if (txn.treeExists(name))
+    {
+      txn.deleteTree(name);
+      dropped = true;
+    }
+    // The record can outlive the trees: see AttributeIndex.dropLeftoversOf.
+    state.deleteRecord(txn, name);
+    return dropped;
+  }
+
   @Override
   void beforeDelete(WriteableTransaction txn) throws StorageRuntimeException
   {