SYSDB: perf improvements in sysdb_add_group_member_overrides(), part 3

[alexey-tikhonov]

(to be rebased on top of #7866)

Since this patch completely skips sysdb_add_group_member_overrides() in the setup described in #7841 (comment) (and used for testing in #7866), sure thing it makes lookup ~ x10..15 times faster - 62..82 msec.

[alexey-tikhonov]

@danlavu, please take a look at the test - 025dcba

[alexey-tikhonov]

@sumit-bose, @aplopez, I added two tests related patches:

(1) 025dcba - since I figured that local override tests were not checking usernames in group members output;
(2) 924ed17 - tmp patch (to be discarded) to test IPA views -- it's a pin of SSSD/sssd-test-framework#146 + SSSD/sssd-test-framework#154 plus test mentioned in the comments of those PRs.

[alexey-tikhonov]

Got confirmation from @marco-kusa that in their env (#7793) this patch makes lookups blazing fast even with "ignore_group_members = false".

[aplopez]

@alexey-tikhonov Most of these commits are already present in part 2

[alexey-tikhonov]

@alexey-tikhonov Most of these commits are already present in part 2

I mentioned this in the PR description:

(to be rebased on top of #7866)

There is only one patch to be reviewed: "DB: skip sysdb_add_group_member_overrides() completely "
Two other patches are tmp tests (to be discarded before a merge) to show it doesn't break things.

[aplopez]

There are two more conditions that use the expect_override_dn parameter in lines 1706 and 1732. They are no longer needed because we now know expect_override_dn is true.

[aplopez]

Now expect_override_dn is used only once, we can avoid passing it as an argument because it can be deduced from the first argument domain with the expression DOM_HAS_VIEWS(domain).

[aplopez]

@alexey-tikhonov Most of these commits are already present in part 2

I mentioned this in the PR description:

(to be rebased on top of #7866)

Sorry. I didn't understand that when I read it.

As discussed, Alexei is going to drop the test PR since it's now covered by #7887

My part of the review is no longer relevant.

[alexey-tikhonov]

There are two more conditions that use the expect_override_dn parameter in lines 1706 and 1732. They are no longer needed because we now know expect_override_dn is true.

No, formally speaking we don't.

We can still can get there with 'expect_override_dn == false' with 'id_provider == ipa'.

Another question is if this exception is justified. It's there because of 'SYSDB_DEFAULT_OVERRIDE_NAME'. I was discussing this with @sumit-bose and didn't fully understand use of 'SYSDB_DEFAULT_OVERRIDE_NAME' so kept ipa as an exception to be on a safe side. This should already provide a huge relief for LDAP and AD providers. We can deal with IPA later.

[aplopez]

There are two more conditions that use the expect_override_dn parameter in lines 1706 and 1732. They are no longer needed because we now know expect_override_dn is true.

No, formally speaking we don't.

We can still can get there with 'expect_override_dn == false' with 'id_provider == ipa'.

Sorry, as the second operator in the condition is an ||, I read or everywhere and missed that the first one is an &&.

However, the idea of removing the parameter still applies.

[alexey-tikhonov]

However, the idea of removing the parameter still applies.

But why? To save stack? This functions is called (and thus arguments created) once per group.

[aplopez]

However, the idea of removing the parameter still applies.

But why? To save stack? This functions is called (and thus arguments created) once per group.

To make the code clearer because in the end, even if we get this as a parameter, the value ALWAYS depends on the domain that we already have.

[alexey-tikhonov]

However, the idea of removing the parameter still applies.

But why? To save stack? This functions is called (and thus arguments created) once per group.

To make the code clearer because in the end, even if we get this as a parameter, the value ALWAYS depends on the domain that we already have.

For me 'expect_override_dn' is semantically clearer in this context than 'DOM_HAS_VIEWS()'.
We can replace function argument with a local variable of the same name, if you think it makes sense.

[aplopez]

For me 'expect_override_dn' is semantically clearer in this context than 'DOM_HAS_VIEWS()'. We can replace function argument with a local variable of the same name, if you think it makes sense.

I actually prefer the local variable, which is computed only once and, more important, assigns a meaning (the name) to that value.

If, today, you want to know what that parameter comes from, you have to find all the calls to this function and there you will discover that all of them are computed the same way; and with a value that the function already has. So why passing it as a separate argument?

[alexey-tikhonov]

For me 'expect_override_dn' is semantically clearer in this context than 'DOM_HAS_VIEWS()'. We can replace function argument with a local variable of the same name, if you think it makes sense.

I actually prefer the local variable, which is computed only once and, more important, assigns a meaning (the name) to that value.

Ok, added a patch.

[aplopez]

I ran some performance tests and the results are interesting.

Setup: 10000 users with private groups. 1 group g1 containing 1000 users. All users and groups in the cache.
Command: time for ((i=1; i <= 1000; i++)); do SSS_NSS_USE_MEMCACHE=NO getent -s sss group g1@ldap.test > /dev/null; done

Branch	2.10.2	master	part3
real	34.599	11.017	5.466
user	0.476	0.450	0.816
sys	1.168	1.063	2.069

[aplopez]

ACK. Thank you.
(Approved considering the tests will be removed).