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

Improve database accesses #71

Open
hanoii opened this issue Sep 22, 2022 · 5 comments
Open

Improve database accesses #71

hanoii opened this issue Sep 22, 2022 · 5 comments
Labels
enhancement New feature or request

Comments

@hanoii
Copy link

hanoii commented Sep 22, 2022

I am setting up a VPS where multiple instances of strapi will be run. These instances connects to a DigitalOcean postgre managed database.

I started about the the lowest tier for the managed database (1GB of RAM) which gives me small amount of concurrent connections (22) something I just learnt. While this still in the PoC phase and the servers will be bigger I found it odd that with a handful of no traffic strapi instances I was quickly getting errors out of postgre when running config-sync import.

I wondered by and while looking at database activity I saw that one run of config-sync import was creating several databases connections at the same time.

I'd assume this is not needed and whatever this is doing it can be done using 1 (or as fewer as possible) connections or at least to have the option to limit it if this is because things are done in parallel.

Any advice/comment is appreciated.

@boazpoolman boazpoolman added the enhancement New feature or request label Sep 23, 2022
@boazpoolman
Copy link
Member

Hi @hanoii,

Thanks for raising this issue.

As of now, preforming the import action will do the following:
For each config type:

And in that last step is probably where most DB requests happen. As for each single config that is imported we do the following:

  • Query the entry from the DB.
  • Compare to the corresponding file to determine wheter it should be created, updated or deleted.
  • Once determined, we will run the create, update or delete query for the entry.
  • Also we will query all the relations of the entry.
  • Compare then to the corresponding file to determine wheter it should be created, updated or deleted.
  • And once determined we will loop over them and run a single update, create or delete query for each individual relation.

It goes without saying that there is much room for improvement here. Some things more obvious then others.
As I'm currently not in a position where I have the time to contribute these performance updates I won't be listing all the possible improvements.

If you, or anyone else looking at this, is looking to contribute to these improvements please reach out and we can discuss the best way to go about this. Where we'll probably start by writing tests to make sure the changes we make don't break any functionality.

@Achder
Copy link

Achder commented Jan 14, 2025

I was running into the same issue today.
We have lots of content types and the config-sync plugin was maxing out our 100 available connections.

@Achder
Copy link

Achder commented Jan 14, 2025

await Promise.all(ctx.request.body.config.map(async (configName) => {

await Promise.all(Object.keys(diff).map(async (file) => {

Here the plugin is running all imports at the same time.
I would recommend using some like https://github.com/sindresorhus/p-queue to queue the jobs.
Maybe make the max parallel jobs configurable.

@boazpoolman
Copy link
Member

boazpoolman commented Jan 14, 2025

Queueing would be nice. But setting up a proper queue system would be beyond the scope of this plugin.
The code of config-sync can be optimized by just shrinking the amount of requests that need to be made for an import.
I've laid out some of the pain points in a comment above. This would be step one in my opinion. That would probably solve the issue for most people.

That being said, I would love to see a queue plugin for Strapi, implementing some open-source job runner server side.
Then that plugin could be a dependency of config-sync, allowing us to implement it's API's.

EDIT:

Another solution we can explore is just wrapping the entire import method in a DB transaction.

@Achder
Copy link

Achder commented Jan 14, 2025

I'm not quite sure if we are talking about the same kind of queue here. I'm not talking about a separate service for background jobs.

My idea is basically this:

const queue = new PQueue({concurrency: 10});

for(const configName of ctx.request.body.config) {
  const promise = strapi.plugin('config-sync').service('main').importSingleConfig(configName, null, ctx.request.body.force);
  queue.add(promise)
}

await queue.onIdle()

The queue is just for chunking the work.
If this is also what you are talking about, could you explain what you mean by But setting up a proper queue system would be beyond the scope of this plugin.?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants