sql: implement combined iterator for transaction statistics #69022
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewed 1 of 5 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @Azhng)
-- commits, line 4 at r1 ([raw file](https://github.com/cockroachdb/cockroach/blob/e179dcf6cb302afa2891048ceffa6e68fe85ccb9/-- commits#L4)):
Can you add a little more description here for future reference?
pkg/sql/sqlstats/persistedsqlstats/combined_iterator.go, line 342 at r1 (raw file):
} // Cur returns the roachpb.CollectedStatementStatistics at the current internal
nit: CollectedTransactionStatistics
pkg/sql/sqlstats/persistedsqlstats/reader_test.go, line 123 at r1 (raw file):
statistics *roachpb.CollectedTransactionStatistics, ) error { if len(statistics.StatementFingerprintIDs) == 1 {
you're iterating transactions, but checking statements fingerprint, is that correct?
pkg/sql/sqlstats/persistedsqlstats/reader_test.go, line 177 at r1 (raw file):
statistics *roachpb.CollectedTransactionStatistics, ) error { if len(statistics.StatementFingerprintIDs) == 1 {
same thing here: you're iterating transactions but checking statements
Azhng
force-pushed
the
sqlstats-combined-txn-iterator
branch
from
e179dcf
to
5a90f21
Compare
This commit introduce combined iterator that iterates through both in-memory and persisted transaction statistics. Follow up to cockroachdb#68675 Release note: None
Azhng
force-pushed
the
sqlstats-combined-txn-iterator
branch
from
5a90f21
to
4f67ea7
Compare
There was a problem hiding this comment.
Reviewable status:
-- commits, line 4 at r1 ([raw file](https://github.com/cockroachdb/cockroach/blob/e179dcf6cb302afa2891048ceffa6e68fe85ccb9/-- commits#L4)):
Previously, maryliag (Marylia Gutierrez) wrote…
Can you add a little more description here for future reference?
Done.
pkg/sql/sqlstats/persistedsqlstats/combined_iterator.go, line 342 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
nit: CollectedTransactionStatistics
Done.
pkg/sql/sqlstats/persistedsqlstats/reader_test.go, line 123 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
you're iterating transactions, but checking statements fingerprint, is that correct?
Yes, since the test cases here is keyed using the statement fingerprint string (this is because it's difficult to compute transaction fingerprint id directly from the statement fingerprint string), and we don't have a way to lookup transaction using statement fingerprint string directly, we do an indirect lookup by using the statement fingerprint id associated with that transaction to lookup the test case result.
pkg/sql/sqlstats/persistedsqlstats/reader_test.go, line 177 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
same thing here: you're iterating transactions but checking statements
replied in the thread above
There was a problem hiding this comment.
Reviewed 2 of 5 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status:
|
TFTR! bors r=maryliag |
|
bors r- |
|
Canceled. |
|
bors r=maryliag |
|
Build failed (retrying...): |
|
Build succeeded: |
69049: sql: implement crdb_internal.transaction_statistics r=ajwerner,maryliag a=Azhng Depends on #68715 and #69022 # First Commit roachpb,sql,server: add TransactionFingerprintID to roachpb.CollectedTransactionStatistics Previously, roachpb.CollectedTransactionStatistics does not contain TransactionFingerprintID. This results in SQLStatusServer's /_status/statements endpoint not able to return transaction fingerprint id in the response. This commit adds transaction fingerprint id into CollectedTransactionStatistics and simplifies the API in multiple layers. Release note: None # Second Commit sql: introduce crdb_internal.transaction_statistics virtual table This commit introduces crdb_internal.transaction_statistics virtual table that exposes both cluster-wide in-memory transaction statistics as well as persited transaction statistics. This new virtual table will be used to replace crdb_internal.node_transaction_statistics virtual table, which only surface node-local in-memory transaction statsitics. Follow up to #68715 Release justification: Category 4 Release note (sql change): introduced new crdb_internal.transaction_statistics virtual table that surfaces both cluster-wide in-memory transaction statistics as well as persisted transaction statistics. 69151: sql/resolver: wrong error when db does not exist for virtual schemas r=ajwerner a=fqazi Fixes: #68060 Previously, when a database did not exist under a virtual schema the code would fall through do a normal look up. Before a recent refactor of the some of the internals, we accidentally changed behavior, so that an undefined relation error was returned. To address this, this patch adds a check inside the resolver layer to return the correct error instead of falling through. Release note: None 69212: workload: fix pgx error cast in kv95 and schemachange r=RichardJCai a=rafiss fixes #69189 #69100 is failing but i'm not sure if this is the cause. (cc @ajwerner) This was done incorrectly after the recent upgrade to pgx4. `pgconn.PgError` does not implement `error`, but `*pgconn.PgError` does. Release justification: test only change Release note: None 69223: changefeedccl: Rework webhook sink flushing implementation. r=miretskiy a=miretskiy Stop relying on wait group to implement flush logic in webhook sink. The wait group does not respect context cancellation. Because of that, it is possible that the caller blocks, waiting for Flush to complete, while immediately after blocking, the context is cancelled. When this happens the go routines running responsible for processing the messages may terminate prior to decrementing wait group counts. Instead of relying on waitgroup, use synchronization provided by the channels themselves, and introduce a new type of worker request (flush) which correctly flushes and waits for flush to complete, while respecting context cancellation. Fixes #69175 Release Notes: None Co-authored-by: Azhng <archer.xn@gmail.com> Co-authored-by: Faizan Qazi <faizan@cockroachlabs.com> Co-authored-by: Rafi Shamim <rafi@cockroachlabs.com> Co-authored-by: Yevgeniy Miretskiy <yevgeniy@cockroachlabs.com>
Follow up to #68675
Release note: None