Skip to content

Commit 7d9a30f

Browse files
committed
stmtdiagnostics: use background context when building the bundle
When the context is canceled, we still want to build the bundle as best as possible. Over in 532274b we introduced the usage of the background context in order to insert the bundle into the system tables, but we still built the bundle with the canceled context. This commit fixes that oversight - in particular, we should now get `env.sql` correctly. Epic: None Release note: None
1 parent e145cc2 commit 7d9a30f

File tree

2 files changed

+21
-22
lines changed

2 files changed

+21
-22
lines changed

pkg/sql/instrumentation.go

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -424,8 +424,27 @@ func (ih *instrumentationHelper) Finish(
424424
if pwe, ok := retPayload.(payloadWithError); ok {
425425
payloadErr = pwe.errorCause()
426426
}
427+
bundleCtx := ctx
428+
if bundleCtx.Err() != nil {
429+
// The only two possible errors on the context are the context
430+
// cancellation or the context deadline being exceeded. The
431+
// former seems more likely, and the cancellation is most likely
432+
// to have occurred due to a statement timeout, so we still want
433+
// to proceed with saving the statement bundle. Thus, we
434+
// override the canceled context, but first we'll log the error
435+
// as a warning.
436+
log.Warningf(
437+
bundleCtx, "context has an error when saving the bundle, proceeding "+
438+
"with the background one (with deadline of 10 seconds): %v", bundleCtx.Err(),
439+
)
440+
// We want to be conservative, so we add a deadline of 10
441+
// seconds on top of the background context.
442+
var cancel context.CancelFunc
443+
bundleCtx, cancel = context.WithTimeout(context.Background(), 10*time.Second) // nolint:context
444+
defer cancel()
445+
}
427446
bundle = buildStatementBundle(
428-
ctx, ih.explainFlags, cfg.DB, ie.(*InternalExecutor), stmtRawSQL, &p.curPlan,
447+
bundleCtx, ih.explainFlags, cfg.DB, ie.(*InternalExecutor), stmtRawSQL, &p.curPlan,
429448
ob.BuildString(), trace, placeholders, res.Err(), payloadErr, retErr,
430449
&p.extendedEvalCtx.Settings.SV,
431450
)
@@ -434,7 +453,7 @@ func (ih *instrumentationHelper) Finish(
434453
// to the current user and aren't included into the bundle.
435454
warnings = append(warnings, bundle.errorStrings...)
436455
bundle.insert(
437-
ctx, ih.fingerprint, ast, cfg.StmtDiagnosticsRecorder, ih.diagRequestID, ih.diagRequest,
456+
bundleCtx, ih.fingerprint, ast, cfg.StmtDiagnosticsRecorder, ih.diagRequestID, ih.diagRequest,
438457
)
439458
telemetry.Inc(sqltelemetry.StatementDiagnosticsCollectedCounter)
440459
}

pkg/sql/stmtdiagnostics/statement_diagnostics.go

Lines changed: 0 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -496,8 +496,6 @@ func (r *Registry) ShouldCollectDiagnostics(
496496

497497
// InsertStatementDiagnostics inserts a trace into system.statement_diagnostics.
498498
//
499-
// traceJSON is either DNull (when collectionErr should not be nil) or a *DJSON.
500-
//
501499
// If requestID is not zero, it also marks the request as completed in
502500
// system.statement_diagnostics_requests. If requestID is zero, a new entry is
503501
// inserted.
@@ -514,24 +512,6 @@ func (r *Registry) InsertStatementDiagnostics(
514512
collectionErr error,
515513
) (CollectedInstanceID, error) {
516514
var diagID CollectedInstanceID
517-
if ctx.Err() != nil {
518-
// The only two possible errors on the context are the context
519-
// cancellation or the context deadline being exceeded. The former seems
520-
// more likely, and the cancellation is most likely to have occurred due
521-
// to a statement timeout, so we still want to proceed with saving the
522-
// statement bundle. Thus, we override the canceled context, but first
523-
// we'll log the error as a warning.
524-
log.Warningf(
525-
ctx, "context has an error when saving the bundle, proceeding "+
526-
"with the background one (with deadline of 10 seconds): %v", ctx.Err(),
527-
)
528-
// We want to be conservative, so we add a deadline of 10 seconds on top
529-
// of the background context.
530-
var cancel context.CancelFunc
531-
ctx, cancel = context.WithTimeout(context.Background(), 10*time.Second) // nolint:context
532-
defer cancel()
533-
}
534-
535515
err := r.db.Txn(ctx, func(ctx context.Context, txn isql.Txn) error {
536516
txn.KV().SetDebugName("stmt-diag-insert-bundle")
537517
if requestID != 0 {

0 commit comments

Comments
 (0)