go sql: add option to panic on non-indexed queries #394

er9781 · 2020-07-03T04:16:22Z

In this commit we add the ability to instrument a connection
with a boolean that, when enabled, will have a query panic
when no index is present in a sql explain for that query.

We also add the ability to bypass this check on any given query
in SelectOptions. Additionally we add some thin wrappers to our
query functions to set this option.

This will permit us to enable this check in
tests and have people explicitly make full table scan queries
when they fully intend to.

Implementation detail:
We check both key and possible keys since there are times where key
will be non-null (eg PRIMARY) while possible keys is null, and also
times when key is null, but possible keys is not null (eg when
mysql chooses a full scan despite the presence of an index due to small
table size)

er9781 · 2020-07-03T04:17:53Z

sqlgen/db.go

-		Schema: schema,
+		Conn:           conn,
+		Schema:         schema,
+		panicOnNoIndex: false,


could let the false be implicit, but I sort of prefer being explicit on this type of stuff.

coveralls · 2020-07-03T04:19:20Z

Pull Request Test Coverage Report for Build 2944

49 of 60 (81.67%) changed or added relevant lines in 3 files are covered.
3 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.08%) to 65.407%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
sqlgen/db.go	47	51	92.16%
livesql/live.go	0	7	0.0%

Files with Coverage Reduction	New Missed Lines	%
sqlgen/reflect.go	1	78.52%
graphql/schemabuilder/input.go	2	59.23%

Totals
Change from base Build 2930:	0.08%
Covered Lines:	5748
Relevant Lines:	8788

💛 - Coveralls

EmmaOnThursday · 2020-07-06T18:10:29Z

sqlgen/db_test.go

+	db, err = db.WithPanicOnNoIndex()
+
+	// A second set will error
+	_, err = db.WithPanicOnNoIndex()


ooc why do we want to panic if we set this a second time? why not just fail silently since there is no change?

It errors, not panics, and I tend to agree with you on silently running, but other functions similar to this in the file error out. It does make sense to some extent since it indicates an odd db setup from the caller.

sqlgen/db.go

CodeBrew28 · 2020-07-08T16:21:24Z

sqlgen/db_test.go

+	assert.NoError(t, err)
+	defer tdb.Close()
+
+	db, err = db.WithPanicOnNoIndex()


Is this a safe change to merge into production. Can we safely know we haven't missed a query that doesn't use the index?

This is not currently on in production for us. There will be followup PRs where we:

only turn this on in test envs (so yes it's safe)

in that PR, we will manually tag all full scan queries as such (we know them all since they'll panic and fail tests, and if there are untested codepaths, this won't be on in production)

sqlgen/db.go

KevinXing · 2020-07-08T18:54:08Z

sqlgen/db.go

@@ -134,6 +138,15 @@ func (db *DB) WithShardLimit(shardLimit Filter) (*DB, error) {
 	return &dbCopy, nil
 }

+func (db *DB) WithPanicOnNoIndex() (*DB, error) {
+	if db.panicOnNoIndex {
+		return nil, errors.New("already is set panic on no index")


Why returning an error? Would a noop enough?

We only want to use this in test right? Can we add a comment to call it out explicitly? I am worried that people use it in production and then cause outages.

I previously had this called IsTest but thought to name it according to what it does. I'd happily add a comment as to what behavior this controls though!!

See above comment from emma re noop. I'm fine with a noop, but other similar functions in this file return an error, so I matched behavior.

KevinXing · 2020-07-08T18:56:35Z

sqlgen/db.go

+		}
+
+		explainRes, err := parseExplainResults(res)
+		res.Close()


prefer defer res.Close() right after the call.

We can't use defer res.Close() here (I had it that way initially). We need to close this query before executing the main query after the explain check

I can do this now that it's a helper!! woot

KevinXing · 2020-07-08T18:57:31Z

sqlgen/db.go

@@ -252,6 +292,36 @@ func (db *DB) BaseQuery(ctx context.Context, query *BaseSelectQuery) ([]interfac

 	clause, args := selectQuery.ToSQL()

+	if db.panicOnNoIndex && (query.Options == nil || !query.Options.AllowNoIndex) {


Can we make this branch a helper function?

KevinXing · 2020-07-09T18:05:18Z

sqlgen/db.go

+	explainRes, err := parseExplainResults(res)
+
+	// If our explain parsing fails, we don't analyze the response or fail the query.
+	if err == nil {


return if err != nil, so we can save an intent?

what do you mean "save an intent"?

sorry, i mean save a tab...
Also, we should return err is err != nil

Discussed offline, we're going to fail the query if explain parsing fails.

In this commit we add the ability to instrument a connection with a boolean that, when enabled, will have a query panic when no index is present in a sql explain for that query. We also add the ability to bypass this check on any given query in SelectOptions. Additionally we add some thin wrappers to our query functions to set this option. This will permit us to enable this check in tests and have people explicitly make full table scan queries when they fully intend to. Implementation detail: We check both key and possible keys since there are times where key will be non-null (eg PRIMARY) while possible keys is null, and also times when key is null, but possible keys is not null (eg when mysql chooses a full scan despite the presence of an index due to small table size)

KevinXing

sgtm!!

er9781 commented Jul 3, 2020

View reviewed changes

er9781 requested a review from CodeBrew28 July 3, 2020 04:18

er9781 force-pushed the simon/panic-on-no-index-option branch 2 times, most recently from 0f763f4 to 1a2ffe7 Compare July 3, 2020 04:32

er9781 requested a review from EmmaOnThursday July 3, 2020 04:32

er9781 force-pushed the simon/panic-on-no-index-option branch 3 times, most recently from b8676bc to 1d9d8ee Compare July 3, 2020 05:00

EmmaOnThursday reviewed Jul 6, 2020

View reviewed changes

CodeBrew28 reviewed Jul 8, 2020

View reviewed changes

CodeBrew28 requested a review from KevinXing July 8, 2020 16:30

KevinXing reviewed Jul 8, 2020

View reviewed changes

er9781 force-pushed the simon/panic-on-no-index-option branch from 1d9d8ee to 491db21 Compare July 9, 2020 18:01

KevinXing reviewed Jul 9, 2020

View reviewed changes

er9781 force-pushed the simon/panic-on-no-index-option branch from 491db21 to ed5411f Compare July 9, 2020 18:15

er9781 force-pushed the simon/panic-on-no-index-option branch from ed5411f to 6a7cdb8 Compare July 9, 2020 18:41

KevinXing approved these changes Jul 9, 2020

View reviewed changes

CodeBrew28 approved these changes Jul 9, 2020

View reviewed changes

er9781 merged commit 69f711c into master Jul 9, 2020

er9781 deleted the simon/panic-on-no-index-option branch July 9, 2020 19:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

go sql: add option to panic on non-indexed queries #394

go sql: add option to panic on non-indexed queries #394

er9781 commented Jul 3, 2020

er9781 Jul 3, 2020

coveralls commented Jul 3, 2020 •

edited

Loading

EmmaOnThursday Jul 6, 2020

er9781 Jul 6, 2020

CodeBrew28 Jul 8, 2020

er9781 Jul 9, 2020 •

edited

Loading

KevinXing Jul 8, 2020

KevinXing Jul 8, 2020

er9781 Jul 9, 2020

er9781 Jul 9, 2020

KevinXing Jul 8, 2020

er9781 Jul 9, 2020

er9781 Jul 9, 2020

KevinXing Jul 8, 2020

er9781 Jul 9, 2020

KevinXing Jul 9, 2020

er9781 Jul 9, 2020

KevinXing Jul 9, 2020

er9781 Jul 9, 2020

KevinXing left a comment

		@@ -252,6 +292,36 @@ func (db DB) BaseQuery(ctx context.Context, query BaseSelectQuery) ([]interfac

		clause, args := selectQuery.ToSQL()

		if db.panicOnNoIndex && (query.Options == nil \|\| !query.Options.AllowNoIndex) {

go sql: add option to panic on non-indexed queries #394

go sql: add option to panic on non-indexed queries #394

Conversation

er9781 commented Jul 3, 2020

Choose a reason for hiding this comment

coveralls commented Jul 3, 2020 • edited Loading

Pull Request Test Coverage Report for Build 2944

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

er9781 Jul 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KevinXing left a comment

Choose a reason for hiding this comment

coveralls commented Jul 3, 2020 •

edited

Loading

er9781 Jul 9, 2020 •

edited

Loading