Skip to content

Issue 6057 - vlv search may result wrong result with lmdb - Fix 2 #6121

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

Merged
merged 2 commits into from
Mar 13, 2024

Conversation

progier389
Copy link
Contributor

@progier389 progier389 commented Mar 12, 2024

Previous fix is failing after a restart because of a chicken and egg issue related to vlv_init and backend initialization.
vlv_init requires that the backend get initialized to be able to generate the vlvSearch struct.
Because of deadlocks, and to be able to roll back the database instance open transaction I found it easier to avoid using vlv_getindices if vlv is not initialized but rather perform a search on cn=config to build a list of all possible vlv indexes filenames (ignoring the configuration errors) and use that list to open the database files for vlv indices and their cache.

Also fixed some minor issues:
@droideck minor remarks done about #6091 after the merge
a typo while logging info about the database environment parameters

Issue: #6057

Reviewed by: @tbordaz, @droideck , @mreynolds389 (Thanks!)

Copy link
Contributor

@tbordaz tbordaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor question. Else the patch LGTM

@progier389
Copy link
Contributor Author

Fixed:

  • use a function instead of vlvSearchList_lock
  • is it entries[0] or entries[i] ?
  • a compilation warning about log message unrelated to that change.

Copy link
Member

@droideck droideck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@progier389 progier389 force-pushed the i6057_fix2 branch 2 times, most recently from cd68b59 to 551d1f2 Compare March 13, 2024 13:22
@progier389
Copy link
Contributor Author

Fixed:
the ==0 typo
the error message about invalid vlv name that I moved without realizing it was wrong in existing code
the indentation

@progier389 progier389 force-pushed the i6057_fix2 branch 2 times, most recently from 9f98987 to 30ba32e Compare March 13, 2024 13:30
@progier389 progier389 merged commit e555c2a into 389ds:main Mar 13, 2024
jchapma pushed a commit that referenced this pull request Apr 10, 2024
)

* Issue 6057 - vlv search may result wrong result with lmdb - Fix 2
* Issue i6057 - Fix2 - Fix review comment

Previous fix is failing after a restart because of a chicken and egg issue related to vlv_init and backend initialization.
vlv_init requires that the backend get initialized to be able to generate the vlvSearch struct.
Because of deadlocks, and to be able to roll back the database instance open transaction I found it easier to avoid using vlv_getindices if vlv is not initialized but rather perform a search on cn=config to build a list of all possible vlv indexes filenames (ignoring the configuration errors) and use that list to open the database files for vlv indices and their cache.

Also fixed some minor issues:
@droideck minor remarks done about #6091 after the merge
a typo while logging info about the database environment parameters

Issue: #6057

Reviewed by: @tbordaz, @droideck , @mreynolds389 (Thanks!)
progier389 added a commit that referenced this pull request May 30, 2024
)

* Issue 6057 - vlv search may result wrong result with lmdb - Fix 2
* Issue i6057 - Fix2 - Fix review comment

Previous fix is failing after a restart because of a chicken and egg issue related to vlv_init and backend initialization.
vlv_init requires that the backend get initialized to be able to generate the vlvSearch struct.
Because of deadlocks, and to be able to roll back the database instance open transaction I found it easier to avoid using vlv_getindices if vlv is not initialized but rather perform a search on cn=config to build a list of all possible vlv indexes filenames (ignoring the configuration errors) and use that list to open the database files for vlv indices and their cache.

Also fixed some minor issues:
@droideck minor remarks done about #6091 after the merge
a typo while logging info about the database environment parameters

Issue: #6057

Reviewed by: @tbordaz, @droideck , @mreynolds389 (Thanks!)

(cherry picked from commit e555c2a)
@progier389 progier389 deleted the i6057_fix2 branch May 20, 2025 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants