#49476 refactor ldbm backend to allow replacement of BDB
Opened a year ago by lkrispen. Modified 13 days ago

We want to be able to use other database libs than BerkeleyDB. Therfore we need to refactor the ldbm layer to separate the database specific implementation from the general backend logic

This ticket is a tracker for the work in this context


Metadata Update from @mreynolds:
- Custom field component adjusted to None
- Custom field origin adjusted to None
- Custom field reviewstatus adjusted to None
- Custom field type adjusted to None
- Custom field version adjusted to None
- Issue set to the milestone: 1.4 backlog

a year ago

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.

@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

Metadata Update from @tbordaz:
- Custom field rhbz adjusted to https://bugzilla.redhat.com/show_bug.cgi?id=1685501

13 days ago

Login to comment on this ticket.

Metadata