sql: run schema changes after CREATE TABLE in txn by vivekmenezes · Pull Request #25410 · cockroachdb/cockroach

vivekmenezes · 2018-05-10T19:45:32Z

Includes a commit from #25362 and should be reviewed after that change.

cockroach-teamcity · 2018-05-10T19:45:39Z

This change is

madelynnblue · 2018-05-14T17:18:49Z

pkg/sql/logictest/testdata/logic_test/schema_change_in_txn

+DROP TABLE hood
+
+statement count 3
+CREATE TABLE hood (item, quantity) AS VALUES ('cups', 10), ('plates', 30), ('forks', 15)


This data should be different from the first CREATE just so we can verify it's indeed the second CREATE that is being queried.

madelynnblue · 2018-05-14T17:19:22Z

pkg/sql/logictest/testdata/logic_test/schema_change_in_txn

+CREATE TABLE hood (item, quantity) AS VALUES ('cups', 10), ('plates', 30), ('forks', 15)
+
+query TI
+SELECT * FROM hood


should have an ORDER BY here

madelynnblue · 2018-05-14T17:20:26Z

pkg/sql/logictest/testdata/logic_test/schema_change_in_txn

+ALTER TABLE shop RENAME TO ship
+
+statement count 3
+CREATE TABLE shop (item, quantity) AS VALUES ('cups', 10), ('plates', 30), ('forks', 15)


same: change the data here

madelynnblue · 2018-05-14T17:20:34Z

pkg/sql/logictest/testdata/logic_test/schema_change_in_txn

+CREATE TABLE shop (item, quantity) AS VALUES ('cups', 10), ('plates', 30), ('forks', 15)
+
+query TI
+SELECT * FROM shop


ORDER BY, ditto below

madelynnblue · 2018-05-14T17:22:03Z

pkg/sql/logictest/testdata/logic_test/schema_change_in_txn

+
+statement ok
+COMMIT
+


I would like to see a few more tests in the error case to make sure we handle those correctly:

CREATE TABLE; add some data, add unique index that fails due to uniqueness violations

CREATE TABLE; add computed column that fails due to an error during computation (like a divide by 0)

vivekmenezes · 2018-05-16T17:57:04Z

Thanks for review this change. I've now boosted it up quite some bit with tests and in doing so uncovered some problems. It's now ready for review. Thanks!

Review status: 0 of 7 files reviewed at latest revision, 5 unresolved discussions.

pkg/sql/logictest/testdata/logic_test/schema_change_in_txn, line 138 at r2 (raw file):

Previously, mjibson (Matt Jibson) wrote…

should have an ORDER BY here

I don't believe that's necessary right?

Comments from Reviewable

vivekmenezes · 2018-05-16T17:57:35Z

Review status: 0 of 7 files reviewed at latest revision, 5 unresolved discussions.

pkg/sql/logictest/testdata/logic_test/schema_change_in_txn, line 135 at r2 (raw file):

Previously, mjibson (Matt Jibson) wrote…

This data should be different from the first CREATE just so we can verify it's indeed the second CREATE that is being queried.

Done.

pkg/sql/logictest/testdata/logic_test/schema_change_in_txn, line 159 at r2 (raw file):

Previously, mjibson (Matt Jibson) wrote…

same: change the data here

Done.

pkg/sql/logictest/testdata/logic_test/schema_change_in_txn, line 177 at r2 (raw file):

Previously, mjibson (Matt Jibson) wrote…

I would like to see a few more tests in the error case to make sure we handle those correctly:

CREATE TABLE; add some data, add unique index that fails due to uniqueness violations

CREATE TABLE; add computed column that fails due to an error during computation (like a divide by 0)

Done.

Comments from Reviewable

vivekmenezes · 2018-05-16T18:09:31Z

Review status: 0 of 7 files reviewed at latest revision, 5 unresolved discussions.

pkg/sql/logictest/testdata/logic_test/schema_change_in_txn, line 177 at r2 (raw file):

Previously, vivekmenezes wrote…

Done.

Done.

Comments from Reviewable

madelynnblue · 2018-05-16T19:27:42Z

Review status: 0 of 7 files reviewed at latest revision, 5 unresolved discussions.

pkg/sql/backfill.go, line 621 at r3 (raw file):

	}

	if len(tableDesc.Mutations) == 0 {

nit: this block can be removed

pkg/sql/backfill.go, line 625 at r3 (raw file):

	}

	for len(tableDesc.Mutations) > 0 {

Why does this line need to be here? Can we just range over tableDesc.Mutations? I don't see how this loop would ever run more than once.

pkg/sql/table.go, line 458 at r3 (raw file):

func (tc *TableCollection) isCreatedTable(id sqlbase.ID) bool {
	if tc.createdTables == nil {

No need for this if block. Code is clearer without it.

pkg/sql/table.go, line 511 at r3 (raw file):

	dbID sqlbase.ID, tn *tree.TableName, required bool,
) (refuseFurtherLookup bool, table *sqlbase.TableDescriptor, err error) {
	// Walk latest to earliest.

Why this change?

pkg/sql/logictest/testdata/logic_test/schema_change_in_txn, line 138 at r2 (raw file):

Previously, vivekmenezes wrote…

I don't believe that's necessary right?

SQL can return data in any order if an ORDER BY is not specified. See #24223. Or you can use the rowsort flag in the logictest.

pkg/sql/logictest/testdata/logic_test/schema_change_in_txn, line 235 at r3 (raw file):


statement ok
COMMIT

You can run a COMMIT after a statement with an error? I thought you had to run ROLLBACK in this case. TIL.

Comments from Reviewable

vivekmenezes · 2018-05-17T15:12:21Z

TFTR! Ready for another viewing. Thanks

Review status: 0 of 7 files reviewed at latest revision, 10 unresolved discussions.

pkg/sql/backfill.go, line 621 at r3 (raw file):

Previously, mjibson (Matt Jibson) wrote…

nit: this block can be removed

Done.

pkg/sql/backfill.go, line 625 at r3 (raw file):

Previously, mjibson (Matt Jibson) wrote…

Why does this line need to be here? Can we just range over tableDesc.Mutations? I don't see how this loop would ever run more than once.

Done.

pkg/sql/table.go, line 458 at r3 (raw file):

Previously, mjibson (Matt Jibson) wrote…

No need for this if block. Code is clearer without it.

Good point. I didn't know a nil map was readable!

pkg/sql/table.go, line 511 at r3 (raw file):

Previously, mjibson (Matt Jibson) wrote…

Why this change?

I've added a comment to explain the need to traverse both uncommitted databases and tables in the reverse order. This is tested i this PR.

pkg/sql/logictest/testdata/logic_test/schema_change_in_txn, line 135 at r2 (raw file):

Previously, vivekmenezes wrote…

Done.

Done.

pkg/sql/logictest/testdata/logic_test/schema_change_in_txn, line 138 at r2 (raw file):

Previously, mjibson (Matt Jibson) wrote…

SQL can return data in any order if an ORDER BY is not specified. See #24223. Or you can use the rowsort flag in the logictest.

Done.

pkg/sql/logictest/testdata/logic_test/schema_change_in_txn, line 159 at r2 (raw file):

Previously, vivekmenezes wrote…

Done.

Done.

pkg/sql/logictest/testdata/logic_test/schema_change_in_txn, line 162 at r2 (raw file):

Previously, mjibson (Matt Jibson) wrote…

ORDER BY, ditto below

Done.

pkg/sql/logictest/testdata/logic_test/schema_change_in_txn, line 235 at r3 (raw file):

Previously, mjibson (Matt Jibson) wrote…

You can run a COMMIT after a statement with an error? I thought you had to run ROLLBACK in this case. TIL.

In order to be postgres compliant a COMMIT after a statement that returns an error doesn't return an error (apparently it returns an error code somewhere else in the protocol). Anyway to make this more readable I've changed all the COMMIT following an error to ROLLBACK.

Comments from Reviewable

madelynnblue · 2018-05-17T18:46:43Z

Review status: 0 of 7 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

pkg/sql/logictest/testdata/logic_test/schema_change_in_txn, line 235 at r3 (raw file):

Previously, vivekmenezes wrote…

In order to be postgres compliant a COMMIT after a statement that returns an error doesn't return an error (apparently it returns an error code somewhere else in the protocol). Anyway to make this more readable I've changed all the COMMIT following an error to ROLLBACK.

Oh neat. If that's the case I think it should be changed back to COMMIT to verify that behavior otherwise we could accidentally regress and COMMIT even in the face of an error.

Comments from Reviewable

vivekmenezes · 2018-05-17T19:04:17Z

TFTR!

Review status: 0 of 7 files reviewed at latest revision, 1 unresolved discussion.

pkg/sql/logictest/testdata/logic_test/schema_change_in_txn, line 235 at r3 (raw file):

Previously, mjibson (Matt Jibson) wrote…

Oh neat. If that's the case I think it should be changed back to COMMIT to verify that behavior otherwise we could accidentally regress and COMMIT even in the face of an error.

changed back to COMMIT

Comments from Reviewable

vivekmenezes · 2018-05-17T19:04:33Z

bors r+

craig · 2018-05-17T19:06:55Z

Merge conflict (retrying...)

This change allows execution of all schema changes following a CREATE TABLE in a transaction. Since the CREATE TABLE is invisible during the transaction to the rest of the cluster, the schema changes can executed without changing the version of the table descriptor and without running through the schema change state machine. This change is being done to make cockroachdb more compatible with postgres. fixes cockroachdb#24626 Release note (sql change): Can follow a CREATE TABLE with schema changes like CREATE INDEX on the table in the same transaction. Release note: None

vivekmenezes · 2018-05-17T20:10:05Z

bors r+

craig · 2018-05-17T20:18:41Z

Canceled (will resume)

24883: dep: Bump grpc-go version r=a-robinson a=a-robinson And pull in new packages -- in particular, the encoding/proto package isn't needed for compilation but is needed at runtime. Release note: None --------------------- We should wait to merge this until they've cut the 1.12 release tag so that we aren't just at an arbitrary commit in their git history but I'm sending out the PR now so that I (or whoever would have done this) don't have to deal with debugging the missing encoding/proto package when it comes time to merge this. As tested in #17370 (comment), this gives a 5-10% boost in whole-cluster throughput and improved tail latencies when run with a highly concurrent workload. It appears to have little performance effect for lower-concurrency workloads. 25410: sql: run schema changes after CREATE TABLE in txn r=vivekmenezes a=vivekmenezes Includes a commit from #25362 and should be reviewed after that change. 25612: util: fix `retry.WithMaxAttempts` context cancelled before run. r=windchan7 a=windchan7 If context gets cancelled right after `retry.WithMaxAttempts` runs, the function passed to it will never gets run. Now `retry.WithMaxAttempts` will at least run the function once otherwise an error will be returned. Making this change because there are places such as `show_cluster_setting.go` require the passed in function to be run at least once. Otherwise there will be seg fault. Fixes: #25600. Fixes: #25603. Fixes: #25570. Fixes: #25567. Fixes: #25566. Fixes: #25511. Fixes: #25485. Release note: None 25625: storage: Adding testing knob to disable automatic lease renewals r=a-robinson a=a-robinson In order to fix the test flakes caused by automatic lease renewals Fixes #25537 Fixes #25540 Fixes #25568 Fixes #25573 Fixes #25576 Fixes #25589 Fixes #25594 Fixes #25599 Fixes #25605 Fixes #25620 Release note: None Co-authored-by: Alex Robinson <alexdwanerobinson@gmail.com> Co-authored-by: Vivek Menezes <vivek@cockroachlabs.com> Co-authored-by: Victor Chen <victor@cockroachlabs.com>

craig · 2018-05-17T20:38:39Z

Build succeeded

GitHub CI (Cockroach)

vivekmenezes requested review from a team and madelynnblue May 10, 2018 19:45

madelynnblue reviewed May 14, 2018

View reviewed changes

vivekmenezes force-pushed the createtxn branch 4 times, most recently from d849554 to 10638ef Compare May 16, 2018 17:54

vivekmenezes force-pushed the createtxn branch from 10638ef to 256826e Compare May 16, 2018 18:09

vivekmenezes force-pushed the createtxn branch from 256826e to 4df258a Compare May 17, 2018 15:11

vivekmenezes force-pushed the createtxn branch from 4df258a to 96ebf1a Compare May 17, 2018 19:03

vivekmenezes force-pushed the createtxn branch from 96ebf1a to 5e37492 Compare May 17, 2018 20:09

craig bot merged commit 5e37492 into cockroachdb:master May 17, 2018

vivekmenezes deleted the createtxn branch May 17, 2018 20:42

Conversation

vivekmenezes commented May 10, 2018

Uh oh!

cockroach-teamcity commented May 10, 2018

Uh oh!

madelynnblue May 14, 2018

Choose a reason for hiding this comment

Uh oh!

madelynnblue May 14, 2018

Choose a reason for hiding this comment

Uh oh!

madelynnblue May 14, 2018

Choose a reason for hiding this comment

Uh oh!

madelynnblue May 14, 2018

Choose a reason for hiding this comment

Uh oh!

madelynnblue May 14, 2018

Choose a reason for hiding this comment

Uh oh!

vivekmenezes commented May 16, 2018

Uh oh!

vivekmenezes commented May 16, 2018

Uh oh!

vivekmenezes commented May 16, 2018

Uh oh!

madelynnblue commented May 16, 2018

Uh oh!

vivekmenezes commented May 17, 2018

Uh oh!

madelynnblue commented May 17, 2018

Uh oh!

vivekmenezes commented May 17, 2018

Uh oh!

vivekmenezes commented May 17, 2018

Uh oh!

craig bot commented May 17, 2018

Merge conflict (retrying...)

Uh oh!

vivekmenezes commented May 17, 2018

Uh oh!

craig bot commented May 17, 2018

Canceled (will resume)

Uh oh!

craig bot commented May 17, 2018

Build succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants