this is a new version of the patch for backend refactoring phase 1.

I tried to address issues raised by the review of the last one and the changes have been substantial, so I did not want to put it on top of the previous PR.

I did run all the test Viktor had reported to fail with ASAN and they did pass in my env, also I did not find new memory leaks.

But there is still room for improvement, but didn't want to delay and will address in an additional patch.
- Mark had asked to address the use of types, eg int/int32_t and I haven't done yet
- the split of archive and restore functions is not yet optimal
- the problem of DBVERSION and upgrade is moved to another ticket
- some bdb types are still accessed in the ldbm layer, have to be accessed via get_fn or moved
- sure there are indentation problems

These functions are removed from ancestorid. Are they replaced by something ?
Is ancestorid management moved to db-bdb directory ?

Is that the name of the database subdirectory (i.e. db-bdb). if yes, should not it be name li_database_implement ?

Where is it set ?

Where is it set ?

in bdb_instance_start()

These functions are removed from ancestorid. Are they replaced by something ?
Is ancestorid management moved to db-bdb directory ?

yes, bdb_import.c

Is that the name of the database subdirectory (i.e. db-bdb). if yes, should not it be name li_database_implement ?

It is the name of the db implementation, cn=,cn=config, cn=ldbm.....", and is used in the dir name where it is implemented, can change it to li_db_implement

Sorry in advance it will be a list of update with dummy questoins and remarks.
In bdb_config
- _bdb_log_version, shouldn't free (string) ?
- bdb_config_db_old_idl_maxids_get should not idl be part of backend code ?
- Some param are part of backend and database code. (online_import_encrypt, import_cache_autosize, import_cachesize, serial_lock, bypass_filter) , I was expecting them to be only on backend.

Yeah, it's difficult to review such a large change - but also just as hard for you to work on it, rebase, migrate etc. It's quite hard to break it to smaller pieces I'm sure too.

Generally, my comment would be so long as it passes tests with ASAN, that's a huge barrier to acceptance cleared, and I think from your comments this is already true.

Another thing to keep in mind is this is a refactor not a rewrite so there will be code quality things we suddenly spot or notice, but it's actually just that the code already existed, so we should be careful about over-reaching and changing too much I guess? There is certainly room to do other clean ups "later" rather than trying to fix everything now I think.

I don't have any other obvious concerns here though :)

@firstyear I agree with you this is a refactor, not a rewrite, and any delay to push the path result in a big work to rebase.

I will continue the review but do not expect to find major issues as it is has already been deeply tested. I suggest that any valid concern that could be raised during this review could trigger a new ticket to fix it, so that this current patch could be pushed as soon as possible. @lkrispen would it be okay for you ?

Regarding bdb_import, I have a general question. Why is it moved to database subdirectory. Several concepts forman/workers/indexes/idlist should be common to any database. ?
Direct access to db files (open/close/put/get/cursor) could be in that file but higher level import algo could also be in backend level.

Same comment regarding bdb_ldif2db.c and some functions in bdb_misc.c (bdb_back_fetch_incl_excl, bdb_back_ok_to_dump)

in bdb_instance_config.c / bdb_instance_search_callback, you may initialize buf= {0} even if it is not strictly necessary

in bdb_layer.c, changelog is replication concept more than bdb. Will_dblayer_get_changelogdir be moved in the refactoring phase2 ?

ldbm_instance_add_instance_entry_callback creates default indexes entries in dse. I do not find their deletion in ldbm_instance_post_delete_instance_entry_callback, how are they deleted when deleting a backend ?

@firstyear I agree with you this is a refactor, not a rewrite, and any delay to push the path result in a big work to rebase.
I will continue the review but do not expect to find major issues as it is has already been deeply tested. I suggest that any valid concern that could be raised during this review could trigger a new ticket to fix it, so that this current patch could be pushed as soon as possible. @lkrispen would it be okay for you ?

First of all thanks for looking into this, and I am sure the more you look the more questions arise. I did myself always find thing I could improve, move, split, rename,..... - but finally decided it is time to get it tested and reviewed and if no real issues, failing tests or mem leaks, are detected commit it and work on the improvement in the next phases or in sepatate tickets.

I will try to answer your questions and keep them on a ToDo list, either change or better explain in comments.

Sorry in advance it will be a list of update with dummy questoins and remarks.
In bdb_config
- _bdb_log_version, shouldn't free (string) ?
yes, but it is old code just moved, didn't see a report by ASAN, so maybe it isn't even called. The VERSION stuff has also to be revisited.

• bdb_config_db_old_idl_maxids_get should not idl be part of backend code ?
  don't think so. BDB and LMDB and others are key/value stores, they are agnostic of what is in a value. In indexes we have idls, in two variants and eventually in a new one, the handling of this belongs to the upper layer

• Some param are part of backend and database code. (online_import_encrypt, import_cache_autosize, import_cachesize, serial_lock, bypass_filter) , I was expecting them to be only on backend.

Need to review this, but maybe in a next step

Regarding bdb_import, I have a general question. Why is it moved to database subdirectory. Several concepts forman/workers/indexes/idlist should be common to any database. ?
Direct access to db files (open/close/put/get/cursor) could be in that file but higher level import algo could also be in backend level.
Same comment regarding bdb_ldif2db.c and some functions in bdb_misc.c (bdb_back_fetch_incl_excl, bdb_back_ok_to_dump)

yes and no, I think you are right that there are parts which belong to the ldbm layer, but there is also stuff like multiple passesand merging db files after the passes which I think will not automatically be needed/possible with another db. And the code is, in my opinion, not so easily split. But it needs another look.

in bdb_layer.c, changelog is replication concept more than bdb. Will_dblayer_get_changelogdir be moved in the refactoring phase2 ?

When the changelog is moved to the "real" database in the next phase we no longer will have a changelogdir. So I didn't care too much about this

@firstyear I agree with you this is a refactor, not a rewrite, and any delay to push the path result in a big work to rebase.
I will continue the review but do not expect to find major issues as it is has already been deeply tested. I suggest that any valid concern that could be raised during this review could trigger a new ticket to fix it, so that this current patch could be pushed as soon as possible. @lkrispen would it be okay for you ?

First of all thanks for looking into this, and I am sure the more you look the more questions arise. I did myself always find thing I could improve, move, split, rename,..... - but finally decided it is time to get it tested and reviewed and if no real issues, failing tests or mem leaks, are detected commit it and work on the improvement in the next phases or in sepatate tickets.
I will try to answer your questions and keep them on a ToDo list, either change or better explain in comments.

Well, something to keep in mind (as I'm sure you already have) is that as we progress the goal is to replace the bdb code with another idl/entry storage layer, so there is questions about the value of how much we fix under that level.

What's more important here is the interface and how we interact with the bdb layer, and what that means for the possible new libraries and how their interactions will look :)

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/3652

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.