Reset pg_stat_statements when nearly full - #775
Conversation
| err = nil | ||
| newHighFreqState.StatementStats = make(state.PostgresStatementStatsMap) | ||
| } else { | ||
| newHighFreqState.StatementStats = resetStatementStats |
There was a problem hiding this comment.
Why do we need to call GetStatementStats here? Why not always set newHighFreqState.StatementStats to an empty map?
There was a problem hiding this comment.
+1, I've also wondered about that.
There was a problem hiding this comment.
Just discussed this with Sean, the problem is that we can't just take an empty map always since that would cause followUpRun to be false here: https://github.com/pganalyze/collector/blob/main/input/full_1min.go#L68
Sean is going to investigate reworking that in a follow-up PR.
ad8bb11 to
af8c4bf
Compare
af8c4bf to
b15c072
Compare
| if resetFreq == 0 { | ||
| return | ||
| } | ||
| if lastReset.Valid && time.Since(lastReset.Time).Minutes() < float64(resetFreq*scheduler.FullSnapshotMinutes) { |
There was a problem hiding this comment.
Only running the reset when it's nearly full does introduce a risk: if users have configured pg_stat_statements.max to be a very large number, this could lead to collector OOM crashes.
Instead of renaming the setting to be minimum frequency, it could become the maximum frequency between runs (so we'd reset if that amount of time has passed). The question then is, should we hard-code a minimum frequency of 1 hour or introduce a new setting?
Another option would be to track the query text size and if it grows too large (500 MB?) proactively do a reset. That would require a new temporary field generated by GetStatementTexts to tell us how large the texts are.
Sidenote but since the setting is a frequency, I think I'm reversing the meaning of minimum and maximum. Thoughts on how to improve the naming to avoid confusion?
There was a problem hiding this comment.
Good idea re: the query text size as a secondary factor, to ensure that those cases still get reset on the schedule - I think we could even use a lower value, like 250MB. Maybe that should be configurable too? (for someone who has a very large system and wants to not reset based on size but does want to reset when deallocations happen)
From a naming perspective, maximum frequency makes most sense to me ("it doesn't happen more often than that maximum frequency").
| if setting.Name == "pg_stat_statements.max" && setting.CurrentValue.Valid { | ||
| max, err = strconv.Atoi(setting.CurrentValue.String) | ||
| if err != nil { | ||
| c.Logger.PrintError("Error parsing pg_stat_statements.max: %s", err) |
There was a problem hiding this comment.
I'd add an early return here because it seems problematic to use the default (or maybe 0 if the Atoi call resets it?)
There was a problem hiding this comment.
+ we can switch that to use the error return value instead
| } | ||
| } | ||
| } | ||
| count, err := GetStatementCount(ctx, c, db) |
There was a problem hiding this comment.
As discussed I think its better if we keep the count from the earlier pg_stat_statements query that ran (roughly around the time we gather the statement reset timestamp information) -- this query is fast, but it still takes a lock of the pg_stat_statements hash table that is currently a single global lock.
| } else if float64(count) >= 0.95*float64(max) { | ||
| c.Logger.PrintWarning("pg_stat_statements is nearly full. We recommend defining the pganalyze.reset_stat_statements helper function to avoid seeing <query text unavailable> in pganalyze") | ||
| } |
There was a problem hiding this comment.
As discussed, this was intended to warn users who do not have reset enabled yet to enable it. It makes more sense to base this off the deallocation counter (when the diff >= 1 for a full snapshot) and put this somewhere where we have access to that diff (runner?). We should also reword the message to emphasize what happened (i.e. the deallocation) vs pg_stat_statements being full, since most production systems will have 100% use of pg_stat_statements when they're not doing resets.
| if resetFreq == 0 { | ||
| return | ||
| } | ||
| if lastReset.Valid && time.Since(lastReset.Time).Minutes() < float64(resetFreq*scheduler.FullSnapshotMinutes) { |
There was a problem hiding this comment.
Good idea re: the query text size as a secondary factor, to ensure that those cases still get reset on the schedule - I think we could even use a lower value, like 250MB. Maybe that should be configurable too? (for someone who has a very large system and wants to not reset based on size but does want to reset when deallocations happen)
From a naming perspective, maximum frequency makes most sense to me ("it doesn't happen more often than that maximum frequency").
| } | ||
|
|
||
| // Reset pg_stat_statements if it's nearly full | ||
| server.HighFreqStateMutex.Lock() |
There was a problem hiding this comment.
As discussed we should make use of that mutex dependent on the resets actually being configured.
789d590 to
2dcb0b4
Compare
msakrejda
left a comment
There was a problem hiding this comment.
Left some minor comments, but looks good to me.
| max := 5_000 | ||
| for _, setting := range ts.Settings { | ||
| if setting.Name == "pg_stat_statements.max" && setting.CurrentValue.Valid { | ||
| max, err = strconv.Atoi(setting.CurrentValue.String) | ||
| if err != nil { | ||
| return | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Do we want to fall back to 5,000 (rather than error out) if we don't find the setting? It should rarely happen, so it's not a big deal either way, but this behavior might be harder to track down if for some reason the setting can't be read.
There was a problem hiding this comment.
Assuming I read your comment right, I agree - we should error out if we can't read the setting, since we can't be sure what the actual value is. It should be rare in practice, but could happen on platforms that don't grant pg_read_all_settings to the pganalyze user (maybe Heroku or Aiven?).
| int32 statement_timeout_ms_query_text = 3; | ||
| // The maximum size (in MB) that pg_stat_statements query text can grow to before | ||
| // triggering a reset. If statement_reset_frequency = 0, the reset won't occur. | ||
| int32 statement_max_size = 4; |
There was a problem hiding this comment.
Maybe
| int32 statement_max_size = 4; | |
| int32 statement_max_size_mb = 4; |
paralleling how we bake units into the name in statement_timeout_ms?
There was a problem hiding this comment.
How about statement_reset_max_mb?
|
|
||
| shouldReset, err := postgres.ShouldResetStatements(server, &ps, &ts, statementSize) | ||
| if err != nil { | ||
| logger.PrintError("Error checking if should reset statements: %s", err) |
There was a problem hiding this comment.
Should we explicitly log that we're skipping the reset in this case. It's the logical conclusion, but the statement is technically a little ambiguous.
There was a problem hiding this comment.
| logger.PrintError("Error checking if should reset statements: %s", err) | |
| logger.PrintError("Failed to determine if reset of pg_stat_statements needed, skipping reset: %s", err) |
| err = nil | ||
| newHighFreqState.StatementStats = make(state.PostgresStatementStatsMap) | ||
| } else { | ||
| newHighFreqState.StatementStats = resetStatementStats |
There was a problem hiding this comment.
+1, I've also wondered about that.
| func SendFull(ctx context.Context, server *state.Server, collectionOpts state.CollectionOpts, logger *util.Logger, newState state.PersistedState, diffState state.DiffState, transientState state.TransientState, collectedIntervalSecs uint32) error { | ||
| s := transform.StateToSnapshot(newState, diffState, transientState, server) | ||
| if s.ServerStatistic.PgStatStatementsDealloc > 0 { | ||
| logger.PrintWarning("pg_stat_statements deallocation detected. We recommend enabling automatic resets on the pganalyze server settings page to avoid <query text unavailable>") |
There was a problem hiding this comment.
| logger.PrintWarning("pg_stat_statements deallocation detected. We recommend enabling automatic resets on the pganalyze server settings page to avoid <query text unavailable>") | |
| logger.PrintWarning("Detected %d pg_stat_statements deallocations in the last %d minutes. Enable/adjust automatic reset settings in pganalyze to avoid <query text unavailable>", s.ServerStatistic.PgStatStatementsDealloc, scheduler.FullSnapshotMinutes) |
|
|
||
| shouldReset, err := postgres.ShouldResetStatements(server, &ps, &ts, statementSize) | ||
| if err != nil { | ||
| logger.PrintError("Error checking if should reset statements: %s", err) |
There was a problem hiding this comment.
| logger.PrintError("Error checking if should reset statements: %s", err) | |
| logger.PrintError("Failed to determine if reset of pg_stat_statements needed, skipping reset: %s", err) |
| return err | ||
| } | ||
| return nil | ||
| c.Logger.PrintInfo("Resetting pg_stat_statements") |
There was a problem hiding this comment.
| c.Logger.PrintInfo("Resetting pg_stat_statements") | |
| c.Logger.PrintInfo("Executing pg_stat_statements_reset() for all queries") |
There was a problem hiding this comment.
Or move it back to the top-level function and reword slightly to make it after the fact (like it was before).
| count := len(ts.Statements) | ||
| max := 5_000 |
There was a problem hiding this comment.
| count := len(ts.Statements) | |
| max := 5_000 | |
| entryCount := len(ts.Statements) | |
| entryMax := 5_000 |
There was a problem hiding this comment.
(nice to have, not sure its needed)
| if setting.Name == "pg_stat_statements.max" && setting.CurrentValue.Valid { | ||
| max, err = strconv.Atoi(setting.CurrentValue.String) |
There was a problem hiding this comment.
I think we could separate this out into a helper (but we can do that in a follow-up PR) - also worth noting that you can use ResetValue and CurrentValue interchangeably here, since this can only be set at server start.
| max := 5_000 | ||
| for _, setting := range ts.Settings { | ||
| if setting.Name == "pg_stat_statements.max" && setting.CurrentValue.Valid { | ||
| max, err = strconv.Atoi(setting.CurrentValue.String) | ||
| if err != nil { | ||
| return | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Assuming I read your comment right, I agree - we should error out if we can't read the setting, since we can't be sure what the actual value is. It should be rare in practice, but could happen on platforms that don't grant pg_read_all_settings to the pganalyze user (maybe Heroku or Aiven?).
| } | ||
| } | ||
| } | ||
| timeElapsed := resetFreq > 0 && time.Since(lastReset.Time).Minutes() >= float64(resetFreq) |
There was a problem hiding this comment.
Just to confirm what I checked: Minutes() will convert the hour/etc portion to minutes, so this will work as expected. See https://pkg.go.dev/time#Duration.Minutes
There was a problem hiding this comment.
Also maybe resetAllowed is a more clear name?
| } | ||
| } | ||
| timeElapsed := resetFreq > 0 && time.Since(lastReset.Time).Minutes() >= float64(resetFreq) | ||
| tooMany := float64(count) >= 0.9*float64(max) |
There was a problem hiding this comment.
We could put the 0.9 into a constant just before the function so it jumps out more that this the thershold.
The collector can optionally reset (empty) pg_stat_statements on a schedule, but a static schedule isn't effective when a database has irregular query activity, leading to
<query text unavailable>to show up in pganalyze during periods of time when queries that Postgres fails to fingerprint accurately have a high call count.That's addressed here by proactively resetting pg_stat_statements when it's nearly full instead of waiting for the schedule. The existing schedule setting is reused as a minimum frequency that the reset can be run, in case users want to avoid resetting too often.
Note: Postgres handles sizing down pg_stat_statements (called deallocation) by removing the entries with the fewest calls. When Postgres fails to normalize a query, so many entries exist for the same pganalyze fingerprint, they often have been called just once so are deallocated first. This results in the collector not being able to capture the query text and fingerprint it before it's removed from pg_stat_statements.