Merge #63238 #63244

63238: roachtest: update libpq blocklist to ignore TestCopyInBinaryError r=rafiss a=RichardJCai

roachtest: update libpq blocklist to ignore TestCopyInBinaryError

TestCopyInBinary's behaviour was incorrect in the test since we were not receiving an expected error (`pq: only text format supported for COPY`). 
Furthermore the test would sporadically panic causing the following tests to fail.

Release note: None

Resolves #57855 

63244: logictest: compare floating point values approximately on s390x r=ajwerner a=jonathan-albrecht-ibm

### Overview
On s390x in the std math package and some c-deps, floating point calculations can produce results that differ from the values calculated on amd64. This patch adds a function to compare logictest floating point and decimal values within a small relative margin on s390x. The existing behavior on all other platforms remains the same.

On s390x, there are three main reasons that floating point calculations sometimes give different results:
* the go compiler generates the s390x "fused multiply and add" (FMA) instruction where possible,
* the go math package uses s390x optimized versions of some functions,
* some c libs eg. libgeos, libproj also have platform specific floating point calculation differences.

### Proposal
The motivation for this work is so that users building CRDB on s390x do not need to diagnose tests that fail because of platform dependent floating point differences.

This PR proposes one possible approach to dealing with platform dependent floating point differences. Since development, testing and CI are done on amd64 it keeps the current logic for determining float equality exactly the same. On s390x, it determines values of decimal and float column types (R and F) in query tests to be equal if they are within a tolerance. See the new pkg/testutils/floatcmp package for the implementation of the approximate equality logic and changes in logictest.go to see how it is applied to only s390x.

There are probably other approaches I haven't thought of that would also work. I'd like to use this proposal to start a conversation on how all tests in CRDB that currently fail due to expected floating point differences could eventually be made to pass.

Of course platforms other than s390x may also have differences but I haven't looked at any other platforms. The changes should be easily extendable to other platforms if needed.

### Future Work
The changes in this PR allow the following tests to pass on s390x:
* TestLogic/fakedist-disk/builtin_function/extra_float_digits_3
*  TestLogic/fakedist-metadata/builtin_function/extra_float_digits_3
*  TestLogic/fakedist-vec-off/builtin_function/extra_float_digits_3
*  TestLogic/fakedist/builtin_function/extra_float_digits_3
*  TestLogic/local-spec-planning/builtin_function/extra_float_digits_3
*  TestLogic/local-vec-off/builtin_function/extra_float_digits_3
*  TestLogic/local/builtin_function/extra_float_digits_3

There are about 70 more tests that currently fail due to platform floating point differences on s390x, many are tests of geospatial functions. Assuming we can come up with a good approach, I'd like to continue working on fixes to be submitted in future PRs.

Release note: None

Co-authored-by: richardjcai <[email protected]>
Co-authored-by: Jonathan Albrecht <[email protected]>

pkg/BUILD.bazel

@@ -267,6 +267,7 @@ ALL_TESTS = [
     "//pkg/storage/enginepb:enginepb_test",
     "//pkg/storage/metamorphic:metamorphic_test",
     "//pkg/storage:storage_test",
+    "//pkg/testutils/floatcmp:floatcmp_test",
     "//pkg/testutils/keysutils:keysutils_test",
     "//pkg/testutils/lint/passes/fmtsafe:fmtsafe_test",
     "//pkg/testutils/lint/passes/forbiddenmethod:forbiddenmethod_test",

pkg/cmd/roachtest/libpq.go

@@ -14,6 +14,9 @@ import (
 	"context"
 	"fmt"
 	"regexp"
+	"strings"
+
+	"github.com/stretchr/testify/require"
 )
 
 var libPQReleaseTagRegex = regexp.MustCompile(`^v(?P<major>\d+)\.(?P<minor>\d+)\.(?P<point>\d+)$`)
@@ -29,19 +32,14 @@ func registerLibPQ(r *testRegistry) {
 		c.Put(ctx, cockroach, "./cockroach", c.All())
 		c.Start(ctx, t, c.All())
 		version, err := fetchCockroachVersion(ctx, c, node[0])
-		if err != nil {
-			t.Fatal(err)
-		}
-		if err := alterZoneConfigAndClusterSettings(ctx, version, c, node[0]); err != nil {
-			t.Fatal(err)
-		}
+		require.NoError(t, err)
+		err = alterZoneConfigAndClusterSettings(ctx, version, c, node[0])
+		require.NoError(t, err)
 
 		t.Status("cloning lib/pq and installing prerequisites")
 		latestTag, err := repeatGetLatestTag(
 			ctx, c, "lib", "pq", libPQReleaseTagRegex)
-		if err != nil {
-			t.Fatal(err)
-		}
+		require.NoError(t, err)
 		c.l.Printf("Latest lib/pq release is %s.", latestTag)
 		c.l.Printf("Supported lib/pq release is %s.", libPQSupportedTag)
 
@@ -55,59 +53,85 @@ func registerLibPQ(r *testRegistry) {
 		)
 
 		// Remove any old lib/pq installations
-		if err := repeatRunE(
+		err = repeatRunE(
 			ctx, c, node, "remove old lib/pq", fmt.Sprintf("rm -rf %s", libPQPath),
-		); err != nil {
-			t.Fatal(err)
-		}
+		)
+		require.NoError(t, err)
 
 		// Install go-junit-report to convert test results to .xml format we know
 		// how to work with.
-		if err := repeatRunE(
-			ctx, c, node, "install go-junit-report", fmt.Sprintf("GOPATH=%s go get -u github.com/jstemmer/go-junit-report", goPath),
-		); err != nil {
-			t.Fatal(err)
-		}
+		err = repeatRunE(ctx, c, node, "install go-junit-report",
+			fmt.Sprintf("GOPATH=%s go get -u github.com/jstemmer/go-junit-report", goPath),
+		)
+		require.NoError(t, err)
 
-		if err := repeatGitCloneE(
+		err = repeatGitCloneE(
 			ctx,
 			t.l,
 			c,
 			fmt.Sprintf("https://%s.git", libPQRepo),
 			libPQPath,
 			libPQSupportedTag,
 			node,
-		); err != nil {
-			t.Fatal(err)
-		}
-
+		)
+		require.NoError(t, err)
 		_ = c.RunE(ctx, node, fmt.Sprintf("mkdir -p %s", resultsDir))
 
-		blocklistName, expectedFailures, ignorelistName, ignoredFailures := libPQBlocklists.getLists(version)
+		blocklistName, expectedFailures, ignorelistName, ignoreList := libPQBlocklists.getLists(version)
 		if expectedFailures == nil {
 			t.Fatalf("No lib/pq blocklist defined for cockroach version %s", version)
 		}
 		c.l.Printf("Running cockroach version %s, using blocklist %s, using ignorelist %s", version, blocklistName, ignorelistName)
 
 		t.Status("running lib/pq test suite and collecting results")
 
+		// List all the tests that start with Test or Example.
+		testListRegex := "^(Test|Example)"
+		buf, err := c.RunWithBuffer(
+			ctx,
+			t.l,
+			node,
+			fmt.Sprintf(`cd %s && PGPORT=26257 PGUSER=root PGSSLMODE=disable PGDATABASE=postgres go test -list "%s"`, libPQPath, testListRegex),
+		)
+		require.NoError(t, err)
+
+		// Convert the output of go test -list into an list.
+		tests := strings.Fields(string(buf))
+		var allowedTests []string
+
+		for _, testName := range tests {
+			// Ignore tests that do not match the test regex pattern.
+			matched, err := regexp.MatchString(testListRegex, testName)
+			require.NoError(t, err)
+			if !matched {
+				continue
+			}
+			// If the test is part of ignoreList, do not run the test.
+			if _, ok := ignoreList[testName]; !ok {
+				allowedTests = append(allowedTests, testName)
+			}
+		}
+
+		allowedTestsRegExp := fmt.Sprintf(`"^(%s)$"`, strings.Join(allowedTests, "|"))
+
 		// Ignore the error as there will be failing tests.
 		_ = c.RunE(
 			ctx,
 			node,
-			fmt.Sprintf("cd %s && PGPORT=26257 PGUSER=root PGSSLMODE=disable PGDATABASE=postgres go test -v 2>&1 | %s/bin/go-junit-report > %s", libPQPath, goPath, resultsPath),
+			fmt.Sprintf("cd %s && PGPORT=26257 PGUSER=root PGSSLMODE=disable PGDATABASE=postgres go test -run %s -v 2>&1 | %s/bin/go-junit-report > %s",
+				libPQPath, allowedTestsRegExp, goPath, resultsPath),
 		)
 
 		parseAndSummarizeJavaORMTestsResults(
 			ctx, t, c, node, "lib/pq" /* ormName */, []byte(resultsPath),
-			blocklistName, expectedFailures, ignoredFailures, version, latestTag,
+			blocklistName, expectedFailures, ignoreList, version, latestTag,
 		)
 	}
 
 	r.Add(testSpec{
 		Name:       "lib/pq",
 		Owner:      OwnerSQLExperience,
-		MinVersion: "v19.2.0",
+		MinVersion: "v20.1.0",
 		Cluster:    makeClusterSpec(1),
 		Tags:       []string{`default`, `driver`},
 		Run:        runLibPQ,

pkg/cmd/roachtest/libpq_blocklist.go

@@ -11,7 +11,6 @@
 package main
 
 var libPQBlocklists = blocklistsForVersion{
-	{"v19.2", "libPQBlocklist19_2", libPQBlocklist19_2, "libPQIgnorelist19_2", libPQIgnorelist19_2},
 	{"v20.1", "libPQBlocklist20_1", libPQBlocklist20_1, "libPQIgnorelist20_1", libPQIgnorelist20_1},
 	{"v20.2", "libPQBlocklist20_2", libPQBlocklist20_2, "libPQIgnorelist20_2", libPQIgnorelist20_2},
 	{"v21.1", "libPQBlocklist21_1", libPQBlocklist21_1, "libPQIgnorelist21_1", libPQIgnorelist21_1},
@@ -33,7 +32,6 @@ var libPQBlocklist20_2 = blocklist{
 	"pq.TestContextCancelExec":                       "41335",
 	"pq.TestContextCancelQuery":                      "41335",
 	"pq.TestCopyFromError":                           "5807",
-	"pq.TestCopyInBinaryError":                       "5807",
 	"pq.TestCopyInRaiseStmtTrigger":                  "5807",
 	"pq.TestCopyInTypes":                             "5807",
 	"pq.TestCopyRespLoopConnectionError":             "5807",
@@ -71,7 +69,6 @@ var libPQBlocklist20_1 = blocklist{
 	"pq.TestContextCancelExec":                                 "41335",
 	"pq.TestContextCancelQuery":                                "41335",
 	"pq.TestCopyFromError":                                     "5807",
-	"pq.TestCopyInBinaryError":                                 "5807",
 	"pq.TestCopyInRaiseStmtTrigger":                            "5807",
 	"pq.TestCopyInTypes":                                       "5807",
 	"pq.TestCopyRespLoopConnectionError":                       "5807",
@@ -107,81 +104,21 @@ var libPQBlocklist20_1 = blocklist{
 	"pq.TestTimeWithoutTimezone/24:00_=>_0000-01-02T00:00:00Z":           "44548",
 }
 
-var libPQBlocklist19_2 = blocklist{
-	"pq.ExampleConnectorWithNoticeHandler":           "unknown",
-	"pq.TestBinaryByteSliceToInt":                    "41547",
-	"pq.TestBinaryByteSlicetoUUID":                   "41547",
-	"pq.TestBindError":                               "5807",
-	"pq.TestByteaOutputFormats":                      "26947",
-	"pq.TestCommit":                                  "5807",
-	"pq.TestConnListen":                              "41522",
-	"pq.TestConnUnlisten":                            "41522",
-	"pq.TestConnUnlistenAll":                         "41522",
-	"pq.TestConnectorWithNoticeHandler_Simple":       "unknown",
-	"pq.TestConnectorWithNotificationHandler_Simple": "unknown",
-	"pq.TestContextCancelBegin":                      "41335",
-	"pq.TestContextCancelExec":                       "41335",
-	"pq.TestContextCancelQuery":                      "41335",
-	"pq.TestCopyFromError":                           "5807",
-	"pq.TestCopyInBinaryError":                       "5807",
-	"pq.TestCopyInMultipleValues":                    "5807",
-	"pq.TestCopyInRaiseStmtTrigger":                  "5807",
-	"pq.TestCopyInStmtAffectedRows":                  "5807",
-	"pq.TestCopyInTypes":                             "5807",
-	"pq.TestCopyInWrongType":                         "5807",
-	"pq.TestCopyRespLoopConnectionError":             "5807",
-	"pq.TestEncodeAndParseTs":                        "41563",
-	"pq.TestErrorDuringStartup":                      "41551",
-	"pq.TestErrorOnExec":                             "5807",
-	"pq.TestErrorOnQuery":                            "5807",
-	"pq.TestErrorOnQueryRowSimpleQuery":              "5807",
-	"pq.TestExec":                                    "5807",
-	"pq.TestInfinityTimestamp":                       "41564",
-	"pq.TestIssue186":                                "41558",
-	"pq.TestIssue196":                                "41689",
-	"pq.TestIssue282":                                "12137",
-	"pq.TestIssue494":                                "5807",
-	"pq.TestListenerFailedQuery":                     "41522",
-	"pq.TestListenerListen":                          "41522",
-	"pq.TestListenerReconnect":                       "41522",
-	"pq.TestListenerUnlisten":                        "41522",
-	"pq.TestListenerUnlistenAll":                     "41522",
-	"pq.TestNotifyExtra":                             "41522",
-	"pq.TestPing":                                    "35897",
-	"pq.TestQueryRowBugWorkaround":                   "5807",
-	"pq.TestReconnect":                               "35897",
-	"pq.TestReturning":                               "5807",
-	"pq.TestRowsColumnTypes":                         "41688",
-	"pq.TestRowsResultTag":                           "5807",
-	"pq.TestRuntimeParameters":                       "12137",
-	"pq.TestStringWithNul":                           "26366",
-	"pq.TestTimeWithTimezone":                        "44548",
-	"pq.TestTimeWithTimezone/11:59:59+00:00_=>_0000-01-01T11:59:59Z":      "44548",
-	"pq.TestTimeWithTimezone/11:59:59+04:00_=>_0000-01-01T11:59:59+04:00": "44548",
-	"pq.TestTimeWithTimezone/24:00+00_=>_0000-01-02T00:00:00Z":            "44548",
-	"pq.TestTimeWithTimezone/24:00-04:00_=>_0000-01-02T00:00:00-04:00":    "44548",
-	"pq.TestTimeWithTimezone/24:00:00+00_=>_0000-01-02T00:00:00Z":         "44548",
-	"pq.TestTimeWithTimezone/24:00:00.0+00_=>_0000-01-02T00:00:00Z":       "44548",
-	"pq.TestTimeWithTimezone/24:00:00.000000+00_=>_0000-01-02T00:00:00Z":  "44548",
-	"pq.TestTimeWithTimezone/24:00Z_=>_0000-01-02T00:00:00Z":              "44548",
-	"pq.TestTimeWithoutTimezone":                                          "44548",
-	"pq.TestTimeWithoutTimezone/24:00:00.000000_=>_0000-01-02T00:00:00Z":  "44548",
-	"pq.TestTimeWithoutTimezone/24:00:00.0_=>_0000-01-02T00:00:00Z":       "44548",
-	"pq.TestTimeWithoutTimezone/24:00:00_=>_0000-01-02T00:00:00Z":         "44548",
-	"pq.TestTimeWithoutTimezone/24:00_=>_0000-01-02T00:00:00Z":            "44548",
-	"pq.TestTimestampWithTimeZone":                                        "41565",
-}
-
 var libPQIgnorelist21_1 = libPQIgnorelist20_2
 
 var libPQIgnorelist20_2 = libPQIgnorelist20_1
 
-var libPQIgnorelist20_1 = libPQIgnorelist19_2
-
-var libPQIgnorelist19_2 = blocklist{
+// The test names here do not include "pq." since `go test -list` returns
+// the test name without "pq.". We use the name returned from `go test -list`
+// to ignore the test.
+var libPQIgnorelist20_1 = blocklist{
 	// TestFormatTsBacked fails due to not returning an error for accepting a
 	// timestamp format that postgres does not.
-	"pq.TestFormatTsBackend": "41690",
+	"TestFormatTsBackend": "41690",
 	// TestTxOptions fails because it attempts to change isolation levels.
-	"pq.TestTxOptions": "41690",
+	"TestTxOptions": "41690",
+	// TestCopyInBinaryError is expected to error with:
+	// pq: only text format supported for COPY, however no error is returned
+	// for CRDB.
+	"TestCopyInBinaryError": "63235",
 }

pkg/sql/logictest/BUILD.bazel

@@ -32,6 +32,7 @@ go_library(
         "//pkg/sql/sessiondata",
         "//pkg/sql/stats",
         "//pkg/testutils",
+        "//pkg/testutils/floatcmp",
         "//pkg/testutils/physicalplanutils",
         "//pkg/testutils/serverutils",
         "//pkg/testutils/skip",

pkg/sql/logictest/logic.go

@@ -27,6 +27,7 @@ import (
 	"path/filepath"
 	"reflect"
 	"regexp"
+	"runtime"
 	"runtime/debug"
 	"runtime/trace"
 	"sort"
@@ -57,6 +58,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
 	"github.com/cockroachdb/cockroach/pkg/sql/stats"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
+	"github.com/cockroachdb/cockroach/pkg/testutils/floatcmp"
 	"github.com/cockroachdb/cockroach/pkg/testutils/physicalplanutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
@@ -2820,9 +2822,16 @@ func (t *logicTest) execQuery(query logicQuery) error {
 			// To find the coltype for the given result, mod the result number
 			// by the number of coltypes.
 			colT := query.colTypes[i%len(query.colTypes)]
-			if !resultMatches && colT == 'F' {
+			if !resultMatches {
 				var err error
-				resultMatches, err = floatsMatch(expected, actual)
+				// On s390x, check that values for both float ('F') and decimal
+				// ('R') coltypes are approximately equal to take into account
+				// platform differences in floating point calculations.
+				if runtime.GOARCH == "s390x" && (colT == 'F' || colT == 'R') {
+					resultMatches, err = floatsMatchApprox(expected, actual)
+				} else if colT == 'F' {
+					resultMatches, err = floatsMatch(expected, actual)
+				}
 				if err != nil {
 					return errors.CombineErrors(makeError(), err)
 				}
@@ -2880,17 +2889,37 @@ func (t *logicTest) execQuery(query logicQuery) error {
 	return nil
 }
 
-// floatsMatch returns whether two floating point numbers represented as
-// strings have matching 15 significant decimal digits (this is the precision
-// that Postgres supports for 'double precision' type).
-func floatsMatch(expectedString, actualString string) (bool, error) {
+// parseExpectedAndActualFloats converts the strings expectedString and
+// actualString to float64 values.
+func parseExpectedAndActualFloats(expectedString, actualString string) (float64, float64, error) {
 	expected, err := strconv.ParseFloat(expectedString, 64 /* bitSize */)
 	if err != nil {
-		return false, errors.Wrap(err, "when parsing expected")
+		return 0, 0, errors.Wrap(err, "when parsing expected")
 	}
 	actual, err := strconv.ParseFloat(actualString, 64 /* bitSize */)
 	if err != nil {
-		return false, errors.Wrap(err, "when parsing actual")
+		return 0, 0, errors.Wrap(err, "when parsing actual")
+	}
+	return expected, actual, nil
+}
+
+// floatsMatchApprox returns whether two floating point represented as
+// strings are equal within a tolerance.
+func floatsMatchApprox(expectedString, actualString string) (bool, error) {
+	expected, actual, err := parseExpectedAndActualFloats(expectedString, actualString)
+	if err != nil {
+		return false, err
+	}
+	return floatcmp.EqualApprox(expected, actual, floatcmp.CloseFraction, floatcmp.CloseMargin), nil
+}
+
+// floatsMatch returns whether two floating point numbers represented as
+// strings have matching 15 significant decimal digits (this is the precision
+// that Postgres supports for 'double precision' type).
+func floatsMatch(expectedString, actualString string) (bool, error) {
+	expected, actual, err := parseExpectedAndActualFloats(expectedString, actualString)
+	if err != nil {
+		return false, err
 	}
 	// Check special values - NaN, +Inf, -Inf, 0.
 	if math.IsNaN(expected) || math.IsNaN(actual) {

pkg/sql/logictest/logic_test.go

@@ -36,7 +36,8 @@ func TestSqlLiteLogic(t *testing.T) {
 	RunSQLLiteLogicTest(t, "" /* configOverride */)
 }
 
-// TestFloatsMatch is a unit test for floatsMatch() function.
+// TestFloatsMatch is a unit test for floatsMatch() and floatsMatchApprox()
+// functions.
 func TestFloatsMatch(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	for _, tc := range []struct {
@@ -60,7 +61,15 @@ func TestFloatsMatch(t *testing.T) {
 			t.Fatal(err)
 		}
 		if match != tc.match {
-			t.Fatalf("wrong result on %v", tc)
+			t.Fatalf("floatsMatch: wrong result on %v", tc)
+		}
+
+		match, err = floatsMatchApprox(tc.f1, tc.f2)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if match != tc.match {
+			t.Fatalf("floatsMatchApprox: wrong result on %v", tc)
 		}
 	}
 }

pkg/testutils/floatcmp/BUILD.bazel

@@ -0,0 +1,19 @@
+load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
+
+go_library(
+    name = "floatcmp",
+    srcs = ["floatcmp.go"],
+    importpath = "github.com/cockroachdb/cockroach/pkg/testutils/floatcmp",
+    visibility = ["//visibility:public"],
+    deps = [
+        "@com_github_google_go_cmp//cmp",
+        "@com_github_google_go_cmp//cmp/cmpopts",
+    ],
+)
+
+go_test(
+    name = "floatcmp_test",
+    size = "small",
+    srcs = ["floatcmp_test.go"],
+    embed = [":floatcmp"],
+)