Merge cockroachdb#122052

122052: import: mark import.constraint_validation cluster setting unsafe r=dt a=ajstorm

Mark the bulkio.import.constraint_validation.enabled cluster setting unsafe, since it could result in inconsistent data, and as such, should never be used on production clusters.

Release note: None
Epic: none
Release justification: Small change to guard against user misuse.

Co-authored-by: Adam Storm <[email protected]>

pkg/ccl/importerccl/BUILD.bazel

@@ -40,6 +40,7 @@ go_test(
         "//pkg/util/leaktest",
         "//pkg/util/log",
         "//pkg/util/randutil",
+        "@com_github_lib_pq//:pq",
         "@com_github_stretchr_testify//require",
     ],
 )

pkg/ccl/importerccl/ccl_test.go

@@ -13,9 +13,11 @@ package importerccl
 import (
 	"context"
 	gosql "database/sql"
+	"errors"
 	"fmt"
 	"net/http"
 	"net/http/httptest"
+	"strings"
 	"sync/atomic"
 	"testing"
 
@@ -38,6 +40,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/testutils/testcluster"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
+	"github.com/lib/pq"
 	"github.com/stretchr/testify/require"
 )
 
@@ -153,7 +156,7 @@ DROP VIEW IF EXISTS v`,
 			table     string
 			sql       string
 			create    string
-			setting   string
+			unsafe    string
 			args      []interface{}
 			errString string
 			data      string
@@ -201,10 +204,10 @@ DROP VIEW IF EXISTS v`,
 				table: "mr_regional_by_row",
 				create: "CREATE TABLE mr_regional_by_row (i INT8 PRIMARY KEY) LOCALITY REGIONAL BY ROW;" +
 					"INSERT INTO mr_regional_by_row (i, crdb_region) VALUES (1, 'us-east2')",
-				setting: "SET CLUSTER SETTING bulkio.import.constraint_validation.enabled=false",
-				sql:     "IMPORT INTO mr_regional_by_row (i, crdb_region) CSV DATA ($1)",
-				args:    []interface{}{srv.URL},
-				data:    "1,us-east1\n",
+				unsafe: "SET CLUSTER SETTING bulkio.import.constraint_validation.unsafe.enabled=false",
+				sql:    "IMPORT INTO mr_regional_by_row (i, crdb_region) CSV DATA ($1)",
+				args:   []interface{}{srv.URL},
+				data:   "1,us-east1\n",
 			},
 			{
 				name:   "import-into-multi-region-regional-by-row-to-multi-region-database-concurrent-table-add",
@@ -278,8 +281,27 @@ CREATE TABLE mr_regional_by_row (i INT8 PRIMARY KEY, s typ, b bytea) LOCALITY RE
 				tdb.Exec(t, fmt.Sprintf(`SET DATABASE = %q`, test.db))
 				tdb.Exec(t, fmt.Sprintf("DROP TABLE IF EXISTS %q CASCADE", test.table))
 
-				if test.setting != "" {
-					tdb.Exec(t, test.setting)
+				if test.unsafe != "" {
+					// We need to first try and set the cluster setting, and
+					// then parse the error to get the unsafe override key.
+					_, err := sqlDB.Exec(test.unsafe)
+					require.Error(t, err)
+
+					getKey := func(err error) string {
+						require.Contains(t, err.Error(), "may cause cluster instability")
+						var pqErr *pq.Error
+						ok := errors.As(err, &pqErr)
+						require.True(t, ok)
+						require.True(t, strings.HasPrefix(pqErr.Detail, "key:"), pqErr.Detail)
+						return strings.TrimPrefix(pqErr.Detail, "key: ")
+					}
+					key := getKey(err)
+
+					// Now set the key and try again. We're not expecting an error any more.
+					_, err = sqlDB.Exec("SET unsafe_setting_interlock_key = $1", key)
+					require.NoError(t, err)
+					_, err = sqlDB.Exec(test.unsafe)
+					require.NoError(t, err)
 				}
 
 				if test.data != "" {

pkg/sql/importer/import_job.go

@@ -107,11 +107,12 @@ var processorsPerNode = settings.RegisterIntSetting(
 
 var performConstraintValidation = settings.RegisterBoolSetting(
 	settings.ApplicationLevel,
-	"bulkio.import.constraint_validation.enabled",
+	"bulkio.import.constraint_validation.unsafe.enabled",
 	"should import perform constraint validation after data load. "+
 		"NOTE: this setting should not be used on production clusters, as it could result in "+
 		"incorrect query results if the imported data set violates constraints (i.e. contains duplicates).",
 	true,
+	settings.WithUnsafe,
 )
 
 type preparedSchemaMetadata struct {