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

withPgClient is intended for mutations - make it clear! #2044

Open
benjie opened this issue May 3, 2024 · 4 comments
Open

withPgClient is intended for mutations - make it clear! #2044

benjie opened this issue May 3, 2024 · 4 comments
Labels

Comments

@benjie
Copy link
Member

benjie commented May 3, 2024

e.g. the example here: https://postgraphile.org/postgraphile/next/make-extend-schema-plugin#example-2 would result in N+1 problem if applied to a non-root type. Need some serious caveats on the withPgClient page and throughout the docs where it's used.

@benjie benjie added the 🐛 bug label May 3, 2024
@Brookke
Copy link

Brookke commented May 3, 2024

For uses of withPgClient that do not do mutations, am I right in understanding that by setting hasSideEffects = false you can avoid the n+1 problem?

This is also hinted at here #2045

I wonder if there's value in adding a withReadOnlyPgClient via a read only connection string that doesn't have side effects set to true. Or even just an example in the docs of how to wrap it with a call that sets hasSideEffects = false and calls SET SESSION CHARACTERISTICS AS TRANSACTION READ ONLY

@benjie
Copy link
Member Author

benjie commented May 4, 2024

No, that is not correct. We added a warning right at the top of the docs page saying it doesn’t use batching:

https://grafast.org/grafast/step-library/dataplan-pg/withPgClient

Definitely worth looking out for these kinds of warnings. Might make sense for us to put “unbatched” in the name, perhaps.

We should maybe create a withBatchedPgClient that works more like loadOne/loadMany; but really people should be using resources/codecs for this so that inlining can happen (in both directions), and that needs a bit of work to make it more ergonomic for common use cases.

@cjheppell-ravio
Copy link

would result in N+1 problem if applied to a non-root type

Is this true for any usage of withPgClient for non root types?

Even if the withPgClient step depends on no prior steps (passes null as the second param)?

@benjie
Copy link
Member Author

benjie commented May 7, 2024

withPgClient will run the callback the number of times that matches the batch size (N) of the layer that it runs in (you can see the layer (or "bucket") it runs in by looking at the plan diagram). If you can trace from the root bucket to the bucket it's in without going through a listItem or stream bucket then the batch size is 1, so it will run once. If it's inside or descends from a listItem bucket then it may run an arbitrary number of times, depending on the size of data you're handling.

Depending on null as the second param does not guarantee that it will run only once, since that may happen in a situation where it cannot be "hoisted" (e.g. in a subprocedure or after a side effect or something). But in most cases, if you pass null then it will indeed only call the callback once.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: 📝 Docs Improvements
Development

No branches or pull requests

3 participants