I will upload two patches to save the state of the work, when the update of the doc is finished I will ask for review.

Tese two patches will-
- make the database configuration implememntation independent
- move the current bdb implementation to a sub directory

NOTE: this does not yet make generic calls to database access functions like c_get, it provides the higher level functionaity

Wow yeah that's a big change. Hard to give it a good review, but as long as TET/CI tests pass I will be okay with it (once we create the 1.4.1 branch)

Wow yeah that's a big change. Hard to give it a good review, but as long as TET/CI tests pass I will be okay with it (once we create the 1.4.1 branch)

yes, but I'll try to provide a good doc to help with reviewing. And one reason that it is this big is that a lot of code has been moved (and only slightly changed) form the generic ldbm functions to the specific bdb dir.
I'll try to add some review guidelines.

Wow yeah that's a big change. Hard to give it a good review, but as long as TET/CI tests pass I will be okay with it (once we create the 1.4.1 branch)

Tests might become a problem :-(
I think the functionality is unchanged and it should be able to just startup existing instances, but some config parameters have moved, eg "deadlock-detection-policy" is very BDB specific and now in the cn=bdb,cn=config,cn=ldbm database entry. Tests might want o change or set params and fail because they are not in the expected place

@lkrispen do you mean most of the tests will fly but only tests that change/read bdb parameters will need to be ported to the new layout ?

With your patch, if a legacy script change a bdb param under cn=config,cn=ldbm database... does it get an error ?

@lkrispen do you mean most of the tests will fly but only tests that change/read bdb parameters will need to be ported to the new layout ?

yes, I hope so

With your patch, if a legacy script change a bdb param under cn=config,cn=ldbm database... does it get an error ?

good point, unfortunately no. the "cn=config,cn=ldbm database" entry has objectclass extensibleobject and you can add anything meaningful or not, eg you can add:

  nsslapd-tralala: hiphop

without any problem. I think this is a bug already in the current implementation, we do not catch eg typos for intended changes on real attributes.

There are a few options to fix it:
1] define a proper objectclass ldbmconfig in the schema, defining allowed and required attributes. This would be a clean way, but would also require us to be a bit more careful with adding config params on the fly, a schema change is a bit more of a burden
2] add code to reject currently undefined attributes. I think it is done eg in repl config.

Now for the problem with the split of configuration by my patch, I think there are also two options
1] implement one of the solutions above and return an error if a parameter no longer there is attempted to be changed
2] keep a list of legacy bdb attributes and return a nice error or be even nicer and log a warning, but apply it to cn=bdb.
Option 2] should be possible and make all clients happy, although I do not really like having too much upgrade specific code

I think porting the tests should be done in a different ticket. Else this one will be soooo large ;)

cn=config will only contain generic settings so it could be a good time to enforce a strict schema definition. But it has been fine for years to have it "opened" so I do not think it worth the effort.
From the others options, I would prefer that an update checks a bdb list of attributes. If an bdb attribute is set, it fails, returns a detailed reason if possible and log a clear warning.

still work in progress, just for reference: http://www.port389.org/docs/389ds/design/backend-redesign.html

@lkrispen trying to clarify ticket/PR. Is PR https://pagure.io/389-ds-base/pull-request/50181 (database code isolation) part of this ticket ?

@lkrispen trying to clarify ticket/PR. Is PR https://pagure.io/389-ds-base/pull-request/50181 (database code isolation) part of this ticket ?

yes, part 1

Issue linked to Bugzilla: Bug 1685501

b7d1118.. 94c7401 master (1.4.2)

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

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.