[Fedora-directory-commits] ldapserver/ldap/servers/plugins/replication windows_private.c, 1.15, 1.16 windows_prot_private.h, 1.7, 1.8 windows_protocol_util.c, 1.32, 1.33 windowsrepl.h, 1.12, 1.13

[Doctor Conrad]

Author: nkinder

Update of /cvs/dirsec/ldapserver/ldap/servers/plugins/replication
In directory cvs-int.fedora.redhat.com:/tmp/cvs-serv19325/plugins/replication

Modified Files:
	windows_private.c windows_prot_private.h 
	windows_protocol_util.c windowsrepl.h 
Log Message:
Resolves: 242551
Summary: Performance cleanup of sync code.  Improve tombstone search performance.



Index: windows_private.c
===================================================================
RCS file: /cvs/dirsec/ldapserver/ldap/servers/plugins/replication/windows_private.c,v
retrieving revision 1.15
retrieving revision 1.16
diff -u -r1.15 -r1.16
--- windows_private.c	12 Sep 2007 23:05:24 -0000	1.15
+++ windows_private.c	17 Sep 2007 19:18:30 -0000	1.16
@@ -66,6 +66,10 @@
   char *windows_domain;
   int isnt4;
   int iswin2k3;
+  /* This filter is used to determine if an entry belongs to this agreement.  We put it
here
+   * so we only have to allocate each filter once instead of doing it every time we
receive a change. */
+  Slapi_Filter *directory_filter; /* Used for checking if local entries need to be
sync'd to AD */
+  Slapi_Filter *deleted_filter; /* Used for checking if an entry is an AD tombstone */
 };
 
 static int
@@ -192,6 +196,8 @@
 	dp = (Dirsync_Private *)slapi_ch_calloc(sizeof(Dirsync_Private),1);
 
 	dp->dirsync_maxattributecount = -1;
+	dp->directory_filter = NULL;
+	dp->deleted_filter = NULL;
 
 	LDAPDebug( LDAP_DEBUG_TRACE, "<= windows_private_new\n", 0, 0, 0 );
 	return dp;
@@ -206,8 +212,8 @@
 
 	PR_ASSERT(dp  != NULL);
 	
-	/* DBDB: need to free payoad here */
-	
+	slapi_filter_free(dp->directory_filter, 1);
+	slapi_filter_free(dp->deleted_filter, 1);
 	slapi_ch_free((void **)dp);
 
 	LDAPDebug( LDAP_DEBUG_TRACE, "<= windows_private_delete\n", 0, 0, 0 );
@@ -278,6 +284,53 @@
 		LDAPDebug( LDAP_DEBUG_TRACE, "<= windows_private_set_iswin2k3\n", 0, 0, 0
);
 }
 
+/* Returns a copy of the Slapi_Filter pointer.  The caller should not free it */
+Slapi_Filter* windows_private_get_directory_filter(const Repl_Agmt *ra)
+{
+		Dirsync_Private *dp;
+
+		LDAPDebug( LDAP_DEBUG_TRACE, "=> windows_private_get_directory_filter\n",
0, 0, 0 );
+
+	PR_ASSERT(ra);
+
+		dp = (Dirsync_Private *) agmt_get_priv(ra);
+		PR_ASSERT (dp);
+
+		if (dp->directory_filter == NULL) {
+			char *string_filter =
slapi_ch_strdup("(&(|(objectclass=ntuser)(objectclass=ntgroup))(ntUserDomainId=*))");
+			/* The filter gets freed in windows_agreement_delete() */
+                        dp->directory_filter = slapi_str2filter( string_filter );
+			slapi_ch_free_string(&string_filter);
+		}
+
+		LDAPDebug( LDAP_DEBUG_TRACE, "<= windows_private_get_directory_filter\n",
0, 0, 0 );
+
+		return dp->directory_filter;
+}
+
+/* Returns a copy of the Slapi_Filter pointer.  The caller should not free it */
+Slapi_Filter* windows_private_get_deleted_filter(const Repl_Agmt *ra)
+{
+		Dirsync_Private *dp;
+
+		LDAPDebug( LDAP_DEBUG_TRACE, "=> windows_private_get_deleted_filter\n", 0,
0, 0 );
+
+	PR_ASSERT(ra);
+
+		dp = (Dirsync_Private *) agmt_get_priv(ra);
+		PR_ASSERT (dp);
+
+		if (dp->deleted_filter == NULL) {
+			char *string_filter = slapi_ch_strdup("(isdeleted=*)");
+			/* The filter gets freed in windows_agreement_delete() */
+			dp->deleted_filter = slapi_str2filter( string_filter );
+			slapi_ch_free_string(&string_filter);
+		}
+
+		LDAPDebug( LDAP_DEBUG_TRACE, "<= windows_private_get_deleted_filter\n", 0,
0, 0 );
+
+		return dp->deleted_filter;
+}
 
 /* Returns a copy of the Slapi_DN pointer, no need to free it */
 const Slapi_DN* windows_private_get_windows_subtree (const Repl_Agmt *ra)


Index: windows_prot_private.h
===================================================================
RCS file:
/cvs/dirsec/ldapserver/ldap/servers/plugins/replication/windows_prot_private.h,v
retrieving revision 1.7
retrieving revision 1.8
diff -u -r1.7 -r1.8
--- windows_prot_private.h	10 Nov 2006 23:45:17 -0000	1.7
+++ windows_prot_private.h	17 Sep 2007 19:18:30 -0000	1.8
@@ -52,35 +52,6 @@
 #define ACQUIRE_CONSUMER_WAS_UPTODATE 104
 #define ACQUIRE_TRANSIENT_ERROR 105
 
-typedef struct windows_private_repl_protocol
-{
-	void (*delete)(struct windows_private_repl_protocol **);
-	void (*run)(struct windows_private_repl_protocol *);
-	int (*stop)(struct windows_private_repl_protocol *);
-	int (*status)(struct windows_private_repl_protocol *);
-	void (*notify_update)(struct windows_private_repl_protocol *);
-	void (*notify_agmt_changed)(struct windows_private_repl_protocol *);
-        void (*notify_window_opened)(struct windows_private_repl_protocol *);
-        void (*notify_window_closed)(struct windows_private_repl_protocol *);
-	void (*update_now)(struct windows_private_repl_protocol *);
-	PRLock *lock;
-	PRCondVar *cvar;
-	int stopped;
-	int terminate;
-	PRUint32 eventbits;
-	Repl_Connection *conn;
-	int last_acquire_response_code;
-	Repl_Agmt *agmt;
-	Object *replica_object;
-	void *private;
-    PRBool replica_acquired;
-} Windows_Private_Repl_Protocol;
-
-/*
-extern Windows_Private_Repl_Protocol *Windows_Inc_Protocol_new();
-extern Windows_Private_Repl_Protocol *Windows_Tot_Protocol_new();
-*/
-
 #define PROTOCOL_TERMINATION_NORMAL 301
 #define PROTOCOL_TERMINATION_ABNORMAL 302
 #define PROTOCOL_TERMINATION_NEEDS_TOTAL_UPDATE 303


Index: windows_protocol_util.c
===================================================================
RCS file:
/cvs/dirsec/ldapserver/ldap/servers/plugins/replication/windows_protocol_util.c,v
retrieving revision 1.32
retrieving revision 1.33
diff -u -r1.32 -r1.33
--- windows_protocol_util.c	12 Sep 2007 23:05:25 -0000	1.32
+++ windows_protocol_util.c	17 Sep 2007 19:18:30 -0000	1.33
@@ -65,10 +65,12 @@
 static Slapi_DN* map_dn_group(Slapi_DN *sdn, int map_to, const Slapi_DN *root);
 static void make_mods_from_entries(Slapi_Entry *new_entry, Slapi_Entry *existing_entry,
LDAPMod ***attrs);
 static void windows_map_mods_for_replay(Private_Repl_Protocol *prp,LDAPMod
**original_mods, LDAPMod ***returned_mods, int is_user, char** password);
-static int is_subject_of_agreemeent_local(const Slapi_Entry *local_entry,const Repl_Agmt
*ra);
+static int is_subject_of_agreement_local(const Slapi_Entry *local_entry,const Repl_Agmt
*ra);
 static int windows_create_remote_entry(Private_Repl_Protocol *prp,Slapi_Entry
*original_entry, Slapi_DN *remote_sdn, Slapi_Entry **remote_entry, char** password);
 static int windows_get_local_entry(const Slapi_DN* local_dn,Slapi_Entry **local_entry);
 static int windows_get_local_entry_by_uniqueid(Private_Repl_Protocol *prp,const char*
uniqueid,Slapi_Entry **local_entry);
+static int windows_get_local_tombstone_by_uniqueid(Private_Repl_Protocol *prp,const char*
uniqueid,Slapi_Entry **local_entry);
+static int windows_search_local_entry_by_uniqueid(Private_Repl_Protocol *prp, const char
*uniqueid, char ** attrs, Slapi_Entry **ret_entry, int tombstone, void *
component_identity);
 static int map_entry_dn_outbound(Slapi_Entry *e, Slapi_DN **dn, Private_Repl_Protocol
*prp, int *missing_entry, int want_guid);
 static char* extract_ntuserdomainid_from_entry(Slapi_Entry *e);
 static char* extract_container(const Slapi_DN *entry_dn, const Slapi_DN *suffix_dn);
@@ -76,7 +78,7 @@
 static int windows_get_remote_tombstone(Private_Repl_Protocol *prp, const Slapi_DN*
remote_dn,Slapi_Entry **remote_entry);
 static int windows_reanimate_tombstone(Private_Repl_Protocol *prp, const Slapi_DN*
tombstone_dn, const char* new_dn);
 static const char* op2string (int op);
-static int is_subject_of_agreemeent_remote(Slapi_Entry *e, const Repl_Agmt *ra);
+static int is_subject_of_agreement_remote(Slapi_Entry *e, const Repl_Agmt *ra);
 static int map_entry_dn_inbound(Slapi_Entry *e, Slapi_DN **dn, const Repl_Agmt *ra);
 static int windows_update_remote_entry(Private_Repl_Protocol *prp,Slapi_Entry
*remote_entry,Slapi_Entry *local_entry);
 static int is_guid_dn(Slapi_DN *remote_dn);
@@ -405,7 +407,7 @@
 				int missing_entry = 0;
 				Slapi_DN *remote_dn = NULL;
 				/* Now map the DN */
-				is_ours = is_subject_of_agreemeent_local(local_entry,prp->agmt);
+				is_ours = is_subject_of_agreement_local(local_entry,prp->agmt);
 				if (is_ours)
 				{
 					map_entry_dn_outbound(local_entry,&remote_dn,prp,&missing_entry, 0 /*
don't want GUID form here */);
@@ -447,7 +449,7 @@
 			retval = windows_get_remote_entry(prp,original_dn,&remote_entry);
 			if (remote_entry && 0 == retval)
 			{
-				is_ours = is_subject_of_agreemeent_remote(remote_entry,prp->agmt);
+				is_ours = is_subject_of_agreement_remote(remote_entry,prp->agmt);
 				if (is_ours)
 				{
 					retval = map_entry_dn_inbound(remote_entry,&local_dn,prp->agmt);	
@@ -1121,10 +1123,16 @@
 	local_dn = slapi_sdn_new_dn_byref( op->target_address.dn );
 
 	/* Since we have the target uniqueid in the op structure, let's
-	 * fetch the local entry here using it.
+	 * fetch the local entry here using it. We do not want to search
+	 * across tombstone entries unless we are dealing with a delete
+	 * operation here since searching across tombstones can be very
+	 * inefficient as the tombstones build up.
 	 */
-	
-	rc = windows_get_local_entry_by_uniqueid(prp, op->target_address.uniqueid,
&local_entry);
+	if (op->operation_type != SLAPI_OPERATION_DELETE) {
+		rc = windows_get_local_entry_by_uniqueid(prp, op->target_address.uniqueid,
&local_entry);
+	} else {
+		rc = windows_get_local_tombstone_by_uniqueid(prp, op->target_address.uniqueid,
&local_entry);
+	}
 
 	if (rc) 
 	{
@@ -1135,7 +1143,7 @@
 		goto error;
 	}
 
-	is_ours = is_subject_of_agreemeent_local(local_entry, prp->agmt);
+	is_ours = is_subject_of_agreement_local(local_entry, prp->agmt);
 	windows_is_local_entry_user_or_group(local_entry,&is_user,&is_group);
 
 	slapi_log_error(SLAPI_LOG_REPL, repl_plugin_name,
@@ -1839,25 +1847,15 @@
 
 /* Is this entry a tombstone ? */
 static int 
-is_tombstone(Slapi_Entry *e)
+is_tombstone(Private_Repl_Protocol *prp, Slapi_Entry *e)
 {
 	int retval = 0;
 
-	char *string_deleted = slapi_ch_strdup("(isdeleted=*)");
-
-	/* DBDB: we should allocate these filters once and keep them around for better
performance */
-	Slapi_Filter *filter_deleted = slapi_str2filter( string_deleted );
-	
-    slapi_ch_free_string(&string_deleted);
-	/* DBDB: this should be one filter, the code originally tested separately and hasn't
been fixed yet */
-	if ( (slapi_filter_test_simple( e, filter_deleted ) == 0) )
+	if ( (slapi_filter_test_simple( e,
(Slapi_Filter*)windows_private_get_deleted_filter(prp->agmt) ) == 0) )
 	{
 		retval = 1;
 	}
 
-	slapi_filter_free(filter_deleted,1);
-	filter_deleted = NULL;
-
 	return retval;
 }
 
@@ -2724,7 +2722,7 @@
  * and does it have the right attribute values for sync ?) 
  */
 static int 
-is_subject_of_agreemeent_local(const Slapi_Entry *local_entry, const Repl_Agmt *ra)
+is_subject_of_agreement_local(const Slapi_Entry *local_entry, const Repl_Agmt *ra)
 {
 	int retval = 0;
 	int is_in_subtree = 0;
@@ -2741,23 +2739,16 @@
 	{
 		/* Next test for the correct kind of entry */
 		if (local_entry) {
-			/* DBDB: we should allocate these filters once and keep them around for better
performance */
-			char *string_filter =
slapi_ch_strdup("(&(|(objectclass=ntuser)(objectclass=ntgroup))(ntUserDomainId=*))");
-			Slapi_Filter *filter = slapi_str2filter( string_filter );
-			
-            slapi_ch_free_string(&string_filter);
-			if (slapi_filter_test_simple( (Slapi_Entry*)local_entry, filter ) == 0)
+			if (slapi_filter_test_simple( (Slapi_Entry*)local_entry,
+					(Slapi_Filter*)windows_private_get_directory_filter(ra)) == 0)
 			{
 				retval = 1;
 			}
-
-			slapi_filter_free(filter,1);
-			filter = NULL;
 		} else 
 		{
 			/* Error: couldn't find the entry */
 			slapi_log_error(SLAPI_LOG_FATAL, windows_repl_plugin_name,
-				"failed to find entry in is_subject_of_agreemeent_local: %d\n", retval);
+				"failed to find entry in is_subject_of_agreement_local: %d\n", retval);
 			retval = 0;
 		}
 	}
@@ -2767,7 +2758,7 @@
 
 /* Tests if the entry is subject to our agreement (i.e. is it in the sync'ed subtree
in AD and either a user or a group ?) */
 static int 
-is_subject_of_agreemeent_remote(Slapi_Entry *e, const Repl_Agmt *ra)
+is_subject_of_agreement_remote(Slapi_Entry *e, const Repl_Agmt *ra)
 {
 	int retval = 0;
 	int is_in_subtree = 0;
@@ -3342,7 +3333,7 @@
 	int missing_entry = 0;
 	const Slapi_DN *local_dn = slapi_entry_get_sdn_const(e);
 	/* First check if the entry is for us */
-	is_ours = is_subject_of_agreemeent_local(e, prp->agmt);
+	is_ours = is_subject_of_agreement_local(e, prp->agmt);
 	slapi_log_error(SLAPI_LOG_REPL, repl_plugin_name,
 		"%s: windows_process_total_entry: Looking dn=\"%s\" (%s)\n",
 		agmt_get_long_name(prp->agmt), slapi_sdn_get_dn(slapi_entry_get_sdn_const(e)),
is_ours ? "ours" : "not ours");
@@ -3366,8 +3357,8 @@
 	return retval;
 }
 
-int
-windows_search_local_entry_by_uniqueid(Private_Repl_Protocol *prp, const char *uniqueid,
char ** attrs, Slapi_Entry **ret_entry , void * component_identity)
+static int
+windows_search_local_entry_by_uniqueid(Private_Repl_Protocol *prp, const char *uniqueid,
char ** attrs, Slapi_Entry **ret_entry, int tombstone, void * component_identity)
 {
     Slapi_Entry **entries = NULL;
     Slapi_PBlock *int_search_pb = NULL;
@@ -3377,7 +3368,15 @@
     
     *ret_entry = NULL;
 	local_subtree = windows_private_get_directory_subtree(prp->agmt);
-	filter_string =
PR_smprintf("(&(|(objectclass=*)(objectclass=ldapsubentry)(objectclass=nsTombstone))(nsUniqueid=%s))",uniqueid);
+
+	/* Searching for tombstones can be expensive, so the caller needs to specify if
+	 * we should search for a tombstone entry, or a normal entry. */
+	if (tombstone) {
+		filter_string =
PR_smprintf("(&(objectclass=nsTombstone)(nsUniqueid=%s))", uniqueid);
+	} else {
+		filter_string =
PR_smprintf("(&(|(objectclass=*)(objectclass=ldapsubentry))(nsUniqueid=%s))",uniqueid);
+	}
+
     int_search_pb = slapi_pblock_new ();
 	slapi_search_internal_set_pb ( int_search_pb,  slapi_sdn_get_dn(local_subtree),
LDAP_SCOPE_SUBTREE, filter_string,
 								   attrs ,
@@ -3412,7 +3411,7 @@
 {
 	int retval = ENTRY_NOTFOUND;
 	Slapi_Entry *new_entry = NULL;
-	windows_search_local_entry_by_uniqueid( prp, uniqueid, NULL, &new_entry,
+	windows_search_local_entry_by_uniqueid( prp, uniqueid, NULL, &new_entry, 0, /*
Don't search tombstones */
 			repl_get_plugin_identity (PLUGIN_MULTIMASTER_REPLICATION));
 	if (new_entry) 
 	{
@@ -3423,6 +3422,21 @@
 }
 
 static int
+windows_get_local_tombstone_by_uniqueid(Private_Repl_Protocol *prp,const char*
uniqueid,Slapi_Entry **local_entry)
+{
+	int retval = ENTRY_NOTFOUND;
+	Slapi_Entry *new_entry = NULL;
+	windows_search_local_entry_by_uniqueid( prp, uniqueid, NULL, &new_entry, 1, /*
Search for tombstones */
+			repl_get_plugin_identity (PLUGIN_MULTIMASTER_REPLICATION));
+	if (new_entry)
+	{
+		*local_entry = new_entry;
+		retval = 0;
+	}
+	return retval;
+}
+
+static int
 windows_get_local_entry(const Slapi_DN* local_dn,Slapi_Entry **local_entry)
 {
 	int retval = ENTRY_NOTFOUND;
@@ -3446,7 +3460,7 @@
 	/* deleted users are outside the 'correct container'. 
 	They live in cn=deleted objects, windows_private_get_directory_subtree( prp->agmt)
*/
 
-	if (is_tombstone(e))
+	if (is_tombstone(prp, e))
 	{
 		rc = map_tombstone_dn_inbound(e, &local_sdn, prp->agmt);
 		if ((0 == rc) && local_sdn)
@@ -3461,7 +3475,7 @@
 	} else 
 	{
 		/* Is this entry one we should be interested in ? */
-		if (is_subject_of_agreemeent_remote(e,prp->agmt)) 
+		if (is_subject_of_agreement_remote(e,prp->agmt)) 
 		{
 			/* First make its local DN */
 			rc = map_entry_dn_inbound(e, &local_sdn, prp->agmt);


Index: windowsrepl.h
===================================================================
RCS file: /cvs/dirsec/ldapserver/ldap/servers/plugins/replication/windowsrepl.h,v
retrieving revision 1.12
retrieving revision 1.13
diff -u -r1.12 -r1.13
--- windowsrepl.h	12 Sep 2007 23:05:25 -0000	1.12
+++ windowsrepl.h	17 Sep 2007 19:18:30 -0000	1.13
@@ -68,6 +68,8 @@
 void windows_private_set_isnt4(const Repl_Agmt *ra, int isit);
 int windows_private_get_iswin2k3(const Repl_Agmt *ra);
 void windows_private_set_iswin2k3(const Repl_Agmt *ra, int isit);
+Slapi_Filter* windows_private_get_directory_filter(const Repl_Agmt *ra);
+Slapi_Filter* windows_private_get_deleted_filter(const Repl_Agmt *ra);
 const char* windows_private_get_purl(const Repl_Agmt *ra);
 
 /* in windows_connection.c */