server: do not use MustBeDString on nullable result col

In the data distribution handler we were attempting to read a
`raw_sql_config` on `crdb_internal.zones` using `MustBeDString`
which panics if the value is null.  This column is nullable.
We now allow null values to be read and make the response
value an empty string in that case.

Fixes: #140044

Release note (bug fix):  Data distribution page in
advanced debug will no longer crash if there are null
values for `raw_sql_config` in `crdb_internal.zones`.

Author: xinhaoz

pkg/server/admin.go

@@ -3054,7 +3054,10 @@ func (s *adminServer) dataDistributionHelper(
 	for hasNext, err = it.Next(ctx); hasNext; hasNext, err = it.Next(ctx) {
 		row := it.Cur()
 		target := string(tree.MustBeDString(row[0]))
-		zcSQL := tree.MustBeDString(row[1])
+		var zcSQL string
+		if zcSQLDatum, ok := tree.AsDString(row[1]); ok {
+			zcSQL = string(zcSQLDatum)
+		}
 		zcBytes := tree.MustBeDBytes(row[2])
 		var zcProto zonepb.ZoneConfig
 		if err := protoutil.Unmarshal([]byte(zcBytes), &zcProto); err != nil {
@@ -3064,7 +3067,7 @@ func (s *adminServer) dataDistributionHelper(
 		resp.ZoneConfigs[target] = serverpb.DataDistributionResponse_ZoneConfig{
 			Target:    target,
 			Config:    zcProto,
-			ConfigSQL: string(zcSQL),
+			ConfigSQL: zcSQL,
 		}
 	}
 	if err != nil {

pkg/server/application_api/storage_inspection_test.go

@@ -197,7 +197,7 @@ func TestAdminAPIDataDistribution(t *testing.T) {
 
 	firstServer := tc.Server(0).ApplicationLayer()
 
-	sqlDB := sqlutils.MakeSQLRunner(tc.ServerConn(0))
+	sqlDB := sqlutils.MakeSQLRunner(firstServer.SQLConn(t))
 
 	{
 		// TODO(irfansharif): The data-distribution page and underyling APIs don't
@@ -217,6 +217,11 @@ func TestAdminAPIDataDistribution(t *testing.T) {
 		post_id INT REFERENCES roachblog.posts,
 		body text
 	)`)
+
+	// Test for null raw sql config column in crdb_internal.zones,
+	// see: https://github.com/cockroachdb/cockroach/issues/140044
+	//sqlDB.Exec(t, `-- ALTER TABLE roachblog.posts CONFIGURE ZONE = ''`)
+
 	sqlDB.Exec(t, `CREATE SCHEMA roachblog."foo bar"`)
 	sqlDB.Exec(t, `CREATE TABLE roachblog."foo bar".other_stuff(id INT PRIMARY KEY, body TEXT)`)
 	// Test special characters in DB and table names.
@@ -265,31 +270,28 @@ func TestAdminAPIDataDistribution(t *testing.T) {
 		},
 	}
 
-	// Wait for the new tables' ranges to be created and replicated.
-	testutils.SucceedsSoon(t, func() error {
-		var resp serverpb.DataDistributionResponse
-		if err := srvtestutils.GetAdminJSONProto(firstServer, "data_distribution", &resp); err != nil {
-			t.Fatal(err)
-		}
+	require.NoError(t, tc.WaitForFullReplication())
 
-		delete(resp.DatabaseInfo, "system") // delete results for system database.
-		if !reflect.DeepEqual(resp.DatabaseInfo, expectedDatabaseInfo) {
-			return fmt.Errorf("expected %v; got %v", expectedDatabaseInfo, resp.DatabaseInfo)
-		}
+	// Wait for the new tables' ranges to be created and replicated.
+	var resp serverpb.DataDistributionResponse
+	if err := srvtestutils.GetAdminJSONProto(firstServer, "data_distribution", &resp); err != nil {
+		t.Fatal(err)
+	}
 
-		// Don't test anything about the zone configs for now; just verify that something is there.
-		if len(resp.ZoneConfigs) == 0 {
-			return fmt.Errorf("no zone configs returned")
-		}
+	delete(resp.DatabaseInfo, "system") // delete results for system database.
+	if !reflect.DeepEqual(resp.DatabaseInfo, expectedDatabaseInfo) {
+		t.Fatal("unexpected data distribution response", resp.DatabaseInfo)
+		//return fmt.Errorf("expected %v; got %v", expectedDatabaseInfo, resp.DatabaseInfo)
+	}
 
-		return nil
-	})
+	// Don't test anything about the zone configs for now; just verify that something is there.
+	require.NotEmpty(t, resp.ZoneConfigs)
 
 	// Verify that the request still works after a table has been dropped,
 	// and that dropped_at is set on the dropped table.
 	sqlDB.Exec(t, `DROP TABLE roachblog.comments`)
 
-	var resp serverpb.DataDistributionResponse
+	//var resp serverpb.DataDistributionResponse
 	if err := srvtestutils.GetAdminJSONProto(firstServer, "data_distribution", &resp); err != nil {
 		t.Fatal(err)
 	}