Skip to content

Commit

Permalink
upgrademanager: unskip TestMigrationFailure
Browse files Browse the repository at this point in the history
The test no longer fails with our change in

Fixes: #106648
Fixes: #106762
Release note: None
  • Loading branch information
adityamaru committed Aug 4, 2023
1 parent 7a597b6 commit 9cd15a2
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 11 deletions.
26 changes: 19 additions & 7 deletions pkg/jobs/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -156,32 +156,44 @@ 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
// timestamp before the job_type migration, then parts of the jobs
// 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
}
1 change: 0 additions & 1 deletion pkg/upgrade/upgrademanager/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
3 changes: 0 additions & 3 deletions pkg/upgrade/upgrademanager/manager_external_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 9cd15a2

Please sign in to comment.