sqlstats: reuse stats container used by current txn and stmts #94650
Assignees
Labels
A-sql-observability
Related to observability of the SQL layer
C-bug
Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
T-observability
Comments
xinhaoz
added
C-bug
|
Cool, I think these allocations are some of the ones reported in #89918. |
This was referenced Aug 3, 2023
xinhaoz
added a commit
to xinhaoz/cockroach
that referenced
this issue
May 16, 2024
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 transacion'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: cockroachdb#94650
xinhaoz
added a commit
to xinhaoz/cockroach
that referenced
this issue
May 20, 2024
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 transacion'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: cockroachdb#94650
xinhaoz
added a commit
to xinhaoz/cockroach
that referenced
this issue
May 21, 2024
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 transacion's statements and the flushTarget for every execution. There's no need to perform the little dance above, and we're likely not managing 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: cockroachdb#94650
xinhaoz
added a commit
to xinhaoz/cockroach
that referenced
this issue
May 23, 2024
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 transacion's statements and the flushTarget for every execution. There's no need to perform the little dance above, and we're likely not managing 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: cockroachdb#94650
xinhaoz
added a commit
to xinhaoz/cockroach
that referenced
this issue
Jun 3, 2024
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
craig bot
pushed a commit
that referenced
this issue
Jun 4, 2024
123698: sqlstats: reuse the temporary statement stats container r=xinhaoz a=xinhaoz 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: #94650 Release note: None Co-authored-by: Xin Hao Zhang <[email protected]>
Dhruv-Sachdev1313
pushed a commit
to Dhruv-Sachdev1313/cockroach
that referenced
this issue
Jun 7, 2024
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Currently, the stats collector for a session allocates a new stats container for each running txn. Prior to #83616, we created a txn-specific stats container only for explicit txns. Now that we all txns use their own container, these allocations are likely causing this regression in kv95.
We should just reuse the container between txns, resetting the appropriate fields on txn start, or introduce a Container pool.
Jira issue: CRDB-23065
Epic CRDB-37544
The text was updated successfully, but these errors were encountered: