I feel like there must be some issue or RFC or other documentation providing context as to what purpose this table will serve, could you provide a link please? I'm not sure where to look, I've looked in the RFCs folder and the Jobs board to no avail.

Reviewed 10 of 10 files at r1, 1 of 1 files at r2, 2 of 2 files at r3, 41 of 41 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @dt, @samiskin, and @stevendanna)

pkg/jobs/registry.go line 975 at r2 (raw file):

		}
		if nDeletedInfos, err = r.ex.Exec(
			ctx, "gc-jobs", nil /* txn */, infoStmt, toDelete,

Shouldn't this be in the same transaction as above? We don't have an explicit foreign key but I'm slightly worried about logical referential integrity, if that makes sense.

pkg/jobs/registry.go line 977 at r2 (raw file):

			ctx, "gc-jobs", nil /* txn */, infoStmt, toDelete,
		); err != nil {
			return false, 0, errors.Wrap(err, "deleting old jobs")

nit: is there any value in giving this query and error message wrapper distinct string literals, versus the system.jobs query?

pkg/jobs/registry.go line 682 at r3 (raw file):

// IterateJobInfo iterates though the info records for a given job and info key
// prefix.
// TODO(dt): use pagination in query.

Shouldn't this be tracked in an issue which is marked as a GA-blocker? It seems important enough. Please ignore if this is the case already.

pkg/jobs/registry.go line 744 at r3 (raw file):

		ctx, "job-info-write", txn,
		sessiondata.InternalExecutorOverride{User: username.NodeUserName()},
		"DELETE FROM system.job_info WHERE job_id = $1 AND info_key = $2",

Why is written in the primary key at all, if there's effectively a 1:1 relationship between it and the (job_id, info_key) pair? This deserves commentary, otherwise one might wonder: why not simply rely on the crdb_internal_mvcc_timestamp column instead?

pkg/jobs/registry.go line 755 at r3 (raw file):

		ctx, "job-info-write", txn,
		sessiondata.InternalExecutorOverride{User: username.NodeUserName()},
		`INSERT INTO system.job_info (job_id, info_key, written, value) VALUES ($1, $2, now(), $3)`,

nit: is the inclusion of written deliberate here? I feel it's fine to remove and rely on its default value, it's a common-enough pattern.

pkg/sql/catalog/systemschema/system.go line 682 at r1 (raw file):

CREATE TABLE system.job_info (
	job_id INT8 NOT NULL,
	info_key BYTES NOT NULL,

I'm missing context here on what this is. Can you please explain, either in the code or in an issue? Am I correct in guessing that this table is a deliberately unstructured key-value store for each job?

pkg/upgrade/upgrades/system_job_info_test.go line 28 at r1 (raw file):

)

func TestSystemJobInfoMigration(t *testing.T) {

nit: FYI we call them "upgrades" now, "migration" is so 2021.

pkg/upgrade/upgrades/system_job_info_test.go line 41 at r1 (raw file):

	tc := testcluster.StartTestCluster(t, 1, base.TestClusterArgs{
		ServerArgs: base.TestServerArgs{
			Settings: settings,

Is this necessary at all? I'm not seeing this pattern in other upgrade tests.

pkg/upgrade/upgrades/system_job_info_test.go line 45 at r1 (raw file):

				Server: &server.TestingKnobs{
					DisableAutomaticVersionUpgrade: make(chan struct{}),
					BinaryVersionOverride:          clusterversion.ByKey(clusterversion.SystemPrivilegesTable - 1),

Shouldn't the key here be CreateSystemJobInfoTable instead of SystemPrivilegesTable? See also elsewhere in this test definition. I'm guessing that's why the test is failing presently.

I very much don't want a central library handing out ImportInfo / RestoreInfo / SchemaChangeInfo / etc; we shouldn't need to edit the core jobs code dependency root every time a leaf job changes a field in its persisted data (we do today and it's not good). I want a "I want to store this thing and I'll ask for it later" API where the library is agnostic to what we're storing.

That said, I could see switching from []byte to proto.Message or a message job.InfoRecord { types.Any payload = 1} wrapper proto or something (though I've started to worry about types.Any and refactoring changing persisted type names more lately).

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @postamar, @samiskin, and @stevendanna)

pkg/jobs/registry.go line 975 at r2 (raw file):

Great question!

I wanted an FK but ultimately we can't have one, since we want to allow job creators to stage info rows before creating a job row, and indeed even to do so in separate transactions, before the txn that creates the job row. Allowing this -- that info rows can be created even without a job row -- is pretty important since we want to do as little as possible in that job-creating transaction since that causes job. So we've accepted that there may be at least temporary inconsistency between these, and indeed more than that -- a job creator could crash after some of its staging, with some info rows and then fail to create a job and not clean them up. So we might need a recon cleanup loop of last resort anyway (though not urgently -- a couple orphaned info rows are pretty harmless). Given that, I'm inclined not to pile them into the same txn here., as long as we delete from the jobs first, then the infos (which we do), just to minimize the risk of something in the info deletion blocking job deletion.

pkg/jobs/registry.go line 977 at r2 (raw file):

I think our errors library usually gets us the stack anyway so I don't think so?

pkg/jobs/registry.go line 682 at r3 (raw file):

Only an issue if/once it has a production caller who is iterating a non-trivial number of rows, so I'd say it isn't a blocker until then -- i.e. if initial callers just persist 2 info rows for each job (payload and progress) then this can wait.

pkg/jobs/registry.go line 744 at r3 (raw file):

As you found/mentioned above, this is mentioned in the design doc but we're doing our own mvcc since we know these job info (logical) rows get lots of "in-place" updates in long-running jobs so we're splitting that into separate crdb table (physical) rows, since KV and SQL don't allow splitting between kv mvcc revisions of a single sql row, but we want to be able to split between revisions of a job row.

pkg/jobs/registry.go line 755 at r3 (raw file):

Personal preference: I like to see what I'm writing and not need to pull up the schema to find the default defs.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @baoalvin1, @postamar, @samiskin, @stevendanna, and @Xiang-Gu)

pkg/sql/catalog/systemschema/system.go line 685 at r1 (raw file):

I added a comment inline here summarizing the reasoning explored in the RFC.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @baoalvin1, @dt, @postamar, @samiskin, @stevendanna, and @Xiang-Gu)

pkg/sql/catalog/systemschema/system.go line 682 at r1 (raw file):

The RFC linked in the PR now is pretty explicit about the fact being arbitrary keys is deliberate.

pkg/jobs/registry_test.go line 1155 at r3 (raw file):

Done.

pkg/jobs/registry_test.go line 1172 at r3 (raw file):

Done.

pkg/upgrade/upgrades/system_job_info_test.go line 41 at r1 (raw file):

I think all the upgrade tests do this, so they can initialize the version to before they run.

pkg/upgrade/upgrades/system_job_info_test.go line 45 at r1 (raw file):

The start should be before the migration so that the test can assert that running the migration actually advances from before to after states as expected; at the time I wrote this SystemPrivilegesTable was the most recent version so that's what it used as its before, but now I makes more sense to be V22_2.

I understand that you're still fighting CI test failures, but this seems close enough.

I very much don't want a central library handing out ImportInfo ... [...] That said, I could see switching from []byte to proto.Message

Fair enough. Also, the []byte -> proto.Message doesn't need to happen now, if ever.

Hope no one slips another system table in before you merge :D

I'm cool with the wait-and-see approach

Thanks for working on this.

It will be nice to start playing with this.

I think we will want to think about the semantics of updating a job_info record with or without the lease, but I'm happy to leave that to a follow up if it ends up being needed, as you already indicated.