sqlstats: set default value of sql.stats.flush.minimum_interval to 10m #123466
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
xinhaoz
requested review from
nkodali,
dhartunian and
abarganier
and removed request for
a team
dhartunian
added
backport-24.1.x
dhartunian
reviewed
May 2, 2024
| SELECT count(*) | ||
| FROM system.transaction_statistics | ||
| WHERE app_name = 'min_flush_test' | ||
| `, [][]string{{"1"}}) | ||
| } |
There was a problem hiding this comment.
try using flushes skipped metric to check here.
| query += `,1` | ||
| } | ||
| sqlConn.Exec(t, query) | ||
| } | ||
|
|
||
| observerConn.CheckQueryResults(t, ` | ||
| SELECT count(*) | ||
| FROM crdb_internal.statement_statistics |
There was a problem hiding this comment.
check persisted table here
This commit sets the minimum flush interval for sql stats to be equivalent to the default flush interval, 10m. This ensures that the flush will not be allowed to trigger consecutively when there is constant memory pressure (ex. due to hitting in-memory fingerprint limit) on the sql stats system. We want to prevent the flush from constantly running in the BG since it can use up resources. Fixes: cockroachdb#123091 Epic: CRDB-37544 Release note: None
|
As per discussion, opened #123567 with just the CS change. I'll modify this PR as a follow-up to that. |
xinhaoz
changed the title
xinhaoz
removed
the
backport-24.1.0-rc
This commit moves the check previously in `MaybeFlush` that aborts the flush if not enough time has passed since the last flush operation began to the sql stats flush loop. `MaybeFlush` should be called by other functions to trigger a flush of sql stats immediately (e.g. on server drain). The minumum flush interval is only relevant to the sql stats flush worker. Other checks, such as aborting due to flush being disabled or due to the maximum number of rows being reached, remain with `MaybeFlush`. Epic: none Part of: cockroachdb#123091 Release note: None
This commit adds the metric `sql.stats.flushes.skipped.count` which tracks the number of flushes skipped (triggered but not started). Epic: none Part of : cockroachdb#123091 Release note (ops change): New metric, `sql.stats.flushes.skipped.count` tracks the number of sql stats flushes that were skipped either due to not meeting the minimum flush duration or the system tables being full.
xinhaoz
changed the title
xinhaoz
changed the title
|
@dhartunian Reviving this PR and closed the one linked above. I omited the small portion that was technically a bug fix, but I think all these other changes can be batched together. Thoughts? |
dhartunian
approved these changes
May 6, 2024
There was a problem hiding this comment.
Reviewed 1 of 2 files at r3, 7 of 7 files at r4, 2 of 3 files at r5.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @abarganier, @nkodali, and @xinhaoz)
pkg/sql/sqlstats/persistedsqlstats/flush_test.go line 376 at r5 (raw file):
testutils.SucceedsSoon(t, func() error { if srv.SQLServer().(*sql.Server).ServerMetrics.StatsMetrics.SQLStatsFlushesSkipped.Count() < 0 {
can you add confirmation prior to the execution of above statements, that skipped count is zero?
pkg/sql/sqlstats/persistedsqlstats/flush_test.go line 402 at r5 (raw file):
srv, conn, _ := serverutils.StartServer(t, base.TestServerArgs{ Knobs: base.TestingKnobs{ SQLStatsKnobs: sqlstats.CreateTestingKnobs(),
does this do anything?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
sqlstats: cleanup flush, move min flush interval check
This commit moves the check previously in
MaybeFlushthataborts the flush if not enough time has passed since the
last flush operation began to the sql stats flush loop.
MaybeFlushshould be called by other functions to triggera flush of sql stats immediately (e.g. on server drain). The
minumum flush interval is only relevant to the sql stats flush
worker. Other checks, such as aborting due to flush being
disabled or due to the maximum number of rows being reached,
remain with
MaybeFlush.Epic: none
Part of: #123091
Release note: None
sqlstats: add metric tracking skipped sql stats flushes
This commit adds the metric
sql.stats.flushes.skipped.countwhich tracks the number of flushes skipped (triggered but
not started).
Epic: none
Part of : #123091
Release note (ops change): New metric,
sql.stats.flushes.skipped.counttracks the number of sql stats flushes that were skipped either due to
not meeting the minimum flush duration or the system tables being full.
sqlstats: set default value of sql.stats.flush.minimum_interval to 10m
This commit sets the minimum flush interval for sql stats to be
equivalent to the default flush interval, 10m. This ensures that
the flush will not be allowed to trigger consecutively when there
is constant memory pressure (ex. due to hitting in-memory fingerprint
limit) on the sql stats system. We want to prevent the flush from
constantly running in the BG since it can use up resources.
Fixes: #123091
Epic: CRDB-37544