Merge pull request cockroachdb#155666 from yuzefovich/backport25.3-155655

release-25.3: sql: don't run EXPLAIN ANALYZE via pausable portals

Author: yuzefovich

pkg/sql/conn_executor_exec.go

@@ -1567,7 +1567,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

@@ -1441,3 +1441,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)
 }