sql: start index drop job for primary key changes immediately #47624

thoszhang · 2020-04-17T20:06:13Z

Previously, the job to drop old indexes after a primary key change was
created at the end of the schema change and left for the job registry to
adopt at some later time. This PR switches the job creation over to
CreateStartableJobWithTxn, which allows for starting the job
immediately after the transaction which creates the job is committed.

This eliminates one source of waiting time before other schema changes,
which are queued behind the mutation to drop the indexes, can run. Note,
however, that successive schema changes don't start running immediately
after the mutations ahead of them in the queue are all cleared; if
they've ever hit a retry error due to not being first in line, then they
will have to wait to be re-adopted by the job registry. This is why the
existing primary key change tests still need to have the adopt interval
set to a low value in order to finish quickly.

Addresses #45150.

Release note (performance improvement): The cleanup job which runs after
a primary key change to remove old indexes, which blocks other schema
changes from running, now starts immediately after the primary key swap
is complete. This reduces the amount of waiting time before subsequent
schema changes can run.

cockroach-teamcity · 2020-04-17T20:06:19Z

This change is

rohany · 2020-04-17T20:28:25Z

I think the alter column set data type implementation will want something similar to this too

blathers-crl · 2020-04-17T20:29:08Z

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

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

thoszhang · 2020-04-18T03:22:03Z

FWIW, on my laptop, TestMultiplePrimaryKeyChanges takes about 1m to run on master without setTestJobsAdoptInterval, and 30s to run after this change. (It takes about 1s to run with setTestJobsAdoptInterval.) The remaining delay is due to retries from schema change jobs not being first in line.

rohany · 2020-04-18T19:24:36Z

pkg/sql/schema_changer_test.go

+	// change operations. This is wrapped in SucceedsSoon to handle cases where
+	// dropped indexes are expected to be GC'ed immediately after the schema
+	// change completes.
+	testutils.SucceedsSoon(t, func() error {


Was the PrimaryKeyChangeWithOperations flaky now (the old indexes got cleaned up too fast?)

rohany · 2020-04-18T19:24:38Z

pkg/sql/schema_changer_test.go

@@ -2992,14 +3001,68 @@ CREATE TABLE t.test (k INT NOT NULL, v INT);
 	})
 }

+// TestPrimaryKeyDropIndexNotCancelable tests that the job to drop indexes after
+// a primary key change is not cancelable.
+func TestPrimaryKeyDropIndexNotCancelable(t *testing.T) {


Does the job finish now in the logic test so we have to move it here?

rohany · 2020-04-18T19:25:32Z

pkg/sql/schema_changer.go

 	update := func(txn *kv.Txn, descs map[sqlbase.ID]*sqlbase.MutableTableDescriptor) error {
 		// Reset vars here because update function can be called multiple times in a retry.
 		isRollback = false
 		indexGCJobs = nil
+		pkChangeIndexDropJob = nil


Can we change this to a slice like "childJobs" or something so that it is easy to add new jobs to start immediately?

rohany · 2020-04-18T19:26:17Z

Thanks for getting to this lucy!

ajwerner

Seems sane to me. I'll let @rohany sign off.

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

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

Previously, rohany (Rohan Yadav) wrote…

Can we change this to a slice like "childJobs" or something so that it is easy to add new jobs to start immediately?

Seems like a reasonable idea to me

Previously, the job to drop old indexes after a primary key change was created at the end of the schema change and left for the job registry to adopt at some later time. This PR switches the job creation over to `CreateStartableJobWithTxn`, which allows for starting the job immediately after the transaction which creates the job is committed. This eliminates one source of waiting time before other schema changes, which are queued behind the mutation to drop the indexes, can run. Note, however, that successive schema changes don't start running immediately after the mutations ahead of them in the queue are all cleared; if they've ever hit a retry error due to not being first in line, then they will have to wait to be re-adopted by the job registry. This is why the existing primary key change tests still need to have the adopt interval set to a low value in order to finish quickly. Release note (performance improvement): The cleanup job which runs after a primary key change to remove old indexes, which blocks other schema changes from running, now starts immediately after the primary key swap is complete. This reduces the amount of waiting time before subsequent schema changes can run.

thoszhang

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

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

Previously, ajwerner wrote…

Seems like a reasonable idea to me

Done.

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

Previously, rohany (Rohan Yadav) wrote…

Was the PrimaryKeyChangeWithOperations flaky now (the old indexes got cleaned up too fast?)

Yeah, but the problem wasn't the indexes being GC'ed too quickly per se, since the test previously had the default GC TTL set for the table. Without setting the GC TTL to a low value, I kept getting a value that was lower than 2 times the number of rows in the table but that didn't decrease over time. I think what's going on is that some writes to the table are now happening after the index starts being dropped, so the dropped non-public index doesn't necessarily have the expected KV count. Does that seem plausible to you?

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

Previously, rohany (Rohan Yadav) wrote…

Does the job finish now in the logic test so we have to move it here?

Yeah, that's why.

blathers-crl · 2020-04-21T18:08:40Z

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

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

rohany

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

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

Previously, lucy-zhang (Lucy Zhang) wrote…

Yeah, but the problem wasn't the indexes being GC'ed too quickly per se, since the test previously had the default GC TTL set for the table. Without setting the GC TTL to a low value, I kept getting a value that was lower than 2 times the number of rows in the table but that didn't decrease over time. I think what's going on is that some writes to the table are now happening after the index starts being dropped, so the dropped non-public index doesn't necessarily have the expected KV count. Does that seem plausible to you?

Yes, i think that makes sense. The test originally had a multiple of 2 (one multiple for the new primary key and one for the old key). Changing the multiple to 1 and having the gc trigger immediately seems correct.

thoszhang · 2020-04-21T18:31:20Z

TFTRs

bors r+

thoszhang · 2020-04-21T18:34:54Z

(hopefully the infra flake is gone)

craig · 2020-04-21T18:53:05Z

Build failed (retrying...)

GitHub CI (Cockroach)

craig · 2020-04-21T19:57:06Z

Build failed (retrying...)

GitHub CI (Cockroach)

craig · 2020-04-21T20:24:11Z

Build failed (retrying...)

GitHub CI (Cockroach)

craig · 2020-04-21T22:05:17Z

Build failed (retrying...)

GitHub CI (Cockroach)

craig · 2020-04-21T23:00:09Z

Build failed

GitHub CI (Cockroach)

thoszhang · 2020-04-22T02:48:46Z

bors r+

craig · 2020-04-22T03:35:57Z

Build succeeded

GitHub CI (Cockroach)

rohany · 2020-04-22T03:43:09Z

I'm wondering, is this something that is backportable to a point release of 20.1? It makes the previous problem of an apparent hang after primary key changes almost disappear.

thoszhang · 2020-04-22T03:46:05Z

Yeah, I opened a backport PR #47818 (though I figured it wasn't worth getting reviews until we actually start merging to the release branch for 20.1.1, which I believe we're not doing right now).

thoszhang mentioned this pull request Apr 17, 2020

sql: (temporary) Hang when dropping unique index created after ALTER PRIMARY KEY #45150

Closed

otan mentioned this pull request Apr 17, 2020

sql: add primary key on empty table very slow #47607

Closed

thoszhang force-pushed the pk-drop-index-startable-job branch from ff66083 to fc96d9d Compare April 18, 2020 03:17

thoszhang marked this pull request as ready for review April 18, 2020 03:18

thoszhang requested review from rohany and ajwerner April 18, 2020 03:22

rohany suggested changes Apr 18, 2020

View reviewed changes

ajwerner approved these changes Apr 21, 2020

View reviewed changes

thoszhang force-pushed the pk-drop-index-startable-job branch from fc96d9d to 9f647aa Compare April 21, 2020 17:57

thoszhang commented Apr 21, 2020

View reviewed changes

rohany approved these changes Apr 21, 2020

View reviewed changes

thoszhang mentioned this pull request Apr 22, 2020

release-20.1: use StartableJobs for GC and index drop jobs created at the end of the schema changer job #47818

Merged

craig bot merged commit 2bcb72d into cockroachdb:master Apr 22, 2020

thoszhang deleted the pk-drop-index-startable-job branch April 22, 2020 03:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: start index drop job for primary key changes immediately #47624

sql: start index drop job for primary key changes immediately #47624

thoszhang commented Apr 17, 2020 •

edited

Loading

cockroach-teamcity commented Apr 17, 2020

rohany commented Apr 17, 2020

blathers-crl bot commented Apr 17, 2020

thoszhang commented Apr 18, 2020

rohany Apr 18, 2020

rohany Apr 18, 2020

rohany Apr 18, 2020

rohany commented Apr 18, 2020

ajwerner left a comment

thoszhang left a comment

blathers-crl bot commented Apr 21, 2020

rohany left a comment

thoszhang commented Apr 21, 2020

thoszhang commented Apr 21, 2020

craig bot commented Apr 21, 2020

craig bot commented Apr 21, 2020

craig bot commented Apr 21, 2020

craig bot commented Apr 21, 2020

craig bot commented Apr 21, 2020

thoszhang commented Apr 22, 2020

craig bot commented Apr 22, 2020

rohany commented Apr 22, 2020 •

edited

Loading

thoszhang commented Apr 22, 2020

sql: start index drop job for primary key changes immediately #47624

sql: start index drop job for primary key changes immediately #47624

Conversation

thoszhang commented Apr 17, 2020 • edited Loading

cockroach-teamcity commented Apr 17, 2020

rohany commented Apr 17, 2020

blathers-crl bot commented Apr 17, 2020

thoszhang commented Apr 18, 2020

rohany Apr 18, 2020

Choose a reason for hiding this comment

rohany Apr 18, 2020

Choose a reason for hiding this comment

rohany Apr 18, 2020

Choose a reason for hiding this comment

rohany commented Apr 18, 2020

ajwerner left a comment

Choose a reason for hiding this comment

thoszhang left a comment

Choose a reason for hiding this comment

blathers-crl bot commented Apr 21, 2020

rohany left a comment

Choose a reason for hiding this comment

thoszhang commented Apr 21, 2020

thoszhang commented Apr 21, 2020

craig bot commented Apr 21, 2020

Build failed (retrying...)

craig bot commented Apr 21, 2020

Build failed (retrying...)

craig bot commented Apr 21, 2020

Build failed (retrying...)

craig bot commented Apr 21, 2020

Build failed (retrying...)

craig bot commented Apr 21, 2020

Build failed

thoszhang commented Apr 22, 2020

craig bot commented Apr 22, 2020

Build succeeded

rohany commented Apr 22, 2020 • edited Loading

thoszhang commented Apr 22, 2020

thoszhang commented Apr 17, 2020 •

edited

Loading

rohany commented Apr 22, 2020 •

edited

Loading