sql: harden logging logic in an error case

We just saw a sentry report where we hit a nil pointer with
`planner.curPlan.instrumentationHelper` being nil when trying to log
a statement. This happened after we hit an error in `handleAOST`, so we
short-circuited "dispatch to execution engine" part, meaning that the
plan is left uninitialized. Previously, in
eac1971 we attempted to handle such
scenarios by tracking a boolean indicating whether the dispatch has
occurred, but in this case we marked the boolean "true" (meaning it has
occurred), yet we short-circuited due to an error.

This commit refactors how we ensure that the plan is initialized by
simply checking whether the corresponding fields are set or not. In
turn, this exposed the fact that we previously didn't unset
`planner.curPlan` when resetting the planner, which is now fixed.

I spent some time trying to come up with a repro but didn't succeed, so
there is no regression test.

Release note: None

Author: yuzefovich

pkg/sql/conn_executor_exec.go

@@ -693,11 +693,6 @@ func (ex *connExecutor) execStmtInOpenState(
 		_ = p.FormatAstAsRedactableString(stmt.AST, p.extendedEvalCtx.Annotations)
 	}
 
-	// This flag informs logging decisions.
-	// Some statements are not dispatched to the execution engine and need
-	// some special plan initialization for logging.
-	dispatchToExecEngine := false
-
 	var logErr error
 	defer func() {
 		// Do not log if this is an eventTxnCommittedDueToDDL event. In that case,
@@ -707,10 +702,12 @@ func (ex *connExecutor) execStmtInOpenState(
 			return
 		}
 
-		// If we did not dispatch to the execution engine, we need to initialize
-		// the plan here.
-		if !dispatchToExecEngine {
+		if p.curPlan.stmt == nil || p.curPlan.instrumentation == nil {
+			// We short-circuited before we could initialize some fields that
+			// are needed for logging, so do that here.
 			p.curPlan.init(&p.stmt, &p.instrumentation)
+		}
+		if logErr == nil {
 			if p, ok := retPayload.(payloadWithError); ok {
 				logErr = p.errorCause()
 			}
@@ -931,8 +928,6 @@ func (ex *connExecutor) execStmtInOpenState(
 		return nil, nil, nil
 	}
 
-	dispatchToExecEngine = true
-
 	// Check if we need to auto-commit the transaction due to DDL.
 	if ev, payload := ex.maybeAutoCommitBeforeDDL(ctx, ast); ev != nil {
 		return ev, payload, nil
@@ -1705,11 +1700,6 @@ func (ex *connExecutor) execStmtInOpenStateWithPausablePortal(
 		_ = p.FormatAstAsRedactableString(vars.stmt.AST, p.extendedEvalCtx.Annotations)
 	}
 
-	// This flag informs logging decisions.
-	// Some statements are not dispatched to the execution engine and need
-	// some special plan initialization for logging.
-	dispatchToExecEngine := false
-
 	defer processCleanupFunc(func() {
 		// Do not log if this is an eventTxnCommittedDueToDDL event. In that case,
 		// the transaction is committed, and the current statement is executed
@@ -1718,10 +1708,12 @@ func (ex *connExecutor) execStmtInOpenStateWithPausablePortal(
 			return
 		}
 
-		// If we did not dispatch to the execution engine, we need to initialize
-		// the plan here.
-		if !dispatchToExecEngine {
+		if p.curPlan.stmt == nil || p.curPlan.instrumentation == nil {
+			// We short-circuited before we could initialize some fields that
+			// are needed for logging, so do that here.
 			p.curPlan.init(&p.stmt, &p.instrumentation)
+		}
+		if vars.logErr == nil {
 			if p, ok := retPayload.(payloadWithError); ok {
 				vars.logErr = p.errorCause()
 			}
@@ -1975,8 +1967,6 @@ func (ex *connExecutor) execStmtInOpenStateWithPausablePortal(
 		return nil, nil, nil
 	}
 
-	dispatchToExecEngine = true
-
 	// Check if we need to auto-commit the transaction due to DDL.
 	if ev, payload := ex.maybeAutoCommitBeforeDDL(ctx, vars.ast); ev != nil {
 		return ev, payload, nil

pkg/sql/planner.go

@@ -941,6 +941,7 @@ func (p *planner) resetPlanner(
 	p.txn = txn
 	p.stmt = Statement{}
 	p.instrumentation = instrumentationHelper{}
+	p.curPlan = planTop{}
 	p.monitor = plannerMon
 	p.sessionMonitor = sessionMon