sql: enable adding columns which are not written to the current primary index

[postamar]

row,schemachanger: use StoreColumnIDs/Names in primary index

In a recent commit, the StoreColumnIDs and StoreColumnNames slices in
primary indexes were populated when previously they had simply been
empty. We simply assumed that all non-virtual columns in a table would
be stored in the primary index: primary key columns in the key, the rest
in the value.

This commit breaks that assumption by using the StoreColumnIDs slice to
determine what goes into the primary index. This makes it possible for
the new schema changer to add columns safely, preventing unwanted writes
to the old primary index while the schema change is underway.

Fixes #59149.

Release note: None


sql,tabledesc: add new IndexDescriptorVersion for primary indexes

Previously, the IndexDescriptorVersion type was only used to describe
the encoding of secondary indexes. This commit adds a new value for
use in primary indexes, PrimaryIndexWithStoredColumnsVersion, to signify
that the StoredColumnIDs and StoredColumnNames slices are populated
correctly.

Previously, these slices did not need to be populated at all. This is
because the set of columns comprising the primary index of a table is
assumed to be all non-virtual columns of that table. Our upcoming work on
the new schema changer will require us to violate that assumption
however. This commit is in preparation of that change.

In our effort to make meaningful the concept of stored columns in
primary indexes, this commit also changes the contents of the
information_schema.statistics table. As a result, SHOW INDEXES and SHOW
COLUMNS behave the same way regardless of whether an index is primary or
secondary.

Release note (sql change): The contents of the statistics table in the
information schema have changed, therefore so have the results of SHOW
INDEX and SHOW COLUMNS. A column which is not in the primary key will
now be listed as belonging to the primary index as a stored column.
Previously, it was simply not listed as belonging to the primary index.

[cockroach-teamcity]

This change is 

[postamar]

Rebased to fix merge conflicts.

[ajwerner]

just small things. Also looks like you've still got some merge skew.

Reviewed 65 of 68 files at r1, 4 of 12 files at r2, 14 of 14 files at r3, 12 of 12 files at r4.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @postamar)

---

pkg/sql/truncate.go, line 226 at r2 (raw file):

Quoted 8 lines of code…

	// Exit early with an error if the table is undergoing a new-style schema
	// change, before we try to get job IDs and update job statuses later. See
	// createOrUpdateSchemaChangeJob.
	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",
			tableDesc.GetName(),
		)

Should this get pushed into checkTableForDisallowedMutationsWithTruncate?

---

pkg/sql/catalog/table_elements.go, line 168 at r2 (raw file):

	GetStoredColumnID(storedColumnOrdinal int) descpb.ColumnID
	GetStoredColumnName(storedColumnOrdinal int) string
	HasOldStoredColumns() bool

I wish this concept (which I understand is not new) was better commented and explained somewhere. There's a small comment in tabledesc but it's not satisfying.

---

pkg/sql/catalog/tabledesc/structured.go, line 680 at r2 (raw file):

		indexHasOldStoredColumns := idx.HasOldStoredColumns()
		// Need to clear KeySuffixColumnIDs and StoreColumnIDs

Can you use more words to explain what's going on here? I know this existed before but this code is hard to follow.

---

pkg/sql/catalog/tabledesc/table_desc.go, line 190 at r2 (raw file):

		return true
	}

Should this be asserting something about the format of idx?

---

pkg/sql/schemachanger/schemachanger_test.go, line 417 at r4 (raw file):

func TestInsertDuringAddColumnNotWritingToCurrentPrimaryIndex(t *testing.T) {
	defer leaktest.AfterTest(t)()
	defer sqltestutils.SetTestJobsAdoptInterval()()

This no longer exists.

[postamar]

Thanks for the review. I pushed a fix to the merge skew concurrently to you posting your review. I'll address your comments ASAP.

[postamar]

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

---

pkg/sql/truncate.go, line 226 at r2 (raw file):

Previously, ajwerner wrote…

	// Exit early with an error if the table is undergoing a new-style schema
	// change, before we try to get job IDs and update job statuses later. See
	// createOrUpdateSchemaChangeJob.
	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",
			tableDesc.GetName(),
		)

Should this get pushed into checkTableForDisallowedMutationsWithTruncate?

I thought about this and my rationale for not putting it in there was that this test didn't really involve mutations. Also it's new-schemachanger stuff and that function is old-schemachanger stuff, I guessed that it would be better to keep them separate.

I don't feel strongly about any of this so if you prefer I'll push it there.

---

pkg/sql/catalog/table_elements.go, line 168 at r2 (raw file):

Previously, ajwerner wrote…

I wish this concept (which I understand is not new) was better commented and explained somewhere. There's a small comment in tabledesc but it's not satisfying.

I completely agree. I poked around and found this, which has a bit more substance to it:

cockroach/pkg/sql/information_schema.go

Lines 1117 to 1128 in eb8e1c8

	// Columns in the primary key that aren't in index.KeyColumnNames or
	// index.StoreColumnNames are implicit columns in the index.
	var implicitCols map[string]struct{}
	var hasImplicitCols bool
	if index.HasOldStoredColumns() {
	// Old STORING format: implicit columns are extra columns minus stored
	// columns.
	hasImplicitCols = index.NumKeySuffixColumns() > index.NumSecondaryStoredColumns()
	} else {
	// New STORING format: implicit columns are extra columns.
	hasImplicitCols = index.NumKeySuffixColumns() > 0
	}

I'll improve the comment on HasOldStoredColumns in light of this.

---

pkg/sql/catalog/tabledesc/structured.go, line 680 at r2 (raw file):

Previously, ajwerner wrote…

Can you use more words to explain what's going on here? I know this existed before but this code is hard to follow.

Yes this is almost certainly an omission on my part, sorry.

---

pkg/sql/catalog/tabledesc/table_desc.go, line 190 at r2 (raw file):

Previously, ajwerner wrote…

Should this be asserting something about the format of idx?

At this point the index is primary and is therefore assumed to be in the upgraded version. Adding an assertion here would be redundant due to on-commit descriptor validation... aaaand I just realized that we don't actually have a validation check for that! I'll address this.

---

pkg/sql/schemachanger/schemachanger_test.go, line 417 at r4 (raw file):

Previously, ajwerner wrote…

This no longer exists.

Thanks.

[postamar]

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)