doctor: skip validation for dropped descriptors

In some cases, dropped descriptors appear in our `system.descriptors`
table with dangling job mutations without an associated job.
This patch teaches debug doctor examine to skip validation
on such dropped descriptors.

Epic: none

Fixes: cockroachdb#123477
Fixes: cockroachdb#122956

Release note: none

pkg/cli/BUILD.bazel

@@ -178,6 +178,7 @@ go_library(
         "//pkg/sql/sem/eval",
         "//pkg/sql/sem/tree",
         "//pkg/sql/sqlstats",
+        "//pkg/sql/stats",
         "//pkg/storage",
         "//pkg/storage/enginepb",
         "//pkg/storage/fs",

pkg/cli/doctor.go

@@ -529,7 +529,7 @@ func fromZipDir(
 				return errors.Wrap(err, "failed unmarshalling job payload")
 			}
 
-			// when can progress be null? if it exists but we have never started it?
+			// Skip NULL job payloads.
 			if fields[5] == "NULL" {
 				return nil
 			}
@@ -554,9 +554,8 @@ func fromZipDir(
 			Payload  string `json:"hex_payload"`
 			Progress string `json:"hex_progress"`
 		}, 0)
-		// TODO(before merge): when does this happen?
-		if err := parseJSONFile(zipDirPath, "system.jobs.json", &jobsTableJSON); err != nil {
-			return nil, nil, nil, errors.Wrapf(err, "failed to parse system.jobs.json")
+		if err := parseJSONFile(zipDirPath, "crdb_internal.system_jobs.json", &jobsTableJSON); err != nil {
+			return nil, nil, nil, errors.Wrapf(err, "failed to parse crdb_internal.system_jobs.json")
 		}
 		for _, job := range jobsTableJSON {
 			row := jobs.JobMetadata{

pkg/cli/doctor_test.go

@@ -20,7 +20,14 @@ import (
 	"github.com/cockroachdb/datadriven"
 )
 
-var descriptorWithFKMutation = `'{
+func descriptorWithFKMutation(isDropped bool) string {
+	dropState := ``
+	dropTime := ``
+	if isDropped {
+		dropState = `"state": "DROP",`
+		dropTime = `"dropTime": "1713940113794672911",`
+	}
+	return `'{
       "table": {
           "columns": [
               {
@@ -32,8 +39,9 @@ var descriptorWithFKMutation = `'{
                       "width": 64
                   }
               }
-          ],
-          "createAsOfTime": {
+          ],` +
+		dropTime +
+		`"createAsOfTime": {
               "logical": 1,
               "wallTime": "1713940112376217646"
           },
@@ -133,15 +141,18 @@ var descriptorWithFKMutation = `'{
           },
           "replacementOf": {
               "time": {}
-          },
-          "unexposedParentSchemaId": 381,
+          },` +
+		dropState +
+		`"unexposedParentSchemaId": 381,
           "version": "2"
       }
   }'`
+}
 
 // This test doctoring a secure cluster.
 func TestDoctorCluster(t *testing.T) {
 	defer leaktest.AfterTest(t)()
+	//
 	c := NewCLITest(TestCLIParams{T: t})
 	defer c.Cleanup()
 
@@ -177,13 +188,15 @@ func TestDoctorClusterBroken(t *testing.T) {
 	c := NewCLITest(TestCLIParams{T: t, DisableAutoStats: true})
 	defer c.Cleanup()
 
-	jobDesc := fmt.Sprintf("SELECT crdb_internal.unsafe_upsert_descriptor('foo'::regclass::oid::int,"+
-		"crdb_internal.json_to_pb('cockroach.sql.sqlbase.Descriptor', %s::jsonb), true)", descriptorWithFKMutation)
+	desc := fmt.Sprintf("SELECT crdb_internal.unsafe_upsert_descriptor('foo'::regclass::oid::int,"+
+		"crdb_internal.json_to_pb('cockroach.sql.sqlbase.Descriptor', %s::jsonb), true)", descriptorWithFKMutation(false /* isDropped */))
 
-	// Introduce a descriptor with an attached job mutation (along with other issues).
+	// Introduce a descriptor with an attached job mutation (along with other issues). We want to ensure that the number of
+	// jobs created is deterministic (auto table stats will be collected due to the "schema change" on foo); therefore,
+	// we should disable automatic stats collection and instead create our own.
 	c.RunWithArgs([]string{"sql", "-e", strings.Join([]string{
 		"CREATE TABLE foo (i INT)",
-		jobDesc,
+		desc,
 		"CREATE STATISTICS foo_stats FROM foo",
 		"SELECT pg_catalog.pg_sleep(1)",
 	}, ";\n"),
@@ -202,6 +215,38 @@ func TestDoctorClusterBroken(t *testing.T) {
 	})
 }
 
+// TestDoctorClusterDropped tests that debug doctor examine will avoid validating dropped descriptors.
+func TestDoctorClusterDropped(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	c := NewCLITest(TestCLIParams{T: t})
+	defer c.Cleanup()
+
+	desc := fmt.Sprintf("SELECT crdb_internal.unsafe_upsert_descriptor('foo'::regclass::oid::int,"+
+		"crdb_internal.json_to_pb('cockroach.sql.sqlbase.Descriptor', %s::jsonb), true)", descriptorWithFKMutation(true /* isDropped */))
+	// Introduce a dropped descriptor with an attached job mutation (along with other issues).
+	c.RunWithArgs([]string{"sql", "-e", strings.Join([]string{
+		"CREATE TABLE foo (i INT)",
+		desc,
+		"INSERT INTO system.users VALUES ('node', NULL, true, 3)",
+		"GRANT node TO root",
+		"DELETE FROM system.namespace WHERE name = 'foo'",
+		"SELECT pg_catalog.pg_sleep(1)",
+	}, ";\n"),
+	})
+
+	t.Run("examine", func(t *testing.T) {
+		out, err := c.RunWithCapture("debug doctor examine cluster")
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		// Using datadriven allows TESTFLAGS=-rewrite.
+		datadriven.RunTest(t, datapathutils.TestDataPath(t, "doctor", "test_examine_cluster_dropped"), func(t *testing.T, td *datadriven.TestData) string {
+			return out
+		})
+	})
+}
+
 // TestDoctorZipDir tests the operation of zip over secure clusters.
 func TestDoctorZipDir(t *testing.T) {
 	defer leaktest.AfterTest(t)()

pkg/cli/testdata/doctor/test_examine_cluster_dropped

@@ -0,0 +1,6 @@
+debug doctor examine cluster
+----
+debug doctor examine cluster
+Examining 61 descriptors and 61 namespace entries...
+Examining 20 jobs...
+No problems found!

pkg/sql/doctor/doctor.go

@@ -189,6 +189,10 @@ func ExamineDescriptors(
 	for _, row := range descTable {
 		id := descpb.ID(row.ID)
 		desc := descLookupFn(id)
+		// No need to validate dropped descriptors
+		if desc.Dropped() {
+			continue
+		}
 		ve := cb.ValidateWithRecover(ctx, version, desc)
 		for _, err := range ve {
 			problemsFound = true

pkg/sql/doctor/doctor_test.go

@@ -196,10 +196,8 @@ func TestExamineDescriptors(t *testing.T) {
 					}}),
 				},
 			},
-			expected: `Examining 1 descriptors and 0 namespace entries...
-  ParentID   0, ParentSchemaID 29: relation "foo" (1): invalid parent ID 0
-  ParentID   0, ParentSchemaID 29: relation "foo" (1): table must contain at least 1 column
-`,
+			valid:    true,
+			expected: "Examining 1 descriptors and 0 namespace entries...\n",
 		},
 		{ // 5
 			descTable: doctor.DescriptorTable{
@@ -415,12 +413,10 @@ func TestExamineDescriptors(t *testing.T) {
 				},
 			},
 			namespaceTable: doctor.NamespaceTable{
-				{NameInfo: descpb.NameInfo{ParentID: 52, ParentSchemaID: 29, Name: "t"}, ID: 51},
 				{NameInfo: descpb.NameInfo{Name: "db"}, ID: 52},
 			},
-			expected: `Examining 2 descriptors and 2 namespace entries...
-  ParentID  52, ParentSchemaID 29: namespace entry "t" (51): descriptor is being dropped
-`,
+			valid:    true,
+			expected: "Examining 2 descriptors and 1 namespace entries...\n",
 		},
 		{ // 17
 			descTable: doctor.DescriptorTable{