crystal icon indicating copy to clipboard operation
crystal copied to clipboard

fix(subscriptions): context hook was not called with queryDocumentAst/etc

Open benjie opened this issue 2 years ago • 3 comments

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.

benjie avatar Aug 24 '21 15:08 benjie

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 avatar Oct 21 '21 10:10 benjie

@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.

johnhaley81 avatar Nov 03 '21 22:11 johnhaley81

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.

benjie avatar Nov 15 '21 11:11 benjie

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.

benjie avatar Sep 29 '23 18:09 benjie