sql: fix handling of errors in schema change rollbacks

[thoszhang]

This PR does two main things to fix error handling for a schema change
being rolled back in the job's OnFailOrCancel hook: (1) errors from
runStateMachineAndBackfill (acting on the reversed schema change) are
now returned instead of being logged and swallowed; and (2) we now
return the error and cause the the job to fail permanently when we
encounter any error that isn't one of the specific whitelisted
"retriable" errors for the schema changer. Transient "retriable" errors
in the schema changer now correctly lead to retries (which are handled
by the job registry).

The reason for implementing (2) in addition to (1) was that I discovered
some pre-existing instances of erroneous behavior in 19.2 and earlier
versions where the "expected" behavior was that we would never be able
to clean up a failed schema change due to inadequacies in the schema
changer and backfiller. (One notable example is that because the
column backfiller can't recover previous values, if a DROP COLUMN schema
change is being rolled back, trying to backfill a default value on a
column with constraints won't work in general.) Given this, it seemed
best to just return a result to the client right away instead of
pointlessly retrying and blocking indefinitely. This seemed like the
best quick fix that isn't a regression from 19.2 and that somewhat
reproduces the existing behavior.

This change also required a few changes to testing: There's now a
sentinel for job retry errors used with the errors package, and a few
testing knobs in the schema change job migration tests that were relying
on the old erroneous behavior have now been corrected.

See #46541 (comment) for an example of a schema change that can't be successfully rolled back.
Closes #47324.

Release note (bug fix): Fixed a bug introduced with the new schema
change job implementation in beta.3 that caused errors when rolling back
a schema change to be swallowed.

[cockroach-teamcity]

This change is 

[blathers-crl]

❌ The GitHub CI (Cockroach) build has failed on e8f3287f.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[blathers-crl]

❌ The GitHub CI (Cockroach) build has failed on 2e45b5ed.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[thoszhang]

The schema change job migration tests were obliquely testing this behavior (and have now been updated), but I still need to write a few tests to focus on this specifically. I think it's ready for a first look, though. (Comments on the approach would be useful, since I wasn't planning to make change (2) described in the commit message until a few hours ago.)

[thoszhang]

Added tests.

[blathers-crl]

❌ The GitHub CI (Cockroach) build has failed on 2c26173a.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[ajwerner]

just the encouragement to adopt require

Reviewed 4 of 4 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @lucy-zhang and @pbardea)

---

pkg/sql/schema_changer_test.go, line 5920 at r1 (raw file):

Quoted 5 lines of code…

	if matched, err := regexp.MatchString("cannot be reverted, manual cleanup may be required", jobErr); err != nil {
		t.Fatal(err)
	} else if !matched {
		t.Fatalf("unexpected job error %s", jobErr)
	}

nit: I highly recommend require.Regexp:

this would all be:

require.Regexp(t, "cannot be reverted, manual cleanup may be required", jobErr)

Also applies to a bunch of other stuff in this test.

[thoszhang]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner)

---

pkg/sql/schema_changer_test.go, line 5920 at r1 (raw file):

Previously, ajwerner wrote…

	if matched, err := regexp.MatchString("cannot be reverted, manual cleanup may be required", jobErr); err != nil {
		t.Fatal(err)
	} else if !matched {
		t.Fatalf("unexpected job error %s", jobErr)
	}

nit: I highly recommend require.Regexp:

this would all be:

require.Regexp(t, "cannot be reverted, manual cleanup may be required", jobErr)

Also applies to a bunch of other stuff in this test.

Done.

[thoszhang]

TFTRs

bors r+

[ajwerner]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner)

---

pkg/sql/schema_changer_test.go, line 5857 at r2 (raw file):

CREATE UNIQUE INDEX i ON t.test(v);
`)
	require.Error(t, err)

totally not worth going back and changing but in the future you don't even need to NoError this, Regexp will check if the argument is nil, fail, and then if it's non-nil, detect if it's an error will call Error() on it and compare it to the regexp

---

pkg/sql/schema_changer_test.go, line 5908 at r2 (raw file):

CREATE UNIQUE INDEX i ON t.test(v);
`)
	require.Error(t, err)

same here

[craig]

Build succeeded

• GitHub CI (Cockroach)