sql,schemachanger: disallow concurrent execution for new schema changes

[thoszhang]

This PR prevents new-style schema changes from running concurrently with
any other schema changes, in two ways:

1. If there are mutations in progress (from either new or old schema
   changes) when attempting to plan a new-style schema change, we wait
   and poll until there are no mutations, and then restart the
   transaction.
2. If we try to write an old-style schema change job while there is a
   new-style schema change on the table, which is detected via a new
   field on the table descriptor for the new-style schema change job ID,
   an error is returned. This effectively prevents all schema changes,
   even the ones without mutations.

Most of this commit consists of testing. Testing knobs for the new
schema changer are introduced. We also now accumulate the statements
involved in the schema change, as strings, in the schema changer state
in extraTxnState. The executor now takes an argument to inject more
relevant state into the testing knobs, including the aforementioned
statements.

Release justification: Non-production code change (the new schema
changer is disabled for 21.1)

Release note: None

[cockroach-teamcity]

This change is 

[ajwerner]

Let's get @postamar reading these and starting to think about this.

[postamar]

Looks good to me, just a few minor comments.

[ajwerner]

nits and commentary from me, otherwise LGTM

Reviewed 19 of 24 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @lucy-zhang)

---

pkg/sql/drop_table.go, line 432 at r1 (raw file):

	if tableDesc.NewSchemaChangeJobID != 0 {
		return pgerror.Newf(pgcode.ObjectNotInPrerequisiteState,
			"cannot perform a schema change on table %q while it is undergoing a new-style schema change",

I wonder what ultimately we should do here. #61256 to track.

---

pkg/sql/schema_change_plan_node.go, line 49 at r1 (raw file):

	// undergoing other schema changes, wait for them to finish, and then restart
	// the transaction.
	if concurrentErr := (*scbuild.ConcurrentSchemaChangeError)(nil); errors.As(err, &concurrentErr) {

This handling is sort of subtle. Could you pull it out into a helper if only so that it can be a conduit for clearer commentary?
Something like maybeHanldeConcurrentSchemaChangeError

Can you also add a comment related to fairness. Namely that this waiting is not done in a fair way such that if there are multiple concurrent transactions operating over an overlapping set of descriptors but accessing them in different orders that this approach is likely to experience livelock.

---

pkg/sql/schema_change_plan_node.go, line 53 at r1 (raw file):

			"schema change waiting for concurrent schema changes on descriptor %d",
			concurrentErr.DescriptorID())

We probably want to abort the transaction prior to waiting. Otherwise we may block other transactions.

I think in terms of kv APIs, this is:

retryErr := p.txn.PrepareRetryableError(ctx, concurrentErr.Error())
txn.CleanupOnError(ctx, retryErr)

before the waiting.

I do not think you need to call ManualRestart. I believe that the layer above should do that, but I could be wrong on that.

---

pkg/sql/schemachanger/scbuild/builder.go, line 83 at r1 (raw file):

	return buf.String()
}

can you stick a TODO that it'd be better to accumulate a list of descriptors which have concurrent schema changes and that eventually we'd like to know about the largest set of descriptors we might be waiting for? We could potentially pull this set out at the end of building.

---

pkg/sql/schemachanger/scexec/executor.go, line 250 at r1 (raw file):

}

func UpdateDescriptorJobIDs(

comment

[ajwerner]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @lucy-zhang)

---

pkg/sql/schema_change_plan_node.go, line 53 at r1 (raw file):

Previously, ajwerner wrote…

We probably want to abort the transaction prior to waiting. Otherwise we may block other transactions.

I think in terms of kv APIs, this is:

retryErr := p.txn.PrepareRetryableError(ctx, concurrentErr.Error())
txn.CleanupOnError(ctx, retryErr)

before the waiting.

I do not think you need to call ManualRestart. I believe that the layer above should do that, but I could be wrong on that.

I was wrong.

[ajwerner]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @lucy-zhang)

---

pkg/sql/schema_change_plan_node.go, line 53 at r1 (raw file):

Previously, ajwerner wrote…

I was wrong.

Is there any hazard if you do both -- like create an error above the waiting and do CleanupOnError and then do the manual restart below?

[thoszhang]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @lucy-zhang)

---

pkg/sql/drop_table.go, line 432 at r1 (raw file):

Previously, ajwerner wrote…

I wonder what ultimately we should do here. #61256 to track.

Thanks.

---

pkg/sql/schema_change_plan_node.go, line 53 at r1 (raw file):

Previously, ajwerner wrote…

Is there any hazard if you do both -- like create an error above the waiting and do CleanupOnError and then do the manual restart below?

CleanupOnError finalizes the txn, so you can't call ManualRestart in that state. It results in a panic.

---

pkg/sql/schemachanger/scbuild/builder.go, line 83 at r1 (raw file):

Previously, ajwerner wrote…

can you stick a TODO that it'd be better to accumulate a list of descriptors which have concurrent schema changes and that eventually we'd like to know about the largest set of descriptors we might be waiting for? We could potentially pull this set out at the end of building.

Done. I don't know if we'd want to actually "accumulate" them in the course of normal builder logic, though, since all that code assumes we have no mutations. Seems like if we want to do this, we should just figure out the set of descriptors that need locking updates at the outset, which is probably possible.

---

pkg/sql/schemachanger/scexec/executor.go, line 250 at r1 (raw file):

Previously, ajwerner wrote…

comment

Done.

[thoszhang]

That also published my draft comments, so let me just push again.

[ajwerner]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @andreimatei, and @lucy-zhang)

---

pkg/sql/schema_change_plan_node.go, line 53 at r1 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

CleanupOnError finalizes the txn, so you can't call ManualRestart in that state. It results in a panic.

gotcha @andreimatei any interest in getting in on this game?

[ajwerner]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @andreimatei, and @lucy-zhang)

---

pkg/sql/schema_change_plan_node.go, line 53 at r1 (raw file):

Previously, ajwerner wrote…

gotcha @andreimatei any interest in getting in on this game?

The hazard here is that if we don't cleanup, then we'll be holding locks for an arbitrarily long period of time.

[thoszhang]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @andreimatei)

---

pkg/sql/schema_change_plan_node.go, line 49 at r1 (raw file):

Previously, ajwerner wrote…

This handling is sort of subtle. Could you pull it out into a helper if only so that it can be a conduit for clearer commentary?
Something like maybeHanldeConcurrentSchemaChangeError

Can you also add a comment related to fairness. Namely that this waiting is not done in a fair way such that if there are multiple concurrent transactions operating over an overlapping set of descriptors but accessing them in different orders that this approach is likely to experience livelock.

I pulled this into a separate function.

We discussed the concurrent schema change scenario offline. This isn't a concern as such because the schema changes don't run into each other's "locks" before they commit.

[ajwerner]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @andreimatei)

---

pkg/sql/schema_change_plan_node.go, line 53 at r1 (raw file):

Previously, ajwerner wrote…

The hazard here is that if we don't cleanup, then we'll be holding locks for an arbitrarily long period of time.

just spent a good while working through this with @andreimatei. I think we need to handle this rather far away from this code here. The stuff dealing with the two version invariant is quite bad.

[ajwerner]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @andreimatei)

---

pkg/sql/schema_change_plan_node.go, line 53 at r1 (raw file):

Previously, ajwerner wrote…

just spent a good while working through this with @andreimatei. I think we need to handle this rather far away from this code here. The stuff dealing with the two version invariant is quite bad.

discussed offline:

So, the current thinking is that we should propagate this error up the call stack and then in ex.makeErrEvent we should treat this structured error as a retryable but shove it into the error payload. Then in ex.txnStateTransitionApplyWrapper detect the error in the error payload and wait for the appropriate descriptor -- then update the transaction accordingly.

[thoszhang]

I got rid of the AfterStage testing knob since it's probably unsound for the executor to return an error if its ops have actually succeeded, but I didn't want to speculatively change its API either, given that it's unused.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner)

---

pkg/sql/schema_change_plan_node.go, line 53 at r1 (raw file):

Previously, ajwerner wrote…

discussed offline:

So, the current thinking is that we should propagate this error up the call stack and then in ex.makeErrEvent we should treat this structured error as a retryable but shove it into the error payload. Then in ex.txnStateTransitionApplyWrapper detect the error in the error payload and wait for the appropriate descriptor -- then update the transaction accordingly.

Done. I think this is more or less working.

[thoszhang]

I think CI is running into #61471.

[postamar]

This got merged as #64291.