sql: enforce SERIALIZABLE isolation for DDL in stored procedures (#170733)

sql: enforce SERIALIZABLE isolation for DDL in stored procedures

Author: trunk-io[bot]

pkg/sql/conn_executor.go

@@ -1991,6 +1991,23 @@ type connExecutor struct {
 	// responds to user queries or an internal one.
 	executorType executorType
 
+	// forceNextTxnSerializable is a one-shot override consumed when the next
+	// implicit txn is opened. It is set by maybeAutoCommitBeforeDDL only
+	// after handleAutoCommit confirms the current txn was committed for a
+	// CALL whose body contains DDL under weaker isolation; the restart must
+	// then begin at SERIALIZABLE so the body's DDL runs safely.
+	//
+	// The flag is cleared in two places to bound its lifetime to the very
+	// next txn: implicitTxnIsoLevel applies and clears it when the restart's
+	// implicit txn opens; the BEGIN branch of execStmtInNoTxnState discards
+	// it if the user opens an explicit txn first, preventing leakage into
+	// a later unrelated implicit txn.
+	//
+	// Lives outside extraTxnState because that struct is reset per-txn and
+	// this flag must survive the boundary between the auto-committed txn
+	// and its restart.
+	forceNextTxnSerializable bool
+
 	// hasCreatedTemporarySchema is set if the executor has created a
 	// temporary schema, which requires special cleanup on close.
 	hasCreatedTemporarySchema bool
@@ -3821,6 +3838,18 @@ var allowBufferedWritesForWeakIsolation = settings.RegisterBoolSetting(
 	),
 )
 
+// implicitTxnIsoLevel returns the isolation level to use for a new implicit
+// transaction. It honors and clears forceNextTxnSerializable: when set, the
+// next implicit txn starts at SERIALIZABLE regardless of the session default.
+// The flag is consumed exactly once.
+func (ex *connExecutor) implicitTxnIsoLevel(ctx context.Context) isolation.Level {
+	if ex.forceNextTxnSerializable {
+		ex.forceNextTxnSerializable = false
+		return isolation.Serializable
+	}
+	return ex.txnIsolationLevelToKV(ctx, tree.UnspecifiedIsolation)
+}
+
 func (ex *connExecutor) txnIsolationLevelToKV(
 	ctx context.Context, level tree.IsolationLevel,
 ) isolation.Level {

pkg/sql/conn_executor_ddl.go

@@ -13,8 +13,11 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/settings"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
+	"github.com/cockroachdb/cockroach/pkg/sql/parser"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgnotice"
+	plpgsqlparser "github.com/cockroachdb/cockroach/pkg/sql/plpgsql/parser"
 	"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scrun"
+	"github.com/cockroachdb/cockroach/pkg/sql/sem/plpgsqltree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/util/fsm"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
@@ -33,19 +36,31 @@ var defaultAutocommitBeforeDDL = settings.RegisterBoolSetting(
 // auto-committed before processing a DDL statement. If so, it auto-commits the
 // transaction and advances the state machine so that the current command gets
 // processed again.
+//
+// Two conditions can trigger an auto-commit:
+//
+//  1. The statement itself is a DDL (CanModifySchema) and either we are in an
+//     explicit transaction or a prior statement has already executed; the
+//     auto-commit is gated on the autocommit_before_ddl session setting.
+//
+//  2. The statement is a CALL whose procedure body contains DDL, the txn
+//     isolation tolerates write skew, and the txn is implicit. In-place
+//     isolation upgrade is unavailable in the routine body (the txn is
+//     already active by the time CALL planning completes), so we
+//     auto-commit and arrange for the next implicit txn to start at
+//     SERIALIZABLE via forceNextTxnSerializable. This case is gated on
+//     implicit txns only — explicit-txn CALLs with DDL are handled
+//     elsewhere (#170019) or rejected by the in-routine safety net.
 func (ex *connExecutor) maybeAutoCommitBeforeDDL(
 	ctx context.Context, ast tree.Statement,
 ) (fsm.Event, fsm.EventPayload) {
-	// We will auto-commit if all of the following conditions are met:
-	// - The query is not internal.
-	// - Auto-commit before DDL is enabled.
-	// - We have an explicit transaction or have executed at least one
-	//   statement in the transaction. The former check ensures that we commit
-	//   if we haven't executed the first statement, which can happen if only the
-	//   BEGIN statement was executed.
+	if ex.executorType == executorTypeInternal {
+		return nil, nil
+	}
 	explicitTxn := !ex.implicitTxn()
-	if ex.executorType != executorTypeInternal &&
-		tree.CanModifySchema(ast) &&
+
+	// Case 1: top-level DDL with autocommit_before_ddl enabled.
+	if tree.CanModifySchema(ast) &&
 		ex.sessionData().AutoCommitBeforeDDL &&
 		(explicitTxn || ex.extraTxnState.firstStmtExecuted) {
 		if err := ex.planner.SendClientNotice(
@@ -55,17 +70,188 @@ func (ex *connExecutor) maybeAutoCommitBeforeDDL(
 		); err != nil {
 			return ex.makeErrEvent(err, ast)
 		}
-		retEv, retPayload := ex.handleAutoCommit(ctx, ast)
-		if _, committed := retEv.(eventTxnFinishCommitted); committed && retPayload == nil {
-			// Use eventTxnCommittedDueToDDL so that the current statement gets
-			// picked up again when the state machine advances.
-			retEv = eventTxnCommittedDueToDDL{}
+		return ex.autoCommitForDDL(ctx, ast)
+	}
+
+	// Case 2: implicit-txn CALL whose body contains DDL under weak isolation.
+	// Skip when:
+	// - A prior statement in the same implicit-txn batch has already
+	//   executed (mirrors Case 1's reluctance to split a multi-statement
+	//   batch with an auto-commit; the in-routine safety net rejects
+	//   instead).
+	// - The planner is not bound to the current txn (prepare path:
+	//   descriptor resolution there hits a stale planner state).
+	// - There are active portals: auto-committing would invalidate them,
+	//   and the extended-query protocol does not handle a portal
+	//   disappearing mid-execute. Users hitting this path fall back to
+	//   the in-routine safety net.
+	if call, ok := ast.(*tree.Call); ok &&
+		!explicitTxn &&
+		!ex.extraTxnState.firstStmtExecuted &&
+		ex.state.mu.txn != nil &&
+		ex.planner.txn == ex.state.mu.txn &&
+		!ex.extraTxnState.prepStmtsNamespace.HasActivePortals() &&
+		ex.state.mu.txn.IsoLevel().ToleratesWriteSkew() {
+		hasDDL, err := ex.callBodyContainsDDL(ctx, call)
+		if err != nil {
+			// Any error from the pre-plan walker (descriptor lookup, body
+			// parse, etc.) is swallowed: the same error will be raised by
+			// normal planning with proper context, and surfacing it from
+			// the auto-commit hook would replace a clearer planning error
+			// with one tagged at the wrong source line.
+			log.VEventf(ctx, 2, "callBodyContainsDDL: %v", err)
+			return nil, nil
+		}
+		if !hasDDL {
+			return nil, nil
+		}
+		if err := ex.planner.SendClientNotice(
+			ctx,
+			pgnotice.Newf("setting transaction isolation level to SERIALIZABLE due to schema change"),
+			false, /* immediateFlush */
+		); err != nil {
+			return ex.makeErrEvent(err, ast)
+		}
+		// Only set forceNextTxnSerializable after confirming the auto-commit
+		// rewrote the event to eventTxnCommittedDueToDDL — meaning the
+		// statement will be re-processed in a fresh txn that should pick up
+		// the override. Otherwise the flag would leak into an unrelated
+		// future implicit txn.
+		retEv, retPayload := ex.autoCommitForDDL(ctx, ast)
+		if _, ok := retEv.(eventTxnCommittedDueToDDL); ok && retPayload == nil {
+			ex.forceNextTxnSerializable = true
 		}
 		return retEv, retPayload
 	}
+
 	return nil, nil
 }
 
+// autoCommitForDDL commits the current txn and rewrites a successful commit
+// into eventTxnCommittedDueToDDL so the current statement is picked up again
+// once the state machine advances.
+func (ex *connExecutor) autoCommitForDDL(
+	ctx context.Context, ast tree.Statement,
+) (fsm.Event, fsm.EventPayload) {
+	retEv, retPayload := ex.handleAutoCommit(ctx, ast)
+	if _, committed := retEv.(eventTxnFinishCommitted); committed && retPayload == nil {
+		retEv = eventTxnCommittedDueToDDL{}
+	}
+	return retEv, retPayload
+}
+
+// callBodyContainsDDL resolves the procedure named by the CALL and returns
+// true if any of its overload bodies contains a DDL statement reachable
+// from the top-level body. Nested CALLs inside the body are not followed;
+// if such a nested CALL has DDL in its own body, the in-routine reject in
+// routineGenerator.startInternal catches it at runtime.
+//
+// The function may activate the txn (descriptor reads); callers must be
+// prepared to auto-commit. If a procedure has multiple overloads, any
+// overload with DDL triggers the upgrade — we cannot identify the selected
+// overload pre-planning without re-doing argument type-checking.
+//
+// The AST's resolved function reference is restored before returning, so
+// the subsequent optbuilder pass re-resolves against the post-restart txn
+// instead of inheriting a stale cached descriptor from this peek.
+func (ex *connExecutor) callBodyContainsDDL(ctx context.Context, call *tree.Call) (bool, error) {
+	// Resolve mutates both FunctionReference (always) and ReferenceByName
+	// (in the *UnresolvedName branch). Snapshot the whole struct so the
+	// post-restart re-plan re-resolves against the new txn instead of
+	// inheriting cached state from this peek.
+	savedRef := call.Proc.Func
+	defer func() { call.Proc.Func = savedRef }()
+	def, err := call.Proc.Func.Resolve(ctx, ex.planner.semaCtx.SearchPath, ex.planner.semaCtx.FunctionResolver)
+	if err != nil {
+		return false, err
+	}
+	for _, ov := range def.Overloads {
+		if ov.Type != tree.ProcedureRoutine {
+			continue
+		}
+		// Resolve typically returns a signature-only overload cached on the
+		// schema descriptor. Fetch the full descriptor by OID to obtain the
+		// body, mirroring the path the optbuilder takes later.
+		full := ov.Overload
+		if ov.UDFContainsOnlySignature || ov.Body == "" {
+			_, fullOv, resolveErr := ex.planner.ResolveFunctionByOID(ctx, ov.Oid)
+			if resolveErr != nil {
+				return false, resolveErr
+			}
+			full = fullOv
+		}
+		if full == nil || full.Body == "" {
+			continue
+		}
+		hasDDL, err := routineBodyContainsDDL(full.Body, full.Language)
+		if err != nil {
+			return false, err
+		}
+		if hasDDL {
+			return true, nil
+		}
+	}
+	return false, nil
+}
+
+// routineBodyContainsDDL parses the routine body and returns true if any
+// SQL statement reachable from the top-level body is DDL. PL/pgSQL bodies
+// are walked via plpgsqltree so that SQL statements embedded as plain
+// statements inside the block (wrapped in *plpgsqltree.Execute) are
+// inspected.
+func routineBodyContainsDDL(body string, lang tree.RoutineLanguage) (bool, error) {
+	switch lang {
+	case tree.RoutineLangSQL:
+		stmts, err := parser.Parse(body)
+		if err != nil {
+			return false, err
+		}
+		for _, s := range stmts {
+			if tree.CanModifySchema(s.AST) {
+				return true, nil
+			}
+		}
+		return false, nil
+	case tree.RoutineLangPLpgSQL:
+		parsed, err := plpgsqlparser.Parse(body)
+		if err != nil {
+			return false, err
+		}
+		v := &plpgsqlDDLFinder{}
+		plpgsqltree.Walk(v, parsed.AST)
+		return v.found, nil
+	default:
+		return false, errors.AssertionFailedf("unknown routine language %q", lang)
+	}
+}
+
+// plpgsqlDDLFinder is a plpgsqltree.StatementVisitor that sets found to true
+// the first time it encounters a PL/pgSQL statement wrapping a DDL SQL
+// statement. Note: *plpgsqltree.Execute is the AST node for any plain SQL
+// statement embedded in a PL/pgSQL block (e.g. a top-level CREATE TABLE),
+// not for the PL/pgSQL EXECUTE keyword (that is *plpgsqltree.DynamicExecute).
+// Dynamic SQL is not inspected because the SQL string is opaque at parse
+// time. Other constructs that wrap queries — CursorDeclaration, Open,
+// ReturnQuery — never carry DDL, so they are not inspected.
+type plpgsqlDDLFinder struct {
+	found bool
+}
+
+var _ plpgsqltree.StatementVisitor = (*plpgsqlDDLFinder)(nil)
+
+func (v *plpgsqlDDLFinder) Visit(stmt plpgsqltree.Statement) (plpgsqltree.Statement, bool) {
+	if v.found {
+		return stmt, false
+	}
+	if e, ok := stmt.(*plpgsqltree.Execute); ok && e.SqlStmt != nil {
+		if tree.CanModifySchema(e.SqlStmt) {
+			v.found = true
+			return stmt, false
+		}
+	}
+	return stmt, true
+}
+
 // maybeAdjustTxnForDDL checks if the statement is a schema change and adjusts
 // the txn if it is. The following adjustments will be performed:
 // - upgrading to serializable isolation. If the txn contains multiple

pkg/sql/conn_executor_exec.go

@@ -3865,6 +3865,11 @@ func (ex *connExecutor) execStmtInNoTxnState(
 				ex.incrementExecutedStmtCounter(ast)
 			}
 		}()
+		// The implicit-CALL+DDL upgrade override (forceNextTxnSerializable)
+		// is meant for the very next implicit txn; an intervening explicit
+		// BEGIN discards it so it can't silently upgrade an unrelated
+		// future implicit txn.
+		ex.forceNextTxnSerializable = false
 		mode, sqlTs, historicalTs, err := ex.beginTransactionTimestampsAndReadMode(ctx, s)
 		if err != nil {
 			return ex.makeErrEvent(err, s)
@@ -3913,7 +3918,7 @@ func (ex *connExecutor) execStmtInNoTxnState(
 				historicalTs,
 				ex.transitionCtx,
 				ex.QualityOfService(),
-				ex.txnIsolationLevelToKV(ctx, tree.UnspecifiedIsolation),
+				ex.implicitTxnIsoLevel(ctx),
 				ex.omitInRangefeeds(),
 				ex.bufferedWritesEnabled(implicitTxn),
 				ex.rng.internal,
@@ -3948,7 +3953,7 @@ func (ex *connExecutor) beginImplicitTxn(
 			historicalTs,
 			ex.transitionCtx,
 			qos,
-			ex.txnIsolationLevelToKV(ctx, tree.UnspecifiedIsolation),
+			ex.implicitTxnIsoLevel(ctx),
 			ex.omitInRangefeeds(),
 			ex.bufferedWritesEnabled(implicitTxn),
 			ex.rng.internal,

pkg/sql/logictest/testdata/logic_test/procedure_dcl

@@ -318,18 +318,29 @@ BEGIN
 END
 $$;
 
+# Under SERIALIZABLE the nested CALL succeeds. Under weak isolation the
+# pre-plan body walker does not recurse into nested CALLs, so the inner
+# GRANT hits the in-routine safety net and the CALL is rejected. See the
+# analogous nested-CALL case in procedure_ddl for the same trade-off.
+skipif config local-read-committed local-repeatable-read
 statement ok
 CALL p_outer_dcl();
 
+skipif config local-read-committed local-repeatable-read
 query TTTTTB colnames
 SHOW GRANTS ON t_dcl FOR r1
 ----
 database_name  schema_name  table_name  grantee  privilege_type  is_grantable
 test           public       t_dcl       r1       SELECT          false
 
+skipif config local-read-committed local-repeatable-read
 statement ok
 REVOKE SELECT ON t_dcl FROM r1;
 
+onlyif config local-read-committed local-repeatable-read
+statement error pgcode 0A000 to use multi-statement transactions involving a schema change under weak isolation levels
+CALL p_outer_dcl();
+
 statement ok
 DROP PROCEDURE p_outer_dcl;
 DROP PROCEDURE p_inner_dcl;