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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make autoIndex syncIndex instead of createIndex #14602

Closed
2 tasks done
GustavoOS opened this issue May 17, 2024 · 8 comments
Closed
2 tasks done

Make autoIndex syncIndex instead of createIndex #14602

GustavoOS opened this issue May 17, 2024 · 8 comments

Comments

@GustavoOS
Copy link

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the feature has not already been requested

馃殌 Feature Proposal

autoIndex will check existing indexes instead of re-creating indexes everytime the app runs

Motivation

Mongoose docs explains that autoIndex is not advised to production environments because it creates indexes everytime the app runs. This makes the process of introducing indexes tricky in production. Mongoose offers synIndexes, but this has to be called individually for each model.

Example

No response

@GustavoOS GustavoOS added enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature new feature This change adds new functionality, like a new method or class labels May 17, 2024
@vkarpov15
Copy link
Collaborator

You can use Connection.prototype.syncIndexes(): https://mongoosejs.com/docs/api/connection.html#Connection.prototype.syncIndexes() . For example, await mongoose.connection.syncIndexes()

We won't make autoIndex use syncIndexes by default for now, because that's potentially expensive and risky (unintentional deletion of index can cause serious performance issues) but we can add an option to make autoIndex use syncIndexes()

@vkarpov15 vkarpov15 added this to the Parking Lot milestone Jun 2, 2024
@vkarpov15
Copy link
Collaborator

@hasezoey @AbdelrahmanHafez what do you think, is it worthwhile to add a separate option to make autoIndex use syncIndexes() under the hood? I'm not entirely convinced because await mongoose.connection.syncIndexes() is already a one-liner, the option wouldn't make things particularly cleaner.

@hasezoey
Copy link
Collaborator

hasezoey commented Jun 3, 2024

@hasezoey @AbdelrahmanHafez what do you think, is it worthwhile to add a separate option to make autoIndex use syncIndexes() under the hood? I'm not entirely convinced because await mongoose.connection.syncIndexes() is already a one-liner, the option wouldn't make things particularly cleaner.

Personally, i dont mind if there would be a extra option to use syncIndexes instead, as long as it is not the default for autoIndex or at least wait until the next major version for that.
Maybe a different option like syncIndex, or allow autoIndex to be a string to be set to bool | 'createIndexes' | 'syncIndexes' (where bool is backwards compatible to createIndexes and any non-proper value either give a error, or a warning and treat it as false; maybe also check it lowercased?), which i think would be cleaner than doing a nested object or another option which would be mutually exclusive.

@GustavoOS
Copy link
Author

GustavoOS commented Jun 3, 2024

Well, the semantics of auto index is "mongoose will take care of index for you". But this isn't true simply because this option should never be used in production (as stated in current docs) as it tries to recreate indexes as many times as possible.
The purpose of this feature request is to make autoIndex actually useful and meaningful, in a way it can be used in production.

As this should not be used in production, the change of behaviour would not break previous code.

@vkarpov15
Copy link
Collaborator

@hasezoey what do you think about using syncIndexes by default in Mongoose 9? I think syncIndexes is what apps should use by default, but switching is risky because syncIndexes could cause some serious performance issues for production apps if it drops an index that it shouldn't

@hasezoey
Copy link
Collaborator

what do you think about using syncIndexes by default in Mongoose 9?

i cannot really say for the impact on performance as i usually at most run personal stuff, but i think it is the behavior that would likely be expected of the autoIndex option.
maybe make it the default in the next major version, but have the old way as a fallback (maybe via the bool | 'createIndexes' | 'syncIndexes' i had in a earlier comment, but reversed true/false handling).

@AbdelrahmanHafez
Copy link
Collaborator

Hi all, late to the party.

While I agree in principle that syncIndexes in general is a better alternative to autoIndex, I second the concern @vkarpov15 voiced regarding performance issues related to index creation/dropping, we still have this issue discussed in this PR where it's very easy to have users unintentionally trigger "syncIndex" on all of their node instances instead of running only on one instance.

It's easy to think that mongoose.set('autoIndexes', 'syncIndexes'); does some magic under the hood prevent race conditions and duplicate commands across different nodes, than await mongoose.syncIndexes(); as part of the app start-up.

Also, the explicit execution gives users finer control over when they want to sync their indexes, and whether or not they want to await it before making their servers available to process requests.

What do you think?

@vkarpov15
Copy link
Collaborator

Thanks for reminding us about the discussion in #11030 @AbdelrahmanHafez , the potential for race conditions in syncIndexes() is a very good point. There isn't a good way for Mongoose to ensure that there isn't already a syncIndexes() running on a given collection without writing an actual document to the database or using some additional external service. And multiple app instances running syncIndexes() in parallel can lead to inconsistent index state, whereas there's no potential for that with createIndexes().

So requiring calling await mongoose.syncIndexes() instead of configuring an option is reasonable. I will close this issue.

@vkarpov15 vkarpov15 removed this from the Parking Lot milestone Jul 6, 2024
@vkarpov15 vkarpov15 added won't fix and removed new feature This change adds new functionality, like a new method or class enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature labels Jul 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants