sql,jobs: create SQL Stats Compaction Job and resumer #68434

Azhng · 2021-08-04T17:50:24Z

Previous PR #67090
Followed by #68401

First Commit

sql,util: refactor checking for running jobs to its own util package

Release note: None

Second Commit

sql,jobs: create SQL Stats Compaction Job and resumer

This commit introduces the SQL Stats Compaction job type
and a barebones implementation of the SQL Stats compaction.

Release justification: category 4

Release note: None

cockroach-teamcity · 2021-08-04T17:50:35Z

This change is

matthewtodd

🎉

Reviewed 3 of 3 files at r1, 16 of 16 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng)

-- commits, line 10 at r2 ([raw file](https://github.com/cockroachdb/cockroach/blob/2b8342d0df54f4634affec19a708aaa5fa425e53/-- commits#L10)):
nit: "barebones"

pkg/sql/compact_sql_stats.go, line 54 at r2 (raw file):

	return db.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error {
		if err != nil {
			if jobErr := r.Job.FractionProgressed(ctx, txn, func(ctx context.Context, _ jobspb.ProgressDetails) float32 {

Maybe a comment here, because it's a little unclear to me what this line is doing. Does the literal 0 in the callback mean anything, like are we checking that the job hasn't made any progress? Or are we just using FractionProgressed for its error-checking capabilities, but we don't actually care about progress?

pkg/sql/sqlstats/persistedsqlstats/cluster_settings.go, line 59 at r2 (raw file):

	"sql.stats.persisted_rows.max",
	"maximum number of rows of statement and transaction"+
		" statistics will be persisted in the system tables",

nit: "statistics that will be"

pkg/sql/sqlstats/persistedsqlstats/compaction_exec.go, line 41 at r2 (raw file):

// StatsCompactorKnobs can be used to tune the behavior of
// StatsCompactor.
type StatsCompactorKnobs struct {

If these are just for testing, maybe call them StatsCompactorTestingKnobs?

pkg/sql/sqlstats/persistedsqlstats/compaction_exec.go, line 69 at r2 (raw file):

	}

	stmtStatsEntryCnt, txnStatsEntryCnt, err := c.getExistingStmtAndTxnStatsEntries(ctx)

Is "cnt" a common abbreviation for "count" around here? Maybe spell it out if not?

pkg/sql/sqlstats/persistedsqlstats/compaction_exec.go, line 87 at r2 (raw file):

	stmt := "SELECT count(*) FROM system.statement_statistics"
	if c.TestingKnobs != nil && c.TestingKnobs.DisableFollowerRead {
		stmt = "SELECT count(*) FROM system.statement_statistics"

This is the same stmt as above, did you mean to make something different happen here?

pkg/sql/sqlstats/persistedsqlstats/compaction_exec.go, line 103 at r2 (raw file):

	stmtStatsEntryCnt = int64(tree.MustBeDInt(row[0]))

	// stmt = "SELECT count(*) FROM system.transaction_statistics AS OF SYSTEM TIME follower_read_timestamp()"

Kill this comment, yeah?

pkg/sql/sqlstats/persistedsqlstats/compaction_exec.go, line 106 at r2 (raw file):

	stmt = "SELECT count(*) FROM system.transaction_statistics"
	if c.TestingKnobs != nil && c.TestingKnobs.DisableFollowerRead {
		stmt = "SELECT count(*) FROM system.transaction_statistics"

Ditto, same stmt as above.

pkg/sql/sqlstats/persistedsqlstats/compaction_exec.go, line 138 at r2 (raw file):

  SELECT %[2]s
  FROM %[1]s
  ORDER BY aggregated_ts

Maybe an explicit ASC here? I get nervous deleting things, lol.

pkg/sql/sqlstats/persistedsqlstats/compaction_scheduling.go, line 53 at r2 (raw file):

// CheckExistingCompactionJob checks for existing SQL Stats Compaction job
// that are either PAUCED, CANCELED, or RUNNING. If so, it returns a

PAUSED

pkg/sql/sqlstats/persistedsqlstats/compaction_test.go, line 54 at r2 (raw file):

	stopper := stop.NewStopper()
	defer stopper.Stop(ctx)

Just curious, what does this stopper stop? I don't see it used below. Does it look for magic stuff in the ctx?

pkg/sql/sqlstats/persistedsqlstats/compaction_test.go, line 100 at r2 (raw file):

	require.Equal(t, maxPersistedRowLimit, len(actualStmtFingerprints))
	require.Equal(t, maxPersistedRowLimit, len(actualTxnFingerprints))
	for fingerprintID := range actualTxnFingerprints {

super-nit: This code usually talks about statements, then transactions, so maybe move this for loop down?

pkg/util/jobutil/job_utils.go, line 28 at r1 (raw file):

// started earlier than this one. If job is nil, CheckRunningJobsHelper just
// checks if there are any pending, running, or paused jobs.
func CheckRunningJobsHelper(

All makes sense, since this is a pure extraction. I'm thinking about names and wondering if this might be called RunningJobsExist?

pkg/util/jobutil/job_utils.go, line 30 at r1 (raw file):

func CheckRunningJobsHelper(
	ctx context.Context,
	job *jobs.Job,

Would it make sense to just pass the JobID here? Or could it be integrated into the filter somehow?

pkg/util/jobutil/job_utils.go, line 33 at r1 (raw file):

	ie sqlutil.InternalExecutor,
	txn *kv.Txn,
	payloadFilter func(payload *jobspb.Payload) bool,

One more naming thought, I think this is a "payloadPredicate"?

Azhng

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @matthewtodd)

pkg/sql/compact_sql_stats.go, line 54 at r2 (raw file):