From d564a71227511a662d73d6867771a68507539e97 Mon Sep 17 00:00:00 2001
From: Rafi Shamim <rafi@cockroachlabs.com>
Date: Tue, 21 Jan 2025 19:15:34 -0500
Subject: [PATCH] sql: retry when reading pg_catalog tables with an old
 timestamp

This changes the logic so we will try to use the cacheTS if the current
txn has an older table version than the cache.

Also, if there is a timestamp error, we will now release the lease and
try to acquire a new one, which will cause another timestamp to be used.

Release note (bug fix): Fixed a bug that could cause SHOW TABLES and
other introspection operations to encounter a "batch timestamp
must be after replica GC threshold" error.
---
 pkg/sql/rolemembershipcache/BUILD.bazel |  1 +
 pkg/sql/rolemembershipcache/cache.go    | 24 ++++++++++++++++++------
 2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/pkg/sql/rolemembershipcache/BUILD.bazel b/pkg/sql/rolemembershipcache/BUILD.bazel
index d690fae5c4b4..31a31f31dd8b 100644
--- a/pkg/sql/rolemembershipcache/BUILD.bazel
+++ b/pkg/sql/rolemembershipcache/BUILD.bazel
@@ -7,6 +7,7 @@ go_library(
     visibility = ["//visibility:public"],
     deps = [
         "//pkg/keys",
+        "//pkg/kv/kvpb",
         "//pkg/roachpb",
         "//pkg/security/username",
         "//pkg/sql/catalog/descpb",
diff --git a/pkg/sql/rolemembershipcache/cache.go b/pkg/sql/rolemembershipcache/cache.go
index 33ab53b6f719..956dc04ace76 100644
--- a/pkg/sql/rolemembershipcache/cache.go
+++ b/pkg/sql/rolemembershipcache/cache.go
@@ -10,6 +10,7 @@ import (
 	"fmt"
 
 	"github.com/cockroachdb/cockroach/pkg/keys"
+	"github.com/cockroachdb/cockroach/pkg/kv/kvpb"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/security/username"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
@@ -83,10 +84,6 @@ func (m *MembershipCache) RunAtCacheReadTS(
 		if tableDesc.GetVersion() > m.tableVersion {
 			return
 		}
-		if tableDesc.GetVersion() < m.tableVersion {
-			readTS = tableDesc.GetModificationTime()
-			return
-		}
 		// The cached ts could be from long ago, so use the table modification
 		// if it's more recent.
 		if m.readTS.Less(tableDesc.GetModificationTime()) {
@@ -102,12 +99,27 @@ func (m *MembershipCache) RunAtCacheReadTS(
 	}
 
 	// If we found a historical timestamp to use, run in a different transaction.
-	return db.DescsTxn(ctx, func(ctx context.Context, newTxn descs.Txn) error {
+	if err := db.DescsTxn(ctx, func(ctx context.Context, newTxn descs.Txn) error {
 		if err := newTxn.KV().SetFixedTimestamp(ctx, readTS); err != nil {
 			return err
 		}
 		return f(ctx, newTxn)
-	})
+	}); err != nil {
+		if errors.HasType(err, (*kvpb.BatchTimestampBeforeGCError)(nil)) {
+			// If we picked a timestamp that has already been GC'd, it means we must
+			// have an older lease. Release the lease and retry.
+			txn.Descriptors().ReleaseSpecifiedLeases(ctx, []lease.IDVersion{
+				{
+					Name:    tableDesc.GetName(),
+					ID:      tableDesc.GetID(),
+					Version: tableDesc.GetVersion(),
+				},
+			})
+			return m.RunAtCacheReadTS(ctx, db, txn, f)
+		}
+		return err
+	}
+	return nil
 }
 
 // GetRolesForMember looks up all the roles 'member' belongs to (direct and