cockroach icon indicating copy to clipboard operation
cockroach copied to clipboard

sqlstats: reuse the temporary statement stats container

Open xinhaoz opened this issue 1 year ago • 2 comments

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 ApplicationStats directly

If explicit txn:

  • set flushTarget as ApplicationStats, allocate temp container for ApplicationStats where the txn's statements will be recorded
  • on txn finish, flush ApplicationStats to flushTarget, set ApplicationStats to flushTarget.

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:

  • FlushTarget is always defined for a stats collector. It represents the current application's sql stats
  • ApplicationStats is renamed to activeTxnStatementStats to 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

xinhaoz avatar May 06 '24 21:05 xinhaoz

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.

blathers-crl[bot] avatar May 06 '24 21:05 blathers-crl[bot]

This change is Reviewable

cockroach-teamcity avatar May 06 '24 21:05 cockroach-teamcity

@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.

xinhaoz avatar Jun 03 '24 14:06 xinhaoz

TFTR! bors r+

xinhaoz avatar Jun 04 '24 18:06 xinhaoz