sqlbase: Add created_by columns to system.jobs

[miretskiy]

Informs #49346

Add created_by_type and created_by_id columns, along with the index over
these columns, to the system.jobs table.

Add sql migration code to migrate old definition to the new one.

See https://github.com/cockroachdb/cockroach/blob/master/docs/RFCS/20200414_scheduled_jobs.md

Release Notes: None

[cockroach-teamcity]

This change is 

[pbardea]

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

---

pkg/clusterversion/cockroach_versions.go, line 514 at r1 (raw file):

	// that wouldn't work since you'd have to go through the final 19.2 binary
	// before going to HEAD.
	binaryMinSupportedVersion = VersionByKey(Version20_1)

I think the comment above needs updating.

---

pkg/sql/sqlbase/system.go, line 885 at r2 (raw file):

			{Name: "progress", ID: 5, Type: types.Bytes, Nullable: true},
		},
		NextColumnID: 8,

Would this still be 6 on the old jobs table?

---

pkg/sqlmigrations/migrations_test.go, line 833 at r2 (raw file):

	// Check that we don't have created_by columns.
	var count int
	require.Error(t, mt.sqlDB.DB.QueryRowContext(ctx, query).Scan(&count))

Do you think it's worthwhile checking that the error we get here is the one that we expect? Alternatively we could also do something like SELECT column_name FROM [SHOW COLUMNS from system.jobs];?

Do you think it's also worth testing that the indexes have been created?

[pbardea]

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

---

pkg/clusterversion/cockroach_versions.go, line 514 at r1 (raw file):

Previously, pbardea (Paul Bardea) wrote…

I think the comment above needs updating.

Nevermind - I see this has been pulled out into another PR.

[miretskiy]

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

---

pkg/sql/sqlbase/system.go, line 885 at r2 (raw file):

Previously, pbardea (Paul Bardea) wrote…

Would this still be 6 on the old jobs table?

YES! Good Catch

---

pkg/sqlmigrations/migrations_test.go, line 833 at r2 (raw file):

Previously, pbardea (Paul Bardea) wrote…

Do you think it's worthwhile checking that the error we get here is the one that we expect? Alternatively we could also do something like SELECT column_name FROM [SHOW COLUMNS from system.jobs];?

Do you think it's also worth testing that the indexes have been created?

Great idea. Done. I also changed index names a bit... Basically created_by_idx is too close
to created_by_id column... I decided to use default names for the index (long, but descriptive).

Finally, I had to explicitly specify familly name when adding new columns (in the migration.go) -- not sure why, but without it, alter columns added new columns to 2 new column families.

Do you know how to check which families exist on a table?

[dt]

we could just move this to the test since that is the only usage of it, right?

[pbardea]

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

---

pkg/sqlmigrations/migrations_test.go, line 833 at r2 (raw file):

Previously, miretskiy (Yevgeniy Miretskiy) wrote…

Great idea. Done. I also changed index names a bit... Basically created_by_idx is too close
to created_by_id column... I decided to use default names for the index (long, but descriptive).

Finally, I had to explicitly specify familly name when adding new columns (in the migration.go) -- not sure why, but without it, alter columns added new columns to 2 new column families.

Do you know how to check which families exist on a table?

Hmm, the only way that comes to mind would be SHOW CREATE system.jobs - you then may be able to compare it against JobsTableSchema.

[thoszhang]

Reviewed 1 of 1 files at r1, 3 of 3 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @miretskiy and @pbardea)

---

pkg/sql/sqlbase/system.go, line 165 at r3 (raw file):

	payload           BYTES     NOT NULL,
	progress          BYTES,
  created_by_type   STRING,

nit: the columns should be aligned

---

pkg/sqlmigrations/migrations_test.go, line 829 at r3 (raw file):

	mt.start(t, base.TestServerArgs{})

	// We expect to create 2 new clolumns

nit: columns

[miretskiy]

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

---

pkg/sql/sqlbase/system.go, line 872 at r3 (raw file):

Previously, dt (David Taylor) wrote…

we could just move this to the test since that is the only usage of it, right?

Good idea. Done. actually, removed descriptor, and just used old schema.

[miretskiy]

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

---

pkg/sql/sqlbase/system.go, line 165 at r3 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

nit: the columns should be aligned

Strange.. These are aligned fined in my editor...

---

pkg/sql/sqlbase/system.go, line 872 at r3 (raw file):

Previously, miretskiy (Yevgeniy Miretskiy) wrote…

Good idea. Done. actually, removed descriptor, and just used old schema.

Done.

[miretskiy]

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

---

pkg/sqlmigrations/migrations_test.go, line 833 at r2 (raw file):

Previously, pbardea (Paul Bardea) wrote…

Hmm, the only way that comes to mind would be SHOW CREATE system.jobs - you then may be able to compare it against JobsTableSchema.

it's alright.. I guess.

[miretskiy]

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

[miretskiy]

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

[dt]

huh, I guess it is OK to change the name of the default family for new jobs tables but not migrate it for old ones? I donno, maybe a question for @lucy-zhang

[thoszhang]

Oh, yeah, what is preventing us from using the existing name?

[miretskiy]

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

---

pkg/sql/sqlbase/system.go, line 170 at r4 (raw file):

Previously, dt (David Taylor) wrote…

part of what worried me when I saw this is now for clusters where the jobs table was created by 20.1 and below, the family will have one name while those that are started at 20.2 and up will have another. I can't actually think of how this will cause a specific problem though, just a little bit of generalized unease -- usually the migration brings an old table up to be identical to a newly created one. Actually, now that I think a bit more I think one way this could bite us is if someone later, say in 21.1, wants to add a column to this family -- what name do they use to specify it? if they use "primary" but that cluster was originally created on 19.1 it will fail, while if they use the family_0_blah_blah name, it'll fail if it was created on 20.2. I think we should either use the current name or include a rename (if that is even possible) in the migration.

Very good point. I've modified migration script and tests to ensure we rename column family. PTAL.

[miretskiy]

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

---

pkg/sqlmigrations/migrations_test.go, line 833 at r2 (raw file):

Previously, miretskiy (Yevgeniy Miretskiy) wrote…

it's alright.. I guess.

Done.

[dt]

LGTM except for the naming bikeshed (the best kind really) over name of the primary family and happy to drop that concern if others are fine with it.

[miretskiy]

I really don't care about the name of the primary family -- just used what we had used in few other system tables.
Do you have other suggestions? Be happy to change it.

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

---

pkg/sqlmigrations/migrations.go, line 1861 at r5 (raw file):

Previously, dt (David Taylor) wrote…

I'll happily defer to the opinions of the sql and sqlmigration experts of @lucy-zhang (and @jordanlewis), and I know I originally mentioned that maybe it'd be possible to resolve my concern by renaming, but seeing this actually worked out, personally, this feels like a lot more migration code and associated complexity/risk for the benefit of just a shorter name. ~nobody has to see / type this name except when adding columns to this table so I don't know if it is worth all this?

Yeah -- it's a bit more code, primarily because I was being extra paranoid
(comparing tableID version, family id, etc). The code itself isn't that terrible -- primarily lifted form the existing examples in this file.

I do see your point, however.

Perhaps a better way is to explicitly name primary family in the updated JobsTable using the
old name. Add a comment -- and just be done with it.

Let me know what you think ( @lucy-zhang , @jordanlewis , @dt ) -- i'd be happy to drop this rename (but I will likely keep the changes to the test -- I kinda like latest version of the test).

[jordanlewis]

Thanks for the ping. Just for the sake of future-us sanity I would prefer that you not do this rename. Things like this, in my experience, are ticking time bombs - they might feel fine now, but unless we rigorously, rigorously test the migration path (including upgrades, downgrades, weird cases of which there are many), we won't feel completely confident in this and if something goes wrong in a 20.1->20.2 upgrade, this small change will now be added to the list of scary things that we need to worry about given a failure.

To be clear, this might be a totally sound change and I would believe you if you said it works. But I think we've learned our lesson that, just like in KV, we should strongly prefer to avoid unnecessary migrations because of the potential headaches they might incur down the line.

[miretskiy]

Reverted; system tables updated to use "old" family name; comments added. PTAL.

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

---

pkg/sql/sqlbase/system.go, line 170 at r4 (raw file):

Previously, miretskiy (Yevgeniy Miretskiy) wrote…

Very good point. I've modified migration script and tests to ensure we rename column family. PTAL.

Done.

---

pkg/sqlmigrations/migrations.go, line 1861 at r5 (raw file):

Previously, miretskiy (Yevgeniy Miretskiy) wrote…

Yeah -- it's a bit more code, primarily because I was being extra paranoid
(comparing tableID version, family id, etc). The code itself isn't that terrible -- primarily lifted form the existing examples in this file.

I do see your point, however.

Perhaps a better way is to explicitly name primary family in the updated JobsTable using the
old name. Add a comment -- and just be done with it.

Let me know what you think ( @lucy-zhang , @jordanlewis , @dt ) -- i'd be happy to drop this rename (but I will likely keep the changes to the test -- I kinda like latest version of the test).

Done.

[dt]

I'm not sure how this works but do we need to mint a new version to put here to ensure existing data dirs / clusters from master binaries prior to this patch, but from after VersionStart20_2, i.e. the last couple months, will still run the migration?

[miretskiy]

I think we should. Added.

[dt]

one more nit: I'd make the title "sqlbase: add created_by columns to system.jobs" or something since just "modify system.jobs" doesn't really summarize the purpose as much.

-- Done

[miretskiy]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)