From 9ee0b8efae4bc01ce130aa7d96c7b85f38861b60 Mon Sep 17 00:00:00 2001
From: Andrei Matei <andrei@cockroachlabs.com>
Date: Tue, 17 Apr 2018 16:36:17 -0400
Subject: [PATCH] sql: Fix "dangling rows" left over by schema change +
 rollback to savepoint

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
---
 pkg/sql/executor.go                           |  4 ++
 .../testdata/logic_test/manual_retry          | 37 +++++++++++++++++++
 2 files changed, 41 insertions(+)

diff --git a/pkg/sql/executor.go b/pkg/sql/executor.go
index c46552a9df6d..99cac3e7bae1 100644
--- a/pkg/sql/executor.go
+++ b/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:
diff --git a/pkg/sql/logictest/testdata/logic_test/manual_retry b/pkg/sql/logictest/testdata/logic_test/manual_retry
index 4383e628adce..f2237d7648c0 100644
--- a/pkg/sql/logictest/testdata/logic_test/manual_retry
+++ b/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