Merge #107254

107254: sql: fix panic in request_job_execution_details r=rafiss a=adityamaru This was fixed in #106904 but it looks like some file movement in #105624 got rid of the nil check. This patch re-adds the nil check along with a test, and also some additional safeguards to not collect any execution details for non-existent jobs. Fixes: #106970 Release note: None Co-authored-by: adityamaru <[email protected]>
cockroachdb · Jul 21, 2023 · 32f22f6 · 32f22f6
2 parents acdb0c7 + 92b05ab
commit 32f22f6
Show file tree

Hide file tree

Showing 3 changed files with 30 additions and 3 deletions.
diff --git a/pkg/jobs/utils.go b/pkg/jobs/utils.go
@@ -17,6 +17,7 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
+	"github.com/cockroachdb/cockroach/pkg/kv"
 	"github.com/cockroachdb/cockroach/pkg/sql/isql"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/errors"
@@ -140,3 +141,15 @@ ORDER BY created`
 	}
 	return false /* exists */, err
 }
+
+// JobExists returns true if there is a row corresponding to jobID in the
+// system.jobs table.
+func JobExists(
+	ctx context.Context, jobID jobspb.JobID, txn *kv.Txn, ex isql.Executor,
+) (bool, error) {
+	row, err := ex.QueryRow(ctx, "check-for-job", txn, `SELECT id FROM system.jobs WHERE id = $1`, jobID)
+	if err != nil {
+		return false, err
+	}
+	return row != nil, nil
+}
diff --git a/pkg/sql/jobs_profiler_execution_details.go b/pkg/sql/jobs_profiler_execution_details.go
@@ -176,12 +176,22 @@ func constructBackupExecutionDetails(
 // RequestExecutionDetailFiles implements the JobProfiler interface.
 func (p *planner) RequestExecutionDetailFiles(ctx context.Context, jobID jobspb.JobID) error {
 	execCfg := p.ExecCfg()
-	if !execCfg.Settings.Version.IsActive(ctx, clusterversion.V23_1) {
+	if !execCfg.Settings.Version.IsActive(ctx, clusterversion.V23_2) {
 		return errors.Newf("execution details can only be requested on a cluster with version >= %s",
-			clusterversion.V23_1.String())
+			clusterversion.V23_2.String())
 	}
 
 	e := makeJobProfilerExecutionDetailsBuilder(execCfg.SQLStatusServer, execCfg.InternalDB, jobID)
+
+	// Check if the job exists otherwise we can bail early.
+	exists, err := jobs.JobExists(ctx, jobID, p.Txn(), e.db.Executor())
+	if err != nil {
+		return err
+	}
+	if !exists {
+		return errors.Newf("job %d not found; cannot request execution details", jobID)
+	}
+
 	// TODO(adityamaru): When we start collecting more information we can consider
 	// parallelize the collection of the various pieces.
 	e.addDistSQLDiagram(ctx)
@@ -238,7 +248,7 @@ func (e *executionDetailsBuilder) addDistSQLDiagram(ctx context.Context) {
 		log.Errorf(ctx, "failed to write DistSQL diagram for job %d: %+v", e.jobID, err.Error())
 		return
 	}
-	if row[0] != tree.DNull {
+	if row != nil && row[0] != tree.DNull {
 		dspDiagramURL := string(tree.MustBeDString(row[0]))
 		filename := fmt.Sprintf("distsql.%s.html", timeutil.Now().Format("20060102_150405.00"))
 		if err := jobs.WriteExecutionDetailFile(ctx, filename,

diff --git a/pkg/sql/jobs_profiler_execution_details_test.go b/pkg/sql/jobs_profiler_execution_details_test.go
@@ -220,6 +220,10 @@ func TestReadWriteProfilerExecutionDetails(t *testing.T) {
 		require.True(t, strings.Contains(string(goroutines), fmt.Sprintf("labels: {\"foo\":\"bar\", \"job\":\"IMPORT id=%d\", \"n\":\"1\"}", importJobID)))
 		require.True(t, strings.Contains(string(goroutines), "github.com/cockroachdb/cockroach/pkg/sql_test.fakeExecResumer.Resume"))
 	})
+
+	t.Run("execution details for invalid job ID", func(t *testing.T) {
+		runner.ExpectErr(t, `job -123 not found; cannot request execution details`, `SELECT crdb_internal.request_job_execution_details(-123)`)
+	})
 }
 
 func TestListProfilerExecutionDetails(t *testing.T) {