Improve the performance when using enumeration#8395
Improve the performance when using enumeration#8395alexey-tikhonov merged 9 commits intoSSSD:masterfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively improves performance by optimizing logging, removing an unused function, and correcting a condition related to enumeration. The changes are well-aligned with the stated goals of enhancing enumeration performance, especially for large user bases. The addition of a new test case for the general enumeration scenario ensures that the modified logic is adequately covered.
|
Mistype in the commit message: "We must look into de TS cache" |
Fixed. |
|
I think fix is correct in the sense it fixes a bug. But I think logic of In particular, if |
|
Note: Covsan is green so far. |
|
Hm, Looks relevant, but why f44 only... race condition? |
I reran the tests and a different test failed. 😮💨 |
|
And now all the tests passed. There is some instability in F44, but not related to this PR. |
It is very suspicious that it was |
I thought the same until I noticed this test failed once and never again. The second time a completely different test failed. The third time, the latest, none. |
aplopez
force-pushed
the
enumerate
branch
2 times, most recently
from
6d46dbe to
043d60e
Compare
|
@alexey-tikhonov Things have changed since you approved. What do you think now? |
sumit-bose
left a comment
sumit-bose
left a comment
There was a problem hiding this comment.
Hi,
thank you for the updates, I have no further comments, ACK.
bye,
Sumit
Besides using '//', looks good to me. |
|
Coverity is green. |
aplopez
added
Accepted
and removed
coverity
Function sysdb_enumpwent() is not used. It was replaced by sysdb_enumpwent_filter(). Reviewed-by: Alexey Tikhonov <atikhono@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com>
When there are too many users (17,000+) this message can be too long. Limit it to the first 50 characters. Resolves: SSSD#6951 Reviewed-by: Alexey Tikhonov <atikhono@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com>
We must look into the TS cache only when a name is provided. Using the TS cache on an unfiltered enumeration is useless. Resolves: SSSD#6951 Reviewed-by: Alexey Tikhonov <atikhono@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com>
Added a case that was not checked before. It is the case when `attr`, `attr_name` and `addtl_filter` are all `NULL`. Reviewed-by: Alexey Tikhonov <atikhono@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com>
Create the filter to retrieve only the requested entries. Do not create a new filter and search for matches if there is no results from the previous search. The called functions handle this case correctly but why wasting time calling them? Reviewed-by: Alexey Tikhonov <atikhono@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com>
Function cache_req_user_by_filter_lookup() will set or not the recent filter depending on whether data->name.attr is set or not. As mentioned in the comment, it should be done base on whether the refernced attribute is name or not. Reviewed-by: Alexey Tikhonov <atikhono@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com>
The message said that sysdb_enumpwent() had failed, but it was actually sysdb_enumpwent_filter() which failed. Reviewed-by: Alexey Tikhonov <atikhono@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com>
The "name" attribute was not being added to the TS cache, even though that it is part of the DN (ldb doesn't enforce it). Adding this attribute requires that the DB version is incremented for the TS cache to be regenerated with the missing attribute. This made the if-block in sysdb_enumpwent_filter() rather useless. In addition, once this if-block is executed, the fuction leaves without further processing. Reviewed-by: Alexey Tikhonov <atikhono@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com>
Although ts_res.count is set to 0 when sysdb_search_ts_users() return ERR_NO_TS, before using it we make an extra check to verify that the returned code is EOK. Reviewed-by: Alexey Tikhonov <atikhono@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com>
4 participants
This PR includes:
Enumeration, specially when there are 15,000+ users, is slow. This fix helps, but it doesn't work miracles.
In my test environment, the enumeration went from 8 minutes to about 1.
It is important to know that, with such an amount of users, many operations time out. It is necessary to increment the
timeoutin[nss]and for the domain, but also set large values forldap_enumeration_refresh_timeoutandldap_search_timeoutin the domain. I used these values to avoid any timeout (YMMV):