sql: wait to start index GC job until schema changer is done

[thoszhang]

Previously, we were creating and starting the index GC job in a separate
transaction from the one used in PublishMultiple() in
SchemaChanger.done(), meaning that the index GC job could be run
before the original transaction to finalize all schema changes on the
table descriptor had been committed. This led to unpredictable behavior
while testing. It could also potentially cause multiple GC jobs to be
created if the PublishMultiple() closure were retried.

In this PR, the GC job is now created as a StartableJob that is
started only after the table descriptor in the finalized state has been
published.

Release justification: Bug fix for new feature.

Release note (bug fix): Fixed a bug introduced in beta.3 that could
cause multiple index GC jobs to be created for the same schema change in
rare cases.

[cockroach-teamcity]

This change is 

[ajwerner]

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

---

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

		)
	})
	if err != nil {

can you add:

if indexGCJob != nil {
    if rollbackErr := indexGCJob.CleanupOnRollback(ctx); rollbackErr != nil {
        log.Warningf(ctx, "failed to cleanup job: %v", err)
    }
}

---

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

		return nil, err
	}
	// TODO (lucy): Can we do the same thing with the PK drop index job?

same thing meaning... I get that this may make sense in the context of this PR but in general this TODO isn't adequately detailed.

[thoszhang]

I screwed up and treated the index GC job as a single job, when in fact there can be multiple indexes per transaction to be dropped. Now it's a slice of jobs.

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

---

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

Previously, ajwerner wrote…

can you add:

if indexGCJob != nil {
    if rollbackErr := indexGCJob.CleanupOnRollback(ctx); rollbackErr != nil {
        log.Warningf(ctx, "failed to cleanup job: %v", err)
    }
}

Done. Unfortunately we still won't run the cleanup if Publish() has to retry, but I guess this is a best-effort thing anyway.

---

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

Previously, ajwerner wrote…

same thing meaning... I get that this may make sense in the context of this PR but in general this TODO isn't adequately detailed.

Done. (I moved it to where we actually create that index drop job.)

[ajwerner]

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

---

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

Previously, lucy-zhang (Lucy Zhang) wrote…

Done. Unfortunately we still won't run the cleanup if Publish() has to retry, but I guess this is a best-effort thing anyway.

yeah... about that.. I've had that on my TODO list for a bit now.

The non-idempotent part of all of this is the fact that we create a new job id every time. If we created the ID above the call to create then it'd all be good. I'll go refactor this eventually. Until then it's potentially a very slow leak that does make me sad.

Or in this case I think that would look like having a slice of ids which we allocate the first pass through and then reuse on retries. That's a bit nasty but I don't have a better answer.

[pbardea]

I left a comment, but if you think it would be best to fix that in a separate PR I'm happy with that too.

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

---

pkg/sql/schema_changer.go, line 1415 at r2 (raw file):

			descriptorIDs = append(descriptorIDs, table.ID)
		}
	}

I think we may have a missing case here to properly handle the empty database case. Something like

} else if details.ParentID != sqlbase.InvalidID {
  descriptorIDs = []sqlbase.ID{details.ParentID}
}

so that jobs dropping empty databases aren't cleaned up accidentally.

Also happy to dig into the drop empty database case in a separate PR.

[thoszhang]

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

---

pkg/sql/schema_changer.go, line 1415 at r2 (raw file):

Previously, pbardea (Paul Bardea) wrote…

I think we may have a missing case here to properly handle the empty database case. Something like

} else if details.ParentID != sqlbase.InvalidID {
  descriptorIDs = []sqlbase.ID{details.ParentID}
}

so that jobs dropping empty databases aren't cleaned up accidentally.

Also happy to dig into the drop empty database case in a separate PR.

Dropping an empty database doesn't cause a GC job to be created (either in 19.2 or now; in 19.2, there's no job at all).

I think an earlier iteration of this PR might have accidentally changed this (or maybe that was on my local branch, I don't remember). But in any case I've now added a test to ensure there's no GC job for an empty database.

[blathers-crl]

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

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

[thoszhang]

Hmm. I managed to also repro the TestProtectedTimestampsDuringBackup CI flake on master, on my laptop, but the results were a bit weird: in one attempt it took >20k runs over 6 hours (I left it running overnight) and in another attempt my computer went to sleep in the middle, which might have affected the timeouts or something. I'm going to try again on roachprod-stress and open an issue once I see what happens in a more controlled environment. (This failure is different from #45932.)

[thoszhang]

Filed #47522 for the TestProtectedTimestampsDuringBackup flake (and #47532, which I encountered in testing), merging now.

bors r+

[craig]

Build failed

• GitHub CI (Cockroach)

[thoszhang]

The flake was #47546.

bors r+

[craig]

Build failed

• GitHub CI (Cockroach)

[thoszhang]

This TestScrubFKConstraintFKNulls flake is persistent. I'll revisit this later.

[thoszhang]

bors r+

[thoszhang]

???

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)