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

Issue 6374 - nsslapd-mdb-max-dbs autotuning doesn't work properly #6400

Merged
merged 2 commits into from
Nov 13, 2024

Conversation

progier389
Copy link
Contributor

@progier389 progier389 commented Nov 12, 2024

Several issues:

  • After restarting the server nsslapd-mdb-max-dbs may not be high enough to add a new backend
    because the value computation is wrong.
  • dbscan fails to open the database if nsslapd-mdb-max-dbs has been increased.
  • dbscan crashes when closing the database (typically when using -S)
    When starting the instance the nsslapd-mdb-max-dbs parameter is increased to ensure that a new backend may be added.
    When dse.ldif path is not specified, the db environment is now open using the INFO.mdb data instead of using the default values.
    synchronization between thread closure and database context destruction is hardened

Issue: #6374

Reviewed by: @tbordaz , @vashirov (Thanks!)

@@ -100,7 +100,7 @@ dbmdb_compute_limits(struct ldbminfo *li)
*/
if (dbmdb_count_config_entries("(objectClass=nsMappingTree)", &nbsuffixes) ||
dbmdb_count_config_entries("(objectClass=nsIndex)", &nbsuffixes) ||
Copy link
Member

@vashirov vashirov Nov 12, 2024

Choose a reason for hiding this comment

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

Suggested change
dbmdb_count_config_entries("(objectClass=nsIndex)", &nbsuffixes) ||
dbmdb_count_config_entries("(objectClass=nsIndex)", &nbindexes) ||

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.

LGTM

@@ -27,6 +27,8 @@
#define NEED_DN_NORM_SP -25
#define NEED_DN_NORM_BT -26

pthread_mutex_t import_ctx_mutex = PTHREAD_MUTEX_INITIALIZER; /* Protect agains import context destruction */
Copy link
Contributor

Choose a reason for hiding this comment

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

static ?

pthread_mutex_lock(get_import_ctx_mutex());
ctx = job->writer_ctx;
job->writer_ctx = NULL;
pthread_mutex_unlock(get_import_ctx_mutex());
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not moving the unlock up to the end of the function ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The writer_ctx is reset in the lock, so we are sure that the other threads cannot use it and
there is no reason to block these threads longer than necessary ...
Furthermore keeping the critical section trivial avoid to have to check if there is a risk of deadlock which is not so obvious if you unlock at the end (because dbmdb_import_q_destroy performs some locking ... )

@progier389 progier389 merged commit 56cd338 into 389ds:main Nov 13, 2024
197 checks passed
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.

3 participants