Bug fix 3.5/make ttl indexes behave like others (#9547)

* make TTL indexes behave like other indexes on creation

if a TTL index is already present on a collection, the previous behavior
was to make subsequent calls to `ensureIndex` fail unconditionally with
the error "there can only be one ttl index per collection".

now, we are comparing the attributes of the to-be-created index with the
attributes of the existing TTL index and make it only fail when the
attributes differ. if the attributes are identical, the `ensureIndex`
call succeeds and returns the existing index.

* added tests

* updated CHANGELOG

CHANGELOG

@@ -1,4 +1,19 @@
-v3.5.0-rc.5 (2019-XX-XX)
+v3.5.0-rc.6 (2019-XX-XX)
+------------------------
+
+* Make TTL indexes behave like other indexes on creation
+    
+  If a TTL index is already present on a collection, the previous behavior
+  was to make subsequent calls to `ensureIndex` fail unconditionally with
+  the error "there can only be one ttl index per collection".
+    
+  Now we are comparing the attributes of the to-be-created index with the
+  attributes of the existing TTL index and make it only fail when the
+  attributes differ. If the attributes are identical, the `ensureIndex`
+  call succeeds and returns the existing index.
+
+
+v3.5.0-rc.5 (2019-07-22)
 ------------------------
 
 * MinReplicationFactor:

arangod/Indexes/IndexFactory.cpp

@@ -23,6 +23,7 @@
 
 #include "Basics/AttributeNameParser.h"
 #include "Basics/Exceptions.h"
+#include "Basics/FloatingPoint.h"
 #include "Basics/StaticStrings.h"
 #include "Basics/StringUtils.h"
 #include "Basics/VelocyPackHelper.h"
@@ -112,6 +113,17 @@ bool IndexTypeFactory::equal(arangodb::Index::IndexType type,
         !arangodb::basics::VelocyPackHelper::equal(value, rhs.get("minLength"), false)) {
       return false;
     }
+  } else if (arangodb::Index::IndexType::TRI_IDX_TYPE_TTL_INDEX == type) {
+    value = lhs.get(StaticStrings::IndexExpireAfter);
+
+    if (value.isNumber() && rhs.get(StaticStrings::IndexExpireAfter).isNumber()) {
+      double const expireAfter = value.getNumber<double>();
+      value = rhs.get(StaticStrings::IndexExpireAfter);
+
+      if (!FloatingPoint<double>{expireAfter}.AlmostEquals(FloatingPoint<double>{value.getNumber<double>()})) {
+        return false;
+      }
+    }
   }
 
   // other index types: fields must be identical if present

arangod/MMFiles/MMFilesCollection.cpp

@@ -2215,6 +2215,13 @@ std::shared_ptr<Index> MMFilesCollection::createIndex(
   if (idx != nullptr) {
     // We already have this index.
     if (idx->type() == arangodb::Index::TRI_IDX_TYPE_TTL_INDEX) {
+      // special handling for TTL indexes
+      // if there is exactly the same index present, we return it
+      if (idx->matchesDefinition(info)) {
+        created = false;
+        return idx;
+      }
+      // if there is another TTL index already, we make things abort here
       THROW_ARANGO_EXCEPTION_MESSAGE(
           TRI_ERROR_BAD_PARAMETER,
           "there can only be one ttl index per collection");

arangod/MMFiles/MMFilesTtlIndex.cpp

@@ -22,6 +22,7 @@
 ////////////////////////////////////////////////////////////////////////////////
 
 #include "MMFilesTtlIndex.h"
+#include "Basics/FloatingPoint.h"
 #include "Basics/StaticStrings.h"
 #include "Transaction/Helpers.h"
 
@@ -46,6 +47,18 @@ MMFilesTtlIndex::MMFilesTtlIndex(
 
 MMFilesTtlIndex::~MMFilesTtlIndex() {}
 
+/// @brief Test if this index matches the definition
+bool MMFilesTtlIndex::matchesDefinition(VPackSlice const& info) const {
+  // call compare method of parent first
+  if (!MMFilesSkiplistIndex::matchesDefinition(info)) {
+    return false;
+  }
+  // compare our own attribute, "expireAfter"
+  TRI_ASSERT(info.isObject());
+  double const expireAfter = info.get(StaticStrings::IndexExpireAfter).getNumber<double>();
+  return FloatingPoint<double>{expireAfter}.AlmostEquals(FloatingPoint<double>{_expireAfter});
+}
+
 void MMFilesTtlIndex::toVelocyPack(arangodb::velocypack::Builder& builder,
                                    std::underlying_type<Index::Serialize>::type flags) const {
   builder.openObject();

arangod/MMFiles/MMFilesTtlIndex.h

@@ -54,6 +54,8 @@ class MMFilesTtlIndex final : public MMFilesSkiplistIndex {
 
   char const* typeName() const override { return "ttl"; }
   
+  bool matchesDefinition(VPackSlice const&) const override;
+  
   void toVelocyPack(arangodb::velocypack::Builder& builder,
                     std::underlying_type<Index::Serialize>::type flags) const override;
 

arangod/RestServer/TtlFeature.cpp

@@ -228,7 +228,7 @@ class TtlThread final : public Thread {
  private:
   /// @brief whether or not the background thread shall continue working
   bool isActive() const {
-    return _ttlFeature->isActive() && !isStopping();
+    return _ttlFeature->isActive() && !isStopping() && !ServerState::readOnly();
   }
 
   void work(TtlStatistics& stats, TtlProperties const& properties) {

arangod/RocksDBEngine/RocksDBCollection.cpp

@@ -330,6 +330,13 @@ std::shared_ptr<Index> RocksDBCollection::createIndex(VPackSlice const& info,
     if ((idx = findIndex(info, _indexes)) != nullptr) {
       // We already have this index.
       if (idx->type() == arangodb::Index::TRI_IDX_TYPE_TTL_INDEX) {
+        // special handling for TTL indexes
+        // if there is exactly the same index present, we return it
+        if (idx->matchesDefinition(info)) {
+          created = false;
+          return idx;
+        }
+        // if there is another TTL index already, we make things abort here
         THROW_ARANGO_EXCEPTION_MESSAGE(
             TRI_ERROR_BAD_PARAMETER,
             "there can only be one ttl index per collection");

arangod/RocksDBEngine/RocksDBTtlIndex.cpp

@@ -22,6 +22,7 @@
 ////////////////////////////////////////////////////////////////////////////////
 
 #include "RocksDBTtlIndex.h"
+#include "Basics/FloatingPoint.h"
 #include "Basics/StaticStrings.h"
 #include "Transaction/Helpers.h"
 #include "VocBase/LogicalCollection.h"
@@ -47,6 +48,18 @@ RocksDBTtlIndex::RocksDBTtlIndex(
 #endif
 }
 
+/// @brief Test if this index matches the definition
+bool RocksDBTtlIndex::matchesDefinition(VPackSlice const& info) const {
+  // call compare method of parent first
+  if (!RocksDBSkiplistIndex::matchesDefinition(info)) {
+    return false;
+  }
+  // compare our own attribute, "expireAfter"
+  TRI_ASSERT(info.isObject());
+  double const expireAfter = info.get(StaticStrings::IndexExpireAfter).getNumber<double>();
+  return FloatingPoint<double>{expireAfter}.AlmostEquals(FloatingPoint<double>{_expireAfter});
+}
+
 void RocksDBTtlIndex::toVelocyPack(arangodb::velocypack::Builder& builder,
                                    std::underlying_type<Index::Serialize>::type flags) const {
   builder.openObject();

arangod/RocksDBEngine/RocksDBTtlIndex.h

@@ -47,6 +47,8 @@ class RocksDBTtlIndex final : public RocksDBSkiplistIndex {
 
   char const* typeName() const override { return "rocksdb-ttl"; }
   
+  bool matchesDefinition(VPackSlice const&) const override;
+  
   void toVelocyPack(arangodb::velocypack::Builder& builder,
                     std::underlying_type<Index::Serialize>::type flags) const override;
   

tests/js/common/shell/shell-ttl.js

@@ -1,5 +1,5 @@
 /*jshint globalstrict:false, strict:false */
-/*global arango, assertEqual, assertTrue, assertEqual, assertNotEqual, fail */
+/*global arango, assertEqual, assertTrue, assertFalse, assertEqual, assertNotEqual, fail */
 
 ////////////////////////////////////////////////////////////////////////////////
 /// @brief test ttl configuration
@@ -215,7 +215,7 @@ function TtlSuite () {
       }
     },
     
-    testCreateIndexMultipleTimes : function () {
+    testCreateIndexMultipleTimesDifferentField : function () {
       let c = db._create(cn, { numberOfShards: 2 });
       c.ensureIndex({ type: "ttl", fields: ["test"], expireAfter: 10 });
       try {
@@ -226,6 +226,27 @@ function TtlSuite () {
       }
     },
     
+    testCreateIndexMultipleTimesDifferentExpire : function () {
+      let c = db._create(cn, { numberOfShards: 2 });
+      c.ensureIndex({ type: "ttl", fields: ["test"], expireAfter: 10 });
+      try {
+        c.ensureIndex({ type: "ttl", fields: ["test"], expireAfter: 11 });
+        fail();
+      } catch (err) {
+        assertEqual(ERRORS.ERROR_BAD_PARAMETER.code, err.errorNum);
+      }
+    },
+    
+    testCreateIndexMultipleTimesSameAttributes : function () {
+      let c = db._create(cn, { numberOfShards: 2 });
+      let idx1 = c.ensureIndex({ type: "ttl", fields: ["test"], expireAfter: 10 });
+      let idx2 = c.ensureIndex({ type: "ttl", fields: ["test"], expireAfter: 10 });
+
+      assertTrue(idx1.isNewlyCreated);
+      assertFalse(idx2.isNewlyCreated);
+      assertEqual(idx1.id, idx2.id);
+    },
+    
     testCreateIndexOnMultipleAttributes : function () {
       let c = db._create(cn, { numberOfShards: 2 });
       try {