sql: TestAmbiguousCommit is susceptible to deadlock

[tamird]

Spotted in https://teamcity.cockroachdb.com/viewLog.html?tab=buildLog&logTab=tree&filter=debug&expand=all&buildId=35691#_focus=8201

Interesting part of the log:

[19:47:18][TestAmbiguousCommit] goroutine 2742 [chan receive, 1 minutes]:
[19:47:18][TestAmbiguousCommit] github.com/cockroachdb/cockroach/pkg/sql_test.TestAmbiguousCommit(0xc4209b6240)
[19:47:18][TestAmbiguousCommit]     /go/src/github.com/cockroachdb/cockroach/pkg/sql/ambiguous_commit_test.go:140 +0x7e7
[19:47:18][TestAmbiguousCommit] testing.tRunner(0xc4209b6240, 0x19e3ff8)
[19:47:18][TestAmbiguousCommit]     /usr/local/go/src/testing/testing.go:610 +0x81
[19:47:18][TestAmbiguousCommit] created by testing.(*T).Run
[19:47:18][TestAmbiguousCommit]     /usr/local/go/src/testing/testing.go:646 +0x2ec

Also reproduced locally:

make stress PKG=./pkg/sql TESTS=TestAmbiguousCommit TESTTIMEOUT=30s
6 runs so far, 0 failures, over 5s
6 runs so far, 0 failures, over 10s
10 runs so far, 0 failures, over 15s
14 runs so far, 0 failures, over 20s
16 runs so far, 0 failures, over 25s
17 runs so far, 0 failures, over 30s
23 runs so far, 0 failures, over 35s
31 runs so far, 0 failures, over 40s
36 runs so far, 0 failures, over 45s
...
goroutine 7 [chan receive]:
github.com/cockroachdb/cockroach/pkg/sql_test.TestAmbiguousCommit(0xc42008c540)
    /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/pkg/sql/ambiguous_commit_test.go:140 +0x7e7
testing.tRunner(0xc42008c540, 0x54bade0)
    /Users/tamird/src/go1.7/src/testing/testing.go:610 +0x81
created by testing.(*T).Run
    /Users/tamird/src/go1.7/src/testing/testing.go:646 +0x2ec

So looks like there's a real problem with the test.

[spencerkimball]

This is not related to the PR to introduce TestAmbiguousCommit. This is caused by the range not being added to the split queue in time. Who authored the code to add newly created SQL tables to the split queue?

[petermattis]

Who authored the code to add newly created SQL tables to the split queue?

I don't think we do this. AFAIK, we only split off newly created SQL tables when the scanner encounters them naturally. Some tests lower the scan-max-idle-time to account for this.

[spencerkimball]

It's done when the system config is gossiped.

[petermattis]

It's done when the system config is gossiped.

Ah, so it is. You can answer your own question using git blame. Looks like this is pretty old code. See b253376.

[spencerkimball]

This is now a duplicate of #10160