From fcf8cbc14ebd164d9f74dcada25e98a6fdd8c3a0 Mon Sep 17 00:00:00 2001 From: Steven Danna Date: Fri, 25 Nov 2022 08:13:13 +0000 Subject: [PATCH] backupccl: don't disable leases in test 399e56b3f56c0dce6c34525bebf58297be320083 introduced a bounded staleness read into the migration machinery. When `lease.TestingDisableTableLeases()` has been set, this bounded staleness read encounters an error: testcluster.go:384: migration-job-find-already-completed: cannot set fixed timestamp, txn "sql txn" meta={id=f4142488 key=/Min pri=0.01688073 epo=0 ts=1669334862.467371575,0 min=1669334862.467371575,0 seq=0} lock=false stat=PENDING rts=1669334862.467371575,0 wto=false gul=1669334862.967371575,0 already performed reads I believe that the read that was already performed in this case was the descriptor lookup. Then, when we go to execute the select, we attempt to SetFixedTimestamp in txn.NegotiateAndSend. When the testing isn't in use, on its face it looks like we don't hit this case because we don't allow a fallback to a store lookup: https://github.com/cockroachdb/cockroach/blob/b5be006bedd7d3cedc3fb3d2248df168e3d64be2/pkg/sql/catalog/descs/leased_descriptors.go#L143-L155 But, when TestingDsiableTableLeases is set, we skip right to the store lookup: https://github.com/cockroachdb/cockroach/blob/b5be006bedd7d3cedc3fb3d2248df168e3d64be2/pkg/sql/catalog/descs/descriptor.go#L489-L491 I haven't looked into why lease.TestingDisableTableLeases() was in place in the past, but it is no longer used in any other backup tests and isn't likely needed here. Fixes #92432 Fixes #92433 Fixes #92434 Fixes #92435 Release note: None --- pkg/ccl/backupccl/BUILD.bazel | 1 - pkg/ccl/backupccl/backup_cloud_test.go | 9 --------- 2 files changed, 10 deletions(-) diff --git a/pkg/ccl/backupccl/BUILD.bazel b/pkg/ccl/backupccl/BUILD.bazel index ead29afdf9a4..02ab85b2124f 100644 --- a/pkg/ccl/backupccl/BUILD.bazel +++ b/pkg/ccl/backupccl/BUILD.bazel @@ -242,7 +242,6 @@ go_test( "//pkg/sql/catalog/descpb", "//pkg/sql/catalog/descs", "//pkg/sql/catalog/desctestutils", - "//pkg/sql/catalog/lease", "//pkg/sql/catalog/systemschema", "//pkg/sql/catalog/tabledesc", "//pkg/sql/execinfra", diff --git a/pkg/ccl/backupccl/backup_cloud_test.go b/pkg/ccl/backupccl/backup_cloud_test.go index c48c38cdf48d..fea4b2b69869 100644 --- a/pkg/ccl/backupccl/backup_cloud_test.go +++ b/pkg/ccl/backupccl/backup_cloud_test.go @@ -19,7 +19,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/cloud" "github.com/cockroachdb/cockroach/pkg/cloud/amazon" "github.com/cockroachdb/cockroach/pkg/cloud/azure" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/lease" "github.com/cockroachdb/cockroach/pkg/testutils/skip" "github.com/cockroachdb/cockroach/pkg/testutils/sqlutils" "github.com/cockroachdb/cockroach/pkg/testutils/testcluster" @@ -51,8 +50,6 @@ func TestCloudBackupRestoreS3(t *testing.T) { defer log.Scope(t).Close(t) creds, bucket := requiredS3CredsAndBucket(t) - // TODO(dan): Actually invalidate the descriptor cache and delete this line. - defer lease.TestingDisableTableLeases()() const numAccounts = 1000 ctx := context.Background() @@ -70,8 +67,6 @@ func TestCloudBackupRestoreS3WithLegacyPut(t *testing.T) { defer log.Scope(t).Close(t) creds, bucket := requiredS3CredsAndBucket(t) - // TODO(dan): Actually invalidate the descriptor cache and delete this line. - defer lease.TestingDisableTableLeases()() const numAccounts = 1000 ctx := context.Background() @@ -131,8 +126,6 @@ func TestCloudBackupRestoreGoogleCloudStorage(t *testing.T) { skip.IgnoreLint(t, "GOOGLE_BUCKET env var must be set") } - // TODO(dan): Actually invalidate the descriptor cache and delete this line. - defer lease.TestingDisableTableLeases()() const numAccounts = 1000 ctx := context.Background() @@ -164,8 +157,6 @@ func TestCloudBackupRestoreAzure(t *testing.T) { skip.IgnoreLint(t, "AZURE_CONTAINER env var must be set") } - // TODO(dan): Actually invalidate the descriptor cache and delete this line. - defer lease.TestingDisableTableLeases()() const numAccounts = 1000 ctx := context.Background()