Merge #158304 #160309 #160642 #160647 #160718 #160730 #160765

158304: roachtest: beef up jobs/stress roachtest r=dt a=msbutler

This patch reconfigures the jobs/stress roachtest logic in a few ways:
- now runs on a 20 node cluster in the weekly suite
- lowers the base interval to 0.1, to simulate internal job query contention at
  a 200 node scale
- refines the job control loop logic to pause 20% of running changefeeds,
  resume 20% of paused changefeeds, recreate up to 200 canceled changefeeds,
per iteration.
- fails the roachtest if a job stays unclaimed for more than 5 minutes.

Informs: #158976

Release note: none

160309: backup: include all indexes r=dt a=dt

Restore already only restored the public indexes so no changes there.

Release note: none.
Epic: none.

160642: authors: add amerani to authors r=amerani a=amerani

Epic: None
Release note: None

160647: kvnemesis: disable SQL ops in safety mode r=stevendanna a=miraradeva

Currently, there is a single kvnemesis operation that executes via SQL: `ToggleGlobalReads`. We have seen this operation get stuck and cause the test to timeout under safety more. The expected behavior is that any stuck operations would time out, but it seems like there is a context cancelation propagation issue, most likely in lib/pq, but not confirmed.

This commit disables `ToggleGlobalReads` in safety mode to reduce test failures. This change would also help confirm that this is the only operation susceptible to the hanging behavior.

Fixes: #160293

Release note: None

160718: sql/tests: use longer stmt timeout in TestRandomSyntaxSQLSmith r=yuzefovich a=yuzefovich

Currently all RSG tests use 35s timeout. If a stmt isn't canceled before that, we fail the test, but such a timeout doesn't make it easy to understand whether the stmt was stuck or not. To improve the situation a bit we now give 125s timeout, under the assumptions that goroutine dumps will have 1-2 min blockage making investigations easier.

Fixes: #160678.

Release note: None

160730: sql: alter type hangs on certain conversion errors r=fqazi a=fqazi

Previously, converting invalid text to JSON would lead to the schema changer hanging. This was because the error returned on failed conversion were not marked as user errors. This led to the schema changer retrying them. To address this, this patch labels type conversion errors are user errors.

Fixes: #159958
Release note: None

160765: revert "sql: remove gossip-based DistSQL physical planning" r=yuzefovich a=yuzefovich

This reverts commit fba7dbf.

Currently, using gossip-based planning is the only way of going around #147755. The cost of maintaining this setting is very low, so let's hold off on removal for a little longer.

Epic: None
Release note: None

Co-authored-by: Michael Butler <[email protected]>
Co-authored-by: David Taylor <[email protected]>
Co-authored-by: Alek Merani <[email protected]>
Co-authored-by: Mira Radeva <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>
Co-authored-by: Faizan Qazi <[email protected]>

Author: 7 people

AUTHORS

@@ -49,6 +49,7 @@ Aid Idrizović <[email protected]>
 Ajaya Agrawal <[email protected]>
 Akshay Joshi <[email protected]> <[email protected]>
 Alan Acosta <[email protected]>
+Alek Anwar Merani <[email protected]> <[email protected]>
 Alex Barganier <[email protected]>
 Alex Gaynor <[email protected]>
 Alex Lunev <[email protected]> <[email protected]>

pkg/backup/backup_job.go

@@ -982,11 +982,20 @@ func includeTableSpans(table *descpb.TableDescriptor) bool {
 	return table.IsPhysicalTable()
 }
 
-// forEachPublicIndexTableSpan constructs a span for each public index of the
-// provided table and runs the given function on each of them. The added map is
-// used to track duplicates. Duplicate indexes are not passed to the provided
-// function.
-func forEachPublicIndexTableSpan(
+// forEachIndexTableSpan constructs a span for each index of the provided table
+// (including indexes in mutations) and runs the given function on each of them.
+// The added map is used to track duplicates. Duplicate indexes are not passed
+// to the provided function.
+//
+// This includes:
+// - The primary index
+// - Public secondary indexes
+// - Indexes in mutations (both ADD and DROP directions)
+//
+// Including mutation indexes ensures that data written to indexes being built
+// (e.g., during CREATE INDEX) is captured incrementally by backup, rather than
+// requiring a large catch-up when the index becomes public.
+func forEachIndexTableSpan(
 	table *descpb.TableDescriptor,
 	added map[tableAndIndex]bool,
 	codec keys.SQLCodec,
@@ -996,15 +1005,25 @@ func forEachPublicIndexTableSpan(
 		return
 	}
 
-	table.ForEachPublicIndex(func(idx *descpb.IndexDescriptor) {
+	addIndex := func(idx *descpb.IndexDescriptor) {
 		key := tableAndIndex{tableID: table.GetID(), indexID: idx.ID}
 		if added[key] {
 			return
 		}
 		added[key] = true
 		prefix := roachpb.Key(rowenc.MakeIndexKeyPrefix(codec, table.GetID(), idx.ID))
 		f(roachpb.Span{Key: prefix, EndKey: prefix.PrefixEnd()})
-	})
+	}
+
+	// Public indexes (primary + secondary).
+	table.ForEachPublicIndex(addIndex)
+
+	// Indexes in mutations (adding or dropping).
+	for i := range table.Mutations {
+		if idx := table.Mutations[i].GetIndex(); idx != nil {
+			addIndex(idx)
+		}
+	}
 }
 
 // spansForAllTableIndexes returns non-overlapping spans for every index and
@@ -1026,7 +1045,7 @@ func spansForAllTableIndexes(
 	}
 
 	for _, table := range tables {
-		forEachPublicIndexTableSpan(table.TableDesc(), added, execCfg.Codec, insertSpan)
+		forEachIndexTableSpan(table.TableDesc(), added, execCfg.Codec, insertSpan)
 	}
 
 	// If there are desc revisions, ensure that we also add any index spans
@@ -1039,7 +1058,7 @@ func spansForAllTableIndexes(
 		// entire interval. DROPPED tables should never later become PUBLIC.
 		rawTbl, _, _, _, _ := descpb.GetDescriptors(rev.Desc)
 		if rawTbl != nil && rawTbl.Public() {
-			forEachPublicIndexTableSpan(rawTbl, added, execCfg.Codec, insertSpan)
+			forEachIndexTableSpan(rawTbl, added, execCfg.Codec, insertSpan)
 		}
 	}
 

pkg/backup/backup_test.go

@@ -6187,9 +6187,9 @@ func getMockTableDesc(
 	return tabledesc.NewBuilder(&mockTableDescriptor).BuildImmutableTable()
 }
 
-// Unit tests for the spansForAllTableIndexes and forEachPublicIndexTableSpan()
+// Unit tests for the spansForAllTableIndexes and forEachIndexTableSpan()
 // methods.
-func TestPublicIndexTableSpans(t *testing.T) {
+func TestIndexTableSpans(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 
 	testCases := []struct {
@@ -6211,15 +6211,17 @@ func TestPublicIndexTableSpans(t *testing.T) {
 			expectedMergedSpans: []string{"/Table/55/{1-3}"},
 		},
 		{
-			name:                "dropped-span-between-two-spans",
+			// Dropping index 2 is now included, so spans are contiguous.
+			name:                "dropping-span-between-two-spans",
 			tableID:             56,
 			pkIndex:             getMockIndexDesc(1),
 			indexes:             []descpb.IndexDescriptor{getMockIndexDesc(1), getMockIndexDesc(3)},
 			droppingIndexes:     []descpb.IndexDescriptor{getMockIndexDesc(2)},
-			expectedSpans:       []string{"/Table/56/{1-2}", "/Table/56/{3-4}"},
-			expectedMergedSpans: []string{"/Table/56/{1-2}", "/Table/56/{3-4}"},
+			expectedSpans:       []string{"/Table/56/{1-2}", "/Table/56/{3-4}", "/Table/56/{2-3}"},
+			expectedMergedSpans: []string{"/Table/56/{1-4}"},
 		},
 		{
+			// No mutations, gap remains.
 			name:                "gced-span-between-two-spans",
 			tableID:             57,
 			pkIndex:             getMockIndexDesc(1),
@@ -6228,18 +6230,20 @@ func TestPublicIndexTableSpans(t *testing.T) {
 			expectedMergedSpans: []string{"/Table/57/{1-2}", "/Table/57/{3-4}"},
 		},
 		{
-			name:    "alternate-spans-dropped",
+			// Dropping indexes 2 and 4 are now included, filling the gaps.
+			name:    "alternate-spans-dropping",
 			tableID: 58,
 			pkIndex: getMockIndexDesc(1),
 			indexes: []descpb.IndexDescriptor{
 				getMockIndexDesc(1), getMockIndexDesc(3),
 				getMockIndexDesc(5),
 			},
 			droppingIndexes:     []descpb.IndexDescriptor{getMockIndexDesc(2), getMockIndexDesc(4)},
-			expectedSpans:       []string{"/Table/58/{1-2}", "/Table/58/{3-4}", "/Table/58/{5-6}"},
-			expectedMergedSpans: []string{"/Table/58/{1-2}", "/Table/58/{3-4}", "/Table/58/{5-6}"},
+			expectedSpans:       []string{"/Table/58/{1-2}", "/Table/58/{3-4}", "/Table/58/{5-6}", "/Table/58/{2-3}", "/Table/58/{4-5}"},
+			expectedMergedSpans: []string{"/Table/58/{1-6}"},
 		},
 		{
+			// No mutations, gaps remain.
 			name:    "alternate-spans-gced",
 			tableID: 59,
 			pkIndex: getMockIndexDesc(1),
@@ -6251,6 +6255,7 @@ func TestPublicIndexTableSpans(t *testing.T) {
 			expectedMergedSpans: []string{"/Table/59/{1-2}", "/Table/59/{3-4}", "/Table/59/{5-6}"},
 		},
 		{
+			// Dropping index 2 fills the gap between 1 and 3, but gap at 4 remains.
 			name:    "one-drop-one-gc",
 			tableID: 60,
 			pkIndex: getMockIndexDesc(1),
@@ -6259,22 +6264,22 @@ func TestPublicIndexTableSpans(t *testing.T) {
 				getMockIndexDesc(5),
 			},
 			droppingIndexes:     []descpb.IndexDescriptor{getMockIndexDesc(2)},
-			expectedSpans:       []string{"/Table/60/{1-2}", "/Table/60/{3-4}", "/Table/60/{5-6}"},
-			expectedMergedSpans: []string{"/Table/60/{1-2}", "/Table/60/{3-4}", "/Table/60/{5-6}"},
+			expectedSpans:       []string{"/Table/60/{1-2}", "/Table/60/{3-4}", "/Table/60/{5-6}", "/Table/60/{2-3}"},
+			expectedMergedSpans: []string{"/Table/60/{1-4}", "/Table/60/{5-6}"},
 		},
 		{
-			// Although there are no keys on index 2, we should not include its
-			// span since it holds an adding index.
-			name:    "empty-adding-index",
+			// Adding index 2 is now included, filling the gap. This ensures data
+			// written during index build is captured incrementally by backup.
+			name:    "adding-index-included",
 			tableID: 61,
 			pkIndex: getMockIndexDesc(1),
 			indexes: []descpb.IndexDescriptor{
 				getMockIndexDesc(1), getMockIndexDesc(3),
 				getMockIndexDesc(4),
 			},
 			addingIndexes:       []descpb.IndexDescriptor{getMockIndexDesc(2)},
-			expectedSpans:       []string{"/Table/61/{1-2}", "/Table/61/{3-4}", "/Table/61/{4-5}"},
-			expectedMergedSpans: []string{"/Table/61/{1-2}", "/Table/61/{3-5}"},
+			expectedSpans:       []string{"/Table/61/{1-2}", "/Table/61/{3-4}", "/Table/61/{4-5}", "/Table/61/{2-3}"},
+			expectedMergedSpans: []string{"/Table/61/{1-5}"},
 		},
 	}
 
@@ -6299,9 +6304,9 @@ func TestPublicIndexTableSpans(t *testing.T) {
 			for _, test := range testCases {
 				tableDesc := getMockTableDesc(test.tableID, test.pkIndex,
 					test.indexes, test.addingIndexes, test.droppingIndexes)
-				t.Run(fmt.Sprintf("%s:%s", "forEachPublicIndexTableSpan", test.name), func(t *testing.T) {
+				t.Run(fmt.Sprintf("%s:%s", "forEachIndexTableSpan", test.name), func(t *testing.T) {
 					var spans []roachpb.Span
-					forEachPublicIndexTableSpan(tableDesc.TableDesc(), unusedMap, codec, func(sp roachpb.Span) {
+					forEachIndexTableSpan(tableDesc.TableDesc(), unusedMap, codec, func(sp roachpb.Span) {
 						spans = append(spans, sp)
 					})
 					var unmergedSpans []string