Skip to content

Commit

Permalink
sql: fix panic in request_job_execution_details
Browse files Browse the repository at this point in the history
This was fixed in  cockroachdb#106904
but it looks like some file movement in cockroachdb#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: cockroachdb#106970
Release note: None
  • Loading branch information
adityamaru committed Jul 20, 2023
1 parent bfa276d commit 5aa50c7
Show file tree
Hide file tree
Showing 4 changed files with 35 additions and 1 deletion.
17 changes: 17 additions & 0 deletions pkg/jobs/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -140,3 +140,20 @@ 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, db isql.DB) (bool, error) {
var exists bool
if err := db.Txn(ctx, func(ctx context.Context, txn isql.Txn) error {
row, err := txn.QueryRow(ctx, "check-for-job", txn.KV(), `SELECT id FROM system.jobs WHERE id = $1`, jobID)
if err != nil {
return err
}
exists = row != nil
return nil
}); err != nil {
return false, err
}
return exists, nil
}
11 changes: 10 additions & 1 deletion pkg/sql/jobs_profiler_execution_details.go
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,15 @@ func (p *planner) RequestExecutionDetailFiles(ctx context.Context, jobID jobspb.
}

e := makeJobProfilerExecutionDetailsBuilder(execCfg.SQLStatusServer, execCfg.InternalDB, jobID)

// Check if the job exists otherwise we can bail early.
if exists, err := jobs.JobExists(ctx, jobID, e.db); err != nil {
return err
} else if !exists {
log.Warningf(ctx, "execution details requested for non-existent job %d", jobID)
return nil
}

// TODO(adityamaru): When we start collecting more information we can consider
// parallelize the collection of the various pieces.
e.addDistSQLDiagram(ctx)
Expand Down Expand Up @@ -238,7 +247,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,
Expand Down
4 changes: 4 additions & 0 deletions pkg/sql/jobs_profiler_execution_details_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.Exec(t, `SELECT crdb_internal.request_job_execution_details(-123)`)
})
}

func TestListProfilerExecutionDetails(t *testing.T) {
Expand Down
4 changes: 4 additions & 0 deletions pkg/sql/sem/builtins/builtins.go
Original file line number Diff line number Diff line change
Expand Up @@ -7721,6 +7721,10 @@ specified store on the node it's run from. One of 'mvccGC', 'merge', 'split',
return nil, errors.New("must be admin to request a job profiler bundle")
}

if len(args) != 1 {
return nil, errors.New("argument list must have one element")
}

jobID := int(tree.MustBeDInt(args[0]))
if err := evalCtx.JobsProfiler.RequestExecutionDetailFiles(
ctx,
Expand Down

0 comments on commit 5aa50c7

Please sign in to comment.