Add support for EXPLAIN options to GraphiQL #1618

angelyan · 2022-04-14T21:12:52Z

Description

Allows setting all available EXPLAIN options (analyze, verbose, costs, settings, buffers, wal, timing, summary, format) from the toolbar.
Adds buttons to copy the SQL query and plan to the clipboard.

Screenshots

"Options" and "Format" menu items are added next to "Explain".

The "Options" menu allows toggling of all boolean options. Note: An asterisk appears to the left of "TIMING" as it is enabled but it requires "ANALYZE" to be enabled as well.

The "Format" menu allows changing between all output formats (text, json, yaml, xml).

Fixes #1235.

Performance impact

unknown

Security impact

unknown

Checklist

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.
I have detailed the new feature in the relevant documentation.
I have added this feature to 'Pending' in the RELEASE_NOTES.md file (if one exists).
If this is a breaking change I've explained why.

benjie · 2022-04-15T09:14:34Z

This is very cool! One significant concern that I have currently is that if we allow explain analyze then for mutations the operation will run twice (once for the explain analyze and once again for the mutation itself) - and this is the main reason that I shyed away from adding that option. I'd like this to be addressed before anything that allows explain analyze to be enabled, but I don't know how best to do this. We could just disable it for mutation operations on the server side, but I'm not sure if this is sufficiently obvious to the user?

benjie · 2022-04-15T09:17:34Z

postgraphiql/src/components/PostGraphiQL.js

@@ -1,12 +1,11 @@
 import React from 'react';
 import GraphiQL from 'graphiql';
-import { getOperationAST, parse } from 'graphql';
+import {buildClientSchema, getIntrospectionQuery, getOperationAST, GraphQLObjectType, isType, parse} from 'graphql';


I do not understand why prettier is not formatting this file in the way I'd expect. It doesn't seem to be in the prettierignore file, and it doesn't seem to be excluded from the prettier glob

https://github.com/graphile/postgraphile/blob/55bff41460b113481c8161ef8f178f5af0a17df3/package.json#L48

And yet, CI passes. Very strange. I will have to investigate at some point.

angelyan · 2022-04-15T18:26:19Z

One significant concern that I have currently is that if we allow explain analyze then for mutations the operation will run twice

What about ~~running the EXPLAIN in a transaction and then rolling it back~~ creating a savepoint before the EXPLAIN and then roll back to it?

benjie · 2022-04-15T23:29:52Z

Other than leaking through things like sequences (serial type, for example) I can’t think of a particularly good reason not to do that. We’d have to know if we’re in a transaction or not: if we are we should always savepoint, and if not then it’s not a mutation so no need to worry. Good idea 👍

To avoid running the mutation twice when ANALYZE is enabled

angelyan · 2022-04-19T22:52:55Z

@benjie I added the savepoint. Could you do another pass when you get a chance?

benjie · 2022-04-20T10:45:10Z

src/postgraphile/withPostGraphileContext.ts

+              explainPromise = pgClient[$$pgClientOrigQuery]
+                // Create a savepoint before running the EXPLAIN, so we can roll back to it to avoid running mutations
+                // twice when ANALYZE is enabled.
+                .call(this, 'savepoint postgraphile_explain')
+                .then(
+                  // Savepoint created - we are in a transaction, which means this is a mutation. We need to roll back
+                  // to the savepoint after running the EXPLAIN.
+                  () => pgClient[$$pgClientOrigQuery].call(this, explain, values)
+                    .then((data: any) => pgClient[$$pgClientOrigQuery]
+                      .call(this, 'rollback to savepoint postgraphile_explain')
+                      .then(() => data)),
+                  // Failed to create savepoint - we are not in a transaction, which means this is a query. No need to
+                  // rollback the EXPLAIN.
+                  () => pgClient[$$pgClientOrigQuery].call(this, explain, values)
+                )
+                .then((data: any) => data.rows)
+                // swallow errors during explain
+                .catch(() => null);


This isn't safe because our queries can occur in parallel, and thus the rollback might be issued out of order (or there may even be concurrent rollbacks for the same savepoint!)

Instead you must ensure that they are all added in the same tick so that they get issued in order without a chance for other (parallel) queries to be inserted in between:

// IMPORTANT: the following 3 lines MUST BE CALLED SYNCHRONOUSLY. pgClient[$$pgClientOrigQuery].query('savepoint postgraphile_explain'); const explainResultPromise = pgClient[$$pgClientOrigQuery].query(explain, values); pgClient[$$pgClientOrigQuery].query('rollback savepoint postgraphile_explain'); // Now use `explainResultPromise` however you want.

…from interleaving

angelyan · 2022-04-27T23:09:13Z

@benjie this is ready for another pass!

benjie

Excellent work - looking good! Ideally you should force a transaction for queries that involve EXPLAIN so we don't get the errors from SAVEPOINT / ROLLBACK (which I'm almost certain I'll start hearing about in a few months time...); but if you can't figure out how to do that I'm happy to do that as part of the final cleanup before merge 👍

benjie · 2022-05-03T08:19:03Z

postgraphiql/src/components/PostGraphiQL.js

    haveActiveSubscription: false,
    socketStatus: POSTGRAPHILE_CONFIG.websockets === 'none' ? null : 'pending',
  };

+  parseFromStorage(key) {
+    const value = this._storage.get(key);
+    return value ? JSON.parse(value) : undefined;


This should probably be wrapped with a try/catch

benjie · 2022-05-03T08:20:46Z

postgraphiql/src/components/PostGraphiQL.js

@@ -375,6 +400,7 @@ class PostGraphiQL extends React.PureComponent {
          ...(this.state.explain && POSTGRAPHILE_CONFIG.allowExplain
            ? { 'X-PostGraphile-Explain': 'on' }
            : null),
+          'X-PostGraphile-Explain-Options': buildExplainOptionsHeader(this.state.explainOptions)


I wonder if we should just replace the { 'X-PostGraphile-Explain': 'on' } header with

{ 'X-PostGraphile-Explain': `on; ${buildExplainOptionsHeader(this.state.explainOptions)}` }

?

src/postgraphile/http/createPostGraphileHttpRequestHandler.ts

benjie · 2022-05-03T08:28:05Z

src/postgraphile/http/createPostGraphileHttpRequestHandler.ts

        explain: allowExplain && req.headers['x-postgraphile-explain'] === 'on',
+        explainOptions: parseExplainOptions(req),


Let's merge explain/explainOptions together. We don't need to support any legacy here since PostGraphile and PostGraphiQL are deeply integrated and tied to the exact same versions, so unified changes are fine - no need for backwards compatibility.

Suggested change

explain: allowExplain && req.headers['x-postgraphile-explain'] === 'on',

explainOptions: parseExplainOptions(req),

explain: allowExplain && parseExplainOptions(req),

benjie · 2022-05-03T08:28:54Z

src/postgraphile/http/createPostGraphileHttpRequestHandler.ts

@@ -1175,6 +1209,7 @@ function addCORSHeaders(res: PostGraphileResponse): void {
      'Content-Length',
      // For our 'Explain' feature
      'X-PostGraphile-Explain',
+      'X-PostGraphile-Explain-Options',


Thinking about it, I'm very keen that we don't introduce another header because it means people may have to rewrite their nginx, CORS and similar config.

angelyan · 2022-05-13T03:21:56Z

@benjie I addressed everything you commented except for forcing the transaction. Couldn't figure out a nice way to do that, so it's all yours.

benjie · 2022-05-16T09:53:00Z

Thanks! Just a heads up that it's going to be a little while before I get around to re-reviewing this, I'm focussed on V5 work currently. I'm excited though!

benjie

So sorry, this slipped off of my TODO list (the notification got removed from my GitHub notifications somehow), I only just noticed it again 😓

I don't have permission to push to your master branch so I've applied a number of minor fixes and tweaks to the explain branch: https://github.com/graphile/crystal/tree/explain

However, the main reason that this can't be merged currently is that it looks ripe for SQL injection attacks. Inserting user-submitted content (sent from a header) directly into an SQL query's raw text is a recipe for disaster. We need server-side validation of the values, and I'd rather see an exhaustive "if options.analyze then sql.push('analyze')" for each option that makes it extremely explicit exactly what text is being pushed into the explain query.

benjie · 2024-02-08T12:02:40Z

src/postgraphile/withPostGraphileContext.ts

+              // Build the EXPLAIN command
+              let explainCommand = 'explain';
+              const explainOptionClauses = Object.entries(pgClient._explainOptions ?? {}).map(
+                ([option, value]) => `${option} ${value}`,


_explainOptions isn't safe to use like this, this opens PostGraphile up to SQL injection attacks.

angelyan · 2024-02-08T13:13:45Z

Hi @benjie. I will not continue working on this PR. I'm happy to hand it off to you if you want. Otherwise, I'll be closing it and deleting my fork.

benjie · 2024-02-08T16:15:48Z

Hi @angelyan; no worries and sorry I dropped the ball here on the review time, I really don't know how the notification disappeared from my inbox. I'll be adding something like this to PostGraphile V5 and your code now exists on the explain branch so you can feel free to close and delete the fork. All the best!

angelyan · 2024-02-08T16:31:11Z

No worries. Thank you!

angelyan added 5 commits April 14, 2022 15:42

Add options to Postgres client's explain

960090b

Support explain options via postgraphile http header

5dbbd23

Support explain options in postgraphiql

03daa6e

Add button to copy SQL query and plan to postgraphiql

0016e0a

Run linter

daee88d

benjie reviewed Apr 15, 2022

View reviewed changes

angelyan added 2 commits April 16, 2022 15:13

Roll back EXPLAIN query for mutations

999cf6f

To avoid running the mutation twice when ANALYZE is enabled

Run prettier

b68688e

angelyan requested a review from benjie April 16, 2022 18:16

benjie requested changes Apr 20, 2022

View reviewed changes

Issue explain queries synchronously to keep other concurrent queries …

059a3a9

…from interleaving

angelyan requested a review from benjie April 20, 2022 12:39

Remove unnecessary promise chaining

22cdbc3

Merge remote-tracking branch 'origin/main'

390872c

benjie requested changes May 3, 2022

View reviewed changes

Merge explain and explain options headers

3333a62

angelyan requested a review from benjie May 13, 2022 03:22

benjie mentioned this pull request Oct 4, 2023

GraphiQL / Explain Improvements #1235

Open

3 tasks

benjie requested changes Feb 8, 2024

View reviewed changes

angelyan closed this Feb 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for EXPLAIN options to GraphiQL #1618

Add support for EXPLAIN options to GraphiQL #1618

angelyan commented Apr 14, 2022

benjie commented Apr 15, 2022

benjie Apr 15, 2022

angelyan commented Apr 15, 2022 •

edited

Loading

benjie commented Apr 15, 2022

angelyan commented Apr 19, 2022

benjie Apr 20, 2022 •

edited

Loading

angelyan commented Apr 27, 2022

benjie left a comment

benjie May 3, 2022

benjie May 3, 2022

benjie May 3, 2022

benjie May 3, 2022

angelyan commented May 13, 2022

benjie commented May 16, 2022

benjie left a comment

benjie Feb 8, 2024

angelyan commented Feb 8, 2024

benjie commented Feb 8, 2024

angelyan commented Feb 8, 2024

		explain: allowExplain && req.headers['x-postgraphile-explain'] === 'on',
		explainOptions: parseExplainOptions(req),

	explain: allowExplain && req.headers['x-postgraphile-explain'] === 'on',
	explainOptions: parseExplainOptions(req),
	explain: allowExplain && parseExplainOptions(req),

Add support for EXPLAIN options to GraphiQL #1618

Add support for EXPLAIN options to GraphiQL #1618

Conversation

angelyan commented Apr 14, 2022

Description

Screenshots

Performance impact

Security impact

Checklist

benjie commented Apr 15, 2022

benjie Apr 15, 2022

Choose a reason for hiding this comment

angelyan commented Apr 15, 2022 • edited Loading

benjie commented Apr 15, 2022

angelyan commented Apr 19, 2022

benjie Apr 20, 2022 • edited Loading

Choose a reason for hiding this comment

angelyan commented Apr 27, 2022

benjie left a comment

Choose a reason for hiding this comment

benjie May 3, 2022

Choose a reason for hiding this comment

benjie May 3, 2022

Choose a reason for hiding this comment

benjie May 3, 2022

Choose a reason for hiding this comment

benjie May 3, 2022

Choose a reason for hiding this comment

angelyan commented May 13, 2022

benjie commented May 16, 2022

benjie left a comment

Choose a reason for hiding this comment

benjie Feb 8, 2024

Choose a reason for hiding this comment

angelyan commented Feb 8, 2024

benjie commented Feb 8, 2024

angelyan commented Feb 8, 2024

angelyan commented Apr 15, 2022 •

edited

Loading

benjie Apr 20, 2022 •

edited

Loading