This looks awkward to me on first sight. We used to return error early before we write schema change where a AssertionError is returned. Now since we remove the validation, user would see a trace for the error plus some descriptor information: http://github.com/cockroachdb/cockroach/blob/master/pkg/sql/table.go#L287-L287 Not sure why we wanted to assert in this situation, but I think it's legit to error out with trace since invalid descriptor is a big deal...

I don't follow how or why we broke this. We should definitely retain this behavior. It's rather important.

it used to return one single error message, because we just return the validation error message from AllocateIDs.

now we remove the validation in AllocateIDs, we validate when writing schema change, and it error with a more severe type AssertionError and added table descriptor data in the error message as well. so it would look like the very loud error message below. While I can match the whole block of strings with wildcard, logic test told me that serious error with code "XX000" occurred; if expected, must use 'error pgcode XX000 ...'. Let me know if I did any better explaining this...

pq: internal error: table descriptor is not valid
tabledesc.Mutable: {ID: 73, Version: 2, IsUncommitted: true, ModificationTime: "0,0", ParentID: 54, ParentSchemaID: 55, State: PUBLIC, NextColumnID: 3, Columns: [{ID: 1, TypeID: 20, Null: false}, {ID: 2, TypeID: 20, Null: false}], NextFamilyID: 1, Families: [{ID: 0, Columns: [1, 2]}], MutationJobs: [{MutationID: 1, JobID: 719608795391819777}], Mutations: [{MutationID: 1, Direction: ADD, State: DELETE_ONLY, Index: {ID: 2, Unique: true, KeyColumns: [{ID: 2, Dir: ASC}], StoreColumns: [1], State: ADD, MutationID: 1}}, {MutationID: 1, Direction: ADD, State: DELETE_ONLY, Index: {ID: 3, Unique: true, KeyColumns: [{ID: 1, Dir: ASC}], KeySuffixColumns: [2], State: ADD, MutationID: 1}}, {MutationID: 1, Direction: ADD, State: DELETE_ONLY, PrimaryKeySwap: {OldPrimaryIndexID: 1, OldIndexes: [], NewPrimaryIndexID: 2, NewIndexes: []}}, {MutationID: 1, Direction: ADD, State: DELETE_ONLY, Index: {ID: 4, Unique: false, KeyColumns: [{ID: 2, Dir: ASC}], KeySuffixColumns: [1], State: ADD, MutationID: 1}}], PrimaryIndex: 1, NextIndexID: 5, Indexes: [{ID: 1, Unique: true, KeyColumns: [{ID: 1, Dir: ASC}], StoreColumns: [2]}]}
: relation "t" (73): unimplemented: cannot perform other schema changes in the same transaction as a primary key change
HINT: You have encountered an unexpected error.
Please check the public issue tracker to check whether this problem is
already tracked. If you cannot find it there, please report the error
with details by creating a new issue.
If you would rather not post publicly, please contact us directly
using the support form.
We appreciate your feedback.
DETAIL: stack trace:
github.com/cockroachdb/cockroach/pkg/sql/table.go:287: writeTableDescToBatch()
github.com/cockroachdb/cockroach/pkg/sql/table.go:265: writeTableDesc()
github.com/cockroachdb/cockroach/pkg/sql/table.go:235: writeSchemaChange()
github.com/cockroachdb/cockroach/pkg/sql/create_index.go:686: startExec()
github.com/cockroachdb/cockroach/pkg/sql/plan.go:515: func2()
github.com/cockroachdb/cockroach/pkg/sql/walk.go:112: func1()
github.com/cockroachdb/cockroach/pkg/sql/walk.go:295: visitInternal()
github.com/cockroachdb/cockroach/pkg/sql/walk.go:79: visit()
github.com/cockroachdb/cockroach/pkg/sql/walk.go:43: walkPlan()
github.com/cockroachdb/cockroach/pkg/sql/plan.go:518: startExec()
github.com/cockroachdb/cockroach/pkg/sql/plan_node_to_row_source.go:146: Start()
github.com/cockroachdb/cockroach/pkg/sql/colexec/columnarizer.go:157: Init()
github.com/cockroachdb/cockroach/pkg/sql/colexec/invariants_checker.go:68: Init()
github.com/cockroachdb/cockroach/pkg/sql/colflow/stats.go:87: Init()
github.com/cockroachdb/cockroach/pkg/sql/colflow/flow_coordinator.go:235: func1()
github.com/cockroachdb/cockroach/pkg/sql/colexecerror/error.go:91: CatchVectorizedRuntimeError()
github.com/cockroachdb/cockroach/pkg/sql/colflow/flow_coordinator.go:234: init()
github.com/cockroachdb/cockroach/pkg/sql/colflow/flow_coordinator.go:275: Run()
github.com/cockroachdb/cockroach/pkg/sql/colflow/vectorized_flow.go:259: Run()
github.com/cockroachdb/cockroach/pkg/sql/distsql_running.go:553: Run()
github.com/cockroachdb/cockroach/pkg/sql/distsql_running.go:1368: PlanAndRun()
github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:1385: execWithDistSQLEngine()
github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:1062: dispatchToExecutionEngine()
github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:700: execStmtInOpenState()
github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:129: execStmt()
github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:1748: func1()
github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:1750: execCmd()
github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:1672: run()
github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:679: ServeConn()
github.com/cockroachdb/cockroach/pkg/sql/pgwire/conn.go:666: func1()

My bad, just realized that I can match with something like error pgcode XX000 (the error message regex here)

The validation code was the earliest attempt to not just validate structure but also input. That turned out to be a rather bad move. As a principle, any time an internal error bubbles up to the client, it should be treated as a bug. I think the right answer here is to detect the invalid condition earlier and explicitly before attempting to modify the descriptor. Please add code to do that so that the error does not change. The end result will be cleaner and more explicit.

One possible small improvement for alter primary key. When we alter primary key, we create a unique index to match the old primary key. However, it doest that no matter if there's already a same unique key exists or not. We make add some deduplication here. Though I don't think this is a must have. Let me know if it make sense to create an issue for it.

I'll be honest, I don't understand this line

consider using commentary to describe this logic and its motivation.

I think the idiom here is:

for i := 0; i < idx.NumKeySuffixColumns(); i++ {
     colID := idx.GetKeySuffixColumnID(i)

nit: period.

Why is it okay to skip columns which do not exist?

Something smells here. Consider the case where the index is adding and is not in the new primary key? what about:

if !col.IsVirtual() {
    continue
}
if idx.Adding() && newPKColIDs.Contains(colID) || 
 !idx.Adding() && curPKColIDs.Contains(colID) {
    errors.Newf("index %q cannot have virtual column %q in its key suffix columns which is not a key column of the corresponding primary key", idx.GetName(), col.GetName())
}

Can you add some cases for your primary key swap situations and virtual columns?