Merge #155655

155655: sql: don't run EXPLAIN ANALYZE via pausable portals r=yuzefovich a=yuzefovich

This commit disables running EXPLAIN ANALYZE stmts via pausable portals model even if it's enabled. Previously, this led to a crash since different assumptions of how EXPLAIN ANALYZE is handled at the connExecutor level were broken, but it also doesn't make much sense to actually support this. After all, the result rows can only be produced _after_ the query execution effectively completed, which contradicts the spirit of the pausable portal feature.

I briefly looked into making it work, but it doesn't seem worth it. There is a fundamental contradiction right now that we defer finishing the instrumentationHelper (which populates the output of EXPLAIN ANALYZE), and it currently happens after we close the commandResult in which we can write the output.

Fixes: #137597.

Release note (bug fix): Previously, CockroachDB would crash when executing EXPLAIN ANALYZE statements with the pausable portal model (meaning that it was run via the extended PGWire protocol using Parse, Bind, Execute, when `multiple_active_portals_enabled` session variable is enabled). The bug has been present since 23.2 and is now fixed.

Co-authored-by: Yahor Yuzefovich <[email protected]>

Author: craig[bot]

pkg/sql/conn_executor_exec.go

@@ -1476,7 +1476,13 @@ func (ex *connExecutor) execStmtInOpenStateWithPausablePortal(
 		if portal.pauseInfo.execStmtInOpenState.ihWrapper == nil {
 			portal.pauseInfo.execStmtInOpenState.ihWrapper = &instrumentationHelperWrapper{
 				ctx: ctx,
-				ih:  *ih,
+				// TODO(yuzefovich): we're capturing the instrumentationHelper
+				// by value here, meaning that modifications that happen later
+				// (notably in makeExecPlan) aren't captured. For example,
+				// explainPlan field will remain unset. However, so far we've
+				// only observed this impact EXPLAIN ANALYZE which doesn't run
+				// through the pausable portal path.
+				ih: *ih,
 			}
 		} else {
 			p.instrumentation = portal.pauseInfo.execStmtInOpenState.ihWrapper.ih

pkg/sql/pgwire/testdata/pgtest/multiple_active_portals

@@ -1629,3 +1629,100 @@ ReadyForQuery
 {"Type":"ReadyForQuery","TxStatus":"I"}
 
 subtest end
+
+subtest explain_analyze
+
+# Regression test that EXPLAIN ANALYZE doesn't run through the pausable portal
+# path even when the feature is enabled (#137597).
+
+send crdb_only
+Query {"String": "SET multiple_active_portals_enabled = 'true';"}
+----
+
+until crdb_only
+ReadyForQuery
+----
+{"Type":"CommandComplete","CommandTag":"SET"}
+{"Type":"ReadyForQuery","TxStatus":"I"}
+
+send
+Query {"String": "BEGIN"}
+Parse {"Name": "qea1", "Query": "EXPLAIN ANALYZE SELECT 1;"}
+Bind {"DestinationPortal": "pea1", "PreparedStatement": "qea1"}
+Execute {"Portal": "pea1", "MaxRows": 1}
+Sync
+----
+
+until crdb_only ignore=DataRow
+ReadyForQuery
+ReadyForQuery
+----
+{"Type":"CommandComplete","CommandTag":"BEGIN"}
+{"Type":"ReadyForQuery","TxStatus":"T"}
+{"Type":"ParseComplete"}
+{"Type":"BindComplete"}
+{"Type":"PortalSuspended"}
+{"Type":"ReadyForQuery","TxStatus":"T"}
+
+send
+Execute {"Portal": "pea1"}
+Sync
+----
+
+until crdb_only ignore=DataRow
+ReadyForQuery
+----
+{"Type":"CommandComplete","CommandTag":"EXPLAIN"}
+{"Type":"ReadyForQuery","TxStatus":"T"}
+
+send
+Query {"String": "COMMIT"}
+----
+
+until
+ReadyForQuery
+----
+{"Type":"CommandComplete","CommandTag":"COMMIT"}
+{"Type":"ReadyForQuery","TxStatus":"I"}
+
+send
+Query {"String": "BEGIN"}
+Parse {"Name": "qea2", "Query": "EXPLAIN ANALYZE (DEBUG) SELECT 1;"}
+Bind {"DestinationPortal": "pea2", "PreparedStatement": "qea2"}
+Execute {"Portal": "pea2", "MaxRows": 1}
+Sync
+----
+
+until crdb_only ignore=DataRow
+ReadyForQuery
+ReadyForQuery
+----
+{"Type":"CommandComplete","CommandTag":"BEGIN"}
+{"Type":"ReadyForQuery","TxStatus":"T"}
+{"Type":"ParseComplete"}
+{"Type":"BindComplete"}
+{"Type":"PortalSuspended"}
+{"Type":"ReadyForQuery","TxStatus":"T"}
+
+send
+Execute {"Portal": "pea2"}
+Sync
+----
+
+until crdb_only ignore=DataRow
+ReadyForQuery
+----
+{"Type":"CommandComplete","CommandTag":"EXPLAIN"}
+{"Type":"ReadyForQuery","TxStatus":"T"}
+
+send
+Query {"String": "COMMIT"}
+----
+
+until
+ReadyForQuery
+----
+{"Type":"CommandComplete","CommandTag":"COMMIT"}
+{"Type":"ReadyForQuery","TxStatus":"I"}
+
+subtest end

pkg/sql/prepared_stmt.go

@@ -120,13 +120,23 @@ func (ex *connExecutor) makePreparedPortal(
 		OutFormats: outFormats,
 	}
 
-	if ex.sessionData().MultipleActivePortalsEnabled && ex.executorType != executorTypeInternal {
-		telemetry.Inc(sqltelemetry.StmtsTriedWithPausablePortals)
-		// We will check whether the statement itself is pausable (i.e., that it
-		// doesn't contain DDL or mutations) when we build the plan.
-		portal.pauseInfo = &portalPauseInfo{}
-		portal.pauseInfo.dispatchToExecutionEngine.queryStats = &topLevelQueryStats{}
-		portal.portalPausablity = PausablePortal
+	if ex.sessionData().MultipleActivePortalsEnabled {
+		// Do not even try running EXPLAIN ANALYZE statements via the pausable
+		// portal path since it doesn't make much sense to do so - the result
+		// rows can only be produced _after_ the query execution completes, so
+		// there are no actual pauses during the execution (plus the
+		// implementation of EXPLAIN ANALYZE in the connExecutor is quite
+		// special, and it seems hard to make it work with pausable portals
+		// model).
+		_, isExplainAnalyze := stmt.AST.(*tree.ExplainAnalyze)
+		if ex.executorType != executorTypeInternal && !isExplainAnalyze {
+			telemetry.Inc(sqltelemetry.StmtsTriedWithPausablePortals)
+			// We will check whether the statement itself is pausable (i.e., that it
+			// doesn't contain DDL or mutations) when we build the plan.
+			portal.pauseInfo = &portalPauseInfo{}
+			portal.pauseInfo.dispatchToExecutionEngine.queryStats = &topLevelQueryStats{}
+			portal.portalPausablity = PausablePortal
+		}
 	}
 	return portal, portal.accountForCopy(ctx, &ex.extraTxnState.prepStmtsNamespaceMemAcc, name)
 }