strapi / strapi

πŸš€ Strapi is the leading open-source headless CMS. It’s 100% JavaScript/TypeScript, fully customizable, and developer-first.
https://strapi.io
Other
63.79k stars 8.13k forks source link

core database possible bug and performance improvement #19964

Open binarybase opened 7 months ago

binarybase commented 7 months ago

Bug report

Required System information

Describe the bug

Strapi is recreating database table indexes during database sync even if it's actually not needed, it should diff the schema and apply only changes.

Steps to reproduce the behavior

  1. Run strapi with 200+ Content-Types
  2. Created new Collection Type and restarted the server

Expected behavior

After creating new Collection-Type, during the server restart - core database module should compare database schema, skip unmodified fields, update existing or create new ones.

Instead of that it forces deleting foreign keys and recreates new ones even if they was not modified. Restarting server takes tooo long because Strapi is reindexing whole database.

Screenshots

Without patched database image image image

With patched database it will just only create new table, but not recreating existing foreign keys for other tables as you can see the launch time was 14 seconds instead of 141 seconds

image image

Code snippets

The first issue I have found in mysql/schema-inspector.js that produces unwanted indexes update based on bad value comparator. knex queries database with SHOW INDEX command and the result will return raw values from database. The result query column called "Non_unique" is not an number or boolean, so the JS comparator !index.Non_unique will fail and the result will be NULL value instead of string 'unique'. I have modified the comparator from !index.Non_unique into index.Non_unique === '0'.

The second issue is with the foreign key onUpdate comparing. Its weird that in the stored database schema (table strapi_database_schema) is this value not included, but database query it includes. So it always looks like this value has been changed.

So, based on that, the comparator fn diffForeignKeys located in the file schema/diff.js will always result that the table foreign keys have been changed. Basically it compares oldForeignKey and foreignKey objects. For some reason the first one does not contain the onUpdate property, so it looks like it is not stored in the schema table, but the second one it includes.

When the comparator is not working correctly, it will cause really heavy performance slow down during recreating database indexes. So the current workaround is to check if both objects contains onDelete and onUpdate properties and then run the comparator.

@strapi+database+4.11.1.patch https://github.com/strapi/strapi/commit/124de6e3ba093f201525c7edb93673b6ef3b0b3c

Additional context

Maybe you have already fixed or you are planning to fix it in the 5.X version.

alexandrebodin commented 7 months ago

Hi, we changed this in some of the recent v4 versions and I cannot reproduce it following your steps

I verified on 4.21.1 and it does what you would expect and not regenerate the indexes.

Screenshot 2024-03-29 at 16 11 39
sunnysonx commented 7 months ago

hey @alexandrebodin, We experience the same behaviour, I'm currently on 4.23.0, but with mysql 5.7.

If we modify at least 1 field, it will alter all the tables/columns/indexes (We have around 500 components and 100 collection types).

For reference, I just removed 1 field from a collection type ipo: image

and it triggered the altering of all 600+ tables and their fields: image

On local, with local db it is fast, but when running on cloud and the db is on a different instance, it gets very slow, for reference, our startup delay on kubernetes is around 7-8minutes, just to cover this cases when schema was modified.

Can you please point to the commit that potentially solved this issue so we could debug what is wrong?

derrickmehaffy commented 7 months ago

Hey @sunnysonx can you test on MySQL 8 as I'm wondering if the issue comes from MySQL 5 not having some of the apis we use.

Also just a heads up also that Strapi 5 will be dropping MySQL 5 support (of coursewe are keeping MySQL 8+ support but MySQL 5 is considered EOL by Oracle)

sunnysonx commented 7 months ago

@derrickmehaffy hey, long time no see!

I have tested on MySQL 8, works perfectly: image So the issue was indeed caused by MySQL 5.

We anyway needed to migrate all our resources from 5 to 8, so now we have a reason to complete it faster πŸš€ As this issue was causing me headaches since strapi v2.

Thank you!

derrickmehaffy commented 7 months ago

Indeed, yeah MySQL 5 was lacking a lot of apis that MySQL 8 has. Most are around determining the schema structure and it's index, FKs, constraints, ECT.