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

Context loading optimization for schema based multi-tenant workloads #493

Merged
merged 1 commit into from
Mar 27, 2024

Conversation

olirice
Copy link
Contributor

@olirice olirice commented Feb 12, 2024

What kind of change does this PR introduce?

DDL changes in any schema force a full context (schema) reload for all connections.

We don't want to change that behavior because invalidating subsections of the GraphQL schema based on SQL DDL would require significant refactoring and be error prone.

Projects using schema based multi-tenancy are negatively impacted by these choices because

  • Updates in any isolated tenant require reloading the schema for all tenants
  • Since all schemas are loaded for all tenants (regardless of search path) the query to reload context slows down with each additional tenant

This PR filters visible schemas (tables/functions/types/etc) during context reload by if the current_user has USAGE privilege.

This change does not solve the issue that DDL in any tenant's DB causes a refresh for all connections, but it does allow the context loading query time to be invariant to the number of tenants in the database.

Note: filtering by search_path is not a viable alternative because we support referencing types that aren't on the current search path

@olirice
Copy link
Contributor Author

olirice commented Feb 12, 2024

@etherealhedgehog @magrinj this solution should work for you so long as each of your tenants connects to the database with a dedicated role and each of those roles does not have USAGE permission on the schemas for the other tenants.

Any thoughts would be great. Performance impact on your projects would be even better if possible

ref #480

@etherealhedgehog
Copy link

@olirice Thank you for looking at this. We use pg_graphql to serve a public API to our users so they can access data within their tenant. Our tenants tend to have lots of data ingested into our system that we make available to them in real-time, so query performance is critical for us. We currently filter solely based on search path, but I think we should be able to supplement that by setting the role on the connection dynamically based on the tenant. For us, this should be a workable solution.

@olirice
Copy link
Contributor Author

olirice commented Feb 20, 2024

@magrinj your buy-in here would go a long way to getting this merged if you have a few minutes to review

@magrinj
Copy link

magrinj commented Feb 21, 2024

Hey @olirice sorry for the delay, and thanks for this proposal, we'll try to take a look on that this week 🙂
Like @etherealhedgehog we're using search_path, so basing that on the current_user will require some changes on our side to test that on a production dump and see if performance are improved.
@charlesBochet will keep you in touch on that !

@olirice
Copy link
Contributor Author

olirice commented Feb 21, 2024

Awesome, thanks!

@olirice
Copy link
Contributor Author

olirice commented Feb 29, 2024

any word @magrinj ?

@martmull
Copy link

Hey @olirice, apologies for the lengthy period of silence. We've conducted tests on this optimization across more than 600 schemas. The results were significant; we managed to reduce the duration of the first request from over 2 seconds to under 200 milliseconds, which is quite impressive. Given these outcomes, I believe we can proceed with this approach. Cheers
@magrinj

@olirice olirice marked this pull request as ready for review March 26, 2024 20:21
@olirice olirice requested a review from imor March 26, 2024 20:23
@olirice
Copy link
Contributor Author

olirice commented Mar 26, 2024

awesome, thank you for following up!

We'll get this approved and merged

@olirice olirice merged commit 31dd51b into master Mar 27, 2024
4 checks passed
@olirice olirice deleted the or/schema-multi-tenant-context branch March 27, 2024 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants