Merge #133130 #133256

133130: sqlccl: deflake TestExplainGist when run with concurrent ALTER PK r=rafiss,michae2 a=spilchen

TestExplainGist occasionally fails when a query using a secondary index tries to fetch a column not included in that index (see issue #130282). This change doesn’t address the root cause, but instead ignores the error when it occurs. I've also created a more reliable reproducer in the TestDMLInjectionTest, which we can use to validate the eventual fix (#133129).

Epic: none
Closes #130282
Release note: none

133256: roachprod: default to buffering file sinks in roachprod r=jbowens a=jbowens

In roachprod clusters, default to using buffering in file sinks. This is required by a subsequent change that will default to using WAL failover in roachprod clusters.

Informs #133248
Informs #129922
Epic: CRDB-37534
Release note: none

Co-authored-by: Matt Spilchen <[email protected]>
Co-authored-by: Jackson Owens <[email protected]>

pkg/ccl/testccl/sqlccl/explain_test.go

@@ -191,6 +191,7 @@ func TestExplainGist(t *testing.T) {
 				for _, knownErr := range []string{
 					"invalid datum type given: RECORD, expected RECORD", // #117101
 					"expected equivalence dependants to be its closure", // #119045
+					"not in index", // #133129
 				} {
 					if strings.Contains(err.Error(), knownErr) {
 						// Don't fail the test on a set of known errors.

pkg/cmd/roachtest/tests/disk_stall.go

@@ -73,9 +73,6 @@ func runDiskStalledWALFailover(
 		startOpts.RoachprodOpts.StoreCount = 2
 	}
 	startOpts.RoachprodOpts.ExtraArgs = []string{
-		// Adopt buffering of the file logging to ensure that we don't block on
-		// flushing logs to the stalled device.
-		"--log", fmt.Sprintf(`{file-defaults: {dir: "%s", buffered-writes: false, buffering: {max-staleness: 1s, flush-trigger-size: 256KiB, max-buffer-size: 50MiB}}}`, s.LogDir()),
 		"--wal-failover=" + failoverFlag,
 	}
 	c.Start(ctx, t.L(), startOpts, startSettings, c.CRDBNodes())

pkg/roachprod/install/files/cockroachdb-logging.yaml

@@ -1,7 +1,11 @@
 ---
 file-defaults:
   auditable: false
-  buffered-writes: true
+  buffered-writes: false
+  buffering:
+    max-staleness: 5s
+    flush-trigger-size: 256KiB
+    max-buffer-size: 50MiB
   dir: #{ .LogDir #}
   exit-on-error: false
   filter: INFO

pkg/sql/schemachanger/dml_injection_test.go

@@ -116,6 +116,9 @@ type testCase struct {
 	schemaChange string
 	expectedErr  string
 	skipIssue    int
+	// Optional: If you want a query to run at each stage, you can include it here.
+	// We don't evaluate the results; we simply assert that the query executes without errors.
+	query string
 }
 
 // Captures testCase before t.Parallel is called.
@@ -299,6 +302,17 @@ func TestAlterTableDMLInjection(t *testing.T) {
 			},
 			schemaChange: "ALTER TABLE tbl ALTER PRIMARY KEY USING COLUMNS (insert_phase_ordinal, operation_phase_ordinal, operation)",
 		},
+		{
+			desc:        "alter primary key and replace rowid in PK",
+			createTable: createTableNoPK,
+			setup: []string{
+				"CREATE INDEX i1 ON tbl (val)",
+			},
+			// Run a query against the secondary index at each stage.
+			query:        "SELECT operation FROM tbl@i1",
+			schemaChange: "ALTER TABLE tbl ALTER PRIMARY KEY USING COLUMNS (insert_phase_ordinal, operation_phase_ordinal, operation)",
+			skipIssue:    133129,
+		},
 		{
 			desc:        "alter primary key using columns using hash",
 			createTable: createTableNoPK,
@@ -523,6 +537,12 @@ func TestAlterTableDMLInjection(t *testing.T) {
 								require.Subset(t, expectedResults, actualResults, errorMessage)
 								require.Subset(t, actualResults, expectedResults, errorMessage)
 
+								// If a query is provided, run it without checking the results—just
+								// ensure it doesn't fail.
+								if tc.query != "" {
+									sqlDB.Exec(t, tc.query)
+								}
+
 								for i := 0; i < poIdx; i++ {
 									insertPO := poSlice[i]
 									// Verify 1 row is correctly deleted.