sql: prevent ROLLBACK TO SAVEPOINT under HIGH PRIORITY and DDL

See issue cockroachdb#46414

Because lease acquisition must operate at a high prio
than schema changes, and SQL "HIGH PRIORITY" operates at the same
prio as lease acquisition, we can't let ROLLBACK TO SAVEPOINT
after schema change in HIGH PRIORITY txns.

Release justification: fixes for high-priority or high-severity bugs in existing functionality

Release note (sql change): ROLLBACK TO SAVEPOINT (for either regular
savepoint or "restart savepoints" defined with `cockroach_restart`)
causes a "feature not supported" error after a DDL statement in a HIGH
PRIORITY transaction, in order to avoid a transaction deadlock. See
issue cockroachdb#46414 for details.

pkg/sql/conn_executor_savepoints.go

@@ -17,6 +17,7 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/kv"
+	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
@@ -189,14 +190,33 @@ func (ex *connExecutor) execRollbackToSavepointInOpenState(
 		return ev, payload
 	}
 
-	// We don't yet support rolling back over DDL. Instead of creating an
-	// inconsistent txn or schema state, prefer to tell the users we don't know
-	// how to proceed yet. Initial savepoints are a special case - we can always
-	// rollback to them because we can reset all the schema change state.
-	if !entry.kvToken.Initial() && ex.extraTxnState.numDDL > entry.numDDL {
-		ev, payload := ex.makeErrEvent(unimplemented.NewWithIssueDetail(10735, "rollback-after-ddl",
-			"ROLLBACK TO SAVEPOINT not yet supported after DDL statements"), s)
-		return ev, payload
+	if ex.extraTxnState.numDDL > entry.numDDL {
+		if !entry.kvToken.Initial() {
+			// We don't yet support rolling back a regular savepoint over
+			// DDL. Instead of creating an inconsistent txn or schema state,
+			// prefer to tell the users we don't know how to proceed
+			// yet. Initial savepoints are a special case - we can always
+			// rollback to them because we can reset all the schema change
+			// state.
+			ev, payload := ex.makeErrEvent(unimplemented.NewWithIssueDetail(10735, "rollback-after-ddl",
+				"ROLLBACK TO SAVEPOINT not yet supported after DDL statements"), s)
+			return ev, payload
+		}
+
+		if ex.state.mu.txn.UserPriority() == roachpb.MaxUserPriority {
+			// Because we use the same priority (MaxUserPriority) for SET
+			// TRANSACTION PRIORITY HIGH and lease acquisitions, we'd get a
+			// deadlock if we let DDL proceed at high priority.
+			// See https://github.com/cockroachdb/cockroach/issues/46414
+			// for details.
+			//
+			// Note: this check must remain even when regular savepoints
+			// are taught to roll back over DDL (that's the check below),
+			// until #46414 gets solved.
+			ev, payload := ex.makeErrEvent(unimplemented.NewWithIssue(46414,
+				"cannot use ROLLBACK TO SAVEPOINT in a HIGH PRIORITY transaction containing DDL"), s)
+			return ev, payload
+		}
 	}
 
 	// Special case for mixed-cluster versions, where regular savepoints

pkg/sql/testdata/savepoints

@@ -191,6 +191,8 @@ subtest end
 
 subtest rollback_after_ddl
 
+subtest rollback_after_ddl/release_normal_savepoint
+
 # DDL under savepoints is fine as long as there is no rollback.
 # Note: we do two DDL; the first one is there just to anchor
 # the txn on the config range. The second DDL is the one
@@ -214,15 +216,121 @@ COMMIT
 -- Open        -> NoTxn       #####  (none)
 
 sql
-DROP TABLE unused
+DROP TABLE unused; DROP TABLE t
+----
+1: DROP TABLE unused; DROP TABLE t -- 0 rows
+-- NoTxn       -> NoTxn       #  (none)
+
+# Also fine at high priority.
+
+sql
+BEGIN TRANSACTION PRIORITY HIGH; CREATE TABLE unused(x INT)
+SAVEPOINT foo
+CREATE TABLE t(x INT)
+RELEASE SAVEPOINT foo
+COMMIT
+----
+1: BEGIN TRANSACTION PRIORITY HIGH; CREATE TABLE unused(x INT) -- 0 rows
+-- NoTxn       -> Open        #....  (none)
+2: SAVEPOINT foo -- 0 rows
+-- Open        -> Open        ##...  foo
+3: CREATE TABLE t(x INT) -- 0 rows
+-- Open        -> Open        ###..  foo
+4: RELEASE SAVEPOINT foo -- 0 rows
+-- Open        -> Open        ####.  (none)
+5: COMMIT -- 0 rows
+-- Open        -> NoTxn       #####  (none)
+
+sql
+DROP TABLE unused; DROP TABLE t
+----
+1: DROP TABLE unused; DROP TABLE t -- 0 rows
+-- NoTxn       -> NoTxn       #  (none)
+
+subtest end
+
+subtest rollback_after_ddl/restart_savepoint
+
+# DDL under a cockroach_restart savepoint can
+# be rolled back.
+sql
+BEGIN; SAVEPOINT cockroach_restart; CREATE TABLE t(x INT)
+INSERT INTO t(x) VALUES (1), (2)
+ROLLBACK TO SAVEPOINT cockroach_restart
+CREATE TABLE t(x INT)
+INSERT INTO t(x) VALUES (3)
+COMMIT
+SELECT * FROM t
+----
+1: BEGIN; SAVEPOINT cockroach_restart; CREATE TABLE t(x INT) -- 0 rows
+-- NoTxn       -> Open        #......  cockroach_restart(r)
+2: INSERT INTO t(x) VALUES (1), (2) -- 2 rows
+-- Open        -> Open        ##.....  cockroach_restart(r)
+3: ROLLBACK TO SAVEPOINT cockroach_restart -- 0 rows
+-- Open        -> Open        #......  cockroach_restart(r)
+4: CREATE TABLE t(x INT) -- 0 rows
+-- Open        -> Open        #..#...  cockroach_restart(r)
+5: INSERT INTO t(x) VALUES (3) -- 1 row
+-- Open        -> Open        #..##..  cockroach_restart(r)
+6: COMMIT -- 0 rows
+-- Open        -> NoTxn       #..###.  (none)
+7: SELECT * FROM t -- 1 row
+-- NoTxn       -> NoTxn       #..####  (none)
+
+sql
+DROP TABLE t
+----
+1: DROP TABLE t -- 0 rows
+-- NoTxn       -> NoTxn       #  (none)
+
+# DDL under a cockroach_restart savepoint cannot
+# be rolled back at high priority, because of #46414.
+sql
+BEGIN TRANSACTION PRIORITY HIGH; SAVEPOINT cockroach_restart; CREATE TABLE t(x INT)
+INSERT INTO t(x) VALUES (1), (2)
+ROLLBACK TO SAVEPOINT cockroach_restart
+ABORT
+----
+1: BEGIN TRANSACTION PRIORITY HIGH; SAVEPOINT cockroach_restart; CREATE TABLE t(x INT) -- 0 rows
+-- NoTxn       -> Open        #...  cockroach_restart(r)
+2: INSERT INTO t(x) VALUES (1), (2) -- 2 rows
+-- Open        -> Open        ##..  cockroach_restart(r)
+3: ROLLBACK TO SAVEPOINT cockroach_restart -- pq: unimplemented: cannot use ROLLBACK TO SAVEPOINT in a HIGH PRIORITY transaction containing DDL
+-- Open        -> Aborted     XXXX  cockroach_restart(r)
+4: ABORT -- 0 rows
+-- Aborted     -> NoTxn       #...  (none)
+
+# However it's fine if there's just a release.
+
+sql
+BEGIN TRANSACTION PRIORITY HIGH; SAVEPOINT cockroach_restart; CREATE TABLE t(x INT)
+INSERT INTO t(x) VALUES (1), (2)
+RELEASE SAVEPOINT cockroach_restart
+COMMIT
+SELECT * FROM t
+----
+1: BEGIN TRANSACTION PRIORITY HIGH; SAVEPOINT cockroach_restart; CREATE TABLE t(x INT) -- 0 rows
+-- NoTxn       -> Open        #....  cockroach_restart(r)
+2: INSERT INTO t(x) VALUES (1), (2) -- 2 rows
+-- Open        -> Open        ##...  cockroach_restart(r)
+3: RELEASE SAVEPOINT cockroach_restart -- 0 rows
+-- Open        -> CommitWait  XXXXX  (none)
+4: COMMIT -- 0 rows
+-- CommitWait  -> NoTxn       ###..  (none)
+5: SELECT * FROM t -- 2 rows
+-- NoTxn       -> NoTxn       ###.#  (none)
+
+sql
 DROP TABLE t
 ----
-1: DROP TABLE unused -- 0 rows
--- NoTxn       -> NoTxn       #.  (none)
-2: DROP TABLE t -- 0 rows
--- NoTxn       -> NoTxn       ##  (none)
+1: DROP TABLE t -- 0 rows
+-- NoTxn       -> NoTxn       #  (none)
+
+subtest end
+
+subtest rollback_after_ddl/regular_savepoint
 
-# Rollback is unsupported after DDL for now.
+# Rollback of regular savepoint is unsupported after DDL for now.
 # TODO(knz): Lift this limitation.
 
 sql
@@ -242,6 +350,8 @@ ROLLBACK TO SAVEPOINT foo
 
 subtest end
 
+subtest end
+
 subtest invalid_uses
 
 sql