sql: squash add and drop mutations in same transaction

[eriktrinh]

Schema elements in an adding state can be dropped if the drop happens in
the same transaction as the one which initiated the add because
the rest of the cluster is not aware that the element ever existed. This
change uses the cluster version of the mutable table descriptor to
squash these new mutations.

Related to #16020.

Release note: None

[cockroach-teamcity]

This change is 

[vivekmenezes]

I think these can be modeled in the logic tests. See the tests in logictest file schema_change_in_txn

You can also add the ADD, DROP, ADD cases there

and the ADD, DROP, RENAME (to the name used by the ADD) case

[vivekmenezes]

I believe this should only care about DROP mutations in the DELETE_AND_WRITE_ONLY state that were not present in the ClusterVersion.

[eriktrinh]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/sql/table.go, line 782 at r1 (raw file):

Previously, vivekmenezes wrote…

I believe this should only care about DROP mutations in the DELETE_AND_WRITE_ONLY state that were not present in the ClusterVersion.

All DROP mutations not present in the cluster version should be in the DELETE_AND_WRITE_ONLY state, I don't think we need to do any checks on state here.

---

pkg/sql/schema_changer_test.go, line 2411 at r1 (raw file):

Previously, vivekmenezes wrote…

I think these can be modeled in the logic tests. See the tests in logictest file schema_change_in_txn

You can also add the ADD, DROP, ADD cases there

and the ADD, DROP, RENAME (to the name used by the ADD) case

Done.

[vivekmenezes]

Thanks for putting this together! It could be nice to get the PR out that ensures that a transaction
uses only a single mutation id before we merge this one. Your call.

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/sql/table.go, line 783 at r2 (raw file):

// the same transaction, and marking the mutation jobs as
// succeeded.
func (p *planner) squashNewMutations(

perhaps rename this to maybeSquashNewMutations

---

pkg/sql/table.go, line 793 at r2 (raw file):

	addDroppedColumns := make(map[sqlbase.ColumnID]bool)
	addDroppedIndexes := make(map[sqlbase.IndexID]bool)
	for _, m := range desc.Mutations[origLen:] {

Can you add a comment here discussing what this code is doing?

---

pkg/sql/table.go, line 819 at r2 (raw file):

	var squashedIDs []sqlbase.MutationID

	for _, m := range desc.Mutations[origLen:] {

Can you add a column here as to what this code is doing.

---

pkg/sql/table.go, line 841 at r2 (raw file):

	var prev sqlbase.MutationID
	for _, id := range squashedIDs {

you should need to only squash a single mutation ID corresponding to the mutation ID of the transaction being processed.

How about adding a TODO to fix the issue of using a mutation ID for each transaction. Even better if you can fix that issue and then merge this PR after that one.

---

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

statement ok
COMMIT

Can you add a SHOW JOBS check to ensure the jobs are cleaned up properly

[eriktrinh]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/sql/table.go, line 783 at r2 (raw file):

Previously, vivekmenezes wrote…

perhaps rename this to maybeSquashNewMutations

Done.

---

pkg/sql/table.go, line 793 at r2 (raw file):

Previously, vivekmenezes wrote…

Can you add a comment here discussing what this code is doing?

Done.

---

pkg/sql/table.go, line 819 at r2 (raw file):

Previously, vivekmenezes wrote…

Can you add a column here as to what this code is doing.

Done.

---

pkg/sql/table.go, line 841 at r2 (raw file):

Previously, vivekmenezes wrote…

you should need to only squash a single mutation ID corresponding to the mutation ID of the transaction being processed.

How about adding a TODO to fix the issue of using a mutation ID for each transaction. Even better if you can fix that issue and then merge this PR after that one.

Rebased after merging that PR, PTAL.

---

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

Previously, vivekmenezes wrote…

Can you add a SHOW JOBS check to ensure the jobs are cleaned up properly

Done.

[cockroach-teamcity]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

---

Erik Trinh seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}