Skip to content

sysdb: consolidate ldb writes in sysdb_add_basic_group()#8666

Draft
alexey-tikhonov wants to merge 1 commit intoSSSD:masterfrom
alexey-tikhonov:sysdb_add_incomplete_group-again
Draft

sysdb: consolidate ldb writes in sysdb_add_basic_group()#8666
alexey-tikhonov wants to merge 1 commit intoSSSD:masterfrom
alexey-tikhonov:sysdb_add_incomplete_group-again

Conversation

@alexey-tikhonov
Copy link
Copy Markdown
Member

sysdb_add_basic_group() now accepts optional extra_attrs that are merged into the ldb_add message. This lets sysdb_add_incomplete_group() and sysdb_add_group() include all attributes in a single ldb_add instead of doing ldb_add followed by ldb_modify via sysdb_set_group_attr().

Per group this eliminates an ldb_modify on the main cache, the preceding ldb_search diff-check, and an ldb_modify on the timestamp cache.

@alexey-tikhonov alexey-tikhonov added backport-to-sssd-2-9 Performance Performance related patches labels Apr 29, 2026
sysdb_add_basic_group() now accepts optional extra_attrs that are
merged into the ldb_add message. This lets sysdb_add_incomplete_group()
and sysdb_add_group() include all attributes in a single ldb_add
instead of doing ldb_add followed by ldb_modify via
sysdb_set_group_attr().

Per group this eliminates an ldb_modify on the main cache, the
preceding ldb_search diff-check, and an ldb_modify on the timestamp
cache.

Assisted-By: Claude Code (Opus 4.6)
@alexey-tikhonov alexey-tikhonov force-pushed the sysdb_add_incomplete_group-again branch from e93b79b to 2ee255f Compare April 29, 2026 15:20
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors group creation in the sysdb layer by allowing sysdb_add_basic_group to accept extra attributes, streamlining operations in sysdb_add_group and sysdb_add_incomplete_group. Feedback suggests replacing the hardcoded SYSDB_POSIX check with a dynamic check for existing attributes in the ldb_message to prevent failures caused by duplicate single-valued attributes.

Comment thread src/db/sysdb_ops.c
Comment on lines +2130 to +2132
if (strcmp(extra_attrs->a[i].name, SYSDB_POSIX) == 0) {
continue; /* already added */
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Instead of hardcoding a check for SYSDB_POSIX, it is safer and more robust to check if the attribute already exists in the ldb_message. This prevents duplicate elements for all attributes added earlier in the function (such as name, objectCategory, gidNumber, and createTimestamp), which would otherwise cause ldb_add to fail for single-valued attributes.

            if (ldb_msg_find_element(msg, extra_attrs->a[i].name) != NULL) {
                continue; /* already added */
            }

Copy link
Copy Markdown
Member Author

@alexey-tikhonov alexey-tikhonov Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is intentional.
There should be no other duplicates.
Checking everything will consume CPU cycles and might hide logic errors / bugs introduced later.

@alexey-tikhonov alexey-tikhonov added coverity Trigger a coverity scan and removed coverity Trigger a coverity scan labels Apr 29, 2026
@alexey-tikhonov
Copy link
Copy Markdown
Member Author

Note: Covscan is clean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-to-sssd-2-9 Performance Performance related patches

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant