I think the patch will work, but I wonder if we cannot pass the information we have.
In repl5_tot_run we do an internal search with "parentid>=1" and the bulk import flag, but we did send the suffix entry before, so we know the entryid of the suffix and the search could be "parentid>=suffix_id". Unfortunately the parentid index is ordered by the string representation of the id and 2>11, but couldn't we pass the real suffix entry id in the search filter and the check in the idl func if it is 1 or requires a special handling, but with an already known id

Indeed this is a nice idea. suffix entryid is known so it worth using it rather than retrieving it. 'parentid>=' will be translated into 'parentid>=2' (if suffix is the second entry) and that will miss in the range search all entries starting with '1'. So suffix ID is not usable for the filter.

I will update the patch.. and also with an automated testcase

After deeper thoughts I realize that the submitted patch is wrong. It does not guarantee that the IDs in the candidate list are ordered so that parent are created before children. Indeed some IDs before the suffix ID are going into the idl but it is not sure that those IDs are directly under the suffix children.

The solution is to start with '=1' key to be sure to grab all keys, then put all IDs before suffix ID into the leftover array. When suffix ID is found put its children into the idl and let the remaining keys tests if they go either to IDL (parent is already in IDL) or to the leftover.
Finally the consolidation of the leftover into the IDL creates the final list.

The difficult point is to determine the suffix key and to use it in idl_new_range_fetch.
Discussing with @lkrispen we evaluated

• add a new suffix_id field in op or pblock and use bulk_import flag to use it.
• put the suffix id directly in the filter and use the bulk_import flag to restore the valid filter
• store the suffix_id (if it does not exist already) in the parentid index (with the key '=0') in repl5_tot_run, then get it to detect the suffix key from the index in idl_new_range_fetch.

The last option looks the less messy.

An other option was abandoned of compatibility issue: send entries randomly and let the consumer create the appropriate glue entries.

In general I think this fix is good, some small comments:
- in check_suffix_entry() you handle a few error cases, but the function is void and doesn't return the errors, could they be serious enought not to continue with the online init ?
- in check_suffix_entry() shoudn't you init all fields of back_info ?
- in get/set_suffix_key always 0 is returned, could the index operations fail, should we propagate errors ?
- in get_suffix_key() do you want to keep the #if 0 sections ?
- in set_suffix_key() you use a pblock just to get the ldbminfo from the backend, but you're inside backend code, so I think
struct ldbminfo li = (struct ldbminfo ) be->be_database->plg_private;

should work

int32_t ?

Same here int32_t please :)

With txn's like this, I think it's good to avoid nested txns in the future, so my question is if we are already in a txn when the function is called here, or if we really are a parent making new txns for data manipulation

I have added some minor comments myself, but I will trust @lkrispens comments about some of the more interesting parts of the change :)

Why ? slapi_back_get_info returns 'int'.
The backend ID are u_int32_t and are retrieved from bck_info structure.

This is callback from slapi_back_get_info that returns 'int' not int32_t.
Do you suggest to change slapi_back_get_info returned type ?

Well currently this is a new interface BACK_INFO_INDEX_KEY is only used during online total init. There is no parent txn.
Now considering it lands in slapi-plugin, any plugin could call it and may have a txn.
One possibility it to add the txn in the 'info' struct.

int and int32_t are the same, but it's clearer, and we should do the right thing.

I think with our current txn mechanism you don't need to know if you are in a txn already, txn_init will check the txn stack in the thread and nest txns automatically.
For me that is good enough for this I do not see a need to do this change using an already existing txn explicitely.

@firstyear , @lkrispen , currently there is no possibility of nested txn. If in the future the interface is called with an opened txn, the current framework will take care of it (like @lkrispen mentioned).
So there is no immediate concern regarding this parent txn.

In order to prepare the interface for a potential need, do you want to add a txn field in the info struct, even if the db layer (set_suffix_key) does not use it ?
Else is it okay to push the patch ?

I'm okay with it, @lkrispen ?

I'm okay with it, @lkrispen ?
ok

This PR caused some compiier issues:

ldap/servers/slapd/back-ldbm/idl_new.c: In function 'idl_new_range_fetch':
ldap/servers/slapd/back-ldbm/idl_new.c:382:17: warning: suggest parentheses around assignment used as truth value [-Wparentheses]
             if (rc = slapi_back_get_info(be, BACK_INFO_INDEX_KEY, (void **)&bck_info)) {
ldap/servers/slapd/back-ldbm/dblayer.c: In function 'ldbm_back_get_info':
ldap/servers/slapd/back-ldbm/dblayer.c:7313:14: warning: implicit declaration of function 'get_suffix_key'; did you mean 'get_filter'? [-Wimplicit-function-declaration]
         rc = get_suffix_key(be, (struct _back_info_index_key *)info);
              ^~~~~~~~~~~~~~
              get_filter
ldap/servers/slapd/back-ldbm/dblayer.c: In function 'ldbm_back_set_info':
ldap/servers/slapd/back-ldbm/dblayer.c:7333:14: warning: implicit declaration of function 'set_suffix_key'; did you mean 'setbuffer'? [-Wimplicit-function-declaration]
         rc = set_suffix_key(be, (struct _back_info_index_key *)info);
              ^~~~~~~~~~~~~~
              setbuffer
ldap/servers/slapd/back-ldbm/index.c: In function 'set_suffix_key':
ldap/servers/slapd/back-ldbm/index.c:1265:9: warning: suggest parentheses around assignment used as truth value [-Wparentheses]
     if (rc = dblayer_txn_begin(be, txn.back_txn_txn, &txn)) {
         ^~
ldap/servers/slapd/back-ldbm/index.c:1275:9: warning: suggest parentheses around assignment used as truth value [-Wparentheses]
     if (rc = index_addordel_values_sv(be, info->index, sv_key, NULL, info->id, BE_INDEX_ADD, &txn)) {
         ^~
ldap/servers/slapd/back-ldbm/index.c:1284:9: warning: suggest parentheses around assignment used as truth value [-Wparentheses]
     if (rc = dblayer_txn_commit(be, &txn)) {

I fixed them in master, but this needs to be done in 1.3.8, and 1.3.7 (this might impact downstream)

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 pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/2995

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.