Merge #89846

89846: sql/schemachanger: prevent concurrent declarative/legacy usage for types r=fqazi a=fqazi

Fixes: #89831, #87572

Previously, it was possible for the declarative schema changer drop type operations and other legacy schema changer operations for types to overlap. This was problematic because if any legacy type descriptor-related schema change job saw that a type descriptor is labelled as dropped, we would delete the descriptor as a part the legacy schema changer job.

This would happen for example if we had a DROP TYPE and ALTER TYPE operations running concurrently, with the former in the declarative schema changer. The declarative schema changer during the commit phase will mark the descriptor as dropped, and delete the descriptor using a job. The alter type legacy schema changer job if started afterwards will drop the type descriptor if it's marked as dropped before the declarative schema change job starts, causing the declarative to hang later on if the descriptor is missing.

To address this, this patch will prevent the legacy schema changer jobs from dropping a type descriptor, if a declarative schema changer exists inside it.

Release note: None

Co-authored-by: Faizan Qazi <[email protected]>

pkg/sql/logictest/testdata/logic_test/drop_type

@@ -485,3 +485,46 @@ DROP TYPE sc2.typ;
 # This is just cleanup.
 statement ok
 DROP SCHEMA sc2 CASCADE;
+
+# Concurrent DROP TYPE and other ALTER TYPE statements can potentially
+# collide with each other, since the code used to as a part of TYPE SCHEMA
+# CHANGER jobs clean up the descriptor if it was marked as dropped. This
+# should no longer happen, if a declarative schema changer state exists.
+# Without the fix this test would hang.
+subtest concurrent_declarative_89831
+
+statement ok
+CREATE DATABASE db1 OWNER testuser;
+CREATE SCHEMA db1.sc1 AUTHORIZATION testuser;
+
+statement ok
+CREATE TYPE db1.sc1.typ AS ENUM ('a');
+
+statement ok
+SET CLUSTER SETTING jobs.debug.pausepoints="typeschemachanger.before.exec"
+
+skipif config local-legacy-schema-changer
+statement error job \d+ was paused before it completed with reason: pause point "typeschemachanger.before.exec" hit
+ALTER TYPE db1.sc1.typ OWNER TO testuser
+
+statement ok
+SET CLUSTER SETTING jobs.debug.pausepoints=""
+
+statement ok
+SET CLUSTER SETTING jobs.debug.pausepoints="newschemachanger.before.exec"
+
+skipif config local-legacy-schema-changer
+statement error job \d+ was paused before it completed with reason: pause point "newschemachanger.before.exec" hit
+DROP SCHEMA db1.sc1 CASCADE
+
+statement ok
+SET CLUSTER SETTING jobs.debug.pausepoints=""
+
+statement ok
+RESUME JOB (SELECT job_id FROM crdb_internal.jobs WHERE description LIKE 'ALTER TYPE%' AND status='paused' FETCH FIRST 1 ROWS ONLY);
+
+statement ok
+RESUME JOB (SELECT job_id FROM crdb_internal.jobs WHERE description LIKE 'ALTER TYPE%' AND status='paused' FETCH FIRST 1 ROWS ONLY);
+
+statement ok
+RESUME JOB (SELECT job_id FROM crdb_internal.jobs WHERE description LIKE 'DROP SCHEMA%' AND status='paused' FETCH FIRST 1 ROWS ONLY);

pkg/sql/type_change.go

@@ -484,8 +484,9 @@ func (t *typeSchemaChanger) exec(ctx context.Context) error {
 		}
 	}
 
-	// If the type is being dropped, remove the descriptor here.
-	if typeDesc.Dropped() {
+	// If the type is being dropped, remove the descriptor here only
+	// if the declarative schema changer is not in use.
+	if typeDesc.Dropped() && typeDesc.GetDeclarativeSchemaChangerState() == nil {
 		if err := t.execCfg.DB.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error {
 			b := txn.NewBatch()
 			b.Del(catalogkeys.MakeDescMetadataKey(codec, typeDesc.GetID()))