Some of my in-progress work depends on assumption that dblayer_get_id2entry() is always correctly paired with dblayer_release_id2entry(). However this isn't currently true for ldap/servers/slapd/back-ldbm/id2entry.c:id2entry() function because following callpath can happen (pseudocode):
cache_find_id() finds entry -> goto bail in the bail section dblayer_release_id2entry() is called without previous dblayer_get_id2entry()
389-ds master branch <img alt="0001-Ticket-49234-Don-t-release-id2entry-when-we-didn-t-g.patch" src="/389-ds-base/issue/raw/files/571347fd5fc794978b3c77d68a86dc5f18b16343377da683cf5ef20d5844d905-0001-Ticket-49234-Don-t-release-id2entry-when-we-didn-t-g.patch" />
Given this was always true, can you remove the macro too? I think macro functions are evil, so you may as well purge it, and just put the calls to rd/wr lock throughout the code. Makes it much cleaner. So I'm happy to ack the DB_CHECKPOINT lock patch if you remove the macro there.
The first patch, I don't know enough about the internals there to comment. @lkrispen or @mreynolds might.
Metadata Update from @firstyear: - Custom field type adjusted to defect
about the MACRO, the change is ok, but the call to th rd/w lock is dependent on the BDB version and would require to move the BDB version check to the calls of the macro, so we should keep it or: - check which versions of BDB we do have to or want to support and do a cleanup of all the BDB version checks, I think thios would make the code much cleaner in many places.
about the cache_find_id I don't think we should move it just to generate something to free it. If the entry is found in the cache we just should return it and not go to bail. It was like this before bz628300 and there is even still the trace log for the return there, I think just returning the entry would be the right thing to do, but we should check the bz to see if there was a reason not to do it
@atkac Thanks for the investigations and patches. The second patch looks good to me. For the first patch (id2entry), I do not fully understand your fix. Before doing lookup into the entries database (id2entry), the servers tries to retrieve the entry from the entrycache. I think it is on purpose to do the in memory lookup before trying to retrieve it from the file
@lkrispen Do we still support or see anyone on bdb41? I thought we only really saw 5 these days?
In RHEL 6.8 we have 4.7.25, I don't think we need to handle any version before that
In RHEL 6.8 we have bdb 4.7.25, I don't think we need to handle any version older than that
Thanks for comments, I will improve both patches - drop support for libdb older than 4.7.X and also improve the second patch to return entry from cache if exists.
Let's use this ticket only for the id2entry get/release issue, I will open a separate one for libdb stuff
Metadata Update from @mreynolds: - Issue set to the milestone: 1.3.7.0
Is this still an issue in RHEL 7?
Metadata Update from @mreynolds: - Custom field origin adjusted to None - Custom field reviewstatus adjusted to None - Issue set to the milestone: 1.4 backlog (was: 1.3.7.0)
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/2293
If you want to receive further updates on the issue, please navigate to the github issue and click on subscribe button.
subscribe
Thank you for understanding. We apologize for all inconvenience.
Metadata Update from @spichugi: - Issue close_status updated to: wontfix - Issue status updated to: Closed (was: Open)
Login to comment on this ticket.