memberof: replace O(N²) dedup and diff with hash-based O(N) equivalents#8668
memberof: replace O(N²) dedup and diff with hash-based O(N) equivalents#8668fcami wants to merge 14 commits into
Conversation
Add perf_snap instrumentation (wall time, CPU, block I/O, RSS) and three benchmark tests for the memberof LDB module: - test_sysdb_memberof_store_large_group: store one group with 10000 members, verify memberOf on sampled users and memberuid on the group. - test_sysdb_memberof_replace_large_group: replace with delta=2 (add one member, remove one), verify the swap. - test_sysdb_memberof_store_multi_group: store 12 groups of 10000 shared members sequentially, verify memberOf count on user 0 and memberuid on each group. - test_sysdb_memberof_many_groups_same_users: store 50 groups of 1000 shared members, verify memberOf on user 0 (50 entries) and memberuid on the last group (1000 entries). Signed-off-by: François Cami <contribs@fcami.net> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: François Cami <contribs@fcami.net>
mbof_append_addop() used a linear scan of the pending_ops linked list to detect duplicate (DN, memberOf-DN) pairs, giving O(M^2) behaviour when M operations accumulated. Add a per-operation hash table (dedup_table) to mbof_add_ctx and use it for O(1) duplicate detection. The hash key is the concatenation of the target DN and the memberOf DN. The initial table size is fixed at 1024 buckets regardless of member count; this overallocates for small groups but resizes automatically for large ones. This is a deliberate semantic change: the old code used sss_linearized_dn_match() which compares the name= value portion case-sensitively (its own FIXME questioned this); the new code uses ldb_dn_get_casefold() which is fully case-insensitive, matching standard LDAP DN comparison semantics. Signed-off-by: François Cami <contribs@fcami.net> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: François Cami <contribs@fcami.net>
Replace the nested-loop member diff in the LDB_FLAG_MOD_REPLACE case of mbof_mod_process_membel() with a hash-set based diff. The new implementation inserts all old members into a hash table keyed on ldb_dn_get_casefold(), then walks the new member array calling hash_delete() for each: success means the member is unchanged (skip it), failure means it was newly added (keep it). A final re-walk of the original old array with hash_lookup() identifies removals (entries still in the table were not matched). Avoids hash_keys() (which is O(bucket_count)) by re-walking the original array with hash probes, keeping the total cost O(N). For 10000 members with delta=2, the old code performed ~100M DN comparisons. The new code performs ~30000 hash operations (N inserts + N lookups + N probes). Replace test: 0.39s -> 0.33s. Both O(N^2) bottlenecks are now eliminated. Signed-off-by: François Cami <contribs@fcami.net> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: François Cami <contribs@fcami.net>
Move the transitive memberOf closure computation from the LDB_REPLY_DONE case of mbof_rcmp_grp_callback() into a new standalone function mbof_memberof_compute(). The extracted code performs two phases: 1. Member resolution: for each group, resolves member DN strings to mbof_member pointers via hash lookup against user_table and group_table. 2. Transitive closure: fixpoint iteration over the group list, propagating memberOf from each group to its members via mbof_member_update(). Groups are re-queued (MBOF_GROUP_TO_DO) when their memberOf set changes. The recompute callback now calls mbof_memberof_compute() where the inline code was. No behaviour change. This makes the computation reusable from the scoped-recompute path introduced in a later commit. Signed-off-by: François Cami <contribs@fcami.net> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: François Cami <contribs@fcami.net>
Add update_user_list and update_group_list fields to mbof_rcmp_context. mbof_rcmp_update() now consumes entries from these fields instead of directly from user_list/group_list. The existing call site in mbof_rcmp_grp_callback sets them to the full lists (ctx->user_list, ctx->group_list). No behaviour change. Using context fields rather than function parameters because mbof_rcmp_update is called recursively via the mbof_rcmp_mod_callback chain, which passes only the context. This allows a future scoped-recompute caller to set the update lists to only the affected entries, avoiding modify operations on entries whose memberOf did not change. Signed-off-by: François Cami <contribs@fcami.net> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: François Cami <contribs@fcami.net>
Replace ldb_dn_get_linearized with ldb_dn_get_casefold at all hash key sites in the recompute path: user_table entries, group_table entries, memberofs entries in mbof_member_update(), and the self-exclusion check in mbof_member_iter(). dhash uses case-sensitive strcmp for key equality. The linearized DN form preserves original case, so two DNs differing only in case would be treated as distinct hash entries. ldb_dn_get_casefold returns a canonical casefolded string, making hash lookups semantically equivalent to ldb_dn_compare(). In mbof_memberof_compute(), member DN strings from the DB are now parsed via ldb_dn_from_ldb_val() and casefolded before lookup, matching the new key format. In mbof_rcmp_update(), use hash_values() instead of hash_keys() to emit memberOf attribute values, extracting the linearized DN from the stored mbof_member pointer. This ensures the stored memberOf values remain in linearized format for compatibility with downstream code. Signed-off-by: François Cami <contribs@fcami.net> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: François Cami <contribs@fcami.net>
test_sysdb_memberof_diamond_dedup: creates a diamond group topology where group C has 10 users, groups A and B each contain C, and a parent group contains both A and B. When the parent is stored, memberOf propagation walks two paths to C (parent→A→C and parent→B→C). mbof_append_addop must deduplicate the second traversal. The test verifies: - Each user has exactly 4 memberOf entries (C, A, B, parent) - No duplicate memberOf values exist on any user - memberuid on C and parent each contain all 10 users Signed-off-by: François Cami <contribs@fcami.net> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: François Cami <contribs@fcami.net>
Add missing NULL checks after ldb_dn_get_casefold() at 5 call sites in the recompute path (mbof_rcmp_usr_callback, mbof_rcmp_grp_callback, mbof_memberof_compute, mbof_member_update, mbof_member_iter). The branch's earlier hunks correctly NULL-check, making the inconsistency conspicuous. The mbof_member_iter case is the most critical: a NULL return would feed directly into strcmp inside a hash_iterate callback where error recovery is limited — it now sets MBOF_ITER_ERROR and returns false to abort iteration. Signed-off-by: François Cami <contribs@fcami.net> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: François Cami <contribs@fcami.net>
Add three new tests to exercise memberof module correctness: - test_sysdb_memberof_rebuild: creates a nested group chain, corrupts memberOf/memberuid by bypassing the memberof module via SSSD_UPGRADE_DB, triggers @MEMBEROF-REBUILD, and verifies all attributes are recomputed correctly. - test_sysdb_memberof_restore_group: stores a group with 30 members, re-stores it with 5 dropped, 25 kept, and 5 new, then verifies memberOf is removed from dropped users, unchanged on kept users, and added to new users. - test_sysdb_memberof_mixed_case_rebuild: stores a group with mixed-case member DN strings, corrupts memberOf, triggers @MEMBEROF-REBUILD, and verifies the recompute path correctly handles non-canonical DN casing via ldb_dn_get_casefold(). The rebuild TCase gets tcase_set_timeout(3600) to match the other heavy memberof TCases, since @MEMBEROF-REBUILD scans every entry in the database (including entries from earlier test cases). Signed-off-by: François Cami <contribs@fcami.net> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: François Cami <contribs@fcami.net>
Replace per-iteration talloc_free(valdn) with a single tmp_ctx that collects all ldb_dn objects across the inner loop and is freed once after the loop completes. For groups with thousands of members, this eliminates thousands of individual malloc/free cycles in a performance-critical path. Signed-off-by: François Cami <contribs@fcami.net> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: François Cami <contribs@fcami.net>
hash_enter failure was returning LDB_ERR_OPERATIONS_ERROR (== 1 == true) from a bool callback, telling hash_iterate to keep going instead of aborting. Use the same error pattern (set MBOF_ITER_ERROR, return false) already used for the mbof_add_memuid failure case a few lines later. Signed-off-by: François Cami <contribs@fcami.net> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: François Cami <contribs@fcami.net>
The rebuild test previously only checked memberOf/memberuid counts (el->num_values == N) without verifying the actual DN strings. If the casefold-to-linearized roundtrip in mbof_rcmp_update produced garbled values, or hash_values returned wrong pointers, the counts would still pass. Add ldb_msg_find_val spot-checks: construct expected group DNs via sysdb_group_dn + ldb_dn_get_linearized and assert they appear in the memberOf element for each user and nested group after rebuild. Signed-off-by: François Cami <contribs@fcami.net> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: François Cami <contribs@fcami.net>
The @MEMBEROF-REBUILD recompute path was only tested with simple
topologies (linear chain, flat group). The transitive-closure
fixpoint loop in mbof_memberof_compute has non-trivial convergence
behavior with diamonds: a group marked MBOF_GROUP_DONE gets
re-promoted to MBOF_GROUP_TO_DO via DLIST_PROMOTE when
mbof_member_update discovers a new memberOf.
Add test_sysdb_memberof_rebuild_diamond which builds a diamond
topology (parent -> {A, B} -> C -> 5 users), strips all memberOf
and memberuid via SSSD_UPGRADE_DB bypass, triggers @MEMBEROF-REBUILD,
and verifies:
- each user has exactly 4 memberOf (C, A, B, parent) with no dupes
- group C has 3 memberOf (A, B, parent)
- groups A and B each have 1 memberOf (parent)
- parent has no memberOf
- all 4 groups have correct memberuid counts
- actual DN values match expected group DNs
Signed-off-by: François Cami <contribs@fcami.net>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: François Cami <contribs@fcami.net>
There was a problem hiding this comment.
Code Review
This pull request optimizes the memberof LDB module by replacing O(N) linked-list traversals and O(N*M) nested-loop set differences with hash-table-based lookups and operations. It also improves correctness by using ldb_dn_get_casefold for case-insensitive DN matching. A comprehensive suite of performance and regression tests has been added to verify these changes. Feedback was provided regarding a missing return value check for a hash table insertion in the set-difference logic, which could lead to database inconsistencies upon failure.
| talloc_free(old_set); | ||
| return LDB_ERR_OPERATIONS_ERROR; | ||
| } | ||
| hash_enter(old_set, &hkey, &hval); |
There was a problem hiding this comment.
The return value of hash_enter is not checked here. If the hash table insertion fails (e.g., due to memory exhaustion), the subsequent set-difference logic will be incorrect, potentially leading to inconsistent membership data in the database. All other hash_enter calls in this file correctly check for success.
hret = hash_enter(old_set, &hkey, &hval);
if (hret != HASH_SUCCESS) {
talloc_free(old_set);
return LDB_ERR_OPERATIONS_ERROR;
}The hash_enter call that populates old_set in the REPLACE diff path did not check its return value. On failure (e.g. memory exhaustion), the set-difference logic would operate on an incomplete hash set, potentially producing incorrect added/removed arrays and inconsistent membership data. Add the same hret check and talloc_free(old_set) error path used by all other hash_enter calls in this file. Signed-off-by: François Cami <contribs@fcami.net> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: François Cami <contribs@fcami.net>
Summary
This branch replaces two O(N²) algorithms in the
memberofLDB module with O(N) hash-based equivalents and normalizes DN comparison to useldb_dn_get_casefold(fully case-insensitive), correcting a semantic inconsistency with RFC 4514. The changes target environments with groups containing thousands of users, where the quadratic cost of the original algorithms dominates wall-clock time during cache writes.This is meant as an experimental / thought-provoking research item, not an urgent branch to merge. There are drawbacks to the approach and obviously the possibility of regressions especially with case-sensitiveness has to be kept in mind.
Changes
mbof_append_addop: dedup via hash tableThe previous implementation scanned a singly-linked list on each insertion to detect duplicates, yielding O(M²) comparisons over M insertions and O(M) per append (pointer walk to tail). The replacement uses a
hash_table_tkeyed byldb_dn_get_casefold(entry_dn)for O(1) expected-time duplicate detection, and anadd_list_tailpointer for O(1) append. The hash table and list are kept in sync: everyhash_enterfollows a successful link-in, and everyhash_lookupprecedes it. Space cost is O(M) for the table. The dedup tables are allocated with a fixed initial size of 1024 buckets; a smaller initial size could be considered to lower the allocation cost for operations on small groups.mbof_mod_process_membel: set-difference via hash setDuring
LDB_FLAG_MOD_REPLACEoperations, the module computes which members were added and which were removed by diffing the old and new member lists. The previous implementation used a doubly-nested loop with in-place array compaction: for each of A added entries, a linear scan of up to R removed entries, with shift-left compaction on match — O(A·R) total, O(N²) when A ≈ R ≈ N. The replacement is a three-pass algorithm: (1) build a hash set from the removed array, O(R); (2) probe and delete for each added entry, compacting the added array in place, O(A); (3) filter the removed array against the residual set, O(R). Total: O(A + R). The hash set is sized toremoved->num * 2buckets.mbof_memberof_compute: function extractionThe
@MEMBEROF-REBUILDrecompute path previously had DN resolution and transitive-closure computation inline in theLDB_REPLY_DONEcase ofmbof_rcmp_grp_callback. This has been extracted into a standalone functionmbof_memberof_computethat returnsLDB_ERR_*codes, with the caller handlingldb_module_done. The transitive-closure fixpoint algorithm (relaxation viaDLIST_DEMOTE/DLIST_PROMOTEwith re-promotion on new memberOf discovery) is unchanged.Casefold normalization in the recompute path
All hash table keys in the recompute path (
user_table,group_table,memberofs) have been changed fromldb_dn_get_linearized(case-preserving) toldb_dn_get_casefold(canonical lowercase). Lookups in the member-resolution loop now parse stored DN values vialdb_dn_from_ldb_valand casefold them before probing, rather than using raw byte strings as keys. This ensures correct matching regardless of how the member DN was stored. The output path (mbof_rcmp_update) recovers linearized DN strings vialdb_dn_get_linearized(parent->dn)from thembof_memberpointers stored as hash values (hash_valuesreplacinghash_keys). Separateupdate_user_list/update_group_listpointers decouple the update phase from the compute phase's use ofgroup_list. NULL checks have been added to all 9ldb_dn_get_casefoldcall sites and to theldb_dn_get_linearizedcall in the output loop.mbof_member_iter: error handling fixA
hash_enterfailure inmbof_member_iterpreviously returnedLDB_ERR_OPERATIONS_ERROR(integer value 1) as abool, which evaluated totrueand causedhash_iterateto continue — silently swallowing the error. This is nowmem->status = MBOF_ITER_ERROR; return false;, matching the error pattern used elsewhere in the same function.Semantic change
The dedup in
mbof_append_addoppreviously usedsss_linearized_dn_match, which performsstrcasecmpon the full DN but a case-sensitivestrncmpon thename=value portion. The replacement usesldb_dn_get_casefold, which is fully case-insensitive. This matches the LDAP DN comparison semantics defined in RFC 4514. The original code contained aFIXME: check if this is rightcomment on this comparison.Test plan
test_sysdb_memberof_store_large_group: store a group with 10,000 user members; verify memberOf on every user and memberuid on the grouptest_sysdb_memberof_replace_large_group: replace membership on a 10,000-member group (delta of 2); verify memberOf removed from dropped user, added to new user, and preserved on retained userstest_sysdb_memberof_store_multi_group: store 12 groups of 10,000 users each; verify memberOf counts and memberuid countstest_sysdb_memberof_many_groups_same_users: store 50 groups of 2,000 users each; verify cumulative memberOf scalingtest_sysdb_memberof_diamond_dedup: diamond topology (parent→A,B→C→10 users); verify each user has exactly 4 memberOf with no duplicatestest_sysdb_memberof_mixed_case_dn: store group with mixed-case member DNs, then replace with canonical-case DNs; verify memberOf is matched correctly across the casing changetest_sysdb_memberof_mixed_case_cross_domain: 3 domains (FILES, example.com, linux.example.com) with 3 distinct casing patterns; verify memberOf across all 6 userstest_sysdb_memberof_rebuild: 3-group nested chain; corrupt memberOf and memberuid viaSSSD_UPGRADE_DBbypass; trigger@MEMBEROF-REBUILD; verify counts and DN values vialdb_msg_find_valtest_sysdb_memberof_rebuild_diamond: diamond topology; corrupt all memberOf and memberuid; trigger@MEMBEROF-REBUILD; verify counts, DN values, and no-duplicate invariant on all users and groupstest_sysdb_memberof_restore_group: store group with 30 members, re-store with 5 dropped + 5 added; verify dropped users lose memberOf, kept users retain it, added users gain ittest_sysdb_memberof_mixed_case_rebuild: store group with mixed-case member DNs; corrupt memberOf; trigger@MEMBEROF-REBUILD; verify recompute path resolves mixed-case DNs correctly