Merge #156403 #156410 #156468

156403: catalog/lease: fix queries against initial versions in locked mode r=fqazi a=fqazi

Previously, all of our testing for locked timestamping included WaitForInitialVersion being enabled. This configuration ensured that we would always acquire the first version of leased descriptors in our tests. However, upon enabling locked leasing across the binary, we encountered issues because bootstrapped tables and other tests might not secure the initial version checkout. Consequently, we would incorrectly declare the object as non-existent if the lease timestamp precoded the object's creation.

Fixes: #156401

Release note: None

156410: *: address a couple of nits r=yuzefovich a=yuzefovich

We can use `settings.Fraction` for non-negative float with max 1.0. Also fix up one comment. (Noticed this while reviewing the probabilistic transaction tracing backport.)

Epic: None
Release note: None

156468: sql: increase timeout for inspect tests r=bghal a=bghal

The `sql/inspect.TestInspectMetrics` test was flaking in CI. Increasing
the size will increase the timeout.

Epic: none
Fixes: #156197
Fixes: #156082
Fixes: #156238

Release note: None


Co-authored-by: Faizan Qazi <faizan@cockroachlabs.com>
Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
Co-authored-by: Brendan Gerrity <brendan.gerrity@cockroachlabs.com>

Author: 4 people

pkg/crosscluster/settings.go

@@ -59,7 +59,7 @@ var ReplanThreshold = settings.RegisterFloatSetting(
 	"fraction of nodes in the producer or consumer job that would need to change to refresh the"+
 		" physical execution plan. If set to 0, the physical plan will not automatically refresh.",
 	0.1,
-	settings.NonNegativeFloatWithMaximum(1),
+	settings.Fraction,
 	settings.WithName("physical_replication.consumer.replan_flow_threshold"),
 )
 
@@ -116,7 +116,7 @@ var LogicalReplanThreshold = settings.RegisterFloatSetting(
 	"fraction of nodes in the producer or consumer job that would need to change to refresh the"+
 		" physical execution plan. If set to 0, the physical plan will not automatically refresh.",
 	0.1,
-	settings.NonNegativeFloatWithMaximum(1),
+	settings.Fraction,
 )
 
 var LogicalReplanFrequency = settings.RegisterDurationSetting(

pkg/sql/catalog/lease/descriptor_set.go

@@ -84,6 +84,13 @@ func (l *descriptorSet) findIndex(version descpb.DescriptorVersion) (int, bool)
 	return i, false
 }
 
+func (l *descriptorSet) findOldest() *descriptorVersionState {
+	if len(l.data) == 0 {
+		return nil
+	}
+	return l.data[0]
+}
+
 func (l *descriptorSet) findNewest() *descriptorVersionState {
 	if len(l.data) == 0 {
 		return nil

pkg/sql/catalog/lease/descriptor_state.go

@@ -105,6 +105,9 @@ func (t *descriptorState) findForTimestampImpl(
 	readTimestamp hlc.Timestamp,
 	expensiveLogEnabled bool,
 ) (*descriptorVersionState, bool, error) {
+	// Normally this is true if an older timestamp is intentionally used for
+	// locked leasing.
+	hasDifferentReadTimeStamp := leaseTimestamp != readTimestamp
 	// Walk back the versions to find one that is valid for the timestamp.
 	for i := len(t.mu.active.data) - 1; i >= 0; i-- {
 		// Check to see if the ModificationTime is valid. If only the initial version
@@ -115,7 +118,7 @@ func (t *descriptorState) findForTimestampImpl(
 				// Existing valid descriptor version.
 				desc.incRefCount(ctx, expensiveLogEnabled)
 				return desc, latest, nil
-			} else if !latest && readTimestamp != leaseTimestamp {
+			} else if !latest && hasDifferentReadTimeStamp {
 				// The lease timestamp is not compatible with the read timestamp, since
 				// the descriptor returned will be expired. This means we are seeing the
 				// first read of this descriptor, since the prior version was not locked.
@@ -133,6 +136,16 @@ func (t *descriptorState) findForTimestampImpl(
 		}
 	}
 
+	// If we have the initial version of the descriptor, and it satisfies the read
+	// timestamp, then the object was just created. We can confirm it satisfies
+	// the request, by executing findForTimestampImpl with the readTimestamp instead.
+	if oldest := t.mu.active.findOldest(); hasDifferentReadTimeStamp &&
+		oldest != nil &&
+		oldest.GetVersion() == 1 &&
+		oldest.GetModificationTime().LessEq(readTimestamp) {
+		return t.findForTimestampImpl(ctx, readTimestamp, readTimestamp, expensiveLogEnabled)
+	}
+
 	return nil, false, errReadOlderVersion
 }
 

pkg/sql/catalog/lease/lease_internal_test.go

@@ -1747,6 +1747,10 @@ func TestLeaseManagerLockedTimestampBasic(t *testing.T) {
 	st := cluster.MakeTestingClusterSettings()
 	ctx := context.Background()
 	LockedLeaseTimestamp.Override(ctx, &st.SV, true)
+	// Intentionally disable WaitForInitialVersion support, so that we can run
+	// historical queries at timestamps before the lease manager is fully caught
+	// up.
+	WaitForInitialVersion.Override(ctx, &st.SV, false)
 	srv, db, _ := serverutils.StartServer(
 		t, base.TestServerArgs{
 			// Avoid using tenants since async tenant migration steps can acquire
@@ -1770,6 +1774,11 @@ func TestLeaseManagerLockedTimestampBasic(t *testing.T) {
 	r := sqlutils.MakeSQLRunner(db)
 
 	r.Exec(t, "CREATE TABLE t1(n int)")
+	// These queries will be intentionally executed before the lease manager is
+	// fully aware of their existence, since WaitForInitialVersion support is
+	// disabled. We should be forced to use a non-locked timestamp to be aware
+	// of the tables existence.
+	r.Exec(t, "SELECT * FROM t1")
 	r.Exec(t, "INSERT INTO t1 VALUES (1)")
 
 	grp := ctxgroup.WithContext(context.Background())

pkg/sql/exec_util.go

@@ -293,7 +293,7 @@ var TraceTxnSampleRate = settings.RegisterFloatSetting(
 		"will have tracing enabled, and only those which exceed the configured "+
 		"threshold will be logged.",
 	1.0,
-	settings.NonNegativeFloatWithMaximum(1.0),
+	settings.Fraction,
 	settings.WithPublic)
 
 // TraceTxnOutputJaegerJSON sets the output format of transaction trace logs.

pkg/sql/inspect/BUILD.bazel

@@ -68,6 +68,7 @@ go_library(
 
 go_test(
     name = "inspect_test",
+    size = "large",
     srcs = [
         "index_consistency_check_test.go",
         "inspect_job_test.go",

pkg/sql/stats/histogram.go

@@ -63,7 +63,7 @@ var MaxFractionHistogramMCVs = settings.RegisterFloatSetting(
 	"sql.stats.histogram_buckets.max_fraction_most_common_values",
 	"maximum fraction of histogram buckets to use for most common values",
 	0.1,
-	settings.NonNegativeFloatWithMaximum(1),
+	settings.Fraction,
 	settings.WithPublic)
 
 // HistogramVersion identifies histogram versions.

pkg/util/log/logtestutils/log_spy.go

@@ -86,7 +86,7 @@ func (s *LogSpy) Reset() {
 	s.mu.logs = []logpb.Entry{}
 }
 
-// ReadAll consumes the specified number of logs contained within
+// Read consumes the specified number of logs contained within
 // the spy and returns them as a list. Once the logs are consumed
 // they cannot be read again.
 func (s *LogSpy) Read(n int) []logpb.Entry {