sql: re-add GC job on schema element deletion

[pbardea]

This commit creates GC jobs upon the deletion of an index, table or
database. Similarly to the previous implementation, it considers the
walltime at which the schema change completed to be the drop time of the
schema element.

Release note (sql change): Previously, after deleting an index, table,
or database the relevant schema change job would change its running
status to waiting for GC TTL. The schema change and the GC process are
now decoupled into 2 jobs.

Release justification: This is a follow up to the migration of turning
schema changes into actual jobs. This commit re-adds the ability to
properly GC indexes and tables.

[cockroach-teamcity]

This change is 

[pbardea]

Ready for a first look. Looks like the races happening on CI are the same ones addressed by #45919.

[ajwerner]

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

---

pkg/sql/drop_test.go, line 298 at r1 (raw file):

	defer func(i time.Duration) { gcjob.MaxSQLGCInterval = i }(gcjob.MaxSQLGCInterval)
	gcjob.MaxSQLGCInterval = 500 * time.Millisecond

nit: make yourself a helper that takes a timestamp and returns a closure so you can go:

defer setMaxGCInterval(500 * time.Millisecond)()

---

pkg/sql/drop_test.go, line 857 at r1 (raw file):

	// Maybe this test API should use an offset starting from the most recent job
	// instead.
	schemaChangeJobOffset := 4

nit: const

---

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

	}

	if err := db.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error {

note to self, @pbardea please don't do anything about this: This pattern is annoying. I'm going to make a library function for it. It also has a problem where if we retry we're going to pollute the job registry map with cancel functions.

Probably

var f func(ctx context.Context, txn *kv.Txn) error
jobRegistry.CreateStartableJob(ctx, jobRecord, resultCh, f)

That func doesn't matter here but it'll be handy with protected timestamps elsewhere. This provides a place to fix the leak and deal with other issues like failure to commit the transaction which is clumsily handled with CleanupOnRollback.

---

pkg/sql/gcjob/descriptor_utils.go, line 81 at r1 (raw file):

func deleteDatabaseZoneConfig(ctx context.Context, db *kv.DB, databaseID sqlbase.ID) error {
	return db.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error {
		if err := txn.SetSystemConfigTrigger(); err != nil {

huh, how did this work before? Is this just the database cache being sort of ridiculous and broken? Is there some new dependency on this?

[pbardea]

Thanks for the review!

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

---

pkg/sql/drop_test.go, line 298 at r1 (raw file):

Previously, ajwerner wrote…

nit: make yourself a helper that takes a timestamp and returns a closure so you can go:

defer setMaxGCInterval(500 * time.Millisecond)()

Nice, since all my tests used the same time I just made a SetSmallMaxGCIntervalForTests.

---

pkg/sql/drop_test.go, line 857 at r1 (raw file):

Previously, ajwerner wrote…

nit: const

Done.

---

pkg/sql/gcjob/descriptor_utils.go, line 81 at r1 (raw file):

Previously, ajwerner wrote…

huh, how did this work before? Is this just the database cache being sort of ridiculous and broken? Is there some new dependency on this?

Ya, I think this can be removed. I think I added it while experimenting with some approaches to get rid of a test flake, not thinking much about which keys this txn was touching.

[pbardea]

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

---

pkg/sql/gcjob/descriptor_utils.go, line 81 at r1 (raw file):

Previously, pbardea (Paul Bardea) wrote…

Ya, I think this can be removed. I think I added it while experimenting with some approaches to get rid of a test flake, not thinking much about which keys this txn was touching.

Nevermind, this is very much needed and it just never worked. In the old world this was done in the same txn that removed the table descriptors.

[thoszhang]

Reviewed 16 of 22 files at r1, 7 of 7 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner and @pbardea)

---

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

		if err := sc.startGCJob(ctx, gcDetails, false /* isRollback */); err != nil {
			return err
		}

Does it make sense to just return immediately from exec() after we create/write a GC job for a dropped table? (I've never really understood why we didn't return immediately in maybeDropTable() in the previous implementation, when we did end up dropping the table.)

I'm fine with leaving this as is, though, since we probably don't want to deal with any unexpected interactions at this point.

[pbardea]

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

---

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

Previously, lucy-zhang (Lucy Zhang) wrote…

Does it make sense to just return immediately from exec() after we create/write a GC job for a dropped table? (I've never really understood why we didn't return immediately in maybeDropTable() in the previous implementation, when we did end up dropping the table.)

I'm fine with leaving this as is, though, since we probably don't want to deal with any unexpected interactions at this point.

Ya, I also can't think of a reason it wouldn't be safe to return immediately here. As you mentioned, I'd prefer to do this in a follow-up PR.

[pbardea]

After fighting a bit with CI, I think that the timeout for the racetests needs to be increased a bit since there are quite a few tests run under stress from the pkg/sql package.

I successfully got CI to pass with that change, and extracted that commit into #46138. This branch will likely need to be rebased off of that one when it gets merged for CI to turn green here.

[pbardea]

TFTRs!
bors r+

[craig]

Build failed (retrying...)

• GitHub CI (Cockroach)

[craig]

Build succeeded

• GitHub CI (Cockroach)