sql: fix DROP COLUMN bug which ignores containing indexes

[otan]

AllNonDropIndexes returns an array of pointers to index descriptors.
After deleting one of the indexes, the array of pointers point
to a different element. This ends up accidentally missing some
indexes that require deletion.

Resolves #54196 (introduced in #51661).

Release note (bug fix): Fixed a bug introduced in earlier v20.2 versions
where when attempting to drop a column which is referenced by multiple
indexes fails to drop all relevant indexes.

[cockroach-teamcity]

This change is 

[mgartner]

By GetAllNonDropIndexes do you mean AllNonDropIndexes?

Can you explain more in the commit message what is causing indexes to be missed in the delete? It's not immediately obvious how my change created this problem. Or was the bug already there, but hidden by the strict dropping rules before my change?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner and @otan)

---

pkg/sql/alter_table.go, line 406 at r1 (raw file):

					"column %q is referenced by the primary key", colToDrop.Name)
			}
			var idxesToDelete []descpb.IndexDescriptor

Could we store just a list of idx.Name since that's the only thing dropIndexesByName needs?

---

pkg/sql/logictest/testdata/logic_test/alter_table, line 1433 at r1 (raw file):

		STORING(col109_2, col109_3, col109_6, col109_16),
	INDEX (col109_12 ASC, col109_4 DESC)
); ALTER TABLE regression_54196 DROP COLUMN col109_15

Can this test case be simplified?

I think you should add a SHOW INDEXES FROM regression_54196 to assert the correct indexes have been dropped.

[otan]

AllNonDropIndexes returns an array of pointers to index descriptors.
After deleting one of the indexes, the array of pointers point
to a different position. This ends up accidentally missing some
indexes that require deletion.

updated commit message ^

i think the index drops when columns drop were added during your change, hence starting the problem
beforehand, we failed to allow drop column when it was referenced by an index, hehe.

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

---

pkg/sql/alter_table.go, line 406 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Could we store just a list of idx.Name since that's the only thing dropIndexesByName needs?

Yeah I can do that.

---

pkg/sql/logictest/testdata/logic_test/alter_table, line 1433 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Can this test case be simplified?

I think you should add a SHOW INDEXES FROM regression_54196 to assert the correct indexes have been dropped.

ya

[ajwerner]

from @cockroachdb/sql-schema

Looking forward to pulling these mutations underneath the mutable descriptor and generally just get rid of mucking around with the protos directly. It's not clear that that alone would solve anything but it would (hopefully) lead to unit testing that might have caught this sort of thing. I have a dream of doing property based testing for sequences of mutations and their impact on retreival methods but that feels not supremely close.

Reviewed 1 of 2 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @mgartner)

[mgartner]

Yikes... The index descriptors are stored as []IndexDescriptor in tabledesc.Immutable. When dropIndexByName is called, the all the indexes after the dropped index are shifted to the left by one, but the pointers from AllNonDropIndexes remain the same, so an index is skipped.

That's nasty. I suppose when I wrote it I was not aware that dropIndexByName mutated the underlying slice of IndexDescriptors. LMK if you have ideas on how to defend against something like this in the future.

Thanks for tracking this down.

Reviewable status: complete! 2 of 0 LGTMs obtained

[otan]

Yeah it's not great how easy this mistake can come about. I think schema's got ideas, I chanced upon this one :P

[otan]

bors r=ajwerner,mgartner

backport will come

[craig]

Build succeeded:

• GitHub CI (Cockroach)