Merge #131048

131048: row: add debugging for TestExternalRowData r=mgartner,mw5h a=michae2 When modifying keys in the BatchRequest for external row data, we sometimes see that the key has already been modified. We suspect that multiple keys in the BatchRequest share an underlying array. Change the crdb-test-build-only assertion to a panic to get a stacktrace during this rare test failure. Informs: #128070 Release note: None Co-authored-by: Michael Erickson <[email protected]>
cockroachdb · Sep 23, 2024 · 1c2c3f0 · 1c2c3f0
2 parents 0463687 + f07ca5e
commit 1c2c3f0
Show file tree

Hide file tree

Showing 3 changed files with 26 additions and 8 deletions.
diff --git a/pkg/sql/row/external_row_data_test.go b/pkg/sql/row/external_row_data_test.go
@@ -12,6 +12,7 @@ package row_test
 
 import (
 	"context"
+	"fmt"
 	"testing"
 
 	"github.com/cockroachdb/cockroach/pkg/base"
@@ -105,7 +106,10 @@ func TestExternalRowData(t *testing.T) {
 				expected: [][]string{{"2", "2", "-2"}},
 			},
 		} {
-			require.Equal(t, tc.expected, r.QueryStr(t, tc.query))
+
+			require.Equal(t, tc.expected, r.QueryStrMeta(
+				t, fmt.Sprintf("vectorize=%v", vectorize), tc.query,
+			))
 		}
 	}
 }
diff --git a/pkg/sql/row/kv_batch_fetcher.go b/pkg/sql/row/kv_batch_fetcher.go
@@ -310,9 +310,11 @@ func makeExternalSpanSendFunc(
 	rewrite := func(k roachpb.Key) (roachpb.Key, error) {
 		if buildutil.CrdbTestBuild {
 			if !bytes.HasPrefix(k, ext.OldPrefix) {
-				return nil, errors.AssertionFailedf(
-					"external row data does not have old prefix, key=%v, oldPrefix=%v", k, ext.OldPrefix,
-				)
+				// Panic in order to get a full stacktrace.
+				panic(errors.AssertionFailedf(
+					"external row data does not have old prefix, key=%v, keybytes=%v oldPrefix=%v",
+					k, []byte(k), ext.OldPrefix,
+				))
 			}
 		}
 		if len(ext.OldPrefix) == len(ext.NewPrefix) {

diff --git a/pkg/testutils/sqlutils/sql_runner.go b/pkg/testutils/sqlutils/sql_runner.go
@@ -279,18 +279,30 @@ func (sr *SQLRunner) QueryRow(t Fataler, query string, args ...interface{}) *Row
 	return &Row{t, sr.DB.QueryRowContext(context.Background(), query, args...)}
 }
 
-// QueryStr runs a Query and converts the result using RowsToStrMatrix. Kills
-// the test on errors.
-func (sr *SQLRunner) QueryStr(t Fataler, query string, args ...interface{}) [][]string {
+// QueryStrMeta runs a Query and converts the result using RowsToStrMatrix. Kills
+// the test on errors, including meta string in error message.
+func (sr *SQLRunner) QueryStrMeta(
+	t Fataler, meta string, query string, args ...interface{},
+) [][]string {
 	helperOrNoop(t)()
 	rows := sr.Query(t, query, args...)
 	r, err := RowsToStrMatrix(rows)
 	if err != nil {
-		t.Fatalf("%v", err)
+		if meta == "" {
+			t.Fatalf("%v", err)
+		} else {
+			t.Fatalf("%s: %v", meta, err)
+		}
 	}
 	return r
 }
 
+// QueryStr runs a Query and converts the result using RowsToStrMatrix. Kills
+// the test on errors.
+func (sr *SQLRunner) QueryStr(t Fataler, query string, args ...interface{}) [][]string {
+	return sr.QueryStrMeta(t, "", query, args...)
+}
+
 // RowsToStrMatrix converts the given result rows to a string matrix; nulls are
 // represented as "NULL". Empty results are represented by an empty (but
 // non-nil) slice.