Trac Ticket #335 - transaction retries need to be cache aware

https://fedorahosted.org/389/ticket/335

Fix description:
When libdb returns DEADLOCK and backend update function retries
the operation, the target entry is reset to the original shape.
The target entry might be or might not be in the entry cache.
Regardless of the status, the original code just releases the
entry with backentry_free before going into the next loop, which
causes the cache error.

This patch checks the status of the entry. If it is in the entry
cache, it removes it from the entry cache and adds a new entry
back to the cache if necessary. To get the accurate cache status
of each entry, the output argument cache_res to id2entry_add_ext
is added.

Additinally, error checking for the conflict value in index_add_mods
was week (curr_attr). This patch is adding the check.

ldap/servers/slapd/back-ldbm/cache.c

@@ -1050,7 +1050,7 @@ static int entrycache_replace(struct cache *cache, struct backentry *olde,
     }
 #endif
     /* adjust cache meta info */
-    newe->ep_refcnt = 1;
+    newe->ep_refcnt++;
     newe->ep_size = cache_entry_size(newe);
     if (newe->ep_size > olde->ep_size) {
         slapi_counter_add(cache->c_cursize, newe->ep_size - olde->ep_size);

ldap/servers/slapd/back-ldbm/id2entry.c

@@ -48,9 +48,12 @@
 
 /* 
  * The caller MUST check for DB_LOCK_DEADLOCK and DB_RUNRECOVERY returned
+ * If cache_res is not NULL, it stores the result of CACHE_ADD of the
+ * entry cache.
  */
 int
-id2entry_add_ext( backend *be, struct backentry *e, back_txn *txn, int encrypt  )
+id2entry_add_ext(backend *be, struct backentry *e, back_txn *txn, 
+                 int encrypt, int *cache_res)
 {
     ldbm_instance *inst = (ldbm_instance *) be->be_instance_info;
     DB     *db = NULL;
@@ -138,8 +141,8 @@ id2entry_add_ext( backend *be, struct backentry *e, back_txn *txn, int encrypt
 
     dblayer_release_id2entry( be, db );
 
-    if (0 == rc)
-    {
+    if (0 == rc) {
+        int cache_rc = 0;
         /* Putting the entry into the entry cache.  
          * We don't use the encrypted entry here. */
         if (entryrdn_get_switch()) {
@@ -200,7 +203,10 @@ id2entry_add_ext( backend *be, struct backentry *e, back_txn *txn, int encrypt
          * should be in the cache.  Thus, this entry e won't be put into the
          * entry cache.  It'll be added by cache_replace.
          */
-        (void) CACHE_ADD( &inst->inst_cache, e, NULL );
+        cache_rc = CACHE_ADD(&inst->inst_cache, e, NULL);
+        if (cache_res) {
+            *cache_res = cache_rc;
+        }
     }
 
 done:
@@ -217,7 +223,7 @@ done:
 int
 id2entry_add( backend *be, struct backentry *e, back_txn *txn  )
 {
-    return id2entry_add_ext(be,e,txn,1);
+    return id2entry_add_ext(be, e, txn, 1, NULL);
 }
 
 /* 

ldap/servers/slapd/back-ldbm/import-threads.c

@@ -2328,7 +2328,7 @@ import_foreman(void *param)
             /* id2entry_add_ext replaces an entry if it already exists. 
              * therefore, the Entry ID stays the same.
              */
-            ret = id2entry_add_ext(be, fi->entry, NULL, job->encrypt);
+            ret = id2entry_add_ext(be, fi->entry, NULL, job->encrypt, NULL);
             if (ret) {
                 /* DB_RUNRECOVERY usually occurs if disk fills */
                 if (LDBM_OS_ERR_IS_DISKFULL(ret)) {

ldap/servers/slapd/back-ldbm/index.c

@@ -550,8 +550,7 @@ index_add_mods(
         /* Get a list of all values specified in the operation.
          */
         if ( mods[i]->mod_bvalues != NULL ) {
-            valuearray_init_bervalarray(mods[i]->mod_bvalues,
-                                        &mods_valueArray);
+            valuearray_init_bervalarray(mods[i]->mod_bvalues, &mods_valueArray);
         }
 
         switch ( mods[i]->mod_op & ~LDAP_MOD_BVALUES ) {
@@ -622,20 +621,24 @@ index_add_mods(
             } else {
                 /* Verify if the value is in newe.
                  * If it is in, we will add the attr value to the index file. */
-                slapi_entry_attr_find( newe->ep_entry, 
-                                       mods[i]->mod_type, &curr_attr );
+                curr_attr = NULL;
+                slapi_entry_attr_find(newe->ep_entry, 
+                                      mods[i]->mod_type, &curr_attr);
                 
-                for (j = 0; mods_valueArray[j] != NULL; j++) {
-                    /* mods_valueArray[j] is in curr_attr ==> return 0 */
-                    if (slapi_attr_value_find(curr_attr,
+                if (curr_attr) { /* found the type */
+                    for (j = 0; mods_valueArray[j] != NULL; j++) {
+                        /* mods_valueArray[j] is in curr_attr ==> return 0 */
+                        if (slapi_attr_value_find(curr_attr,
                                 slapi_value_get_berval(mods_valueArray[j]))) {
-                        /* The value is NOT in newe, remove it. */
-                        Slapi_Value *rval = valuearray_remove_value(curr_attr,
-                                                            mods_valueArray,
-                                                            mods_valueArray[j]);
-                        slapi_value_free( &rval );
-                        /* indicates there was some conflict */
-                        mods[i]->mod_op |= LDAP_MOD_IGNORE;
+                            /* The value is NOT in newe, remove it. */
+                            Slapi_Value *rval;
+                            rval = valuearray_remove_value(curr_attr,
+                                                           mods_valueArray,
+                                                           mods_valueArray[j]);
+                            slapi_value_free( &rval );
+                            /* indicates there was some conflict */
+                            mods[i]->mod_op |= LDAP_MOD_IGNORE;
+                        }
                     }
                 }
                 if (mods_valueArray) {
@@ -724,7 +727,9 @@ index_add_mods(
                      * BE_INDEX_EQUALITY flag so the equality index is
                      * removed.
                      */
-                    slapi_entry_attr_find( olde->ep_entry, mods[i]->mod_type, &curr_attr );
+                    curr_attr = NULL;
+                    slapi_entry_attr_find(olde->ep_entry,
+                                          mods[i]->mod_type, &curr_attr);
                     if (curr_attr) {
                         int found = 0;
                         for (j = 0; mods_valueArray[j] != NULL; j++ ) {

ldap/servers/slapd/back-ldbm/ldbm_add.c

@@ -695,16 +695,32 @@ ldbm_back_add( Slapi_PBlock *pb )
 			dblayer_txn_abort(li,&txn);
 			slapi_pblock_set(pb, SLAPI_TXN, parent_txn);
 
-			backentry_free(&addingentry);
+			if (addingentry_in_cache) {
+				/* addingentry is in cache.  Remove it once. */
+				CACHE_REMOVE(&inst->inst_cache, addingentry);
+				CACHE_RETURN(&inst->inst_cache, &addingentry);
+			} else {
+				backentry_free(&addingentry);
+			}
 			slapi_pblock_set( pb, SLAPI_ADD_ENTRY, originalentry->ep_entry );
 			addingentry = originalentry;
 			if ( (originalentry = backentry_dup( addingentry )) == NULL ) {
 				ldap_result_code= LDAP_OPERATIONS_ERROR;
 				goto error_return;
 			}
+			if (addingentry_in_cache) {
+				/* Adding the resetted addingentry to the cache. */
+				if (cache_add_tentative(&inst->inst_cache,
+				                        addingentry, NULL) != 0) {
+					LDAPDebug0Args(LDAP_DEBUG_CACHE,
+					              "cache_add_tentative concurrency detected\n");
+					ldap_result_code = LDAP_ALREADY_EXISTS;
+					goto error_return;
+				}
+			}
 
 			/* We're re-trying */
-			LDAPDebug( LDAP_DEBUG_TRACE, "Add Retrying Transaction\n", 0, 0, 0 );
+			LDAPDebug0Args(LDAP_DEBUG_BACKLDBM, "Add Retrying Transaction\n");
 #ifndef LDBM_NO_BACKOFF_DELAY
 			{
 				PRIntervalTime interval;

ldap/servers/slapd/back-ldbm/ldbm_delete.c

@@ -90,6 +90,7 @@ ldbm_back_delete( Slapi_PBlock *pb )
 	int delete_tombstone_entry = 0;	/* We must remove the given tombstone entry from the DB	*/
 	int create_tombstone_entry = 0;	/* We perform a "regular" LDAP delete but since we use	*/
 									/* replication, we must create a new tombstone entry	*/
+	int e_in_cache = 0;
 	int tombstone_in_cache = 0;
 	entry_address *addr;
 	int addordel_flags = 0; /* passed to index_addordel */
@@ -186,6 +187,7 @@ ldbm_back_delete( Slapi_PBlock *pb )
 		/* retval is -1 */
 		goto error_return; /* error result sent by find_entry2modify() */
 	}
+	e_in_cache = 1;
 
 	if ( slapi_entry_has_children( e->ep_entry ) )
 	{
@@ -481,8 +483,18 @@ ldbm_back_delete( Slapi_PBlock *pb )
 		if (txn.back_txn_txn && (txn.back_txn_txn != parent_txn)) {
 			dblayer_txn_abort(li,&txn);
 			slapi_pblock_set(pb, SLAPI_TXN, parent_txn);
-
-			backentry_free(&e);
+			if (e_in_cache) {
+				/* entry 'e' is in the entry cache.  Since we reset 'e' to
+				 * the original_entry, remove it from the cache.  */
+				CACHE_REMOVE(&inst->inst_cache, e);
+				cache_unlock_entry(&inst->inst_cache, e);
+				CACHE_RETURN(&inst->inst_cache, &e);
+				/* As we are about to delete it, 
+				 * we don't put the entry back to cache */
+				e_in_cache = 0; 
+			} else {
+				backentry_free(&e);
+			}
 			slapi_pblock_set( pb, SLAPI_DELETE_EXISTING_ENTRY, original_entry->ep_entry );
 			e = original_entry;
 			if ( (original_entry = backentry_dup( e )) == NULL ) {
@@ -490,10 +502,11 @@ ldbm_back_delete( Slapi_PBlock *pb )
 				goto error_return;
 			}
 			/* We're re-trying */
-			LDAPDebug( LDAP_DEBUG_TRACE, "Delete Retrying Transaction\n", 0, 0, 0 );
+			LDAPDebug0Args(LDAP_DEBUG_BACKLDBM,
+			               "Delete Retrying Transaction\n");
 #ifndef LDBM_NO_BACKOFF_DELAY
-            {
-	        PRIntervalTime interval;
+			{
+			PRIntervalTime interval;
 			interval = PR_MillisecondsToInterval(slapi_rand() % 100);
 			DS_Sleep(interval);
 			}
@@ -1006,9 +1019,11 @@ ldbm_back_delete( Slapi_PBlock *pb )
 	}
 
 	/* delete from cache and clean up */
-	CACHE_REMOVE(&inst->inst_cache, e);
-	cache_unlock_entry( &inst->inst_cache, e );
-	CACHE_RETURN( &inst->inst_cache, &e );
+	if (e_in_cache) {
+		CACHE_REMOVE(&inst->inst_cache, e);
+		cache_unlock_entry(&inst->inst_cache, e);
+		CACHE_RETURN(&inst->inst_cache, &e);
+	}
 	if (tombstone_in_cache) {
 		if (CACHE_ADD( &inst->inst_cache, tombstone, NULL ) == 0) {
 			tombstone_in_cache = 1;
@@ -1127,7 +1142,7 @@ common_return:
 
 	/* Need to return to cache after post op plugins are called */
 	if (retval) { /* error case */
-		if (e) {
+		if (e && e_in_cache) {
 			cache_unlock_entry( &inst->inst_cache, e );
 			CACHE_RETURN( &inst->inst_cache, &e );
 		}

ldap/servers/slapd/back-ldbm/ldbm_modify.c

@@ -151,7 +151,7 @@ int modify_update_all(backend *be, Slapi_PBlock *pb,
 	 * Update the ID to Entry index. 
 	 * Note that id2entry_add replaces the entry, so the Entry ID stays the same.
 	 */
-	retval = id2entry_add_ext( be, mc->new_entry, txn, mc->attr_encrypt );
+	retval = id2entry_add_ext( be, mc->new_entry, txn, mc->attr_encrypt, NULL );
 	if ( 0 != retval ) {
 		if (DB_LOCK_DEADLOCK != retval)
 		{
@@ -444,7 +444,7 @@ ldbm_back_modify( Slapi_PBlock *pb )
 
 	txn.back_txn_txn = NULL; /* ready to create the child transaction */
 	for (retry_count = 0; retry_count < RETRY_TIMES; retry_count++) {
-
+		int cache_rc = 0;
 		if (txn.back_txn_txn && (txn.back_txn_txn != parent_txn)) {
 			dblayer_txn_abort(li,&txn);
 			slapi_pblock_set(pb, SLAPI_TXN, parent_txn);
@@ -456,15 +456,31 @@ ldbm_back_modify( Slapi_PBlock *pb )
 			slapi_pblock_get(pb, SLAPI_MODIFY_MODS, &mods);
 			ldap_mods_free(mods, 1);
 			slapi_pblock_set(pb, SLAPI_MODIFY_MODS, copy_mods(mods_original));
-			backentry_free(&ec);
+			if (ec_in_cache) {
+				/* New entry 'ec' is in the entry cache.
+				 * Remove it from the cache once. */
+				CACHE_REMOVE(&inst->inst_cache, ec);
+				cache_unlock_entry(&inst->inst_cache, e);
+				CACHE_RETURN(&inst->inst_cache, ec);
+			} else {
+				backentry_free(&ec);
+			}
 			slapi_pblock_set( pb, SLAPI_MODIFY_EXISTING_ENTRY, original_entry->ep_entry );
 			ec = original_entry;
 			if ( (original_entry = backentry_dup( e )) == NULL ) {
 				ldap_result_code= LDAP_OPERATIONS_ERROR;
 				goto error_return;
 			}
-
-			LDAPDebug( LDAP_DEBUG_TRACE, "Modify Retrying Transaction\n", 0, 0, 0 );
+			/* Put new entry 'ec' into the entry cache. */
+			if (ec_in_cache && CACHE_ADD(&inst->inst_cache, ec, NULL) < 0) {
+				LDAPDebug1Arg(LDAP_DEBUG_ANY, 
+				              "ldbm_back_modify: adding %s to cache failed\n",
+				              slapi_entry_get_dn_const(ec->ep_entry));
+				ldap_result_code = LDAP_OPERATIONS_ERROR;
+				goto error_return;
+			}
+			LDAPDebug0Args(LDAP_DEBUG_BACKLDBM,
+			               "Modify Retrying Transaction\n");
 #ifndef LDBM_NO_BACKOFF_DELAY
 			{
 			PRIntervalTime interval;
@@ -503,9 +519,10 @@ ldbm_back_modify( Slapi_PBlock *pb )
 
 		/*
 		 * Update the ID to Entry index. 
-		 * Note that id2entry_add replaces the entry, so the Entry ID stays the same.
+		 * Note that id2entry_add replaces the entry, so the Entry ID 
+		 * stays the same.
 		 */
-		retval = id2entry_add( be, ec, &txn ); 
+		retval = id2entry_add_ext( be, ec, &txn, 1, &cache_rc ); 
 		if (DB_LOCK_DEADLOCK == retval)
 		{
 			/* Abort and re-try */
@@ -518,7 +535,14 @@ ldbm_back_modify( Slapi_PBlock *pb )
 			MOD_SET_ERROR(ldap_result_code, LDAP_OPERATIONS_ERROR, retry_count);
 			goto error_return;
 		}
-		ec_in_cache = 1;
+		/* 
+		 * id2entry_add tries to put ec into the entry cache,
+		 * but due to the conflict with original 'e',
+		 * the cache_add (called vai id2entry_add) could fail.
+		 */
+		if (0 == cache_rc) {
+			ec_in_cache = 1;
+		}
 		retval = index_add_mods( be, mods, e, ec, &txn );
 		if (DB_LOCK_DEADLOCK == retval)
 		{
@@ -596,6 +620,7 @@ ldbm_back_modify( Slapi_PBlock *pb )
 		MOD_SET_ERROR(ldap_result_code, LDAP_OPERATIONS_ERROR, retry_count);
 		goto error_return;
 	}
+	ec_in_cache = 1;
 
 	postentry = slapi_entry_dup( ec->ep_entry );
 	slapi_pblock_set( pb, SLAPI_ENTRY_POST_OP, postentry );

ldap/servers/slapd/back-ldbm/ldbm_modrdn.c

@@ -764,14 +764,30 @@ ldbm_back_modrdn( Slapi_PBlock *pb )
             slapi_sdn_free(&dn_newsuperiordn);
             slapi_pblock_set(pb, SLAPI_MODRDN_NEWSUPERIOR_SDN, orig_dn_newsuperiordn);
             orig_dn_newsuperiordn = slapi_sdn_dup(orig_dn_newsuperiordn);
-
-            backentry_free(&ec);
+            if (ec_in_cache) {
+                /* New entry 'ec' is in the entry cache.
+                 * Remove it from teh cache once. */
+                CACHE_REMOVE(&inst->inst_cache, ec);
+                cache_unlock_entry(&inst->inst_cache, e);
+                CACHE_RETURN(&inst->inst_cache, ec);
+            } else {
+                backentry_free(&ec);
+            }
             slapi_pblock_set( pb, SLAPI_MODRDN_EXISTING_ENTRY, original_entry->ep_entry );
             ec = original_entry;
             if ( (original_entry = backentry_dup( ec )) == NULL ) {
                 ldap_result_code= LDAP_OPERATIONS_ERROR;
                 goto error_return;
             }
+            if (ec_in_cache && 
+                /* Put the resetted entry 'ec' into the cache again. */
+                (cache_add_tentative( &inst->inst_cache, ec, NULL ) != 0)) {
+                LDAPDebug1Arg(LDAP_DEBUG_ANY, 
+                              "ldbm_back_modrdn: adding %s to cache failed\n",
+                              slapi_entry_get_dn_const(ec->ep_entry));
+                ldap_result_code = LDAP_OPERATIONS_ERROR;
+                goto error_return;
+            }
 
             slapi_pblock_get(pb, SLAPI_MODRDN_TARGET_ENTRY, &target_entry );
             slapi_entry_free(target_entry);
@@ -801,7 +817,8 @@ ldbm_back_modrdn( Slapi_PBlock *pb )
                 goto error_return;
             }
             /* We're re-trying */
-            LDAPDebug( LDAP_DEBUG_TRACE, "Modrdn Retrying Transaction\n", 0, 0, 0 );
+            LDAPDebug0Args(LDAP_DEBUG_BACKLDBM,
+                           "Modrdn Retrying Transaction\n");
         }
         retval = dblayer_txn_begin(li,parent_txn,&txn);
         if (0 != retval) {

ldap/servers/slapd/back-ldbm/proto-back-ldbm.h

@@ -220,7 +220,7 @@ int has_children( struct ldbminfo *li, struct backentry *p, back_txn *txn, int *
  * id2entry.c
  */
 int id2entry_add( backend *be, struct backentry *e, back_txn *txn );
-int id2entry_add_ext( backend *be, struct backentry *e, back_txn *txn, int encrypt );
+int id2entry_add_ext( backend *be, struct backentry *e, back_txn *txn, int encrypt, int *cache_res );
 int id2entry_delete( backend *be, struct backentry *e, back_txn *txn );
 struct backentry * id2entry( backend *be, ID id, back_txn *txn, int *err );