sql,multiregion: prevent DROP REGION in user DB from cleaning up system.sql_instances

Previously, DROP REGION cleanup logic for system.sql_instances was
unintentionally applied to all databases when in multiregion setup,
not just the system database. The original intent was to only perform
this cleanup when extending `DROP REGION` for the system database.

However, the logic leaked through, affecting user databases as well.

This PR implements the fix described in #151789, and after a quick check
of these changes with the blocked tests in the #150730, this fixes them
plus passes the original tests created for testing drop region in system
database.

Epic: None
Fixes: #151789
Release note: None

Co-authored-by: Shubham Dhama <[email protected]>

Author: Nukitt

pkg/ccl/multiregionccl/multiregion_system_table_test.go

@@ -598,3 +598,46 @@ func TestMrSystemDatabaseUpgrade(t *testing.T) {
 		{"ALTER PARTITION \"us-east3\" OF INDEX system.public.lease@primary CONFIGURE ZONE USING\n\tnum_voters = 3,\n\tvoter_constraints = '[+region=us-east3]',\n\tlease_preferences = '[[+region=us-east3]]'"},
 	})
 }
+
+// TestDropRegionFromUserDatabaseCleansUpSystemTables verifies that
+// dropping a region from a user database doesn't remove  rows
+// from the system.sql_instances table.
+func TestDropRegionFromUserDatabaseCleansUpSystemTables(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+
+	ctx := context.Background()
+
+	makeSettings := func() *cluster.Settings {
+		cs := cluster.MakeTestingClusterSettings()
+		instancestorage.ReclaimLoopInterval.Override(ctx, &cs.SV, 150*time.Millisecond)
+		return cs
+	}
+
+	_, systemSQL, cleanup := multiregionccltestutils.TestingCreateMultiRegionCluster(t, 3,
+		base.TestingKnobs{},
+		multiregionccltestutils.WithSettings(makeSettings()))
+	defer cleanup()
+
+	sDB := sqlutils.MakeSQLRunner(systemSQL)
+
+	// Create a user database with multiple regions
+	sDB.Exec(t, `CREATE DATABASE userdb PRIMARY REGION "us-east1" REGIONS "us-east2", "us-east3" SURVIVE ZONE FAILURE`)
+
+	// Verify sql_instances has data before the operation
+	initialCount := sDB.QueryStr(t, `SELECT count(*) FROM system.sql_instances`)
+	require.NotEmpty(t, initialCount)
+	require.Equal(t, "13", initialCount[0][0], "sql_instances should have data initially")
+
+	// Drop a region from the USER database (not system database)
+	sDB.Exec(t, `ALTER DATABASE userdb DROP REGION "us-east2"`)
+
+	// BUG: sql_instances table is now empty, but it shouldn't be
+	// The region still exists in the system database, so instances should remain
+	finalCount := sDB.QueryStr(t, `SELECT count(*) FROM system.sql_instances`)
+	require.NotEmpty(t, finalCount)
+	// This assertion will fail, demonstrating the bug.
+	// The count should remain the same since we only dropped from userdb, not system
+	require.Equal(t, initialCount[0][0], finalCount[0][0],
+		"sql_instances count should not change when dropping region from user database")
+}

pkg/sql/type_change.go

@@ -407,7 +407,7 @@ func (t *typeSchemaChanger) exec(ctx context.Context) error {
 		var idsToRemove []int
 		populateIDsToRemove := func(holder context.Context, txn descs.Txn) error {
 			typeDesc, err := txn.Descriptors().MutableByID(txn.KV()).Type(ctx, t.typeID)
-			if err != nil {
+			if err != nil || typeDesc.GetParentID() != keys.SystemDatabaseID {
 				return err
 			}
 			for _, member := range typeDesc.EnumMembers {