sql: reimplement sqlstats API using iterator #68620
Conversation
|
This change is |
Azhng
force-pushed
the
sqlstats-iterators
branch
2 times, most recently
from
ff35ece to
26c5d26
Compare
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @Azhng)
pkg/sql/sqlstats/sslocal/sslocal_iterator.go, line 48 at r1 (raw file):
// Next increments the internal counter of the StmtStatsIterator. It returns // true if the following Cur() call will be valid, false otherwise.
nit: ... call is valid, false...
pkg/sql/sqlstats/sslocal/sslocal_iterator.go, line 54 at r1 (raw file):
if s.curIter == nil || !s.curIter.Next() { s.idx++ if s.idx >= len(s.appNames) {
what is the connection between idx counter and appNames?
pkg/sql/sqlstats/sslocal/sslocal_iterator.go, line 62 at r1 (raw file):
} // This happens when our child iterator is valid and s.curIter.Next() call
from this comment, does this means you should be using s.curIter.Next() instead of s.Next() as you do above?
pkg/sql/sqlstats/sslocal/sslocal_iterator.go, line 95 at r1 (raw file):
// Next increments the internal counter of the TxnStatsIterator. It returns // true if the following Cur() call will be valid, false otherwise.
nit: ... call is valid, false...
pkg/sql/sqlstats/ssmemstorage/ss_mem_iterator.go, line 56 at r1 (raw file):
// Next increments the internal counter of the StmtStatsIterator. It returns // true if the following Cur() call will be valid, false otherwise.
nit: ... call is valid, false...
pkg/sql/sqlstats/ssmemstorage/ss_mem_iterator.go, line 69 at r1 (raw file):
stmtFingerprintID := constructStatementFingerprintIDFromStmtKey(stmtKey) statementStats, _, _ := s.container.getStatsForStmtWithKey(stmtKey, invalidStmtFingerprintID, false /* createIfNonexistent */)
where this invalidStmtFingerprintID is coming from? What is the reason for it?
pkg/sql/sqlstats/ssmemstorage/ss_mem_iterator.go, line 90 at r1 (raw file):
Query: stmtKey.anonymizedStmt, DistSQL: distSQLUsed, Opt: true,
there is no cases this value could be false? why not use the value you got from the stats, as you're doing with all other parameters?
pkg/sql/sqlstats/ssmemstorage/ss_mem_iterator.go, line 136 at r1 (raw file):
// Next increments the internal counter of the TxnStatsIterator. It returns // true if the following Cur() call will be valid, false otherwise.
nit: ... call is valid, false...
Azhng
force-pushed
the
sqlstats-iterators
branch
from
26c5d26 to
7226668
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sqlstats/sslocal/sslocal_iterator.go, line 54 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
what is the connection between idx counter and appNames?
Thesslocal.StmtStatsIterator maintains a slice of appNames that it will iterate through. The idx is the the index into that appName slice.
pkg/sql/sqlstats/sslocal/sslocal_iterator.go, line 62 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
from this comment, does this means you should be using
s.curIter.Next()instead ofs.Next()as you do above?
This is because s.curIter can be empty. Direct call to s.curIter.Next() would return false in that case and prematurely abort the iteration. A recursive call to s.Next() has this scenario handled through the if check in the beginning of the function.
pkg/sql/sqlstats/ssmemstorage/ss_mem_iterator.go, line 69 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
where this invalidStmtFingerprintID is coming from? What is the reason for it?
This code is directly lifted from the existing code. The idea here is that since we are not creating new stmtStats entry, (createIfNonexistent = false), we are simply performing a lookup using stmtKey. So instead of computing a new stmtFingerprintID, we simply just look it up and use a invalidStmtFingerprintID here.
pkg/sql/sqlstats/ssmemstorage/ss_mem_iterator.go, line 90 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
there is no cases this value could be false? why not use the value you got from the stats, as you're doing with all other parameters?
This code is also directly lifted from the old code. I think it's always true due to historical reasons. #67805 (review). We have an issue for removing it from the StatementStatistics
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sqlstats/ssmemstorage/ss_mem_iterator.go, line 90 at r1 (raw file):
Previously, Azhng (Archer Zhang) wrote…
This code is also directly lifted from the old code. I think it's always true due to historical reasons. #67805 (review). We have an issue for removing it from the
StatementStatistics
Issue link: #68077
There was a problem hiding this comment.
Reviewed 2 of 7 files at r1, all commit messages.
Reviewable status:
pkg/sql/sqlstats/sslocal/sslocal_iterator.go, line 54 at r1 (raw file):
Previously, Azhng (Archer Zhang) wrote…
The
sslocal.StmtStatsIteratormaintains a slice ofappNamesthat it will iterate through. Theidxis the the index into thatappNameslice.
Can you add a comment that the iteration is over the appNames (and from each one is where you get the stats)?
pkg/sql/sqlstats/ssmemstorage/ss_mem_iterator.go, line 56 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
nit: ... call is valid, false...
you marked as resolved without actually changing it
Azhng
force-pushed
the
sqlstats-iterators
branch
from
7226668 to
231395c
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sqlstats/sslocal/sslocal_iterator.go, line 54 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
Can you add a comment that the iteration is over the appNames (and from each one is where you get the stats)?
Done.
pkg/sql/sqlstats/ssmemstorage/ss_mem_iterator.go, line 56 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
you marked as resolved without actually changing it
Done.
There was a problem hiding this comment.
Reviewed 3 of 7 files at r1, 2 of 2 files at r3, all commit messages.
Reviewable status:
|
TFTR! bors r+ |
|
Build failed: |
|
bors r+ |
As we move to implement an aggregated virtual table of in-memory sqlstats and persisted sqlstats, current SQL Stats API makes it difficult to implement such behaviour. This commit introduces lower level iterator APIs that can be used later for the aggregated virtual table. Release note: None
|
Build failed: |
Azhng
force-pushed
the
sqlstats-iterators
branch
from
231395c to
c6627e3
Compare
|
CI keeps flaking. 😭 😭 😭. |
|
bors r+ |
|
Build succeeded: |
68567: colrpc: enhance warnings from the outbox r=yuzefovich a=yuzefovich This commit marks several string constants as "safe" from the redactability perspective so that the warnings logged by the outboxes are more helpful. Additionally, several minor nits around error formatting are addressed. Release note: None 68675: sql: implement IterateStatementStats() for PersistedSQLStats r=maryliag a=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 68738: sql/catalog/dbdesc: repair old descriptors corrupted due to DROPPED SCHEMA name r=ajwerner a=sajjadrizvi #63119 fixes a bug that corrupts a database descriptor when a child schema was dropped, adding an entry in schema-info structure erroneously with database name instead of schema name . Although the bug was fixed , there can be database backups with corrupted descriptors. This commit adds a post-deserialization function to repair a corrupted descriptor. Moreover, it adds a test function to ensure that descriptors with such corruption are fixed during migration. Release note: None Fixes: #63148 68903: github: redirect KV code reviews to @cockroachdb/kv-prs r=irfansharif a=irfansharif We try to use the @cockroachdb/kv alias for notifying the entire team on issues. There's no way to disable notifications for github's automatic codeowners driven review requests, and that tends to be a firehose, so lets use this sub-team alias instead. Release note: None 68978: changefeedccl: detect sink URLs with no scheme r=HonoreDB a=stevendanna Previously, if a user provided a sink URL with no scheme (such as ` kafka%3A%2F%2Fnope%0A`), a changefeed job would be started. However, this changefeed job would be writing into a bufferSink. The bufferSink is used by core changefeeds. The user may have provided such a URL because of confusion over how to URL encode their sink URL. Now, they will receive an error such as ``` pq: no scheme found for sink URL "kafka%3A%2F%2Fnope%0A" ``` Release note (enterprise change): CHANGEFEED statements now error if the provided sink URL does not contain a scheme. Such URLs are typically a mistake and will result in non-functional changefeeds. Co-authored-by: Yahor Yuzefovich <[email protected]> Co-authored-by: Azhng <[email protected]> Co-authored-by: Sajjad Rizvi <[email protected]> Co-authored-by: irfan sharif <[email protected]> Co-authored-by: Steven Danna <[email protected]>
As we move to implement an aggregated virtual table of in-memory
sqlstats and persisted sqlstats, current SQL Stats API makes it
difficult to implement such behaviour.
This commit introduces lower level iterator APIs that can be used
later for the aggregated virtual table.
Release note: None