sql: fix bug where bad mutation job state could block dropping tables

[thoszhang]

Previously, while dropping a table, we would mark all the jobs
associated with mutations on the table as succeeded, under the
assumption that they were running. The job registry API prohibits this
when the jobs are not running (or pending), so if a mutation was
stuck on the table descriptor with a failed or nonexistent job, dropping
the table would fail.

This PR fixes the bug by checking the job state before attempting to
update the job. It also fixes a related failure to drop a table caused
by a valid mutation job not being in a running state.

Fixes #57597.

Release note (bug fix): Fixed a bug where prior schema changes on a
table that failed and could not be fully reverted could prevent the
table from being dropped.

[cockroach-teamcity]

This change is 

[thoszhang]

Ultimately I went for what seemed to be the simplest fix, which is to keep marking these jobs as succeeded whenever possible. I also realized that this was also broken if we have reverting mutation jobs. That could use a separate test.

[ajwerner]

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

---

pkg/sql/drop_table.go, line 470 at r1 (raw file):

		// a failed state. Such jobs could have already been GCed from the jobs
		// table by the time this code runs.
		row, err := p.ExtendedEvalContext().InternalExecutor.(*InternalExecutor).QueryRowEx(

Is this really better than loading the job and reading its status using the jobs APIs?

[thoszhang]

Also added a test for when we're dropping a table with mutation jobs in the reverting state.

Reviewed 2 of 2 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner)

---

pkg/sql/drop_table.go, line 470 at r1 (raw file):

Previously, ajwerner wrote…

Is this really better than loading the job and reading its status using the jobs APIs?

That's a fair point. I updated it to use UpdateJobWithTxn. I also decided I didn't want to overwrite the original error message in the case of jobs that need to be marked as failed.

[ajwerner]

Does this need to go back to 20.1 as well?

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @lucy-zhang)

---

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

		// Try to create a unique index which won't be valid and will need a rollback.
		_, err = sqlDB.Exec(`CREATE UNIQUE INDEX i ON t.test(v);`)
		require.Regexp(t, "violates unique constraint", err)

Use assert in a goroutine.

---

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

	ctx := context.Background()

	runTest := func(params base.TestServerArgs, gcJobRecord bool) {

pass in the testing.T

[thoszhang]

I added a query before calling JobRegistry.UpdateJobWithTxn to check that the job exists at all (which I don't think is possible through the registry API) and to not return an error in that case. Should be fixed now.

Does this need to go back to 20.1 as well?

Probably. I'll figure that out.

Reviewed 2 of 2 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner)

---

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

Previously, ajwerner wrote…

Use assert in a goroutine.

Done.

---

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

Previously, ajwerner wrote…

pass in the testing.T

Done.

[ajwerner]

Reviewable status: complete! 1 of 0 LGTMs obtained

[thoszhang]

TFTR!

bors r+

[thoszhang]

I'm realizing now that this is not going to backport cleanly to 20.2 because UpdateJobWithTxn is only on master, and doesn't quite do what I want anyway. So I'm going to rework this slightly and just load the job itself.

bors r-

[craig]

Canceled.

[thoszhang]

RFAL

[ajwerner]

Reviewed 2 of 2 files at r4.
Reviewable status: complete! 1 of 0 LGTMs obtained

[thoszhang]

TFTR

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)