crystal
crystal copied to clipboard
fix(subscriptions): context hook was not called with queryDocumentAst/etc
Description
Fixes https://github.com/graphile/issues/issues/8
Because queryDocumentAst
was not sent through, plugin could not determine it was a subscription and thus could not direct the SQL queries to the replica rather than the primary.
Performance impact
Allows offloading subscription queries to replica rather than primary (performance++).
Security impact
There's some data synchronisation concerns - e.g. if your replica is lagging then the event may trigger before the data is available and thus you may do stale reads. This is a problem for @graphile/pro
to solve.
Is this a breaking change?
Users of the experimental postgraphile:ws:onOperation
and postgraphile:ws:onSubscribe
hooks will now receive an empty params.context
/args.contextValue
since this value is now populated after that hook is called since that hook is responsible for making sure the request parameters (e.g. the query/variables/operationName) are sensible - i.e. in the case you're using https://github.com/graphile/persisted-operations/blob/7190bb38bc98c88a11bfceb60319016a116dd292/src/index.ts#L355
I do not know of anyone using context within these hooks and since they're experimental and this is a bugfix, this can be released without a major version bump.
Checklist
- [x] My code matches the project's code style and
yarn lint:fix
passes. - [ ] ~~I've added tests for the new feature, and
yarn test
passes.~~ - [x] ~~I have detailed the new feature in the relevant documentation.~~
- [x] ~~I have added this feature to 'Pending' in the
RELEASE_NOTES.md
file (if one exists).~~ - [x] If this is a breaking change I've explained why.
I'm concerned this will cause issues for people that I'm not sure how to solve, so I'm going to leave it in draft until I figure it out.
@benjie I'd like to confirm what you were suspecting and let you know that when we tested it we did run into some issues. It seemed like we got random results back, it was very hard to track down. Mostly just "it's not showing the correct data" type of feedback.
I think it's due to replica lagging. To solve this properly for subscriptions we'd need to have the "listen" occur on the same database connection that the subscription would be executed through - at first this seems simple, but when you realise a lot of people use readOnlyConnection
via a PG proxy there's no way for us to know that the same connection string relates to strictly the same DB. One potential option, which is a bit of a hammer, is to just add an argument for "subscription delay" so you can say that subscriptions always fire 2 seconds (or whatever you want to configure) after the even occurs on the primary... but that's a) suboptimal experience, b) not even guaranteed to solve the problem.
I think for now keeping subscription events on the primary is the only option.
I'm going to give up on this one. V5 allows you to provide your own PostgreSQL adaptor, including your own subscriptions client, so this will allow everyone to build the adaptor that works best for their environment without it having to be a "handle every edge case" thing.