Skip to content

Commit

Permalink
backupccl: don't disable leases in test
Browse files Browse the repository at this point in the history
399e56b 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
  • Loading branch information
stevendanna committed Nov 25, 2022
1 parent b5be006 commit fcf8cbc
Show file tree
Hide file tree
Showing 2 changed files with 0 additions and 10 deletions.
1 change: 0 additions & 1 deletion pkg/ccl/backupccl/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
9 changes: 0 additions & 9 deletions pkg/ccl/backupccl/backup_cloud_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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()
Expand All @@ -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()
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down

0 comments on commit fcf8cbc

Please sign in to comment.