`CheckpointHarness` and analyzer mutexes #995

max-hoffman · 2022-05-04T21:45:29Z

No description provided.

zachmu

Not super happy with these interface choices. See the comments.

On a meta note, I think this is probably a lot cleaner if you separate out the changes to make things parallel and just focus on the checkpoint stuff for this PR.

I'll hold off reviewing the dolt PR until you respond to these comments unless you think I should look now.

enginetest/enginetests.go

zachmu · 2022-05-05T21:13:56Z

enginetest/queries.go

@@ -6863,6 +6761,111 @@ var KeylessQueries = []QueryTest{
 	},
 }

+var ParallelUnsafeQueries = []QueryTest{


Not at all obvious why most of these are unsafe to run in parallel

zachmu · 2022-05-05T21:17:29Z

enginetest/testdata.go

 // createSubsetTestData creates test tables and data. Passing a non-nil slice for includedTables will restrict the
 // table creation to just those tables named.
 func CreateSubsetTestData(t *testing.T, harness Harness, includedTables []string) []sql.Database {
 	dbs := harness.NewDatabases("mydb", "foo")
+	if spatialSupported {


Do not do this

Either a) make the harness itself decide which tables to include in test setup in some way or b) figure out how to make this separable via a distinct test method, as it was before this

zachmu · 2022-05-05T21:17:49Z

memory/table.go

@@ -61,6 +61,53 @@ type Table struct {
 	autoColIdx int
 }

+func CopyTable(t *Table) *Table {


Still an issue

zachmu · 2022-05-05T21:26:17Z

sql/information_schema/information_schema.go

@@ -1863,6 +1913,8 @@ func (t *informationSchemaTable) Schema() Schema {
 }

 func (t *informationSchemaTable) AssignCatalog(cat Catalog) Table {


These changes are kind of nonsensical

It doesn't matter if you prevent a data race in assigning the catalog to these table objects if there is one global copy of each that gets used across all sessions / queries. Sure you are preventing literal races, but it's still the case that e.g. the same table object in use by one session about to start returning rows can have its catalog object re-assigned by another session. The mutex needs to guard the unit of work, which is the transaction. It's just not a workable solution in this context.

What you need to do return a new copy of each table every time when asked, rather than keeping a single global instance of each.

You can avoid doing this (I think) if you just back out the parallel changes for now.

zachmu · 2022-05-05T21:29:37Z

enginetest/harness.go

+type CheckpointHarness interface {
+	Harness
+	// RestoreCheckpoint resets the database to a saved point
+	RestoreCheckpoint(*sql.Context, *testing.T, *sqle.Engine) *sqle.Engine


Rather than this, make NewEngine the sole method in CheckpointHarness. Then in each test method, if the harness implements CheckpointHarness, get a new engine from that. Otherwise, call enginetests.NewEngine().

Then the lifecycle of these objects is clear: it does any initialization work (including data setup) at instantiation, and creates a new blank engine with that initial state every time this method is called. No need for explicit checkpoint restore operations.

Also document this lifecycle in these interfaces/

…ql-server into max/versioned-enginetests

This reverts commit 43194b9.

max-hoffman · 2022-05-13T19:04:11Z

enginetest/enginetests.go

 			{"a2", "a3"},
 			{"a4", "a3"},
-		}, nil)
-
-		// Assert that query plan this follows correctly uses an IndexedTableAccess


super redundant test that I've had to fix 5-6 times

max-hoffman · 2022-05-13T19:08:38Z

enginetest/memory_engine_test.go

@@ -426,171 +434,6 @@ func TestIndexQueryPlans(t *testing.T) {
 	}
 }

-// This test will write a new set of query plan expected results to a file that you can copy and paste over the existing


moved to helper file

…ql-server into max/versioned-enginetests

zachmu

Overall this is quite good, so much better than what we had before.

My main concerns are 1) formalizing codegen of setup scripts and 2) getting rid of the old NewTable etc. methods from Harness. Both can happen on a second pass, you should get what you have in ASAP.

zachmu · 2022-05-18T20:43:36Z

memory/table.go

@@ -61,6 +61,53 @@ type Table struct {
 	autoColIdx int
 }

+func CopyTable(t *Table) *Table {


Still an issue

sql/plan/show_indexes.go

zachmu · 2022-05-18T21:04:07Z

enginetest/test_writers_test.go

+// This test will write a new set of query plan expected results to a file that you can copy and paste over the existing
+// query plan results. Handy when you've made a large change to the analyzer or node formatting, and you want to examine
+// how query plans have changed without a lot of manual copying and pasting.
+func TestWriteQueryPlans(t *testing.T) {


I think I like this textual format better

But if we're going to go this route we need full code gen. Write a tool that takes the textual representations and transforms them into .go files with data structures

zachmu · 2022-05-18T21:05:37Z

enginetest/queries/trigger_queries.go

-			"insert into test.a values (0), (2), (4), (6), (8)",
-			"insert into test.b values (1), (3), (5), (7), (9)",
-			"use test",
+			"create table foo.a (x int primary key)",


Why change this? I can't create a database, or you just didn't think it was appropriate for this test?

can't reset or clear a new database. I could do cartwheels in the reset scripts to get and delete unexpected dbs, but this was easier.

enginetest/enginetests.go

zachmu · 2022-05-18T21:40:30Z

enginetest/enginetests.go

@@ -2045,13 +1793,13 @@ func TestScriptWithEnginePrepared(t *testing.T, e *sqle.Engine, harness Harness,
 }

 func TestTransactionScripts(t *testing.T, harness Harness) {
-	for _, script := range TransactionTests {
+	for _, script := range queries.TransactionTests {


no Setup() here?

zachmu · 2022-05-18T21:50:17Z

enginetest/enginetests.go

 	})
 }

 func TestScripts(t *testing.T, harness Harness) {
-	for _, script := range ScriptTests {
+	for _, script := range queries.ScriptTests {


No Setup()?

zachmu · 2022-05-18T21:55:22Z

enginetest/file_setup.go

+	Data() Testdata
+}
+
+type Testdata struct {


Not sure how much of this is a draft

But this struct should be used solely for parsing the test data files / code gen. We should have generated golang data structures checked into source corresponding to each of the textual setup files

…ql-server into max/versioned-enginetests

max-hoffman added 11 commits May 2, 2022 08:29

starter

45ac6f9

Merge branch 'main' into max/versioned-enginetests

57793be

some tests passing

373cb26

prog

d5a861c

merge main

a8593ef

faster

892be2e

cleanup

5861076

fix races

50a90c6

format

72612ca

cleanup

66d65cc

parallel

608bef3

max-hoffman force-pushed the max/versioned-enginetests branch from e0074c8 to 608bef3 Compare May 5, 2022 20:07

Merge branch 'main' into max/versioned-enginetests

c67ad01

max-hoffman requested a review from zachmu May 5, 2022 20:42

max-hoffman assigned zachmu May 5, 2022

zachmu reviewed May 5, 2022

View reviewed changes

max-hoffman and others added 14 commits May 6, 2022 09:08

starter

74e16de

prog

5332557

[ga-format-pr] Run ./format_repo.sh to fix formatting

2a812d8

prog

6b11ffa

Merge branch 'max/versioned-enginetests' of github.com:dolthub/go-mys…

0094956

…ql-server into max/versioned-enginetests

merge main

25365c6

prog

ac01e0e

complex index tests fast again

9b7223c

merge

43194b9

Revert "merge"

47fc1e8

This reverts commit 43194b9.

stash

d937eb7

merge stash

472a144

prog

aa26a1e

revert versioned tests

7eaf240

max-hoffman commented May 13, 2022

View reviewed changes

max-hoffman added 12 commits May 13, 2022 12:11

cleanup

c7abdd8

revert readOnlyDatabase

8441c8f

drop extraneous tests

e9bc14c

refactor new test

94f800b

queries in folder

39494cb

format

5d17410

Merge branch 'max/versioned-enginetests' of github.com:dolthub/go-mys…

e193eef

…ql-server into max/versioned-enginetests

tests in go structs

a27da2b

prog

29dfd19

merge main

cdc6900

hacky fix for tests

e1697e6

Merge branch 'main' into max/versioned-enginetests

3f4513b

zachmu approved these changes May 18, 2022

View reviewed changes

max-hoffman and others added 13 commits May 18, 2022 16:39

prog

d703c51

prog

2401d38

merge

06ef369

new storage engine edits

386a625

[ga-format-pr] Run ./format_repo.sh to fix formatting

039d137

skip codegen tets

1b91d9c

Merge branch 'max/versioned-enginetests' of github.com:dolthub/go-mys…

3e91ac5

…ql-server into max/versioned-enginetests

refactor scriptgen

13b2371

prog remove old harness interface

852a651

fix orderby refactor

c449331

copyright headers

57fe2c5

better scriptgen package nesting

28ea779

[ga-format-pr] Run ./format_repo.sh to fix formatting

5349494

max-hoffman merged commit 86955e3 into main May 19, 2022

max-hoffman deleted the max/versioned-enginetests branch May 19, 2022 23:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`CheckpointHarness` and analyzer mutexes #995

`CheckpointHarness` and analyzer mutexes #995

max-hoffman commented May 4, 2022

zachmu left a comment

zachmu May 5, 2022

zachmu May 5, 2022

zachmu May 5, 2022

zachmu May 18, 2022

zachmu May 5, 2022

zachmu May 5, 2022

zachmu May 5, 2022

max-hoffman May 13, 2022

max-hoffman May 13, 2022

zachmu left a comment

zachmu May 18, 2022

zachmu May 18, 2022

zachmu May 18, 2022

max-hoffman May 18, 2022

zachmu May 18, 2022

zachmu May 18, 2022

zachmu May 18, 2022

		@@ -1863,6 +1913,8 @@ func (t *informationSchemaTable) Schema() Schema {
		}

		func (t *informationSchemaTable) AssignCatalog(cat Catalog) Table {

CheckpointHarness and analyzer mutexes #995

CheckpointHarness and analyzer mutexes #995

Conversation

max-hoffman commented May 4, 2022

zachmu left a 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

zachmu left a 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

`CheckpointHarness` and analyzer mutexes #995

`CheckpointHarness` and analyzer mutexes #995