Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rohany and @solongordon)

pkg/sql/sqlbase/structured.go, line 2991 at r1 (raw file):

			// Actually write the new primary index into the descriptor, and remove it from the indexes list.
			// Additionally, schedule the old primary index for deletion. Note that if needed, a copy of the primary index

How come we'd create a copy of the primary index that doesn't store any columns, and then create another copy below that does store all columns? Why wouldn't the primary index always store all columns to begin with?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andy-kimball and @solongordon)

pkg/sql/sqlbase/structured.go, line 2991 at r1 (raw file):

The code implicitly assumes everywhere that primary indexes store all the columns in the table -- the StoreColumn* fields are only populated for secondary indexes. When we demote the primary index here we have to now populate these fields.

Where do you see two copies of the index being created? I might be misunderstanding your comment.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rohany and @solongordon)

pkg/sql/sqlbase/structured.go, line 2991 at r1 (raw file):

I don't know details - I was just reacting to the juxtaposition of these two comments:

// Additionally, schedule the old primary index for deletion. Note that if needed, *a copy of the primary index*
// *that doesn't store any columns* was created at the beginning of the primary key change operation.

and

// the schema change stages are correct. Additionally, we want to denote that
// *this index stores all the columns in the table.*

Just seemed strange to create a copy without stored columns, and then a copy with them, if that's indeed what's happening.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andy-kimball and @solongordon)

pkg/sql/sqlbase/structured.go, line 2991 at r1 (raw file):

Ah I see -- I think I can word this better. Whats happening is this:

When you alter the primary key, by default we create a new secondary index that indexes all of the same columns as the primary key, but doesn't store any columns. This is so that user's workloads that depend on the primary index don't suddenly start to regress in performance. That index is separate from the primary index that is being deleted right here and moved into the mutations queue.

Looks good aside from one confusing comment.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andy-kimball, @rohany, and @solongordon)

pkg/sql/sqlbase/structured.go, line 2997 at r2 (raw file):

			// We need to make the old primary key denoted with the PrimaryIndexEncoding
			// so that operations on it that rely on its encoding succeed while it is in
			// the schema change stages are correct. Additionally, we want to denote that

This sentence is kind of breaking my brain when I try to parse it. How about something like this: "Update the old primary index's descriptor to denote that it uses the PrimaryIndexEncoding and stores all columns. This ensures that it will be properly decoded if it is accessed after it is no longer the primary key but before it is dropped entirely." That's probably not quite right but you get the idea.