[PATCH] Remove unnecessary additional search in show_deleted LDB module
Garming Sam
garming at catalyst.net.nz
Thu Jun 29 00:27:48 UTC 2017
Hi,
I recently discovered that 10% of the LDB connect time is spent in a
pointless database search checking if the recycling bin is enabled
(which in turn noticeably affected the overall LDAP bind time). It turns
out that every search performs this extra search, however, analyzing the
code, there was no need to check for its presence if neither the
show_deleted control or the show_recycled control is supplied. These
patches remove the search if it is unnecessary. In the case of adds, it
will hit this anyways due to ACL and other objectClass checks but the
cost of an add is much higher.
In removing the search though, I found another subtle bug. The rootDSE
module fails to check the return codes for a number of dynamically
constructed attributes. I can see that there are reasons for this, but
it meant that during provision, the recycling bin enabled checks would
fail but never be picked up (and would assert that it no longer needed
to check the value again). The patches appear to pass a full make test now.
Please review and push.
Cheers,
Garming
-------------- next part --------------
From e3dab6ff77dc458677263ad0e952003a00c88672 Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet at samba.org>
Date: Wed, 28 Jun 2017 12:22:05 +1200
Subject: [PATCH 1/5] dsdb: Add a dummy module to replace show_deleted
This helps when we improve show_deleted in a way that the fake database in samba3sam can not cover
Signed-off-by: Andrew Bartlett <abartlet at samba.org>
Reviewed-by: Garming Sam <garming at catalyst.net.nz>
---
python/samba/tests/samba3sam.py | 2 +-
source4/dsdb/samdb/ldb_modules/samba3sam.c | 35 ++++++++++++++++++++++++++++++
2 files changed, 36 insertions(+), 1 deletion(-)
diff --git a/python/samba/tests/samba3sam.py b/python/samba/tests/samba3sam.py
index 3a189e0..929523b 100644
--- a/python/samba/tests/samba3sam.py
+++ b/python/samba/tests/samba3sam.py
@@ -53,7 +53,7 @@ class MapBaseTestCase(TestCaseInTempDir):
"@TO": "sambaDomainName=TESTS," + s3.basedn})
ldb.add({"dn": "@MODULES",
- "@LIST": "rootdse,paged_results,server_sort,asq,samldb,password_hash,operational,objectguid,rdn_name,samba3sam,samba3sid,show_deleted,dsdb_flags_ignore,partition"})
+ "@LIST": "rootdse,paged_results,server_sort,asq,samldb,password_hash,operational,objectguid,rdn_name,samba3sam,samba3sid,show_deleted_ignore,dsdb_flags_ignore,partition"})
ldb.add({"dn": "@PARTITION",
"partition": ["%s" % (s4.basedn_casefold),
diff --git a/source4/dsdb/samdb/ldb_modules/samba3sam.c b/source4/dsdb/samdb/ldb_modules/samba3sam.c
index e9830c9..8820351 100644
--- a/source4/dsdb/samdb/ldb_modules/samba3sam.c
+++ b/source4/dsdb/samdb/ldb_modules/samba3sam.c
@@ -934,8 +934,43 @@ static const struct ldb_module_ops ldb_samba3sam_module_ops = {
.init_context = samba3sam_init,
};
+
+/* A dummy module to help the samba3sam tests */
+static int show_deleted_ignore_search(struct ldb_module *module, struct ldb_request *req)
+{
+ struct ldb_control *show_del, *show_rec;
+
+ /* check if there's a show deleted control */
+ show_del = ldb_request_get_control(req, LDB_CONTROL_SHOW_DELETED_OID);
+ /* check if there's a show recycled control */
+ show_rec = ldb_request_get_control(req, LDB_CONTROL_SHOW_RECYCLED_OID);
+
+ /* mark the controls as done */
+ if (show_del != NULL) {
+ show_del->critical = 0;
+ }
+ if (show_rec != NULL) {
+ show_rec->critical = 0;
+ }
+
+ /* perform the search */
+ return ldb_next_request(module, req);
+}
+
+static const struct ldb_module_ops ldb_show_deleted_module_ops = {
+ .name = "show_deleted_ignore",
+ .search = show_deleted_ignore_search
+};
+
int ldb_samba3sam_module_init(const char *version)
{
+ int ret;
+
LDB_MODULE_CHECK_VERSION(version);
+ ret = ldb_register_module(&ldb_show_deleted_module_ops);
+ if (ret != LDB_SUCCESS) {
+ return ret;
+ }
+
return ldb_register_module(&ldb_samba3sam_module_ops);
}
--
1.9.1
From ac0e193df24a675aef1288808aa17ffe23a8b56d Mon Sep 17 00:00:00 2001
From: Garming Sam <garming at catalyst.net.nz>
Date: Tue, 27 Jun 2017 13:02:49 +1200
Subject: [PATCH 2/5] show-deleted: Do not indicate an error if an object is
missing.
This happens during provision, however due to the fact that the first
search in the rootDSE init does not check return codes, this was done
implicitly (and coincidentally).
Signed-off-by: Garming Sam <garming at catalyst.net.nz>
---
source4/dsdb/samdb/ldb_modules/show_deleted.c | 13 ++++++++++++-
source4/dsdb/samdb/ldb_modules/util.c | 4 ++--
2 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/source4/dsdb/samdb/ldb_modules/show_deleted.c b/source4/dsdb/samdb/ldb_modules/show_deleted.c
index 6b5fdaa..156a98a 100644
--- a/source4/dsdb/samdb/ldb_modules/show_deleted.c
+++ b/source4/dsdb/samdb/ldb_modules/show_deleted.c
@@ -62,10 +62,21 @@ static int show_deleted_search(struct ldb_module *module, struct ldb_request *re
/* note that state may be NULL during initialisation */
if (state != NULL && state->need_refresh) {
+ /* Do not move this assignment, it can cause recursion loops! */
state->need_refresh = false;
ret = dsdb_recyclebin_enabled(module, &state->recycle_bin_enabled);
if (ret != LDB_SUCCESS) {
- return ret;
+ state->recycle_bin_enabled = false;
+ /*
+ * We can fail to find the feature object
+ * during provision. Ignore any such error and
+ * assume the recycle bin cannot be enabled at
+ * this point in time.
+ */
+ if (ret != LDB_ERR_NO_SUCH_OBJECT) {
+ state->need_refresh = true;
+ return LDB_ERR_UNWILLING_TO_PERFORM;
+ }
}
}
diff --git a/source4/dsdb/samdb/ldb_modules/util.c b/source4/dsdb/samdb/ldb_modules/util.c
index 1d3dab0..36d35b7 100644
--- a/source4/dsdb/samdb/ldb_modules/util.c
+++ b/source4/dsdb/samdb/ldb_modules/util.c
@@ -719,7 +719,7 @@ int dsdb_check_optional_feature(struct ldb_module *module, struct GUID op_featur
"Could not find the feature object - dn: %s\n",
ldb_dn_get_linearized(feature_dn));
talloc_free(tmp_ctx);
- return LDB_ERR_OPERATIONS_ERROR;
+ return LDB_ERR_NO_SUCH_OBJECT;
}
if (res->msgs[0]->num_elements > 0) {
const char *attrs2[] = {"msDS-OptionalFeatureGUID", NULL};
@@ -1055,7 +1055,7 @@ int dsdb_recyclebin_enabled(struct ldb_module *module, bool *enabled)
ret = dsdb_check_optional_feature(module, recyclebin_guid, enabled);
if (ret != LDB_SUCCESS) {
ldb_asprintf_errstring(ldb, "Could not verify if Recycle Bin is enabled \n");
- return LDB_ERR_UNWILLING_TO_PERFORM;
+ return ret;
}
return LDB_SUCCESS;
--
1.9.1
From 358de9ba94e6c9b372af7b7a54439ffe5fe34831 Mon Sep 17 00:00:00 2001
From: Garming Sam <garming at catalyst.net.nz>
Date: Fri, 23 Jun 2017 12:18:35 +1200
Subject: [PATCH 3/5] show-deleted: Remove an unnecessary search during connect
This is only required if you supply SHOW_RECYCLED or SHOW_DELETED. Note
that any add does trigger this (through callbacks in the modules in acl,
objectclass etc.).
Signed-off-by: Garming Sam <garming at catalyst.net.nz>
---
source4/dsdb/samdb/ldb_modules/show_deleted.c | 94 ++++++++++++++-------------
1 file changed, 48 insertions(+), 46 deletions(-)
diff --git a/source4/dsdb/samdb/ldb_modules/show_deleted.c b/source4/dsdb/samdb/ldb_modules/show_deleted.c
index 156a98a..fba71bd 100644
--- a/source4/dsdb/samdb/ldb_modules/show_deleted.c
+++ b/source4/dsdb/samdb/ldb_modules/show_deleted.c
@@ -58,28 +58,6 @@ static int show_deleted_search(struct ldb_module *module, struct ldb_request *re
ldb = ldb_module_get_ctx(module);
- state = talloc_get_type(ldb_module_get_private(module), struct show_deleted_state);
-
- /* note that state may be NULL during initialisation */
- if (state != NULL && state->need_refresh) {
- /* Do not move this assignment, it can cause recursion loops! */
- state->need_refresh = false;
- ret = dsdb_recyclebin_enabled(module, &state->recycle_bin_enabled);
- if (ret != LDB_SUCCESS) {
- state->recycle_bin_enabled = false;
- /*
- * We can fail to find the feature object
- * during provision. Ignore any such error and
- * assume the recycle bin cannot be enabled at
- * this point in time.
- */
- if (ret != LDB_ERR_NO_SUCH_OBJECT) {
- state->need_refresh = true;
- return LDB_ERR_UNWILLING_TO_PERFORM;
- }
- }
- }
-
/* This is the logic from MS-ADTS 3.1.1.3.4.1.14 that
determines if objects are visible
@@ -100,34 +78,58 @@ static int show_deleted_search(struct ldb_module *module, struct ldb_request *re
/* check if there's a show recycled control */
show_rec = ldb_request_get_control(req, LDB_CONTROL_SHOW_RECYCLED_OID);
-
- if (state == NULL || !state->recycle_bin_enabled) {
- /* when recycle bin is not enabled, then all we look
- at is the isDeleted attribute. We hide objects with this
- attribute set to TRUE when the client has not specified either
- SHOW_DELETED or SHOW_RECYCLED
- */
- if (show_del != NULL || show_rec != NULL) {
- attr_filter = NULL;
- } else {
- attr_filter = "isDeleted";
- }
+ if (show_rec == NULL && show_del == NULL) {
+ attr_filter = "isDeleted";
} else {
- /* the recycle bin is enabled
- */
- if (show_rec != NULL) {
- attr_filter = NULL;
- } else if (show_del != NULL) {
- /* we want deleted but not recycled objects */
- attr_filter = "isRecycled";
- } else {
- /* we don't want deleted or recycled objects,
- * which we get by filtering on isDeleted */
- attr_filter = "isDeleted";
+ state = talloc_get_type(ldb_module_get_private(module), struct show_deleted_state);
+
+ /* note that state may be NULL during initialisation */
+ if (state != NULL && state->need_refresh) {
+ /* Do not move this assignment, it can cause recursion loops! */
+ state->need_refresh = false;
+ ret = dsdb_recyclebin_enabled(module, &state->recycle_bin_enabled);
+ if (ret != LDB_SUCCESS) {
+ state->recycle_bin_enabled = false;
+ /*
+ * We can fail to find the feature object
+ * during provision. Ignore any such error and
+ * assume the recycle bin cannot be enabled at
+ * this point in time.
+ */
+ if (ret != LDB_ERR_NO_SUCH_OBJECT) {
+ state->need_refresh = true;
+ return LDB_ERR_UNWILLING_TO_PERFORM;
+ }
+ }
}
- }
+ if (state == NULL || !state->recycle_bin_enabled) {
+ /* when recycle bin is not enabled, then all we look
+ at is the isDeleted attribute. We hide objects with this
+ attribute set to TRUE when the client has not specified either
+ SHOW_DELETED or SHOW_RECYCLED
+ */
+ if (show_del != NULL || show_rec != NULL) {
+ attr_filter = NULL;
+ } else {
+ attr_filter = "isDeleted";
+ }
+ } else {
+ /* the recycle bin is enabled
+ */
+ if (show_rec != NULL) {
+ attr_filter = NULL;
+ } else if (show_del != NULL) {
+ /* we want deleted but not recycled objects */
+ attr_filter = "isRecycled";
+ } else {
+ /* we don't want deleted or recycled objects,
+ * which we get by filtering on isDeleted */
+ attr_filter = "isDeleted";
+ }
+ }
+ }
if (attr_filter != NULL) {
new_tree = talloc(req, struct ldb_parse_tree);
if (!new_tree) {
--
1.9.1
From 2cfb30f548319f31a948a48c5b3ebcdcaf6f9459 Mon Sep 17 00:00:00 2001
From: Garming Sam <garming at catalyst.net.nz>
Date: Fri, 23 Jun 2017 12:35:56 +1200
Subject: [PATCH 4/5] show-deleted: Simplify the code to require as little
logic as needed
Signed-off-by: Garming Sam <garming at catalyst.net.nz>
---
source4/dsdb/samdb/ldb_modules/show_deleted.c | 38 +++++++++++----------------
1 file changed, 15 insertions(+), 23 deletions(-)
diff --git a/source4/dsdb/samdb/ldb_modules/show_deleted.c b/source4/dsdb/samdb/ldb_modules/show_deleted.c
index fba71bd..aa11ebe 100644
--- a/source4/dsdb/samdb/ldb_modules/show_deleted.c
+++ b/source4/dsdb/samdb/ldb_modules/show_deleted.c
@@ -78,12 +78,20 @@ static int show_deleted_search(struct ldb_module *module, struct ldb_request *re
/* check if there's a show recycled control */
show_rec = ldb_request_get_control(req, LDB_CONTROL_SHOW_RECYCLED_OID);
+ /*
+ * When recycle bin is not enabled, then all we look
+ * at is the isDeleted attribute. We hide objects with this
+ * attribute set to TRUE when the client has not specified either
+ * SHOW_DELETED or SHOW_RECYCLED
+ */
if (show_rec == NULL && show_del == NULL) {
+ /* We don't want deleted or recycled objects,
+ * which we get by filtering on isDeleted */
attr_filter = "isDeleted";
} else {
state = talloc_get_type(ldb_module_get_private(module), struct show_deleted_state);
- /* note that state may be NULL during initialisation */
+ /* Note that state may be NULL during initialisation */
if (state != NULL && state->need_refresh) {
/* Do not move this assignment, it can cause recursion loops! */
state->need_refresh = false;
@@ -103,33 +111,17 @@ static int show_deleted_search(struct ldb_module *module, struct ldb_request *re
}
}
- if (state == NULL || !state->recycle_bin_enabled) {
- /* when recycle bin is not enabled, then all we look
- at is the isDeleted attribute. We hide objects with this
- attribute set to TRUE when the client has not specified either
- SHOW_DELETED or SHOW_RECYCLED
- */
- if (show_del != NULL || show_rec != NULL) {
- attr_filter = NULL;
- } else {
- attr_filter = "isDeleted";
- }
- } else {
- /* the recycle bin is enabled
+ if (state != NULL && state->recycle_bin_enabled) {
+ /*
+ * The recycle bin is enabled, so we want deleted not
+ * recycled.
*/
- if (show_rec != NULL) {
- attr_filter = NULL;
- } else if (show_del != NULL) {
- /* we want deleted but not recycled objects */
+ if (show_rec == NULL) {
attr_filter = "isRecycled";
- } else {
- /* we don't want deleted or recycled objects,
- * which we get by filtering on isDeleted */
- attr_filter = "isDeleted";
}
}
-
}
+
if (attr_filter != NULL) {
new_tree = talloc(req, struct ldb_parse_tree);
if (!new_tree) {
--
1.9.1
From 6a24e943d80b45edff532cfcf0f41cc4b2d0b710 Mon Sep 17 00:00:00 2001
From: Garming Sam <garming at catalyst.net.nz>
Date: Fri, 23 Jun 2017 12:37:01 +1200
Subject: [PATCH 5/5] show-deleted: Rename attr_filter to exclude_filter for
clarity
Signed-off-by: Garming Sam <garming at catalyst.net.nz>
---
source4/dsdb/samdb/ldb_modules/show_deleted.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/source4/dsdb/samdb/ldb_modules/show_deleted.c b/source4/dsdb/samdb/ldb_modules/show_deleted.c
index aa11ebe..e3dcad5 100644
--- a/source4/dsdb/samdb/ldb_modules/show_deleted.c
+++ b/source4/dsdb/samdb/ldb_modules/show_deleted.c
@@ -49,7 +49,7 @@ static int show_deleted_search(struct ldb_module *module, struct ldb_request *re
struct ldb_parse_tree *new_tree = req->op.search.tree;
struct show_deleted_state *state;
int ret;
- const char *attr_filter = NULL;
+ const char *exclude_filter = NULL;
/* do not manipulate our control entries */
if (ldb_dn_is_special(req->op.search.base)) {
@@ -87,7 +87,7 @@ static int show_deleted_search(struct ldb_module *module, struct ldb_request *re
if (show_rec == NULL && show_del == NULL) {
/* We don't want deleted or recycled objects,
* which we get by filtering on isDeleted */
- attr_filter = "isDeleted";
+ exclude_filter = "isDeleted";
} else {
state = talloc_get_type(ldb_module_get_private(module), struct show_deleted_state);
@@ -117,12 +117,12 @@ static int show_deleted_search(struct ldb_module *module, struct ldb_request *re
* recycled.
*/
if (show_rec == NULL) {
- attr_filter = "isRecycled";
+ exclude_filter = "isRecycled";
}
}
}
- if (attr_filter != NULL) {
+ if (exclude_filter != NULL) {
new_tree = talloc(req, struct ldb_parse_tree);
if (!new_tree) {
return ldb_oom(ldb);
@@ -142,7 +142,7 @@ static int show_deleted_search(struct ldb_module *module, struct ldb_request *re
return ldb_oom(ldb);
}
new_tree->u.list.elements[0]->u.isnot.child->operation = LDB_OP_EQUALITY;
- new_tree->u.list.elements[0]->u.isnot.child->u.equality.attr = attr_filter;
+ new_tree->u.list.elements[0]->u.isnot.child->u.equality.attr = exclude_filter;
new_tree->u.list.elements[0]->u.isnot.child->u.equality.value = data_blob_string_const("TRUE");
new_tree->u.list.elements[1] = req->op.search.tree;
}
--
1.9.1
More information about the samba-technical
mailing list