sql: rollback disallowing statements following schema change

[vivekmenezes]

rolls back #15339 #14368 for the most part and #14619 partially.

We used a very big hammer to prevent some statements from
following a schema change. This was done to reduce surprises
but it ended up introducing more surprises. ORMs and schema
migration tools depends on multiple schema changes executing
in the same transaction.

fixes #15269

[cockroach-teamcity]

This change is 

[petermattis]

Given that we're reverting something that seemed like a useful change, I think we should document somewhere in the code why we need to support the current behavior (i.e. because of ORM compatibility). A quick scan of the code didn't reveal a great place. Perhaps a comment associated with one of the tests being re-added.

Also, my pedantic side notes that this change doesn't allow executing multiple schema changes in a transaction (as noted in the commit message), only "staging" multiple schema changes.

Lastly, the way we handle schema changes is different than Postgres. We should review the documentation in this area and make sure the differences are clear.

---

Review status: 0 of 6 files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from Reviewable

[vivekmenezes]

I've changed the commit to say "staging" multiple schema changes and have also added a test
case for the multiple schema change transaction including running an INSERT on an auxiliary table
in the same transaction. which covers #15297

---

Review status: 0 of 6 files reviewed at latest revision, all discussions resolved, some commit checks pending.

---

Comments from Reviewable

[petermattis]

---

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

---

pkg/sql/schema_changer_test.go, line 1919 at r2 (raw file):

			`INSERT INTO t.origin VALUES ('c', 'd')`, ``},
		// Support multiple schema changes for ORMs: #15269
		// Support insert into another table after schema changes: 15297

For consistency: s/15297/#15297/g

---

Comments from Reviewable

[vivekmenezes]

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

---

pkg/sql/schema_changer_test.go, line 1919 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

For consistency: s/15297/#15297/g

Done.

---

Comments from Reviewable

[bdarnell]

For release note purposes, what are the new rules for schema changes in transactions?

---

Review status: 0 of 6 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

Comments from Reviewable

[vivekmenezes]

We want to say that we allow schema changes with other statements in a transaction, although some such transactions can fail: CREATE TABLE with FK or Interleaved table, followed by statements referencing the table #13505 DROP TABLE FOLLOWED BY CREATE fails: #12123 database writes followed by a schema change fails: INSERT on a table, followed by a table CREATE for example #7570 . This is because the transaction anchor needs to be on the system range for schema change transactions.

…

On Sun, Apr 30, 2017 at 12:05 PM Ben Darnell ***@***.***> wrote: For release note purposes, what are the new rules for schema changes in transactions? ------------------------------ Review status: 0 of 6 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful. ------------------------------ *Comments from Reviewable <https://reviewable.io:443/reviews/cockroachdb/cockroach/15511#-:-KizaoibgmOwBhSJ-08t:bnz628a>* — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#15511 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ALOpBKJBFOuJ-guehYDN_IYXyGZ6sAl9ks5r1LE7gaJpZM4NMHz0> .

[petermattis]

Cc @jseldess (in case you weren't already paying attention).

[jseldess]

Thanks, @petermattis. So to summarize, it's now safe to say that multiple schema changes, or a schema change followed by non-schema changing statements, are allowed within a single transaction, with the above exceptions?

[petermattis]

@vivekmenezes ?

[vivekmenezes]

Yes those three issues are the only known outstanding issues. We can't promise to fix them in 1.1