sql: delete descriptor when function is dropped

[chengxiong-ruan]

Fixes: #95364

sql: delete descriptor when function is dropped

Previously, in legacy schema changer, descriptor stayed as
a orphan recrod in system.descriptor when a function is dropped.
While this is not a big issue, we should keep data clean.
This PR adds schema change job logic to handle the deletions.

upgrades: add upgrade to delete dropped function descriptors

This commit adds an upgrade to delete orphan descriptors of
functions dropped through legacy schema changer before 23.1.

[cockroach-teamcity]

This change is 

[chengxiong-ruan]

hmm.. looks like the TestDeleteDescriptorsOfDroppedFunctions test is flaky for some reason.
(edit: looks like the fix is to use txn.Exec instead of txn.Query friends.)

[fqazi]

Reviewed 8 of 8 files at r1, 9 of 9 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @chengxiong-ruan)

---

pkg/jobs/jobspb/jobs.proto line 808 at r1 (raw file):

    (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb.ID"
  ];
  repeated uint32 dropped_functions = 12 [(gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb.ID"];

Nit: Maybe a comment here

---

pkg/upgrade/upgrades/delete_descriptors_of_dropped_functions.go line 66 at r2 (raw file):

		const batchSize = 50

		for curID := 1; curID <= maxID; curID += batchSize {

What does the batching buy is here or what are we trying to avoid by doing it?

[chengxiong-ruan]

pkg/upgrade/upgrades/delete_descriptors_of_dropped_functions.go line 66 at r2 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

What does the batching buy is here or what are we trying to avoid by doing it?

I think this is just in case there're too many descriptors which is rare though. But I think it's just nicer to do small batch to avoid one heavy operation.

[chengxiong-ruan]

pkg/jobs/jobspb/jobs.proto line 808 at r1 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

Nit: Maybe a comment here

will do

[chengxiong-ruan]

pkg/jobs/jobspb/jobs.proto line 808 at r1 (raw file):

Previously, chengxiong-ruan (Chengxiong Ruan) wrote…

will do

done

[fqazi]

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

---

pkg/upgrade/upgrades/delete_descriptors_of_dropped_functions.go line 66 at r2 (raw file):

Previously, chengxiong-ruan (Chengxiong Ruan) wrote…

I think this is just in case there're too many descriptors which is rare though. But I think it's just nicer to do small batch to avoid one heavy operation.

I think implicitly each delete will get its own KV batch, so this might not be necessary. Thinking the SQL layer would handle the select side nicely, but I'm okay with it.

[chengxiong-ruan]

TFTR!
bors r+

[craig]

This PR was included in a batch that was canceled, it will be automatically retried

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)