From bb0d19e86bb1bad5874897549ac991f74df8cd17 Mon Sep 17 00:00:00 2001
From: Faizan Qazi <faizan@cockroachlabs.com>
Date: Thu, 10 Feb 2022 10:49:38 -0500
Subject: [PATCH] sql: stale descriptor state could be observed in
 crdb_internal tables

Fixes: #75825

Previously, when an uncommitted descriptor was added
to a descriptor collection, we would not reset the cache of
all immutable descriptors (used by some crdb_internal functions).
This was inadequate because this could result in
incorrect results from crdb_internal tables which returned
information related to constraints, columns. To address this,
this patch will reset the descriptor cache for all
fetched descriptors when an uncommitted descriptor is
added into the collection.

Release note: None
---
 pkg/sql/catalog/descs/collection.go         |  3 ++
 pkg/sql/catalog/descs/descriptor.go         |  1 -
 pkg/sql/logictest/testdata/logic_test/table | 39 +++++++++++++++++++++
 3 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/pkg/sql/catalog/descs/collection.go b/pkg/sql/catalog/descs/collection.go
index 5f0afc3c52e2..915f93ec6493 100644
--- a/pkg/sql/catalog/descs/collection.go
+++ b/pkg/sql/catalog/descs/collection.go
@@ -207,6 +207,9 @@ var _ = (*Collection).HasUncommittedTypes
 // immutably will return a copy of the descriptor in the current state. A deep
 // copy is performed in this call.
 func (tc *Collection) AddUncommittedDescriptor(desc catalog.MutableDescriptor) error {
+	// Invalidate all the cached descriptors since a stale copy of this may be
+	// included.
+	tc.kv.releaseAllDescriptors()
 	return tc.uncommitted.checkIn(desc)
 }
 
diff --git a/pkg/sql/catalog/descs/descriptor.go b/pkg/sql/catalog/descs/descriptor.go
index fa7c71582d24..bbb7dfb4745f 100644
--- a/pkg/sql/catalog/descs/descriptor.go
+++ b/pkg/sql/catalog/descs/descriptor.go
@@ -341,7 +341,6 @@ func (tc *Collection) withReadFromStore(
 		}
 		descs[i] = desc
 	}
-	tc.kv.releaseAllDescriptors()
 	return descs, nil
 }
 
diff --git a/pkg/sql/logictest/testdata/logic_test/table b/pkg/sql/logictest/testdata/logic_test/table
index 54aa6746ac7b..446f0d04bf99 100644
--- a/pkg/sql/logictest/testdata/logic_test/table
+++ b/pkg/sql/logictest/testdata/logic_test/table
@@ -855,3 +855,42 @@ query I
 SELECT estimated_row_count FROM [SHOW TABLES from rowtest] where table_name = 't1'
 ----
 1000
+
+# Tests that the collections cache is properly invalidated when uncommitted
+# descriptors are added as observed in #75825.
+subtest collection-improper-release
+
+statement ok
+BEGIN;
+CREATE TABLE tblmodified (price INT8, quantity INT8);
+ALTER TABLE tblmodified ADD CONSTRAINT quan_check CHECK (quantity > 0);
+ALTER TABLE tblmodified ADD CONSTRAINT pr_check CHECK (price > 0);
+
+# Check out the descriptor for tblmodified under the all descriptors cache,
+query TTB
+SELECT conname,
+       pg_get_constraintdef(c.oid) AS constraintdef,
+       c.convalidated AS valid
+  FROM pg_constraint AS c JOIN pg_class AS t ON c.conrelid = t.oid
+ WHERE c.contype = 'c' AND t.relname = 'tblmodified' ORDER BY conname ASC;
+----
+pr_check    CHECK ((price > 0))     true
+quan_check  CHECK ((quantity > 0))  true
+
+# Descriptor is modified, so the cache should be invalidated.
+statement ok
+ALTER TABLE tblmodified DROP CONSTRAINT quan_check;
+
+# If the cache is properly invalidated then we should only see a single
+# constraint.
+query TTB
+SELECT conname,
+       pg_get_constraintdef(c.oid) AS constraintdef,
+       c.convalidated AS valid
+  FROM pg_constraint AS c JOIN pg_class AS t ON c.conrelid = t.oid
+ WHERE c.contype = 'c' AND t.relname = 'tblmodified' ORDER BY conname ASC;
+----
+pr_check    CHECK ((price > 0))     true
+
+statement ok
+COMMIT;