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

refactor: reduce OpenAPI(PKcols in table, ViewKeyDependency type) #2254

Merged
merged 1 commit into from
Apr 21, 2022

Conversation

steve-chavez
Copy link
Member

  • Get PKcols inside tables - done with SQL for tables and with an additional step in Haskell for views.

    This fixes an fk column being considered as a pk column on views and corrects the test added on https://github.com/PostgREST/postgrest/pull/1875/files/1d549768580310e18aac4ffa6dbd01c5b77934a7#r853674126

  • classify view key dependencies in SQL

  • remove Column from Relationship

  • Merge cols/fcols in Relationship and ensure allM2ORels and allViewsKeyDependencies fk columns are ordered - done by attnum in SQL

  • Cardinality now contains relColumns instead of Relationship - this simplifies getJoinConditions.

@steve-chavez
Copy link
Member Author

steve-chavez commented Apr 20, 2022

I tested this PR on the big schema and compared it with a commit previous to the recent refactors(bc7d54c).

bc7d54c7d2df309a6d84a9b04ccda0912c2112f2:
PGRST_DB_SCHEMAS='apflora' time postgrest-with-postgresql-14 -f apflora.sql  postgrest-run --dump-schema > old.json

- 21.73user 1.04system 0:23.89elapsed 95%CPU
- 21.49user 0.97system 0:23.62elapsed 95%CPU
- 21.89user 1.00system 0:24.06elapsed 95%CPU


this PR:
PGRST_DB_SCHEMAS='apflora' time postgrest-with-postgresql-14 -f apflora.sql  postgrest-run --dump-schema > new.json

- 11.98user 0.42system 0:13.52elapsed 91%CPU
- 11.77user 0.31system 0:13.20elapsed 91%CPU
- 11.86user 0.32system 0:13.29elapsed 91%CPU

It shows that the PR makes the metadata(OpenAPI should follow as well) generation faster.

The size of the output was also reduced:

du -sh old.json new.json

325M    old.json
68M     new.json

@steve-chavez
Copy link
Member Author

Btw, just noted the big schema(apflora) is OSS: https://github.com/FNSKtZH/apflora_api/tree/master/sql. I think we can now include it in our tests.

@steve-chavez
Copy link
Member Author

OpenAPI on the big schema also goes down from ~45s(#2256 (comment)) to ~18s:

time curl localhost:3000  > output.json

- real    0m18.029s user    0m0.006s sys     0m0.009s
- real    0m17.999s user    0m0.007s sys     0m0.007s
- real    0m17.963s user    0m0.009s sys     0m0.005s

@steve-chavez steve-chavez changed the title refactor: PKcols in table, ViewKeyDependency type refactor: PKcols in table, ViewKeyDependency type(reduce OpenAPI) Apr 21, 2022
@steve-chavez steve-chavez changed the title refactor: PKcols in table, ViewKeyDependency type(reduce OpenAPI) refactor: reduce OpenAPI(PKcols in table, ViewKeyDependency type) Apr 21, 2022
* Get PKcols inside tables - done with SQL for tables and
  with an additional step in Haskell for views.

  This fixes an fk column being considered as a pk column on views
  and corrects the test added on
  https://github.com/PostgREST/postgrest/pull/1875/files/1d549768580310e18aac4ffa6dbd01c5b77934a7#r853674126

* classify view key dependencies in SQL

* remove Column from Relationship

* Merge cols/fcols in Relationship and
  ensure allM2ORels and allViewsKeyDependencies fk columns
  are ordered - done by attnum in SQL

* Cardinality now contains relColumns instead of Relationship -
  this simplifies getJoinConditions.
@wolfgangwalther
Copy link
Member

It seems like this PR not only caused the regression in #2356, but also the regressions in #2458 and #2518.

This would have benefited greatly from a proper review, yet it was merged within a day.

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

Successfully merging this pull request may close these issues.

3 participants