jobs: ensure that async index dropping is needed

[pbardea]

We drop indexes through an async schema changer which eventually gets removes an
index with ClearRange. When this happens the index drop job is not marked as
completed, but instead marked as waiting for GC. However, if we DROP the table
that the index references in the same transaction, then we mark the index drop
job as done and we don't want to override this.

This PR ensures and tests that we don't override already successful index dropping
jobs that would be created as explained above.

Addresses #39260.

Release note: None

[cockroach-teamcity]

This change is 

[thoszhang]

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

---

pkg/jobs/jobs.go, line 150 at r1 (raw file):

// CheckSuccessStatus verifies the status of the job and returns an error if the job's
// status isn't Succeeded.
func (j *Job) CheckSuccessStatus(ctx context.Context) error {

This method is kind of unintuitive. I see that it's supposed to be like CheckRunning(), but CheckRunning() is supposed to be used to basically assert that the job is running and return an error otherwise (as far as I can tell), whereas CheckSuccessStatus() can be expected to return either result. Would returning a boolean instead make more sense?

---

pkg/sql/schema_changer.go, line 1324 at r1 (raw file):

			// was on was dropped in the same txn, we mark the index drop job
			// as successful.
			jobSucceeded = true

Is it also possible for the job to be failed?

---

pkg/sql/logictest/testdata/logic_test/schema_change_in_txn, line 1693 at r1 (raw file):

DROP TABLE t, t2

I think we usually just leave a single empty line between subtests.

---

pkg/sql/logictest/testdata/logic_test/schema_change_in_txn, line 1725 at r1 (raw file):

COMMIT;

Same here.

[pbardea]

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

---

pkg/jobs/jobs.go, line 150 at r1 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

This method is kind of unintuitive. I see that it's supposed to be like CheckRunning(), but CheckRunning() is supposed to be used to basically assert that the job is running and return an error otherwise (as far as I can tell), whereas CheckSuccessStatus() can be expected to return either result. Would returning a boolean instead make more sense?

Done. I agree that it's a good idea to make this method return a bool (and updated it to do so). The underlying call to j.Update still returns an error which still feels a bit awkward to me, but it seems to be the standard interface for reading jobs metadata info (see https://github.com/cockroachdb/cockroach/blob/master/pkg/jobs/update.go#L78 for more info).

---

pkg/sql/schema_changer.go, line 1324 at r1 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

Is it also possible for the job to be failed?

I wasn't able to find a case where we currently would call done() on a job that is already failed. However, giving it more thought I don't think it makes sense to update the status of a job that is already in a terminal state, especially since this is intended to be a logging function. I updated the CheckSuccessfulStatus to CheckTerminalStatus which checks to see if the jobs is in any terminal state (i.e. in a state where the state should no longer change).

---

pkg/sql/logictest/testdata/logic_test/schema_change_in_txn, line 1693 at r1 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

I think we usually just leave a single empty line between subtests.

Done.

---

pkg/sql/logictest/testdata/logic_test/schema_change_in_txn, line 1725 at r1 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

Same here.

Done.

[pbardea]

RFAL

[thoszhang]

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

---

pkg/sql/logictest/testdata/logic_test/schema_change_in_txn, line 1725 at r1 (raw file):

Previously, pbardea (Paul Bardea) wrote…

Done.

I forgot one thing. Could you add a brief comment explaining what this is testing?

[pbardea]

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

---

pkg/sql/logictest/testdata/logic_test/schema_change_in_txn, line 1725 at r1 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

I forgot one thing. Could you add a brief comment explaining what this is testing?

Done.

[pbardea]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)