do you have an example ? Idl list processing can be aborted if the number of entries is below the filter treshold and the filter has to be tested.
Also, skipping filter test even for indexed searches can be risky, the entrie in an idl list created by an indexed search can contain an entry which was modified before sending it (and no longer match the filter).

now I am confused. Your test looks like it SHOULD evaluate the filter. And even if you think filter evaluation is not necessary, it should not give different results if you do not skip it

I agree it is a sensitive part and better to evaluate the filter if unecessary than skipping it when we should not.
I think the suspicious part is in build_candidate_list where we set SLAPI_SEARCH_FILTER with a filter where we added (|(originalfilter)(objectclass=referral)). Although this was not the requested filter.

Regarding entries being changed while processing the search, is there a mechanism to detect this collision. I am not sure it is an issue either

we could only fully rely on the index if we had a copy on write txn and until the search is fully completed it would not see the modified entry
This is on of Williams goals

it can be a issue, if a client issues a search attr=xxx and gets an entry with attr=yyy

Up to 1.3.7, the search found indexed candidates and only check the filter for access rights.
So evaluation of the filter against the entry was skipped.

This is changed since #49372, where the filter is systematically evaluated. I think it is a side effect of #49372 but not the purpose of the ticket.

From your comment, I understand that it is preferable to systematically evaluate the filter because without copy/write we can return entries not matching the requested filter. Am I understanding correctly ?

This is an other symptom of the issue. If a entry contains 'objectclass=referral' it is returned even if the search filter does not match

There are two symptoms (2 testcases). It may come from the same RC.
We may consider to split the ticket into two tickets:
- optimization skip filter evaluation does no longer occur. It does not change the external behavior and we may decide to keep this behavior
- return entries having 'objectclass=referral' whatever it matches the original final

You know, I think this is working as intended. https://access.redhat.com/documentation/en-us/red_hat_directory_server/10/html/administration_guide/configuring_directory_databases-using_referrals#Using_Referrals-Creating_Smart_Referrals

I remember checking the referral case when I wrote this change. I think if you create a user like this, and you search subtree, you should recieve it because you don't know if the referral will match the filter you are seeking either. So your client would process first the results of "originalfilter" and then it would chase the referral and apply originalfilter to the referral url.

So when you create this "weird" users, you are kind of making them a weird hybrid referral object which returns a local answer AND a remote answer.

Second, the 'skip filter evaluation" can't really run until we know at least one idl content. We have to process some filters at some point, and in fact, I expect this would remain as behaviour because I (we?) want to move nsTombstone to be a default (!) filter, and then we can have correct indexing of tombstones.

So the content "original filter" will always transform to -> (&(|(originalfilter)(objectClass=referral)) (!(objectClass=nsTombstone)) )

The trick is that filter optimisation will always process the objectClass=nsTombstone as being 'less than the filter threshold' if the result of the (|(originalfilter)(objectClass=referral) is small, and we'll actually use the objectClass=nsTombstone index really effectively on large candidate sets to eliminate tombstones from large searches.

When you call original filter with (objectClass=nsTombstone) then we actually would transform to:

(|(originalfilter)(objectClass=referral))

Because originalFilter now contains the oc=nsTombstone assertion, so we don't add the (!).

Finally, What I need to finish is the performance analysis of idlistscanlimit, because once that is lifted this process gets much quicker (I need to submit hard numbers and reproducable test cases first of course).

For bonus marks, we probably need to re-examine the filtertest threshold as I think the current value of 10 may actually be too LOW. I have a suspicion something closer to 16 or 32 will be better, but I need to again, run tests on this to be sure.

Anyway, I think this is all working as intended .....

@firstyear, thanks for your explanation. I agree that it makes sense to systematically return referral entries even if they do not match the filter. Note that it is a change of behavior and some application not ready to process referral can be impacted.

Regarding the second part of the ticket (optimisation of skip filter evaluation). As the evaluated filter is now (|(originalfilter)(objectClass=referral)), we can not skip filter evaluation. Should we consider to remove that dead code ?

I think so yes. I think we'll always need to evaluate the filter, even partially to get our results.

The candidates of indexed search can satisfy all the components of the filter. In such case it is useless to test filter matching before returning the entries (SR_FLAG_CAN_SKIP_FILTER_TEST flag). Since https://pagure.io/389-ds-base/issue/49372, a OR component (oc=referral) is append to the original filter so the filter needs to be systematically evaluated before returning the entry.
The SR_FLAG_CAN_SKIP_FILTER_TEST code is now useless and needs cleanup

This ticket is changed to do this cleanup

Great! I love it when we get to clean up code :)

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This issue has been cloned to Github and is available here:
- https://github.com/389ds/389-ds-base/issues/2624

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.