Skip to content

Conversation

@meherchandan
Copy link
Contributor

@meherchandan meherchandan commented Jun 14, 2022

…gn key references

What does it do?

Describe the technical changes you did.
Optimize the "getForeignKeys" when synchronizing with the database for postgres db.

Why is it needed?

Describe the issue you are solving.
On existing database, knex timeout error is coming when adding a new component. The PR is simplifying the complex(costly) database queries to remove the timeout error.

How to test it?

Provide information about the environment and the path to verify the behaviour.

Related issue(s)/PR(s)

Let us know if this is related to any issue/pull request
#11860

@meherchandan meherchandan marked this pull request as ready for review June 14, 2022 13:25
@meherchandan
Copy link
Contributor Author

meherchandan commented Jun 14, 2022

Fix for mysql is already added on #12843. I think the issue doesn't exist for sqlite as the query already looks optimized FOREIGN_KEY_LIST: 'pragma foreign_key_list(??)',.

@meherchandan meherchandan changed the title fix: postgres knex timeout error due to complex join queries to fetch foreign keys fix: postgres - Knex: Timeout acquiring a connection. The pool is probably full. Are you missing a .transacting(trx) call? on adding new component/collection Jun 16, 2022
@Convly Convly added source: core:database Source is core/database package pr: fix This PR is fixing a bug labels Jun 20, 2022
@Convly Convly added this to the 4.2.1 milestone Jun 20, 2022
@codecov
Copy link

codecov bot commented Jun 20, 2022

Codecov Report

Merging #13550 (584ed88) into master (632672a) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #13550   +/-   ##
=======================================
  Coverage   54.25%   54.25%           
=======================================
  Files        1199     1198    -1     
  Lines       30600    30598    -2     
  Branches     5571     5571           
=======================================
  Hits        16601    16601           
+ Misses      12181    12179    -2     
  Partials     1818     1818           
Flag Coverage Δ
front 56.60% <ø> (+<0.01%) ⬆️
unit 48.56% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...admin/src/pages/SettingsPage/utils/customRoutes.js

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 632672a...584ed88. Read the comment docs.

@derrickmehaffy derrickmehaffy changed the title fix: postgres - Knex: Timeout acquiring a connection. The pool is probably full. Are you missing a .transacting(trx) call? on adding new component/collection Optimize database foreign keys references - PostgreSQL Jun 27, 2022
@derrickmehaffy
Copy link
Member

Performance improvement here is not as good as #12843 but PG as already a bit faster than MySQL anyway

  • MySQL/MariaDB was roughly ~1.7 second cold boot pre fix
  • MySQL/MariaDB is roughly ~0.7 second cold boot post fix
  • PostgreSQL is roughly ~1.3 second cold boot pre fix
  • PostgreSQL is roughly ~1 second cold boot with this fix

This could just be differences between the two databases in general or how we are handling other parts of the schema inspector. Regardless it's still an improvement. Tests were also performed on a very small application so some larger apps may see much better gains.

Patch files based on v4.2.0
patches-13550.zip

Copy link
Member

@derrickmehaffy derrickmehaffy left a comment

Choose a reason for hiding this comment

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

Much like the MySQL PR it looks good to me but we do need @alexandrebodin to take a look here. Automated tests already passed fine so good there.

@alexandrebodin alexandrebodin added pr: enhancement This PR adds or updates some part of the codebase or features and removed pr: fix This PR is fixing a bug labels Jun 28, 2022
Copy link
Member

@alexandrebodin alexandrebodin left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for those contributions 👍

@alexandrebodin alexandrebodin merged commit 08fecfc into strapi:master Jun 28, 2022
Copy link

@shrmaky shrmaky left a comment

Choose a reason for hiding this comment

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

Great work @meherchandan

@Convly Convly modified the milestones: 4.2.1, 4.2.2 Jun 29, 2022
@strapi-bot
Copy link

This pull request has been mentioned on Strapi Community Forum. There might be relevant details there:

https://forum.strapi.io/t/new-release-strapi-v4-2-2/19927/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr: enhancement This PR adds or updates some part of the codebase or features source: core:database Source is core/database package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants