upgrade: improve delete dropped udf upgrade job efficiency

[chengxiong-ruan]

Informs: https://github.com/cockroachlabs/support/issues/2364

Release note (performance improvement): this commit makes the delete descriptors of dropped functions upgrade job more efficient. It used to look at every single id until the max descriptor id which was very inefficient when the max descriptor id really large, in which case the upgrade job took very long even there was no function descriptor at all. This commit changed it to actually query upper bound id of each batch.

[blathers-crl]

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[andyyang890]

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

---

pkg/upgrade/upgrades/delete_descriptors_of_dropped_functions.go line 45 at r1 (raw file):

    FROM 
      system.descriptor
    WHERE id > $1

Just wondering, would it be a lot more inefficient to instead have the to_json CTE not have a WHERE clause and have to_delete maybe have something like LIMIT 50 on it? (Or maybe have the LIMIT 50 on the main SELECT itself.) And then we'd loop the query until no more rows are affected?

I still worry that looping over the descriptor IDs might be too slow and if these kind of descriptors are rare to begin with, somewhat inefficient still?

[chengxiong-ruan]

pkg/upgrade/upgrades/delete_descriptors_of_dropped_functions.go line 45 at r1 (raw file):

Just wondering, would it be a lot more inefficient to instead have the to_json CTE not have a WHERE clause and have to_delete maybe have something like LIMIT 50 on it? (Or maybe have the LIMIT 50 on the main SELECT itself.) And then we'd loop the query until no more rows are affected?

I think the optimizer would push the where clause down to to_json if we put it in the main SELECT (which do the delete).

And then we'd loop the query until no more rows are affected?

I think with this approach, we still need to track the progress somehow. Otherwise it would start from the very first descriptor if there're actually some function descriptors. Maybe we could have the delete query to return the IDs as well then still filter by IDs. But I think by explicitly looping through IDs, each small batch is light and won't scan too much data.

I still worry that looping over the descriptor IDs might be too slow and if these kind of descriptors are rare to begin with, somewhat inefficient still?

yeah, the thing is that we have to look at every single descriptor to tell if it's a function descriptor and then to tell if it's dropped. But I won't worry about the IDs looping as long as it only look at actual existing IDs. From what I saw by testing this new. upgrade with the 4 thousand descriptor debug zip, it's much much faster now.

[andyyang890]

just one small nit

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

---

pkg/upgrade/upgrades/delete_descriptors_of_dropped_functions.go line 45 at r1 (raw file):

I think with this approach, we still need to track the progress somehow. Otherwise it would start from the very first descriptor if there're actually some function descriptors.

Ah I see, I guess because it's not as straightforward as DELETE FROM ..., you can't just use a LIMIT directly.

---

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

				batchSize,
			)
			batchMaxID := int64(tree.MustBeDInt(row[0]))

nit: this line should probably go after the err != nil check

[chengxiong-ruan]

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

Previously, andyyang890 (Andy Yang) wrote…

nit: this line should probably go after the err != nil check

done. good catch!

[chengxiong-ruan]

tftr!
bors r+

[craig]

Build failed:

• Bazel Essential CI (Cockroach)

[chengxiong-ruan]

bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)