sqlstats: reuse the temporary statement stats container
sqlstats: remove ApplicationStats interface impl from StatsCollector
Previously, StatsCollector had an embedded sqlstats.ApplicationStats
interface. There isn't a need for StatsCollector to exist as this
interface over using its concrete type. It also makes working with
the various containers within StatsCollector more difficult.
Epic: none
Release note: None
sqlstats: reuse the temporary statement stats container
This commit cleans up the confusing swapping and uses of the
2 fields on the StatsCollector, ApplicationStats and
flushTarget.
The original purpose of flushTarget seems to have been for
explicit transactions, where we would need to use a temp container
to record statement stats and flush them to the app container at
the end of transaction execution in order to correlate the txn
fingerprint id. The assumption then was that implicit transactions
have 1 statement, thus we'd only need to use a flush target for
explicit transactions - for implicit txns we could get away with
using the ApplicationStats field directly.
Before: If implict txn:
- do nothing, stats will be recorded to
ApplicationStatsdirectly
If explicit txn:
- set
flushTargetasApplicationStats, allocate temp container forApplicationStatswhere the txn's statements will be recorded - on txn finish, flush
ApplicationStatstoflushTarget, setApplicationStatstoflushTarget.
Eventually we corrected that assumption and are now separating the current transaction's statements and the flushTarget for every execution. There's no need to perform the little dance above, and we're likely not mmanaging our statement stats container as efficiently as we could be.
Now:
-
FlushTargetis always defined for a stats collector. It represents the current application's sql stats -
ApplicationStatsis renamed toactiveTxnStatementStatsto reflect that it is use to store the current transaction's statement stats - Instead of allocating a new container for the current transaction
each time, we'll clear and reuse
activeTxnStatementStats
Fixes: https://github.com/cockroachdb/cockroach/issues/94650
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?
:owl: Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.
@dhartunian Can you take another look? I added a call to Free in the conn exec cleanup and had to add an additional flag on the stats collector setup for a special ase internal statement. This additional allows us to preserve current behaviour when it comes to recording stats for internal statements from outer txns.
TFTR! bors r+