Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove ldbm_back_entry_release #2546

Closed
389-ds-bot opened this issue Sep 13, 2020 · 21 comments
Closed

remove ldbm_back_entry_release #2546

389-ds-bot opened this issue Sep 13, 2020 · 21 comments
Labels
closed: fixed Migration flag - Issue
Milestone

Comments

@389-ds-bot
Copy link

Cloned from Pagure issue: https://pagure.io/389-ds-base/issue/49487


This function is never used.

There are calls in a section with
#if 0
and defining an iterate_with_lookahead

but this is commented out since 2005, so we could remove it completely

@389-ds-bot 389-ds-bot added the closed: fixed Migration flag - Issue label Sep 13, 2020
@389-ds-bot 389-ds-bot added this to the 1.4.4 milestone Sep 13, 2020
@389-ds-bot
Copy link
Author

Comment from lkrispen (@elkris) at 2017-12-05 17:39:41

This dead code is also the only user of ldbm_back_next_search_entry_ext with useextension set to 1.
otherwise:

 ldbm_back_next_search_entry()
     ldbm_back_next_search_entry_ext(pb,0)

so we could merge these two and simplify by eliminating use_extension

@389-ds-bot
Copy link
Author

Comment from lkrispen (@elkris) at 2017-12-05 17:39:41

Metadata Update from @elkris:

  • 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

@389-ds-bot
Copy link
Author

Comment from mreynolds (@mreynolds389) at 2018-02-13 18:43:40

Metadata Update from @mreynolds389:

  • Issue set to the milestone: 1.4.0

@389-ds-bot
Copy link
Author

Comment from vashirov (@vashirov) at 2020-02-19 16:55:06

Metadata Update from @vashirov:

  • Issue priority set to: normal
  • Issue set to the milestone: 1.4.3 (was: 1.4.0)

@389-ds-bot
Copy link
Author

Comment from mreynolds (@mreynolds389) at 2020-07-21 17:51:11

Metadata Update from @mreynolds389:

@389-ds-bot
Copy link
Author

Comment from mreynolds (@mreynolds389) at 2020-07-21 17:51:12

Issue linked to Bugzilla: Bug 1859282

@389-ds-bot
Copy link
Author

Comment from mreynolds (@mreynolds389) at 2020-07-24 21:02:31

Metadata Update from @mreynolds389:

  • Issue assigned to mreynolds389

@389-ds-bot
Copy link
Author

389-ds-bot commented Sep 13, 2020

Comment from mreynolds (@mreynolds389) at 2020-07-24 21:53:04

#4271

@389-ds-bot
Copy link
Author

Comment from mreynolds (@mreynolds389) at 2020-07-27 20:10:22

Commit c6aae1e5 relates to this ticket

@389-ds-bot
Copy link
Author

Comment from mreynolds (@mreynolds389) at 2020-07-27 20:10:56

Metadata Update from @mreynolds389:

  • Issue close_status updated to: fixed
  • Issue set to the milestone: 1.4.4 (was: 1.4.3)
  • Issue status updated to: Closed (was: Open)

@389-ds-bot
Copy link
Author

@389-ds-bot
Copy link
Author

Comment from vashirov (@vashirov) at 2020-07-28 09:46:12

Metadata Update from @vashirov:

  • Issue status updated to: Open (was: Closed)

@389-ds-bot
Copy link
Author

Comment from mreynolds (@mreynolds389) at 2020-07-28 15:46:55

Turns out we do use ldbm_back_entry_release() afterall in opshared.c:

static void
cache_return_target_entry(Slapi_PBlock *pb, Slapi_Backend *be, Slapi_Operation *operation)
{
    if (operation_get_target_entry(operation) && be->be_entry_release) {
        (*be->be_entry_release)(pb, operation_get_target_entry(operation));
        operation_set_target_entry(operation, NULL);
        operation_set_target_entry_id(operation, 0);
    }
}

Adding that code back...

@389-ds-bot
Copy link
Author

Comment from elkris at 2020-07-28 16:48:12

Ok, I missed this usage when creating the ticket, but I think it is still not clean. The call is in opshared.c when handling the return code of be_search - and it does operate on the entry cache, which it should not know of.
Instead of dealing with the ec when handling rc of be_search I think it should be done inside be_search before returning, and could use functions directly instead of a pb function

@389-ds-bot
Copy link
Author

Comment from mreynolds (@mreynolds389) at 2020-07-28 17:02:05

Sorry, yeah I'm trying to clean it all up - I am not adding it back as it was...

I found some other pblock params that are not used (or not used as intended):

  • SLAPI_PLUGIN_PRECEDENCE
  • SLAPI_PLUGIN_POSTSTART_FN
  • SLAPI_PLUGIN_EXT_OP_BACKEND_FN
  • SLAPI_PLUGIN_MR_FLAGS
  • SLAPI_PLUGIN_MR_NORMALIZE

But I will open another ticket to look at these...

Ok, I missed this usage when creating the ticket, but I think it is still not clean. The call is in opshared.c when handling the return code of be_search - and it does operate on the entry cache, which it should not know of.
Instead of dealing with the ec when handling rc of be_search I think it should be done inside be_search before returning, and could use functions directly instead of a pb function

I will look into this, thanks

@389-ds-bot
Copy link
Author

Comment from mreynolds (@mreynolds389) at 2020-07-28 20:34:43

So we call cache_return_target_entry() in opshared.c when be_search fails, but also when it succeeds. When it succeeds we do checks for psearches, and how results are sent back, and depending on the error and how things happen we "release" the target or we don't. It's not straightforward.

To try and move this "cache returning" would require changes to the search function and to the result handling. I feel, in this ticket, we would be trying to fix something that is not broken, and the entry cache is fragile enough. Maybe if we rework the entry cache at a later date we can look into this when we go to make bigger changes to the design?

Anyway I was able to do more cleanup, and I'm going to update the PR...

@389-ds-bot
Copy link
Author

Comment from mreynolds (@mreynolds389) at 2020-07-28 20:34:43

Metadata Update from @mreynolds389:

  • Issue priority set to: None (was: normal)

@389-ds-bot
Copy link
Author

389-ds-bot commented Sep 13, 2020

Comment from mreynolds (@mreynolds389) at 2020-07-28 20:43:59

#4274

@389-ds-bot
Copy link
Author

Comment from elkris at 2020-07-28 22:03:54

To try and move this "cache returning" would require changes to the search function and to the result handling. I feel, in this ticket, we would be trying to fix something that is not broken, and the entry cache is fragile enough.

yes, there is no real need to do it, can be part of another cleanup

@389-ds-bot
Copy link
Author

Comment from mreynolds (@mreynolds389) at 2020-07-29 17:59:29

Commit b7865bf1 relates to this ticket

@389-ds-bot
Copy link
Author

Comment from mreynolds (@mreynolds389) at 2020-07-29 18:31:14

Metadata Update from @mreynolds389:

  • Issue close_status updated to: fixed
  • Issue status updated to: Closed (was: Open)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed: fixed Migration flag - Issue
Projects
None yet
Development

No branches or pull requests

1 participant