Merge #24888

24888: sql: Fix "dangling rows" left over by schema change + rollback to savepoint r=andreimatei a=andreimatei

Prior to this patch, the following sequence would write a row that's
then unaccessible:
begin; savepoint cockroach_restart; create table t(x int primary key); rollback to savepoint cockroach_restart;
insert into t(x) values(1); release savepoint cockroach_restart; commit;

The insert manages to resolve the table because we weren't clearing the
"uncommitted descriptors" set that the session maintains. Since the
table descriptor doesn't end up being committed, the row will be
essentially unreachable.

This patch is original work on the 1.1 branch. This bug is not present
in the 2.0 release, where we're better about reseting state on
transaction retries.
I believe on 1.1 there's still a similar issue present when doing
automatic retries (as opposed to user-directed ones through rollback to
savepoint). I think fixing those would be more involved because it's
less clear where to stick the cleanup done in this patch; I'd rather not
do anything this point.

Also note that, with the fix, the sequence above doesn't actually work.
Instead, it deadlocks, just like it does on 2.0, because of #24885.
However, the following works:
begin; savepoint cockroach_restart; create table t(x int primary key); rollback to savepoint cockroach_restart;
create table t(x int primary key); insert into t(x) values(1); release savepoint cockroach_restart; commit;

Fixes #24785

cc @cockroachdb/release  @gpaul 

Co-authored-by: Andrei Matei <andrei@cockroachlabs.com>

pkg/sql/executor.go

@@ -1479,6 +1479,8 @@ func (e *Executor) execStmtInAbortedTxn(
 				false /* implicitTxn */, true, /* retryIntent */
 				curTs /* sqlTimestamp */, curIso /* isolation */, curPri /* priority */)
 		}
+		session.testingVerifyMetadataFn = nil
+		session.tables.releaseTables(session.Ctx())
 		// TODO(andrei/cdo): add a counter for user-directed retries.
 		return nil
 	default:
@@ -1752,6 +1754,8 @@ func (e *Executor) execStmtInOpenTxn(
 			txnState.mu.txn.Proto().Restart(
 				0 /* userPriority */, 0 /* upgradePriority */, hlc.Timestamp{})
 		}
+		session.testingVerifyMetadataFn = nil
+		session.tables.releaseTables(session.Ctx())
 		return nil
 
 	case *parser.Prepare:

pkg/sql/logictest/testdata/logic_test/manual_retry

@@ -34,3 +34,40 @@ SELECT CRDB_INTERNAL.FORCE_RETRY('500ms':::INTERVAL)
 
 statement ok
 COMMIT
+
+# Test that creating a table repeatedly across restarts doesn't leave dangling
+# rows behind (the rows are  associated with the correct descriptor).
+# See #24785.
+
+statement ok
+BEGIN
+
+statement ok
+SAVEPOINT cockroach_restart
+
+statement ok
+CREATE TABLE t (
+id INT PRIMARY KEY
+)
+
+statement ok
+ROLLBACK TO SAVEPOINT cockroach_restart
+
+# The following CREATE shouldn't be necessary. This test would like to just run
+# the next insert (or a select) and check that it fails to resolve the table
+# name. However, that doesn't currently work because of #24885.
+statement ok
+CREATE TABLE t (
+id INT PRIMARY KEY
+)
+
+statement ok
+INSERT INTO t (id) VALUES (1);
+
+statement ok
+COMMIT
+
+query I
+SELECT id FROM t
+----
+1