From 9cd15a2014eff85e163878f4dc38f6d87048e7f7 Mon Sep 17 00:00:00 2001 From: adityamaru Date: Wed, 2 Aug 2023 12:47:20 -0400 Subject: [PATCH] upgrademanager: unskip TestMigrationFailure The test no longer fails with our change in Fixes: #106648 Fixes: #106762 Release note: None --- pkg/jobs/utils.go | 26 ++++++++++++++----- pkg/upgrade/upgrademanager/BUILD.bazel | 1 - .../upgrademanager/manager_external_test.go | 3 --- 3 files changed, 19 insertions(+), 11 deletions(-) diff --git a/pkg/jobs/utils.go b/pkg/jobs/utils.go index 17c39fee9724..367b52241b31 100644 --- a/pkg/jobs/utils.go +++ b/pkg/jobs/utils.go @@ -156,16 +156,24 @@ func JobExists( return row != nil, nil } -// IsJobTypeColumnDoesNotExistError returns true if the error is of the form +// isJobTypeColumnDoesNotExistError returns true if the error is of the form // `column "job_type" does not exist`. func isJobTypeColumnDoesNotExistError(err error) bool { return pgerror.GetPGCode(err) == pgcode.UndefinedColumn && strings.Contains(err.Error(), "column \"job_type\" does not exist") } +// isJobInfoTableDoesNotExistError returns true if the error is of the form +// `related "job_info" does not exist`. +func isJobInfoTableDoesNotExistError(err error) bool { + return pgerror.GetPGCode(err) == pgcode.UndefinedTable && + strings.Contains(err.Error(), "relation \"system.job_info\" does not exist") +} + // MaybeGenerateForcedRetryableError returns a // TransactionRetryWithProtoRefreshError that will cause the txn to be retried -// if the error is because of an undefined job_type column. +// if the error is because of an undefined job_type column or missing job_info +// table. // // In https://github.com/cockroachdb/cockroach/issues/106762 we noticed that if // a query is executed with an AS OF SYSTEM TIME clause that picks a transaction @@ -173,15 +181,19 @@ func isJobTypeColumnDoesNotExistError(err error) bool { // infrastructure will attempt to query the job_type column even though it // doesn't exist at the transaction's timestamp. // -// As a short term fix, when we encounter an `UndefinedColumn` error we -// generate a synthetic retryable error so that the txn is pushed to a -// higher timestamp at which the upgrade will have completed and the -// `job_type` column will be visible. The longer term fix is being tracked -// in https://github.com/cockroachdb/cockroach/issues/106764. +// As a short term fix, when we encounter an `UndefinedTable` or +// `UndefinedColumn` error we generate a synthetic retryable error so that the +// txn is pushed to a higher timestamp at which the upgrade will have completed +// and the table/column will be visible. The longer term fix is being tracked in +// https://github.com/cockroachdb/cockroach/issues/106764. func MaybeGenerateForcedRetryableError(ctx context.Context, txn *kv.Txn, err error) error { if err != nil && isJobTypeColumnDoesNotExistError(err) { return txn.GenerateForcedRetryableError(ctx, "synthetic error "+ "to push timestamp to after the `job_type` upgrade has run") } + if err != nil && isJobInfoTableDoesNotExistError(err) { + return txn.GenerateForcedRetryableError(ctx, "synthetic error "+ + "to push timestamp to after the `job_info` upgrade has run") + } return err } diff --git a/pkg/upgrade/upgrademanager/BUILD.bazel b/pkg/upgrade/upgrademanager/BUILD.bazel index d1f2f7612ff3..5e46cec877d3 100644 --- a/pkg/upgrade/upgrademanager/BUILD.bazel +++ b/pkg/upgrade/upgrademanager/BUILD.bazel @@ -68,7 +68,6 @@ go_test( "//pkg/sql/sem/eval", "//pkg/testutils", "//pkg/testutils/serverutils", - "//pkg/testutils/skip", "//pkg/testutils/sqlutils", "//pkg/testutils/testcluster", "//pkg/upgrade", diff --git a/pkg/upgrade/upgrademanager/manager_external_test.go b/pkg/upgrade/upgrademanager/manager_external_test.go index f965243f8fc3..5cd231e9492f 100644 --- a/pkg/upgrade/upgrademanager/manager_external_test.go +++ b/pkg/upgrade/upgrademanager/manager_external_test.go @@ -38,7 +38,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/sem/eval" "github.com/cockroachdb/cockroach/pkg/testutils" "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" - "github.com/cockroachdb/cockroach/pkg/testutils/skip" "github.com/cockroachdb/cockroach/pkg/testutils/sqlutils" "github.com/cockroachdb/cockroach/pkg/testutils/testcluster" "github.com/cockroachdb/cockroach/pkg/upgrade" @@ -683,8 +682,6 @@ func TestMigrationFailure(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) - skip.WithIssue(t, 106648) - ctx := context.Background() // Configure the range of versions used by the test