Store query texts in temp file #620
Conversation
seanlinsley
changed the title
seanlinsley
force-pushed
the
statements-tmpfile
branch
from
e79ae35 to
9f785d1
Compare
input/postgres/statements.go
Outdated
| queryStats := make([]state.PostgresStatementStats, 0) | ||
| queryLength := make([]int, 0) | ||
| if showtext { | ||
| tmpFile, err = ioutil.TempFile("", "") |
There was a problem hiding this comment.
This function is deprecated in Go now; it just calls another function.
| tmpFile, err = ioutil.TempFile("", "") | |
| tmpFile, err = os.CreateTemp("", "") |
I'll clean up other usage, but we should avoid introducing more.
input/postgres/statements.go
Outdated
| // Currently, Aurora gives wildly incorrect blk_read_time and blk_write_time values | ||
| // for utility statements; ignore I/O timing in this situation. | ||
| if !postgresVersion.IsAwsAurora || !receivedQuery.Valid { | ||
| if !postgresVersion.IsAwsAurora || receivedQuery != "" { |
There was a problem hiding this comment.
| if !postgresVersion.IsAwsAurora || receivedQuery != "" { | |
| if !postgresVersion.IsAwsAurora || receivedQuery == "" { |
input/postgres/statements.go
Outdated
| queryLength = append(queryLength, len(receivedQuery.String)) | ||
| tmpFile.WriteString(receivedQuery.String) | ||
| } else { | ||
| if ignoreIOTiming(postgresVersion, "") { |
There was a problem hiding this comment.
As I just discussed with Sean, this is actually not doing what we intended, because it will only ignore I/O timing when showtext=true (i.e. the once every 10 minute snapshot), not in the every 1min snapshots - effectively causing those to collect the bogus I/O times for utility statements.
We should be able to address this correctly by moving this logic to the output part of the collector (where we have the text available and can cleanup the stats for a given query). At that point we may also want to check if Aurora changelog notes this bug being fixed.
For this PR we can remove this check here (since it doesn't do anything). Note this PR also changes the ignoreIOTiming for showtext=true to run outside the for loop. That's actually very good to do because it removes a pg_query parse call inside the for loop (and as such should reduce the observed runtime for this query on the database side).
input/postgres/statements.go
Outdated
| @@ -230,6 +234,18 @@ func GetStatements(ctx context.Context, server *state.Server, logger *util.Logge | |||
| statementTextsByFp := make(state.PostgresStatementTextMap) | |||
| statementStats := make(state.PostgresStatementStatsMap) | |||
|
|
|||
| queryKeys := make([]state.PostgresStatementKey, 0) | |||
| queryStats := make([]state.PostgresStatementStats, 0) | |||
| queryLength := make([]int, 0) | |||
There was a problem hiding this comment.
This would make more sense to me if its called queryLengths (i.e. with a "s" at the end)
input/postgres/statements.go
Outdated
| } | ||
|
|
||
| if err = rows.Err(); err != nil { | ||
| return nil, nil, nil, err | ||
| } | ||
|
|
||
| tmpFile.Seek(0, io.SeekStart) |
There was a problem hiding this comment.
Whilst this doesn't crash when tmpFile is not initialized (I don't understand why, but [Go Playground confirms it](tmpFile.Seek(0, io.SeekStart))), it might be best if we wrap this and the for loop below in if showtext, just so a future reader doesn't think it might be a problem.
There was a problem hiding this comment.
Ah, I think that's because the first thing Seek does is call File.checkValid, which returns an error if File is nil. I agree wrapping this in a showtext check is worthwhile, and checking the return value from Seek may be worth doing, too, to avoid surprises.
input/postgres/statements.go
Outdated
| if err != nil { | ||
| return nil, nil, nil, err | ||
| } | ||
| query := string(bytes) |
There was a problem hiding this comment.
We could technically avoid the memory copy here (see e.g. https://www.reddit.com/r/golang/comments/14xvgoj/converting_string_byte/ for a way of doing it), but not sure if its worth saving on that (maybe worth a benchmark, not necessarily right now)
#605 succeeded in reducing memory usage, but it also caused collector query time in pg_stat_statements to increase. This PR tries to solve both issues by writing query texts to a tempfile so the query can finish quickly, and then reading each query text in one-by-one to keep memory usage down.