Merge cockroachdb#107475 cockroachdb#108019 cockroachdb#108067 cockroachdb#108075 cockroachdb#108094 cockroachdb#108097

107475: changefeedccl: update nemeses test to use the declarative schema changer r=HonoreDB a=jayshrivastava

### sql: only use public indexes when fingerprinting

Previously, running `SHOW EXPERIMENTAL_FINGERPRINTS` on a table
in the middle of a schema change would sometimes cause an error
due to missing indexes. The root cause of this error is that
fingerprinting attempted to read non-public indexes which exist
during the schema change:
```
1 foo_pkey
2 crdb_internal_index_2_name_placeholder
3 crdb_internal_index_3_name_placeholder
```
The fix to this change is a one line change which ensures fingerprinting
reads active indexes only. This change also adds a regression test for this
scenario. There was a test util method in `changefeedccl` which was useful
for this test. This change moves that util method to `schematestutils` so it
can be called outside of `changefeedccl`.

Epic: none

Release note: None

---

### changefeedccl: update nemeses test to use the declarative schema changer

This change updates the nemeses test, which asserts the behavior of changefeeds
under a randomized DML/DDL workload. Previously, the test would only run
with the legacy schema changer. This is not ideal because, by default, the
declarative schema changer is used in production. This change makes it so that
the declarative schema changer is used 90% of the time instead.

Informs: cockroachdb#106906
Epic: none

Release note: None

108019: rpc: fix various test bugs r=aliher1911,pavelkalinnikov a=knz

Fixes cockroachdb#108008.

TLDR: Prior to this patch, multiple unit tests in the `rpc` package were misconfiguring their heartbeat interval and timeout. This patch fixes this.

The issue was that there were two separate locations for these tunable parameters:

- `RPCHeartbeat{Interval,Timeout}` fields in `ContextOptions` (previously via `*base.Config`, recently as direct fields in `ContextOptions`)
- also, two private fields `heartbeat{Interval,Timeout}`

The former was copied into the latter during `NewContext()`. Only the latter (the private fields) matter for the actual behavior.

Prior to this commit, the tests would call `NewContext()` and then afterwards would override the fields in `ContextOptions`. These overrides were thus ineffective because the value was already copied by that time.

In addition to this design bug, there were a couple of additional bugs lurking: the overrides defined by the tests were problematic in some cases. If these overrides had been effective, the tests would break.

The problem was discovered after
8d2d2bf merged, because in that commit some of the overrides from broken tests were taken on board and became effective. This started causing flakes.

The fix is to remove the ambiguity in configuration and remove the problematic overrides from offending tests.

Release note: None
Epic: CRDB-28893

108067: sql: unskip TestCancelSchemaChange r=chengxiong-ruan a=chengxiong-ruan

Informs cockroachdb#51796

This commit unskip TestCancelSchemaChange by fixing several wrong assumption of schema changer jobs. Also removed some variables which made no effects on the test.

Release note: None
Release justification: test only change.

108075: storage: fix overwrite RemoteStorage factory bug r=joshimhoff a=joshimhoff

Tomorrow AM, I plan to add a unit test that runs an integration test server with in-memory shared storage. If that ends up being difficult, I may back out and test manually with a local cockroach binary. But we should add such a test eventually either way.

**storage: fix overwrite RemoteStorage factory bug**

As of this commit, if both cfg.SharedStorage and cfg.RemoteStorageFactory are set, CRDB uses cfg.SharedStorage. Note that eventually we will enable using both at the same time, but with the abstractions available today, that is not easy.

We prefer cfg.SharedStorage, since the Locator -> Storage mapping contained in it is needed for CRDB to function properly.

Release note: None.
Epic: None.

108094: timeutil/ptp: fix conversion from seconds to Time r=knz,erikgrinaker a=pavelkalinnikov

Epic: none
Release note (bug fix): since 22.2.0, using a PTP clock device (enabled by the --clock-device flag) would generate timestamps in the far future. It now generates the correct time. This could cause nodes to crash due to incorrect timestamps, or in the worst case irreversibly advance the cluster's HLC clock into the far future.

108097: ccl/sqlproxyccl: avoid holding onto the lock in ListenForDenied r=JeffSwenson a=jaylim-crl

#### ccl/sqlproxyccl: avoid holding onto the lock in ListenForDenied 

Previously, ListenForDenied would grab the watcher's lock (which is tied to
the scope of the proxy) before calling checkConnection. As part of the updated
ACL work, checkConnection now will grab a lock when it calls Initialize on a
given tenant. Given that checkConnection can be blocking, grabbing onto the
global watcher lock isn't ideal as it could hold up new connections, leading
to timeout issues.

This commit updates ListenForDenied such that the watcher's lock gets released
before invoking checkConnection.

#### ccl/sqlproxyccl: avoid tenant lookups if we know the type of connection 

Previously, we were performing a tenant lookup call before checking on the
type of connection. This can be unnecessary (e.g. doing a lookup call for the
private endpoints ACL, even if we knew that the connection was a public one).
This commit addresses that.

Release note: None

Epic: none

Co-authored-by: Jayant Shrivastava <[email protected]>
Co-authored-by: Raphael 'kena' Poss <[email protected]>
Co-authored-by: Chengxiong Ruan <[email protected]>
Co-authored-by: Josh Imhoff <[email protected]>
Co-authored-by: Pavel Kalinnikov <[email protected]>
Co-authored-by: Jay <[email protected]>

Author: 7 people

pkg/BUILD.bazel

@@ -694,6 +694,7 @@ ALL_TESTS = [
     "//pkg/util/timeofday:timeofday_test",
     "//pkg/util/timetz:timetz_test",
     "//pkg/util/timeutil/pgdate:pgdate_test",
+    "//pkg/util/timeutil/ptp:ptp_test",
     "//pkg/util/timeutil:timeutil_test",
     "//pkg/util/tochar:tochar_test",
     "//pkg/util/tracing/collector:collector_test",
@@ -2416,6 +2417,7 @@ GO_TARGETS = [
     "//pkg/util/timeutil/pgdate:pgdate",
     "//pkg/util/timeutil/pgdate:pgdate_test",
     "//pkg/util/timeutil/ptp:ptp",
+    "//pkg/util/timeutil/ptp:ptp_test",
     "//pkg/util/timeutil:timeutil",
     "//pkg/util/timeutil:timeutil_test",
     "//pkg/util/tochar:tochar",

pkg/ccl/changefeedccl/BUILD.bazel

@@ -218,6 +218,7 @@ go_test(
         "//pkg/ccl/changefeedccl/changefeedbase",
         "//pkg/ccl/changefeedccl/changefeedpb",
         "//pkg/ccl/changefeedccl/kvevent",
+        "//pkg/ccl/changefeedccl/schemafeed/schematestutils",
         "//pkg/ccl/kvccl/kvtenantccl",
         "//pkg/ccl/multiregionccl",
         "//pkg/ccl/multiregionccl/multiregionccltestutils",
@@ -254,7 +255,6 @@ go_test(
         "//pkg/sql/catalog/bootstrap",
         "//pkg/sql/catalog/catpb",
         "//pkg/sql/catalog/colinfo",
-        "//pkg/sql/catalog/descbuilder",
         "//pkg/sql/catalog/descpb",
         "//pkg/sql/catalog/desctestutils",
         "//pkg/sql/catalog/schemaexpr",
@@ -279,7 +279,6 @@ go_test(
         "//pkg/sql/sessiondata",
         "//pkg/sql/sessiondatapb",
         "//pkg/sql/types",
-        "//pkg/storage",
         "//pkg/testutils",
         "//pkg/testutils/jobutils",
         "//pkg/testutils/serverutils",

pkg/ccl/changefeedccl/cdctest/BUILD.bazel

@@ -31,7 +31,6 @@ go_library(
         "//pkg/util/fsm",
         "//pkg/util/hlc",
         "//pkg/util/log",
-        "//pkg/util/randutil",
         "//pkg/util/syncutil",
         "//pkg/util/timeutil",
         "@com_github_cockroachdb_errors//:errors",

pkg/ccl/changefeedccl/cdctest/nemeses.go

@@ -18,7 +18,6 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/util/fsm"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
-	"github.com/cockroachdb/cockroach/pkg/util/randutil"
 	"github.com/cockroachdb/errors"
 )
 
@@ -30,7 +29,9 @@ import (
 // duplicates, which the two cdctest.Validator implementations verify for the
 // real output of a changefeed. The output rows and resolved timestamps of the
 // tested feed are fed into them to check for anomalies.
-func RunNemesis(f TestFeedFactory, db *gosql.DB, isSinkless bool) (Validator, error) {
+func RunNemesis(
+	f TestFeedFactory, db *gosql.DB, isSinkless bool, rng *rand.Rand,
+) (Validator, error) {
 	// possible additional nemeses:
 	// - schema changes
 	// - merges
@@ -42,7 +43,6 @@ func RunNemesis(f TestFeedFactory, db *gosql.DB, isSinkless bool) (Validator, er
 	// - sink chaos
 
 	ctx := context.Background()
-	rng, _ := randutil.NewPseudoRand()
 
 	eventPauseCount := 10
 	if isSinkless {

pkg/ccl/changefeedccl/changefeed_test.go

@@ -37,6 +37,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/ccl/changefeedccl/cdctest"
 	"github.com/cockroachdb/cockroach/pkg/ccl/changefeedccl/changefeedbase"
 	"github.com/cockroachdb/cockroach/pkg/ccl/changefeedccl/kvevent"
+	"github.com/cockroachdb/cockroach/pkg/ccl/changefeedccl/schemafeed/schematestutils"
 	_ "github.com/cockroachdb/cockroach/pkg/ccl/kvccl/kvtenantccl" // multi-tenant tests
 	_ "github.com/cockroachdb/cockroach/pkg/ccl/multiregionccl"    // locality-related table mutations
 	_ "github.com/cockroachdb/cockroach/pkg/ccl/partitionccl"
@@ -55,8 +56,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/server/telemetry"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/sql"
-	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
-	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descbuilder"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/desctestutils"
 	"github.com/cockroachdb/cockroach/pkg/sql/execinfra"
 	"github.com/cockroachdb/cockroach/pkg/sql/execinfrapb"
@@ -68,7 +67,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/randgen"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/eval"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
-	"github.com/cockroachdb/cockroach/pkg/storage"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/jobutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
@@ -77,7 +75,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/testutils/testcluster"
 	"github.com/cockroachdb/cockroach/pkg/util"
 	"github.com/cockroachdb/cockroach/pkg/util/ctxgroup"
-	"github.com/cockroachdb/cockroach/pkg/util/encoding"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
@@ -2151,7 +2148,7 @@ func TestChangefeedSchemaChangeAllowBackfill(t *testing.T) {
 				`add_column_def: [2]->{"after": {"a": 2}}`,
 			})
 			sqlDB.Exec(t, `ALTER TABLE add_column_def ADD COLUMN b STRING DEFAULT 'd'`)
-			ts := fetchDescVersionModificationTime(t, s, `add_column_def`, 7)
+			ts := schematestutils.FetchDescVersionModificationTime(t, s.TestServer.Server, `d`, `public`, `add_column_def`, 7)
 			assertPayloads(t, addColumnDef, []string{
 				fmt.Sprintf(`add_column_def: [1]->{"after": {"a": 1, "b": "d"}, "updated": "%s"}`,
 					ts.AsOfSystemTime()),
@@ -2171,7 +2168,7 @@ func TestChangefeedSchemaChangeAllowBackfill(t *testing.T) {
 				`add_col_comp: [2]->{"after": {"a": 2, "b": 7}}`,
 			})
 			sqlDB.Exec(t, `ALTER TABLE add_col_comp ADD COLUMN c INT AS (a + 10) STORED`)
-			ts := fetchDescVersionModificationTime(t, s, `add_col_comp`, 7)
+			ts := schematestutils.FetchDescVersionModificationTime(t, s.TestServer.Server, `d`, `public`, `add_col_comp`, 7)
 			assertPayloads(t, addColComp, []string{
 				fmt.Sprintf(`add_col_comp: [1]->{"after": {"a": 1, "b": 6, "c": 11}, "updated": "%s"}`,
 					ts.AsOfSystemTime()),
@@ -2192,7 +2189,7 @@ func TestChangefeedSchemaChangeAllowBackfill(t *testing.T) {
 			})
 			sqlDB.Exec(t, `ALTER TABLE drop_column DROP COLUMN b`)
 			sqlDB.Exec(t, `INSERT INTO drop_column VALUES (3)`)
-			ts := fetchDescVersionModificationTime(t, s, `drop_column`, 2)
+			ts := schematestutils.FetchDescVersionModificationTime(t, s.TestServer.Server, `d`, `public`, `drop_column`, 2)
 
 			// Backfill for DROP COLUMN b.
 			assertPayloads(t, dropColumn, []string{
@@ -2243,9 +2240,9 @@ func TestChangefeedSchemaChangeAllowBackfill(t *testing.T) {
 			// version 2. Then, when adding column c, it goes from 9->17, with the schema change being visible at
 			// the 7th step (version 15). Finally, when adding column d, it goes from 17->25 ith the schema change
 			// being visible at the 7th step (version 23).
-			dropTS := fetchDescVersionModificationTime(t, s, `multiple_alters`, 2)
-			addTS := fetchDescVersionModificationTime(t, s, `multiple_alters`, 15)
-			addTS2 := fetchDescVersionModificationTime(t, s, `multiple_alters`, 23)
+			dropTS := schematestutils.FetchDescVersionModificationTime(t, s.TestServer.Server, `d`, `public`, `multiple_alters`, 2)
+			addTS := schematestutils.FetchDescVersionModificationTime(t, s.TestServer.Server, `d`, `public`, `multiple_alters`, 15)
+			addTS2 := schematestutils.FetchDescVersionModificationTime(t, s.TestServer.Server, `d`, `public`, `multiple_alters`, 23)
 
 			assertPayloads(t, multipleAlters, []string{
 				fmt.Sprintf(`multiple_alters: [1]->{"after": {"a": 1}, "updated": "%s"}`, dropTS.AsOfSystemTime()),
@@ -2296,7 +2293,7 @@ func TestChangefeedSchemaChangeBackfillScope(t *testing.T) {
 			sqlDB.Exec(t, `ALTER TABLE add_column_def ADD COLUMN b STRING DEFAULT 'd'`)
 
 			// The primary index swap occurs at version 7.
-			ts := fetchDescVersionModificationTime(t, s, `add_column_def`, 7)
+			ts := schematestutils.FetchDescVersionModificationTime(t, s.TestServer.Server, `d`, `public`, `add_column_def`, 7)
 			assertPayloads(t, combinedFeed, []string{
 				fmt.Sprintf(`add_column_def: [1]->{"after": {"a": 1, "b": "d"}, "updated": "%s"}`,
 					ts.AsOfSystemTime()),
@@ -2319,82 +2316,6 @@ func TestChangefeedSchemaChangeBackfillScope(t *testing.T) {
 	}
 }
 
-// fetchDescVersionModificationTime fetches the `ModificationTime` of the specified
-// `version` of `tableName`'s table descriptor.
-func fetchDescVersionModificationTime(
-	t testing.TB, s TestServerWithSystem, tableName string, version int,
-) hlc.Timestamp {
-	tblKey := s.Codec.TablePrefix(keys.DescriptorTableID)
-	header := kvpb.RequestHeader{
-		Key:    tblKey,
-		EndKey: tblKey.PrefixEnd(),
-	}
-	dropColTblID := sqlutils.QueryTableID(t, s.DB, `d`, "public", tableName)
-	req := &kvpb.ExportRequest{
-		RequestHeader: header,
-		MVCCFilter:    kvpb.MVCCFilter_All,
-		StartTime:     hlc.Timestamp{},
-	}
-	hh := kvpb.Header{Timestamp: hlc.NewClockForTesting(nil).Now()}
-	res, pErr := kv.SendWrappedWith(context.Background(),
-		s.SystemServer.DB().NonTransactionalSender(), hh, req)
-	if pErr != nil {
-		t.Fatal(pErr.GoError())
-	}
-	for _, file := range res.(*kvpb.ExportResponse).Files {
-		it, err := storage.NewMemSSTIterator(file.SST, false /* verify */, storage.IterOptions{
-			KeyTypes:   storage.IterKeyTypePointsAndRanges,
-			LowerBound: keys.MinKey,
-			UpperBound: keys.MaxKey,
-		})
-		if err != nil {
-			t.Fatal(err)
-		}
-		defer it.Close()
-		for it.SeekGE(storage.NilKey); ; it.Next() {
-			if ok, err := it.Valid(); err != nil {
-				t.Fatal(err)
-			} else if !ok {
-				continue
-			}
-			k := it.UnsafeKey()
-			if _, hasRange := it.HasPointAndRange(); hasRange {
-				t.Fatalf("unexpected MVCC range key at %s", k)
-			}
-			remaining, _, _, err := s.Codec.DecodeIndexPrefix(k.Key)
-			if err != nil {
-				t.Fatal(err)
-			}
-			_, tableID, err := encoding.DecodeUvarintAscending(remaining)
-			if err != nil {
-				t.Fatal(err)
-			}
-			if tableID != uint64(dropColTblID) {
-				continue
-			}
-			unsafeValue, err := it.UnsafeValue()
-			require.NoError(t, err)
-			if unsafeValue == nil {
-				t.Fatal(errors.New(`value was dropped or truncated`))
-			}
-			value := roachpb.Value{RawBytes: unsafeValue, Timestamp: k.Timestamp}
-			b, err := descbuilder.FromSerializedValue(&value)
-			if err != nil {
-				t.Fatal(err)
-			}
-			require.NotNil(t, b)
-			if b.DescriptorType() == catalog.Table {
-				tbl := b.BuildImmutable().(catalog.TableDescriptor)
-				if int(tbl.GetVersion()) == version {
-					return tbl.GetModificationTime()
-				}
-			}
-		}
-	}
-	t.Fatal(errors.New(`couldn't find table desc for given version`))
-	return hlc.Timestamp{}
-}
-
 // Regression test for #34314
 func TestChangefeedAfterSchemaChangeBackfill(t *testing.T) {
 	defer leaktest.AfterTest(t)()

pkg/ccl/changefeedccl/helpers_test.go

@@ -73,13 +73,6 @@ func maybeDisableDeclarativeSchemaChangesForTest(
 	return disable
 }
 
-// disableDeclarativeSchemaChangesForTest tests that are disabled due to differences
-// in changefeed behaviour and are tracked by issue #80545.
-func disableDeclarativeSchemaChangesForTest(t testing.TB, sqlDB *sqlutils.SQLRunner) {
-	sqlDB.Exec(t, "SET use_declarative_schema_changer='off'")
-	sqlDB.Exec(t, "SET CLUSTER SETTING  sql.defaults.use_declarative_schema_changer='off'")
-}
-
 func waitForSchemaChange(
 	t testing.TB, sqlDB *sqlutils.SQLRunner, stmt string, arguments ...interface{},
 ) {

pkg/ccl/changefeedccl/nemeses_test.go

@@ -19,6 +19,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
+	"github.com/cockroachdb/cockroach/pkg/util/randutil"
 )
 
 func TestChangefeedNemeses(t *testing.T) {
@@ -27,12 +28,15 @@ func TestChangefeedNemeses(t *testing.T) {
 	skip.UnderRace(t, "takes >1 min under race")
 
 	testFn := func(t *testing.T, s TestServer, f cdctest.TestFeedFactory) {
+		rng, seed := randutil.NewPseudoRand()
+		t.Logf("random seed: %d", seed)
+
 		sqlDB := sqlutils.MakeSQLRunner(s.DB)
-		disableDeclarativeSchemaChangesForTest(t, sqlDB)
+		_ = maybeDisableDeclarativeSchemaChangesForTest(t, sqlDB, rng)
 		// TODO(dan): Ugly hack to disable `eventPause` in sinkless feeds. See comment in
 		// `RunNemesis` for details.
 		isSinkless := strings.Contains(t.Name(), "sinkless")
-		v, err := cdctest.RunNemesis(f, s.DB, isSinkless)
+		v, err := cdctest.RunNemesis(f, s.DB, isSinkless, rng)
 		if err != nil {
 			t.Fatalf("%+v", err)
 		}

pkg/ccl/changefeedccl/schemafeed/schematestutils/BUILD.bazel

@@ -6,12 +6,23 @@ go_library(
     importpath = "github.com/cockroachdb/cockroach/pkg/ccl/changefeedccl/schemafeed/schematestutils",
     visibility = ["//visibility:public"],
     deps = [
+        "//pkg/keys",
+        "//pkg/kv",
+        "//pkg/kv/kvpb",
+        "//pkg/roachpb",
         "//pkg/sql/catalog",
         "//pkg/sql/catalog/catpb",
+        "//pkg/sql/catalog/descbuilder",
         "//pkg/sql/catalog/descpb",
         "//pkg/sql/catalog/tabledesc",
         "//pkg/sql/types",
+        "//pkg/storage",
+        "//pkg/testutils/serverutils",
+        "//pkg/testutils/sqlutils",
+        "//pkg/util/encoding",
         "//pkg/util/hlc",
+        "@com_github_cockroachdb_errors//:errors",
         "@com_github_gogo_protobuf//proto",
+        "@com_github_stretchr_testify//require",
     ],
 )