[PATCH] More DRS performance improvements for large databases
Tim Beale
timbeale at catalyst.net.nz
Mon Nov 12 21:09:02 UTC 2018
Here are some more changes that improve DRS performance for large
databases. The main improvements in efficiency are:
1. Try to minimize get_parsed_dns_trusted() calls, as these can be
costly for large groups (in particular, this makes --full-sync much more
efficient).
2. dsdb_get_deleted_objects_dn() was quite costly and we were calling
this unnecessarily for every object replicated.
The rest of the patch-set is basically reworking the replmd code to make
#1 possible.
CI pass: https://gitlab.com/catalyst-samba/samba/pipelines/36223829
Review/feedback appreciated. Thanks.
-------------- next part --------------
From e4550950d6608e4d252b98730a08d9fb29d5406f Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Mon, 12 Nov 2018 10:43:39 +1300
Subject: [PATCH 1/7] replmd: replmd_process_link_attribute() returns type of
change made
In order to share work across related link attribute updates, we need
replmd_process_link_attribute() to let the caller know what actually
changed.
This patch adds an extra return type that'll be used in the next patch.
What we're interested in is: the update was ignored (i.e. it's old news),
a new link attribute was added (because this affects the overall
msg/element memory), and an existing link attribute was modified (due to
how links are actually stored, this includes deleting the link, as in
reality it simply involves setting the existing link to 'inactive').
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 22 ++++++++++++++++++++--
1 file changed, 20 insertions(+), 2 deletions(-)
diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
index cab24be..4c7fb0f 100644
--- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
+++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
@@ -134,6 +134,17 @@ struct replmd_replicated_request {
bool fix_link_sid;
};
+/*
+ * the result of replmd_process_linked_attribute(): either there was no change
+ * (update was ignored), a new link was added (either inactive or active), or
+ * an existing link was modified (active/inactive status may have changed).
+ */
+typedef enum {
+ LINK_CHANGE_NONE,
+ LINK_CHANGE_ADDED,
+ LINK_CHANGE_MODIFIED,
+} replmd_link_changed;
+
static int replmd_replicated_apply_merge(struct replmd_replicated_request *ar);
static int replmd_delete_internals(struct ldb_module *module, struct ldb_request *req, bool re_delete);
static int replmd_check_upgrade_links(struct ldb_context *ldb,
@@ -7922,7 +7933,8 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
struct ldb_message *msg,
const struct dsdb_attribute *attr,
struct la_entry *la_entry,
- struct ldb_request *parent)
+ struct ldb_request *parent,
+ replmd_link_changed *change)
{
struct drsuapi_DsReplicaLinkedAttribute *la = la_entry->la;
struct ldb_context *ldb = ldb_module_get_ctx(module);
@@ -7941,6 +7953,8 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
bool add_as_inactive = false;
WERROR status;
+ *change = LINK_CHANGE_NONE;
+
/* the value blob for the attribute holds the target object DN */
status = dsdb_dn_la_from_blob(ldb, attr, schema, mem_ctx,
la->value.blob, &dsdb_dn);
@@ -8044,6 +8058,7 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
val_to_update = pdn->v;
old_dsdb_dn = pdn->dsdb_dn;
+ *change = LINK_CHANGE_MODIFIED;
} else {
unsigned offset;
@@ -8084,6 +8099,7 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
val_to_update = &old_el->values[offset];
old_dsdb_dn = NULL;
+ *change = LINK_CHANGE_ADDED;
}
/* set the link attribute's value to the info that was received */
@@ -8207,6 +8223,7 @@ static int replmd_process_la_group(struct ldb_module *module,
enum deletion_state deletion_state = OBJECT_NOT_DELETED;
struct ldb_context *ldb = ldb_module_get_ctx(module);
const struct dsdb_attribute *attr = NULL;
+ replmd_link_changed change_type;
/*
* get the attribute being modified and the search result for the
@@ -8259,7 +8276,8 @@ static int replmd_process_la_group(struct ldb_module *module,
DLIST_REMOVE(la_group->la_entries, la);
ret = replmd_process_linked_attribute(module, tmp_ctx,
replmd_private,
- msg, attr, la, NULL);
+ msg, attr, la, NULL,
+ &change_type);
if (ret != LDB_SUCCESS) {
replmd_txn_cleanup(replmd_private);
return ret;
--
2.7.4
From 0bed74f901bb5aa4bb256d17ea7e399542657650 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Mon, 12 Nov 2018 11:00:52 +1300
Subject: [PATCH 2/7] replmd: Only modify the object if it actually changed
Commit 775054afbe1512 reworked replmd_process_link_attribute() so that
we batch together DB operations for the same source object. However, it
was possible that the object had not actually changed at all, e.g.
- link was already processed by critical-objects-only during join, or
- we were doing a full-sync and processing info that was already
up-to-date in our DB.
In these cases we modified the object anyway, even though nothing had
changed. This patch fixes it up, so we check that the object has
actually changed before modifying the DB.
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
index 4c7fb0f..d2a1703 100644
--- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
+++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
@@ -8224,6 +8224,7 @@ static int replmd_process_la_group(struct ldb_module *module,
struct ldb_context *ldb = ldb_module_get_ctx(module);
const struct dsdb_attribute *attr = NULL;
replmd_link_changed change_type;
+ uint32_t num_changes = 0;
/*
* get the attribute being modified and the search result for the
@@ -8283,6 +8284,10 @@ static int replmd_process_la_group(struct ldb_module *module,
return ret;
}
+ if (change_type != LINK_CHANGE_NONE) {
+ num_changes++;
+ }
+
if ((++replmd_private->num_processed % 8192) == 0) {
DBG_NOTICE("Processed %u/%u linked attributes\n",
replmd_private->num_processed,
@@ -8290,6 +8295,15 @@ static int replmd_process_la_group(struct ldb_module *module,
}
}
+ /*
+ * it's possible we're already up-to-date and so don't need to modify
+ * the object at all (e.g. doing a 'drs replicate --full-sync')
+ */
+ if (num_changes == 0) {
+ TALLOC_FREE(tmp_ctx);
+ return LDB_SUCCESS;
+ }
+
/* apply the link changes to the source object */
ret = linked_attr_modify(module, msg, NULL);
if (ret != LDB_SUCCESS) {
--
2.7.4
From 75b71a0247f9846d9236ba326aaaf479c8db20eb Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Mon, 12 Nov 2018 11:13:28 +1300
Subject: [PATCH 3/7] replmd: Move where we update the usnChanged/whenChanged
Move this closer to where the source object actually gets modified.
The main reason to do this is that adding fields can cause the
msg->elements to be reallocated, which will invalidate all the
old_el and pdn_list pointers which are derived from the msg.
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 39 ++++++++++++++-----------
1 file changed, 22 insertions(+), 17 deletions(-)
diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
index d2a1703..3a49501 100644
--- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
+++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
@@ -7943,7 +7943,6 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
struct dsdb_dn *dsdb_dn = NULL;
uint64_t seq_num = 0;
struct ldb_message_element *old_el;
- time_t t = time(NULL);
struct parsed_dn *pdn_list, *pdn, *next;
struct GUID guid = GUID_zero();
bool active = (la->flags & DRSUAPI_DS_LINKED_ATTRIBUTE_FLAG_ACTIVE)?true:false;
@@ -8138,22 +8137,6 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
}
}
- /* we only change whenChanged and uSNChanged if the seq_num
- has changed */
- ldb_msg_remove_attr(msg, "whenChanged");
- ldb_msg_remove_attr(msg, "uSNChanged");
- ret = add_time_element(msg, "whenChanged", t);
- if (ret != LDB_SUCCESS) {
- ldb_operr(ldb);
- return ret;
- }
-
- ret = add_uint64_element(ldb, msg, "uSNChanged", seq_num);
- if (ret != LDB_SUCCESS) {
- ldb_operr(ldb);
- return ret;
- }
-
old_el = ldb_msg_find_element(msg, attr->lDAPDisplayName);
if (old_el == NULL) {
return ldb_operr(ldb);
@@ -8225,6 +8208,8 @@ static int replmd_process_la_group(struct ldb_module *module,
const struct dsdb_attribute *attr = NULL;
replmd_link_changed change_type;
uint32_t num_changes = 0;
+ time_t t;
+ uint64_t seq_num = 0;
/*
* get the attribute being modified and the search result for the
@@ -8304,6 +8289,26 @@ static int replmd_process_la_group(struct ldb_module *module,
return LDB_SUCCESS;
}
+ /* update whenChanged/uSNChanged as the object has changed */
+ t = time(NULL);
+ ret = ldb_sequence_number(ldb, LDB_SEQ_HIGHEST_SEQ,
+ &seq_num);
+ if (ret != LDB_SUCCESS) {
+ return ret;
+ }
+
+ ret = add_time_element(msg, "whenChanged", t);
+ if (ret != LDB_SUCCESS) {
+ ldb_operr(ldb);
+ return ret;
+ }
+
+ ret = add_uint64_element(ldb, msg, "uSNChanged", seq_num);
+ if (ret != LDB_SUCCESS) {
+ ldb_operr(ldb);
+ return ret;
+ }
+
/* apply the link changes to the source object */
ret = linked_attr_modify(module, msg, NULL);
if (ret != LDB_SUCCESS) {
--
2.7.4
From f380dc09fb92fe91282a94c921fbe705885009ae Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Mon, 12 Nov 2018 11:21:36 +1300
Subject: [PATCH 4/7] replmd: Remove some redundant code
At first glance, this code seemed completely unnecessary. However, it
was added (by commit f6bc4c08b19f5615) for a valid reason: adding the
whenChanged/uSNChanged attributes to the message can cause msg->elements
to be reallocated, which means the old_el pointer (which points to
msg->elements memory) can be out of date.
whenChanged/uSNChanged now get added to the msg last, just before the DB
modify operation. So old_el can no longer become out of date within
replmd_process_link_attribute(), so re-fetching it is now redundant.
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
index 3a49501..22aa042 100644
--- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
+++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
@@ -8137,11 +8137,6 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
}
}
- old_el = ldb_msg_find_element(msg, attr->lDAPDisplayName);
- if (old_el == NULL) {
- return ldb_operr(ldb);
- }
-
ret = dsdb_check_single_valued_link(attr, old_el);
if (ret != LDB_SUCCESS) {
return ret;
--
2.7.4
From b669981c6b7ba78b55026ae5ed66c46153ce0110 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Mon, 12 Nov 2018 12:00:47 +1300
Subject: [PATCH 5/7] replmd: Pass old_el into
replmd_process_linked_attribute()
We should only need to lookup the msg attribute once per source object.
The old_el->values may change due to link-processing, but old_el itself
should not.
This is not aimed at improving performance, but we need to change how
old_el is used before we can change pdn_list (which is more costly
processing-wise).
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 28 +++++++++++++------------
1 file changed, 15 insertions(+), 13 deletions(-)
diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
index 22aa042..416c53a 100644
--- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
+++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
@@ -7934,6 +7934,7 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
const struct dsdb_attribute *attr,
struct la_entry *la_entry,
struct ldb_request *parent,
+ struct ldb_message_element *old_el,
replmd_link_changed *change)
{
struct drsuapi_DsReplicaLinkedAttribute *la = la_entry->la;
@@ -7942,7 +7943,6 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
int ret;
struct dsdb_dn *dsdb_dn = NULL;
uint64_t seq_num = 0;
- struct ldb_message_element *old_el;
struct parsed_dn *pdn_list, *pdn, *next;
struct GUID guid = GUID_zero();
bool active = (la->flags & DRSUAPI_DS_LINKED_ATTRIBUTE_FLAG_ACTIVE)?true:false;
@@ -7965,17 +7965,6 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
return LDB_ERR_OPERATIONS_ERROR;
}
- old_el = ldb_msg_find_element(msg, attr->lDAPDisplayName);
- if (old_el == NULL) {
- ret = ldb_msg_add_empty(msg, attr->lDAPDisplayName, LDB_FLAG_MOD_REPLACE, &old_el);
- if (ret != LDB_SUCCESS) {
- ldb_module_oom(module);
- return LDB_ERR_OPERATIONS_ERROR;
- }
- } else {
- old_el->flags = LDB_FLAG_MOD_REPLACE;
- }
-
/* parse the existing links */
ret = get_parsed_dns_trusted(module, replmd_private, mem_ctx, old_el, &pdn_list,
attr->syntax->ldap_oid, parent);
@@ -8201,6 +8190,7 @@ static int replmd_process_la_group(struct ldb_module *module,
enum deletion_state deletion_state = OBJECT_NOT_DELETED;
struct ldb_context *ldb = ldb_module_get_ctx(module);
const struct dsdb_attribute *attr = NULL;
+ struct ldb_message_element *old_el = NULL;
replmd_link_changed change_type;
uint32_t num_changes = 0;
time_t t;
@@ -8251,6 +8241,18 @@ static int replmd_process_la_group(struct ldb_module *module,
ldb_msg_remove_attr(msg, "isDeleted");
ldb_msg_remove_attr(msg, "isRecycled");
+ old_el = ldb_msg_find_element(msg, attr->lDAPDisplayName);
+ if (old_el == NULL) {
+ ret = ldb_msg_add_empty(msg, attr->lDAPDisplayName,
+ LDB_FLAG_MOD_REPLACE, &old_el);
+ if (ret != LDB_SUCCESS) {
+ ldb_module_oom(module);
+ return LDB_ERR_OPERATIONS_ERROR;
+ }
+ } else {
+ old_el->flags = LDB_FLAG_MOD_REPLACE;
+ }
+
/* go through and process the link targets for this source object */
for (la = DLIST_TAIL(la_group->la_entries); la; la=prev) {
prev = DLIST_PREV(la);
@@ -8258,7 +8260,7 @@ static int replmd_process_la_group(struct ldb_module *module,
ret = replmd_process_linked_attribute(module, tmp_ctx,
replmd_private,
msg, attr, la, NULL,
- &change_type);
+ old_el, &change_type);
if (ret != LDB_SUCCESS) {
replmd_txn_cleanup(replmd_private);
return ret;
--
2.7.4
From 79bfdf73ac118a34712489321f38057bdf3fb1ff Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Mon, 12 Nov 2018 12:11:38 +1300
Subject: [PATCH 6/7] replmd: Minimize get_parsed_dns_trusted() calls during
replication
When a group has 10,000+ links, get_parsed_dns_trusted() can be costly
(simply the talloc calls alone are expensive). Instead of re-generating
the pdn_list for every single link attribute, we can change to only
re-generate it when we really need to.
When we add a new link, it reallocates old_el->values, and so we need to
recreate the pdn_list because all the memory pointers will have changed.
However, in the other cases, where we're simply updating the existing
link value (or ignoring the update, if it's already applied), we can
continue using the same pdn_list (rather than re-parsing it again).
This would generally only save time with a full-sync - it won't really
help with the join case (because every link processed results in a
realloc).
On a DB with 5000 users, this makes a full-sync about ~13% faster.
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 36 ++++++++++++++++++-------
1 file changed, 26 insertions(+), 10 deletions(-)
diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
index 416c53a..6b7259d 100644
--- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
+++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
@@ -7935,6 +7935,7 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
struct la_entry *la_entry,
struct ldb_request *parent,
struct ldb_message_element *old_el,
+ struct parsed_dn *pdn_list,
replmd_link_changed *change)
{
struct drsuapi_DsReplicaLinkedAttribute *la = la_entry->la;
@@ -7943,7 +7944,7 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
int ret;
struct dsdb_dn *dsdb_dn = NULL;
uint64_t seq_num = 0;
- struct parsed_dn *pdn_list, *pdn, *next;
+ struct parsed_dn *pdn, *next;
struct GUID guid = GUID_zero();
bool active = (la->flags & DRSUAPI_DS_LINKED_ATTRIBUTE_FLAG_ACTIVE)?true:false;
bool ignore_link;
@@ -7965,14 +7966,6 @@ static int replmd_process_linked_attribute(struct ldb_module *module,
return LDB_ERR_OPERATIONS_ERROR;
}
- /* parse the existing links */
- ret = get_parsed_dns_trusted(module, replmd_private, mem_ctx, old_el, &pdn_list,
- attr->syntax->ldap_oid, parent);
-
- if (ret != LDB_SUCCESS) {
- return ret;
- }
-
ret = replmd_check_target_exists(module, dsdb_dn, la_entry, msg->dn,
true, &guid, &ignore_link);
@@ -8191,6 +8184,7 @@ static int replmd_process_la_group(struct ldb_module *module,
struct ldb_context *ldb = ldb_module_get_ctx(module);
const struct dsdb_attribute *attr = NULL;
struct ldb_message_element *old_el = NULL;
+ struct parsed_dn *pdn_list = NULL;
replmd_link_changed change_type;
uint32_t num_changes = 0;
time_t t;
@@ -8257,15 +8251,37 @@ static int replmd_process_la_group(struct ldb_module *module,
for (la = DLIST_TAIL(la_group->la_entries); la; la=prev) {
prev = DLIST_PREV(la);
DLIST_REMOVE(la_group->la_entries, la);
+
+ /* parse the existing links */
+ if (pdn_list == NULL) {
+ ret = get_parsed_dns_trusted(module, replmd_private,
+ tmp_ctx, old_el,
+ &pdn_list,
+ attr->syntax->ldap_oid,
+ NULL);
+
+ if (ret != LDB_SUCCESS) {
+ return ret;
+ }
+ }
ret = replmd_process_linked_attribute(module, tmp_ctx,
replmd_private,
msg, attr, la, NULL,
- old_el, &change_type);
+ old_el, pdn_list,
+ &change_type);
if (ret != LDB_SUCCESS) {
replmd_txn_cleanup(replmd_private);
return ret;
}
+ /*
+ * Adding a link reallocs memory, and so invalidates all the
+ * pointers in pdn_list. Reparse the PDNs on the next loop
+ */
+ if (change_type == LINK_CHANGE_ADDED) {
+ TALLOC_FREE(pdn_list);
+ }
+
if (change_type != LINK_CHANGE_NONE) {
num_changes++;
}
--
2.7.4
From af2141644bda684b1d7243288f1fac2b00a4c864 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Mon, 12 Nov 2018 15:49:28 +1300
Subject: [PATCH 7/7] replmd: Avoid redundant dsdb_get_deleted_objects_dn()
checks
Quite a bit of time was spent in dsdb_get_deleted_objects_dn()
processing during either a join (~9%) or a full-sync (~28%).
The problem is we're *always* doing the dsdb_get_deleted_objects_dn()
call for each object, regardless of whether it's actually deleted or
not. i.e. we were doing an expensive query and a lot of the time just
ignoring the query result.
If it's not a deleted object we're dealing with, we can just return
early and skip the unnecessary processing.
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
index 6b7259d..73b26c7 100644
--- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
+++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
@@ -6803,9 +6803,18 @@ static int replmd_replicated_apply_isDeleted(struct replmd_replicated_request *a
{
struct ldb_dn *deleted_objects_dn;
struct ldb_message *msg = ar->objs->objects[ar->index_current].msg;
- int ret = dsdb_get_deleted_objects_dn(ldb_module_get_ctx(ar->module), msg, msg->dn,
- &deleted_objects_dn);
- if (ar->isDeleted && (ret != LDB_SUCCESS || ldb_dn_compare(msg->dn, deleted_objects_dn) != 0)) {
+ int ret;
+
+ if (!ar->isDeleted) {
+
+ /* not a deleted object, so nothing to do */
+ ar->index_current++;
+ return replmd_replicated_apply_next(ar);
+ }
+
+ ret = dsdb_get_deleted_objects_dn(ldb_module_get_ctx(ar->module), msg,
+ msg->dn, &deleted_objects_dn);
+ if (ret != LDB_SUCCESS || ldb_dn_compare(msg->dn, deleted_objects_dn) != 0) {
/*
* Do a delete here again, so that if there is
* anything local that conflicts with this
--
2.7.4
More information about the samba-technical
mailing list