Merge #94153 #95855 #95861 #95876 #95997 #95998 #96003

94153: sql: Remove type annotations from `column_default` in `information_schema.columns` r=ZhouXing19 a=e-mbrown

Informs: #87774

The type annotation on `column_default` caused confusion in postgres compatible apps. This change formats the `column_default` without the type annotation. It does not address the incorrect type annotation.

Release note (bug fix): The content of `column_default` in `information_schema.columns` no longer have type annotations.

95855: backupccl: run restore/tpce/32tb/aws/nodes=15/cpus=16 once a week r=benbardin a=msbutler

This patch adds the restore/tpce/32TB/aws/nodes=15/cpus=16 test to our weekly
test suite. The backup fixture was generated by initing a tpce cluster with 2
Million customers, and running this workload while 48 incremental backups were
taken at 15 minute increments. The roachtest restores from a system time
captured in the 24th backup.

The cluster used to restore the backup will run on aws with 15 nodes each with
16 vcpus.

To verify this test exists, run:
`roachtest list tag:weekly`

Release note: None

Epic: none

95861: kv: remove lastToReplica and lastFromReplica from raftMu r=nvanbenschoten a=nvanbenschoten

Extracted from #94165 without modification.

----

In 410ef29, we moved these fields from under the Replica.mu to under the Replica.raftMu. This was done to avoid lock contention.

In this commit, we move these fields under their own mutex so that they can be accessed without holding the raftMu. This allows us to send Raft messages from other goroutines.

The commit also switches from calling RawNode.ReportUnreachable directly in sendRaftMessage to using the more flexible unreachablesMu set, which defers the call to ReportUnreachable until the next Raft tick. As a result, the commit closes #84246.

Release note: None
Epic: None

95876: sql: remove round-trips from common DISCARD ALL r=ajwerner a=ajwerner

#### sql: avoid checking if a role exists when setting to the current role

This is an uncached round-trip. It makes `DISCARD` expensive.
In #86485 we implemented
`SET SESSION AUTHORIZATION DEFAULT`, this added an extra sql query to
`DISCARD ALL` to check if the role we'd become exists. This is almost
certainly unintentional in the case where the role we'd become is the
current session role. I think this just happened because of code
consolidation.

We now no longer check if the current session role exists. This removes
the last round-trip from DISCARD ALL.

#### sql: use in-memory session data to decide what to do in DISCARD

In #86246 we introduced logic to discard schemas when running DISCARD ALL and DISCARD TEMP. This logic did expensive kv operations unconditionally; if the session knew it had never created a temporary schema, we'd still fetch all databases and proceed to search all databases for a temp schema. This is very expensive.

Fixes: #95864

Release note (performance improvement): In 22.2, logic was added to make
`SET SESSION AUTHORIZATION DEFAULT` not a no-op. This implementation used
more general code for setting the role for a session which made sure that
the role exists. The check for whether a role exists is currently uncached.
We don't need to check if the role we already are exists. This improves the
performance of `DISCARD ALL` in addition to `SET SESSION AUTHORIZATION
DEFAULT`.


Release note (performance improvement): In 22.2, we introduced support for `DISCARD TEMP` and made `DISCARD ALL` actually discard temp tables. This implementation ran expensive logic to discover temp schemas rather than consulting in-memory data structures. As a result, `DISCARD ALL`, which is issued regularly by connection pools, became an expensive operation when it should be cheap. This problem is now resolved.

95997: ui: hide list of statement for non-admins r=maryliag a=maryliag

To get the list of fingerprints used by an index, we use the `system.statement_statistics` directly, but non-admins don't have access to system table.
Until #95756 or #95770 are done, we need to hide
this feature for non-admins.

Part Of #93087

Release note (ui change): Hide list of used fingerprint per index on Index Details page, for non-admin users.

95998: roachtest: update activerecord blocklist r=ZhouXing19 a=andyyang890

This patch removes a few tests from the blocklist that do not
exist in the test suite for rails 7.0.3, which were being run
prior to the fix for version pinning.

Informs #94211

Release note: None

96003: kv: add MVCC local timestamp details to ReadWithinUncertaintyIntervalError r=nvanbenschoten a=nvanbenschoten

This PR adds details about MVCC local timestamps to ReadWithinUncertaintyIntervalErrors. This provides more information about the cause of uncertainty errors.

The PR also includes a series of minor refactors to clean up this code.

Release note: None
Epic: None

Co-authored-by: e-mbrown <[email protected]>
Co-authored-by: Michael Butler <[email protected]>
Co-authored-by: Nathan VanBenschoten <[email protected]>
Co-authored-by: Andrew Werner <[email protected]>
Co-authored-by: maryliag <[email protected]>
Co-authored-by: Andy Yang <[email protected]>

pkg/bench/rttanalysis/BUILD.bazel

@@ -38,6 +38,7 @@ go_test(
         "alter_table_bench_test.go",
         "bench_test.go",
         "create_alter_role_bench_test.go",
+        "discard_bench_test.go",
         "drop_bench_test.go",
         "generate_objects_bench_test.go",
         "grant_revoke_bench_test.go",

pkg/bench/rttanalysis/discard_bench_test.go

@@ -0,0 +1,53 @@
+// Copyright 2020 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+package rttanalysis
+
+import "testing"
+
+func BenchmarkDiscard(b *testing.B) { reg.Run(b) }
+func init() {
+	reg.Register("Discard", []RoundTripBenchTestCase{
+		{
+			Name: "DISCARD ALL, no tables",
+			Setup: `DROP DATABASE IF EXISTS db CASCADE; 
+CREATE DATABASE db;`,
+			Stmt: "DISCARD ALL",
+		},
+		{
+			Name: "DISCARD ALL, 1 tables in 1 db",
+			Setup: `
+SET experimental_enable_temp_tables = true;
+DROP DATABASE IF EXISTS db CASCADE; 
+CREATE DATABASE db; 
+USE db; 
+CREATE TEMPORARY TABLE foo(i int);
+`,
+			Stmt: "DISCARD ALL",
+		},
+		{
+			Name: "DISCARD ALL, 2 tables in 2 dbs",
+			Setup: `
+SET experimental_enable_temp_tables = true;
+DROP DATABASE IF EXISTS db CASCADE; 
+CREATE DATABASE db; 
+USE db; 
+CREATE TEMPORARY TABLE foo(i int);
+CREATE TEMPORARY TABLE bar(i int);
+DROP DATABASE IF EXISTS db2 CASCADE; 
+CREATE DATABASE db2; 
+USE db2; 
+CREATE TEMPORARY TABLE foo(i int);
+CREATE TEMPORARY TABLE bar(i int);
+`,
+			Stmt: "DISCARD ALL",
+		},
+	})
+}

pkg/bench/rttanalysis/testdata/benchmark_expectations

@@ -31,6 +31,9 @@ exp,benchmark
 15,CreateRole/create_role_with_2_options
 18,CreateRole/create_role_with_3_options
 13,CreateRole/create_role_with_no_options
+12,"Discard/DISCARD_ALL,_1_tables_in_1_db"
+15,"Discard/DISCARD_ALL,_2_tables_in_2_dbs"
+0,"Discard/DISCARD_ALL,_no_tables"
 10,DropDatabase/drop_database_0_tables
 11,DropDatabase/drop_database_1_table
 11,DropDatabase/drop_database_2_tables
@@ -47,11 +50,11 @@ exp,benchmark
 12,DropView/drop_1_view
 13,DropView/drop_2_views
 13,DropView/drop_3_views
-5,GenerateObjects/generate_50000_tables
 5,GenerateObjects/generate_1000_tables_-_this_test_should_use_the_same_number_of_RTTs_as_for_10_tables
-5,GenerateObjects/generate_10_tables
 12,GenerateObjects/generate_100_tables_from_template
+5,GenerateObjects/generate_10_tables
 16,GenerateObjects/generate_10x10_schemas_and_tables_in_existing_db
+5,GenerateObjects/generate_50000_tables
 9,Grant/grant_all_on_1_table
 9,Grant/grant_all_on_2_tables
 9,Grant/grant_all_on_3_tables

pkg/cmd/roachtest/tests/activerecord_blocklist.go

@@ -30,10 +30,6 @@ var activeRecordBlocklist = blocklist{
 	"ActiveRecord::CockroachDB::Migration::PGChangeSchemaTest#test_change_type_with_symbol_using_timestamp_with_timestamptz_as_default": "49329",
 	"ActiveRecord::CockroachDB::Migration::PGChangeSchemaTest#test_change_type_with_symbol_with_timestamptz_as_default":                 "49329",
 	"ActiveRecord::CockroachDB::Migration::PGChangeSchemaTest#test_change_type_with_symbol_with_timestamptz":                            "49329",
-	"PostgresqlRangeTest#test_timezone_array_awareness_tsrange":                                                                         "27791",
-	"PostgresqlRangeTest#test_timezone_array_awareness_tzrange":                                                                         "27791",
-	// TODO(yang): investigate cause further
-	"PostgreSQLReferentialIntegrityTest#test_all_foreign_keys_valid_having_foreign_keys_in_multiple_schemas": "unknown",
 }
 
 var activeRecordIgnoreList = blocklist{

pkg/cmd/roachtest/tests/restore.go

@@ -685,12 +685,20 @@ func registerRestore(r registry.Registry) {
 				workload: tpceRestore{customers: 500000}}),
 			timeout: 5 * time.Hour,
 		},
+		{
+			hardware: makeHardwareSpecs(hardwareSpecs{nodes: 15, cpus: 16, volumeSize: 5000}),
+			backup: makeBackupSpecs(backupSpecs{
+				version:  "v22.2.1",
+				aost:     "'2023-01-12 03:00:00'",
+				workload: tpceRestore{customers: 2000000}}),
+			timeout: 24 * time.Hour,
+			tags:    []string{"weekly"},
+		},
 		// TODO(msbutler): add the following tests once roachperf/grafana is hooked up and old tests are
 		// removed:
 		// - restore/tpce/400GB/nodes=10
 		// - restore/tpce/400GB/nodes=30
 		// - restore/tpce/400GB/cpu=16
-		// - restore/tpce/45TB/nodes=15/cpu=16/
 		// - restore/tpce/400GB/encryption
 	} {
 		sp := sp
@@ -707,6 +715,7 @@ func registerRestore(r registry.Registry) {
 			// These tests measure performance. To ensure consistent perf,
 			// disable metamorphic encryption.
 			EncryptionSupport: registry.EncryptionAlwaysDisabled,
+			Tags:              sp.tags,
 			Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
 
 				t.L().Printf("Full test specs: %s", sp.computeName(true))
@@ -881,6 +890,8 @@ func (tpce tpceRestore) String() string {
 		builder.WriteString("400GB")
 	case 500000:
 		builder.WriteString("8TB")
+	case 2000000:
+		builder.WriteString("32TB")
 	default:
 		panic("tpce customer count not recognized")
 	}
@@ -891,6 +902,7 @@ type restoreSpecs struct {
 	hardware hardwareSpecs
 	backup   backupSpecs
 	timeout  time.Duration
+	tags     []string
 }
 
 func (sp restoreSpecs) computeName(full bool) string {

pkg/kv/kvclient/kvcoord/dist_sender_server_test.go

@@ -1811,7 +1811,8 @@ func TestPropagateTxnOnError(t *testing.T) {
 			case *roachpb.ConditionalPutRequest:
 				if k.Equal(keyB) {
 					if atomic.AddInt32(&numCPuts, 1) == 1 {
-						pErr := roachpb.NewReadWithinUncertaintyIntervalError(hlc.Timestamp{}, hlc.Timestamp{}, hlc.Timestamp{}, nil)
+						pErr := roachpb.NewReadWithinUncertaintyIntervalError(
+							hlc.Timestamp{}, hlc.ClockTimestamp{}, nil, hlc.Timestamp{}, hlc.ClockTimestamp{})
 						return roachpb.NewErrorWithTxn(pErr, fArgs.Hdr.Txn)
 					}
 				}
@@ -2046,7 +2047,7 @@ func TestTxnCoordSenderRetries(t *testing.T) {
 					return nil
 				}
 				err := roachpb.NewReadWithinUncertaintyIntervalError(
-					fArgs.Hdr.Timestamp, s.Clock().Now(), hlc.Timestamp{}, fArgs.Hdr.Txn)
+					fArgs.Hdr.Timestamp, hlc.ClockTimestamp{}, fArgs.Hdr.Txn, s.Clock().Now(), hlc.ClockTimestamp{})
 				return roachpb.NewErrorWithTxn(err, fArgs.Hdr.Txn)
 			}
 			return nil

pkg/kv/kvclient/kvcoord/txn_coord_sender_test.go

@@ -757,10 +757,11 @@ func TestTxnCoordSenderTxnUpdatedOnError(t *testing.T) {
 			pErrGen: func(txn *roachpb.Transaction) *roachpb.Error {
 				pErr := roachpb.NewErrorWithTxn(
 					roachpb.NewReadWithinUncertaintyIntervalError(
-						origTS,          // readTS
-						plus10,          // existingTS
-						hlc.Timestamp{}, // localUncertaintyLimit
-						txn,
+						origTS,               // readTS
+						hlc.ClockTimestamp{}, // localUncertaintyLimit
+						txn,                  // txn
+						plus10,               // valueTS
+						hlc.ClockTimestamp{}, // localTS
 					),
 					txn)
 				return pErr
@@ -777,10 +778,11 @@ func TestTxnCoordSenderTxnUpdatedOnError(t *testing.T) {
 			pErrGen: func(txn *roachpb.Transaction) *roachpb.Error {
 				pErr := roachpb.NewErrorWithTxn(
 					roachpb.NewReadWithinUncertaintyIntervalError(
-						origTS, // readTS
-						plus10, // existingTS
-						plus20, // localUncertaintyLimit
-						txn,
+						origTS,                     // readTS
+						hlc.ClockTimestamp(plus20), // localUncertaintyLimit
+						txn,                        // txn
+						plus10,                     // valueTS
+						hlc.ClockTimestamp{},       // localTS
 					),
 					txn)
 				return pErr
@@ -1579,7 +1581,7 @@ func TestAbortTransactionOnCommitErrors(t *testing.T) {
 				txn.UpdateObservedTimestamp(nodeID, makeTS(123, 0).UnsafeToClockTimestamp())
 				return roachpb.NewErrorWithTxn(
 					roachpb.NewReadWithinUncertaintyIntervalError(
-						hlc.Timestamp{}, hlc.Timestamp{}, hlc.Timestamp{}, nil),
+						hlc.Timestamp{}, hlc.ClockTimestamp{}, nil, hlc.Timestamp{}, hlc.ClockTimestamp{}),
 					&txn)
 			},
 			asyncAbort: false},

pkg/kv/kvclient/kvcoord/txn_interceptor_span_refresher_test.go

@@ -159,7 +159,7 @@ func TestTxnSpanRefresherRefreshesTransactions(t *testing.T) {
 			pErr: func() *roachpb.Error {
 				return roachpb.NewError(
 					&roachpb.ReadWithinUncertaintyIntervalError{
-						ExistingTimestamp: txn.WriteTimestamp.Add(25, 0),
+						ValueTimestamp: txn.WriteTimestamp.Add(25, 0),
 					})
 			},
 			expRefresh:   true,
@@ -169,8 +169,8 @@ func TestTxnSpanRefresherRefreshesTransactions(t *testing.T) {
 			pErr: func() *roachpb.Error {
 				return roachpb.NewError(
 					&roachpb.ReadWithinUncertaintyIntervalError{
-						ExistingTimestamp:     txn.WriteTimestamp.Add(25, 0),
-						LocalUncertaintyLimit: txn.WriteTimestamp.Add(30, 0),
+						ValueTimestamp:        txn.WriteTimestamp.Add(25, 0),
+						LocalUncertaintyLimit: hlc.ClockTimestamp(txn.WriteTimestamp.Add(30, 0)),
 					})
 			},
 			expRefresh:   true,

pkg/kv/kvclient/kvcoord/txn_test.go

@@ -648,9 +648,10 @@ func TestTxnCommitTimestampAdvancedByRefresh(t *testing.T) {
 				refreshTS = txn.WriteTimestamp.Add(0, 1)
 				pErr := roachpb.NewReadWithinUncertaintyIntervalError(
 					txn.ReadTimestamp,
+					hlc.ClockTimestamp{},
+					txn,
 					refreshTS,
-					hlc.Timestamp{},
-					txn)
+					hlc.ClockTimestamp{})
 				return roachpb.NewErrorWithTxn(pErr, txn)
 			}
 			return nil

pkg/kv/kvserver/client_replica_test.go

@@ -739,7 +739,7 @@ func testTxnReadWithinUncertaintyIntervalAfterIntentResolution(
 		require.Equal(t, readerTxn.ReadTimestamp, rwuiErr.ReadTimestamp)
 		require.Equal(t, readerTxn.GlobalUncertaintyLimit, rwuiErr.GlobalUncertaintyLimit)
 		require.Equal(t, readerTxn.ObservedTimestamps, rwuiErr.ObservedTimestamps)
-		require.Equal(t, writerTxn.WriteTimestamp, rwuiErr.ExistingTimestamp)
+		require.Equal(t, writerTxn.WriteTimestamp, rwuiErr.ValueTimestamp)
 	}
 }
 

pkg/kv/kvserver/client_split_test.go

@@ -2062,7 +2062,7 @@ func TestStoreRangeSplitRaceUninitializedRHS(t *testing.T) {
 			}
 			return roachpb.NewError(
 				roachpb.NewReadWithinUncertaintyIntervalError(
-					args.Hdr.Timestamp, args.Hdr.Timestamp, hlc.Timestamp{}, nil,
+					args.Hdr.Timestamp, hlc.ClockTimestamp{}, nil, args.Hdr.Timestamp, hlc.ClockTimestamp{},
 				))
 		}
 		return nil

pkg/kv/kvserver/replica.go

@@ -258,47 +258,52 @@ type Replica struct {
 		stateMachine replicaStateMachine
 		// decoder is used to decode committed raft entries.
 		decoder replicaDecoder
+	}
 
-		// The last seen replica descriptors from incoming Raft messages. These are
-		// stored so that the replica still knows the replica descriptors for itself
-		// and for its message recipients in the circumstances when its RangeDescriptor
-		// is out of date.
-		//
-		// Normally, a replica knows about the other replica descriptors for a
-		// range via the RangeDescriptor stored in Replica.mu.state.Desc. But that
-		// descriptor is only updated during a Split or ChangeReplicas operation.
-		// There are periods during a Replica's lifetime when that information is
-		// out of date:
-		//
-		// 1. When a replica is being newly created as the result of an incoming
-		// Raft message for it. This is the common case for ChangeReplicas and an
-		// uncommon case for Splits. The leader will be sending the replica
-		// messages and the replica needs to be able to respond before it can
-		// receive an updated range descriptor (via a snapshot,
-		// changeReplicasTrigger, or splitTrigger).
-		//
-		// 2. If the node containing a replica is partitioned or down while the
-		// replicas for the range are updated. When the node comes back up, other
-		// replicas may begin communicating with it and it needs to be able to
-		// respond. Unlike 1 where there is no range descriptor, in this situation
-		// the replica has a range descriptor but it is out of date. Note that a
-		// replica being removed from a node and then quickly re-added before the
-		// replica has been GC'd will also use the last seen descriptors. In
-		// effect, this is another path for which the replica's local range
-		// descriptor is out of date.
-		//
-		// The last seen replica descriptors are updated on receipt of every raft
-		// message via Replica.setLastReplicaDescriptors (see
-		// Store.HandleRaftRequest). These last seen descriptors are used when
-		// the replica's RangeDescriptor contains missing or out of date descriptors
-		// for a replica (see Replica.sendRaftMessageRaftMuLocked).
-		//
-		// Removing a replica from Store.mu.replicas is not a problem because
-		// when a replica is completely removed, it won't be recreated until
-		// there is another event that will repopulate the replicas map in the
-		// range descriptor. When it is temporarily dropped and recreated, the
-		// newly recreated replica will have a complete range descriptor.
-		lastToReplica, lastFromReplica roachpb.ReplicaDescriptor
+	// The last seen replica descriptors from incoming Raft messages. These are
+	// stored so that the replica still knows the replica descriptors for itself
+	// and for its message recipients in the circumstances when its RangeDescriptor
+	// is out of date.
+	//
+	// Normally, a replica knows about the other replica descriptors for a
+	// range via the RangeDescriptor stored in Replica.mu.state.Desc. But that
+	// descriptor is only updated during a Split or ChangeReplicas operation.
+	// There are periods during a Replica's lifetime when that information is
+	// out of date:
+	//
+	// 1. When a replica is being newly created as the result of an incoming
+	// Raft message for it. This is the common case for ChangeReplicas and an
+	// uncommon case for Splits. The leader will be sending the replica
+	// messages and the replica needs to be able to respond before it can
+	// receive an updated range descriptor (via a snapshot,
+	// changeReplicasTrigger, or splitTrigger).
+	//
+	// 2. If the node containing a replica is partitioned or down while the
+	// replicas for the range are updated. When the node comes back up, other
+	// replicas may begin communicating with it and it needs to be able to
+	// respond. Unlike 1 where there is no range descriptor, in this situation
+	// the replica has a range descriptor but it is out of date. Note that a
+	// replica being removed from a node and then quickly re-added before the
+	// replica has been GC'd will also use the last seen descriptors. In
+	// effect, this is another path for which the replica's local range
+	// descriptor is out of date.
+	//
+	// The last seen replica descriptors are updated on receipt of every raft
+	// message via Replica.setLastReplicaDescriptors (see
+	// Store.HandleRaftRequest). These last seen descriptors are used when
+	// the replica's RangeDescriptor contains missing or out of date descriptors
+	// for a replica (see Replica.sendRaftMessageRaftMuLocked).
+	//
+	// Removing a replica from Store.mu.replicas is not a problem because
+	// when a replica is completely removed, it won't be recreated until
+	// there is another event that will repopulate the replicas map in the
+	// range descriptor. When it is temporarily dropped and recreated, the
+	// newly recreated replica will have a complete range descriptor.
+	//
+	// Locking notes: Replica.raftMu < Replica.mu < Replica.lastSeenReplicas
+	lastSeenReplicas struct {
+		syncutil.Mutex
+		to, from roachpb.ReplicaDescriptor
 	}
 
 	// Contains the lease history when enabled.
@@ -654,11 +659,8 @@ type Replica struct {
 	// loadBasedSplitter keeps information about load-based splitting.
 	loadBasedSplitter split.Decider
 
-	// TODO(tbg): this is effectively unused, we only use it to call ReportUnreachable
-	// when a heartbeat gets dropped but it's unclear whether a) that ever fires in
-	// practice b) if it provides any benefit.
-	//
-	// See: https://github.com/cockroachdb/cockroach/issues/84246
+	// unreachablesMu contains a set of remote ReplicaIDs that are to be reported
+	// as unreachable on the next raft tick.
 	unreachablesMu struct {
 		syncutil.Mutex
 		remotes map[roachpb.ReplicaID]struct{}
@@ -1087,12 +1089,24 @@ func (r *Replica) mergeInProgressRLocked() bool {
 	return r.mu.mergeComplete != nil
 }
 
-// setLastReplicaDescriptors sets the most recently seen replica
-// descriptors to those contained in the *RaftMessageRequest.
-func (r *Replica) setLastReplicaDescriptorsRaftMuLocked(req *kvserverpb.RaftMessageRequest) {
-	r.raftMu.AssertHeld()
-	r.raftMu.lastFromReplica = req.FromReplica
-	r.raftMu.lastToReplica = req.ToReplica
+// setLastReplicaDescriptors sets the most recently seen replica descriptors to
+// those contained in the *RaftMessageRequest.
+// See the comment on Replica.lastSeenReplicas.
+func (r *Replica) setLastReplicaDescriptors(req *kvserverpb.RaftMessageRequest) {
+	lsr := &r.lastSeenReplicas
+	lsr.Lock()
+	defer lsr.Unlock()
+	lsr.to = req.ToReplica
+	lsr.from = req.FromReplica
+}
+
+// getLastReplicaDescriptors gets the most recently seen replica descriptors.
+// See the comment on Replica.lastSeenReplicas.
+func (r *Replica) getLastReplicaDescriptors() (to, from roachpb.ReplicaDescriptor) {
+	lsr := &r.lastSeenReplicas
+	lsr.Lock()
+	defer lsr.Unlock()
+	return lsr.to, lsr.from
 }
 
 // GetMVCCStats returns a copy of the MVCC stats object for this range.