sql: implement IterateStatementStats() for PersistedSQLStats

[Azhng]

Depends on: #68555, #68620
Related to: #64743

Previously, IterateStatementStats() for PersistedSQLStats was
left unimplemented and it defaults to the implementation of
SQLStats.IterateStatementStats(). This means calls to
IterateStatementStats() on PersistedSQLStats cannot read
the statement statistitcs stored in system table.

This commit implements the IterateStatementStats() through
the new CombinedStmtStatsIterator which enables this method
to read both in-memory and persited statement statistics.

Release note: None

[cockroach-teamcity]

This change is 

[Azhng]

This PR forks off #68555 and cherry picks the change from #68620. So only the last commit is the new addition.

IterateTransactionStats() is not yet implemented in this PR (it will be implemented in a follow up PR later) because most of the code will just be almost identical duplicate of this PR since they share very similar underlying APIs. I want to split the change into 2 PRs so the code change would be smaller.

[Azhng]

Rebased to master to remove the previous two merged commits from the branch.

[maryliag]

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

---

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

// IteratorOptions provides the ability to the caller to change how it iterates
// the statements and transactions.
// TODO(azhng): we want to support pagination/continuation tokens as well as

are this todo no longer valid? was this implemented or dropped?

---

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

	mem struct {
		canBeAdvaned bool

nit: Advanced

---

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

	// If memIter is exhausted, but disk iterator can still move forward.
	// In this case, we promote the disk.Cur() and unpause the disk iterator if
	// it is paused and can be advanced.

nit: If memIter is exhausted, but the disk iterator can still move forward,
we promote the disk.Cur() and resume the disk iterator if it was paused.

---

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

	// If diskIter is exhausted, but mem iterator can still move forward.
	// In this case, we promote mem.Cur() and unpause mem iterator if it is paused
	// and can be advanced.

nit: If diskIter is exhausted, but the mem iterator can still move forward,
we promote the mem.Cur() and resume the mem iterator if it was paused.

---

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

	// Both iterators can be moved forward. Now we check the value of Cur()
	// for both iterator. We will have few scenarios:

nit: both iterators
nit: have a few

---

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

	// 1. mem.Cur() < disk.Cur()
	//    this case, we promote mem.Cur() to c.nextToRead. We then pause
	//    the disk iterator and unpause the mem iterator for next iteration.

nit: unpause > resume
nit: you can remove the this case of all 3 cases, go directly to we promote...

---

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

	// 2. mem.Cur() == disk.Cur()
	//    this case, we promote both mem.Cur() and disk.Cur() by merging both
	//    stats. We unpause both iterators for next iteration.

nit: unpause > resume

---

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

	// 3. mem.Cur() > disk.Cur()
	//    this case, we promote disk.Cur() to c.nextToRead. We then pause
	//    mem iterator and unpause disk iterator for next iteration.

nit: unpause > resume

---

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

	// If and only if all three of the following fields are equal, lhs and rhs
	// are the same and they should be merged.
	if lhs.AggregatedTs.Equal(rhs.AggregatedTs) &&

isn't this case already being treated on the following checks? I can imagine this case would not be common, so you're making all other cases do this extra check without the need

---

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

}

// Cur calls the m.StmtStatsIterator.Cur() but also populates the m.aggregatedTs

nit: ... Cur() and populates

---

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

	ctx context.Context, options *sqlstats.IteratorOptions, visitor sqlstats.StatementVisitor,
) (err error) {
	// We override the sorting options since otherwise we need to implement

nit: otherwise we would need

---

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

	}
	stats.ID = roachpb.StmtFingerprintID(stmtFingerprintID)

nit: remove this blank line

---

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

	stats.Key.PlanHash = uint64(tree.MustBeDInt(row[2]))

nit: remove this blank line

[Azhng]

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

---

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

Previously, maryliag (Marylia Gutierrez) wrote…

are this todo no longer valid? was this implemented or dropped?

I didn't have a specific implementation in mind when I wrote the initial TODOs. My original thought is that the statusServer API would directly use this interface to handle API calls.

Now that it has become clear that it would be better to let the statusServer to directly query the crdb_internal tables because we can directly leverage our entire SQL Engine to do all the filtering/grouping/projection logic instead of redoing them ourselves.

Though, we should still implement the StartTime and EndTime fields here to support virtual index. This is so that we can issue a more optimal query against the system table.

---

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

Previously, maryliag (Marylia Gutierrez) wrote…

isn't this case already being treated on the following checks? I can imagine this case would not be common, so you're making all other cases do this extra check without the need

Ah good call. Removed this check.
I think this case would be common if we are forced to do premature flushes due to memory pressure.

[maryliag]

Reviewed 2 of 19 files at r2, 2 of 5 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @Azhng)

[Azhng]

bors r=maryliag

[craig]

Build succeeded:

• GitHub CI (Cockroach)