sqlstats: reuse the temporary statement stats container #123698
Conversation
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
xinhaoz
requested review from
nkodali,
dhartunian and
abarganier
and removed request for
a team
xinhaoz
force-pushed
the
stats-collector
branch
3 times, most recently
from
c81db58
to
9bfd81b
Compare
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @abarganier, @nkodali, and @xinhaoz)
-- commits line 41 at r2:
nit managing
pkg/sql/sqlstats/sslocal/sslocal_stats_collector.go line 32 at r1 (raw file):
type StatsCollector struct { // currentTransactionStatements is the current transaction's statement statistics. // They will be flushed to flushTarget when the transaction is done.
This docstring doesn't match the field name.
pkg/sql/sqlstats/sslocal/sslocal_stats_collector.go line 53 at r1 (raw file):
sendInsights bool flushTarget sqlstats.ApplicationStats
can you add a docstring for this field and explain its purpose?
pkg/sql/sqlstats/sslocal/sslocal_stats_collector.go line 131 at r1 (raw file):
) discardedStats += s.flushTarget.MergeApplicationTransactionStats(
You've removed the only usage of this function so can we delete the impl as well?
pkg/sql/sqlstats/sslocal/sslocal_stats_collector.go line 141 at r1 (raw file):
} s.ApplicationStats.Free(ctx)
was this the only usage of Free? can we delete it?
xinhaoz
force-pushed
the
stats-collector
branch
2 times, most recently
from
c5f9c24
to
2603522
Compare
There was a problem hiding this comment.
Reviewable status:
Previously, dhartunian (David Hartunian) wrote…
nit
managing
Done.
pkg/sql/sqlstats/sslocal/sslocal_stats_collector.go line 32 at r1 (raw file):
Previously, dhartunian (David Hartunian) wrote…
This docstring doesn't match the field name.
Fixed.
pkg/sql/sqlstats/sslocal/sslocal_stats_collector.go line 53 at r1 (raw file):
Previously, dhartunian (David Hartunian) wrote…
can you add a docstring for this field and explain its purpose?
Done.
pkg/sql/sqlstats/sslocal/sslocal_stats_collector.go line 131 at r1 (raw file):
Previously, dhartunian (David Hartunian) wrote…
You've removed the only usage of this function so can we delete the impl as well?
Done.
pkg/sql/sqlstats/sslocal/sslocal_stats_collector.go line 141 at r1 (raw file):
Previously, dhartunian (David Hartunian) wrote…
was this the only usage of
Free? can we delete it?
Done.
There was a problem hiding this comment.
This ends up reading better as a single diff, given the rename churn, can you squash before merge and combine the commit messages? The lines changed number is not too high so I think it's fine.
Reviewable status:
pkg/sql/sqlstats/ssprovider.go line 96 at r10 (raw file):
// Clear is like Free but also prepares the container for reuse. Clear(context.Context)
Can you add a unit test for this new method? Or do we have coverage on that elsewhere?
xinhaoz
force-pushed
the
stats-collector
branch
8 times, most recently
from
076ace4
to
36a5485
Compare
|
@dhartunian Can you take another look? I added a call to |
There was a problem hiding this comment.
Reviewable status:
-- commits line 18 at r11:
I expect to see a test that verifies that the container between transactions is cleared correctly. Do we have coverage for that?
pkg/sql/conn_executor.go line 1264 at r11 (raw file):
// Free any memory used by the stats collector. ex.statsCollector.Free(ctx)
Do we need to be careful not to free the transaction container if it comes from the outer txn?
There was a problem hiding this comment.
Reviewable status:
Previously, dhartunian (David Hartunian) wrote…
I expect to see a test that verifies that the container between transactions is cleared correctly. Do we have coverage for that?
This should be covered in other tests since we'd expect to see statements associated with certain transactions absent or incorrectly associated with other txns. One test I could add to also verify this behaviour is to assert that the stats collector's local transaction container is empty after a transaction - not sure how useful that would be in this case.
pkg/sql/conn_executor.go line 1264 at r11 (raw file):
Previously, dhartunian (David Hartunian) wrote…
Do we need to be careful not to free the transaction container if it comes from the outer txn?
Good call. I added a guard within Free.
pkg/sql/sqlstats/ssprovider.go line 96 at r10 (raw file):
Previously, dhartunian (David Hartunian) wrote…
Can you add a unit test for this new method? Or do we have coverage on that elsewhere?
We have coverage for this elsewhere.
This commit cleans up the confusing swapping and usages of 2 fields on the StatsCollector, `ApplicationStats` and `flushTarget`. Originally the field `flushTarget`was only used for explicit transactions. In order to provide each statement with the correct txn fingerprint id, statements for explicit txns were stored in a temp stats container and flushed to the app container at the end of transaction execution. The assumption for implicit transactions was that they only had 1 statement and so such statements could be written directly to the parent `ApplicationStats` container rather than waiting for the entire transaction to finish. Now that we create temporary containers for both explicit and implicit txns we should reuse the temporary container between transactions instead of discarding and allocating a new one for each. Summary: - `ApplicationStats` field is on sslocal.StatsCollector is renamed to `currentTransactionStatementStats` - `FlushTarget` is always defined for a stats collector. It represents the current application's sql stats - Instead of allocating a new container for each new txn, we'll clear and reuse `currentTxnStatementStats` between transactions - StatsCollector no longer implementats the `sqlstats.ApplicationStats` interface. There isn't a need for StatsCollector to be used as an interface and doing so makes working with the various containers within StatsCollector more difficult. An exception is made for stats collectors belonging to an internal executor for an outer transaction. Such executors do not start or end the transaction, and so no temporary containers will be used in this case. Fixes: cockroachdb#94650 Release note: None
|
TFTR! |
This commit cleans up the confusing swapping and usages of 2 fields on
the StatsCollector,
ApplicationStatsandflushTarget.Originally the field
flushTargetwas only used for explicit transactions.In order to provide each statement with the correct txn fingerprint id,
statements for explicit txns were stored in a temp stats container and
flushed to the app container at the end of transaction execution.
The assumption for implicit transactions was that they only had 1
statement and so such statements could be written directly to the parent
ApplicationStatscontainer rather than waiting for the entire transactionto finish.
Now that we create temporary containers for both explicit and implicit
txns we should reuse the temporary container between transactions instead
of discarding and allocating a new one for each.
Summary:
ApplicationStatsfield is on sslocal.StatsCollector is renamedto
currentTransactionStatementStatsFlushTargetis always defined for a stats collector. It representsthe current application's sql stats
reuse
currentTxnStatementStatsbetween transactionssqlstats.ApplicationStatsinterface. There isn't a need for StatsCollector to be used as an
interface and doing so makes working with the various containers
within StatsCollector more difficult.
An exception is made for stats collectors belonging to an internal executor
for an outer transaction. Such executors do not start or end the transaction,
and so no temporary containers will be used in this case.
Fixes: #94650
Release note: None