Track Postgres buffer cache usage #633

seanlinsley · 2024-11-13T23:08:55Z

This PR adds Postgres buffer cache tracking on a per-table/index basis.

Since querying pg_buffercache is slow and grows slower with larger buffer cache sizes, by default we will not collect buffer cache stats for a database with more than 200 GB of shared_buffers. This can be configured with the max_buffer_cache_monitoring_gb setting, which accepts an integer number of gigabytes to use as the threshold.

seanlinsley · 2024-11-13T23:09:18Z

input/full.go

+	select {
+	case <-ctx.Done():
+	case ts.BufferCache = <-bufferCacheReady:
+	}


Since querying pg_buffercache can be slow (~12 seconds for 200 GB of shared buffers), the query is performed in a separate goroutine so full snapshot collection can proceed without blocking on the buffer cache data, until it's needed.

Does this mean that there can be num_of_servers extra connections happening concurrently? Could this be a problem with the pganalyze user connection limit?

Yes, this will run an additional database connection concurrently when generating a full snapshot, so we could run into the connection limit. But I don't think that's likely to happen because by default we have a limit of 10 connections per server.

But I don't think that's likely to happen because by default we have a limit of 10 connections per server.

Ah true, it's per server, not per collector - yeah in that case, it's at most one additional connection per server, so it's gonna be okay 👍

I had another question around here: you say "until it's needed", and it's true - if the full snapshot finishes running (outputting buffer cache) before this ts.BufferCache = <-bufferCacheReady, it'll output the data of 10 mins ago, correct? IOW, full snapshot with a timely buffer cache info can be obtained only when this goroutine finishes before the "outputting buffer cache" part is done.
I think it's better than making a full snapshot runs longer (so I'm not opposed to it), but just wanted to double check if my understanding is correct.

if the full snapshot finishes running (outputting buffer cache) before this ts.BufferCache = <-bufferCacheReady, it'll output the data of 10 mins ago, correct?

No, the code is blocking on the message from that channel. It will wait for the new buffer cache stats to be available. For our production server that has 198 GB of shared buffers, querying pg_buffercache takes 12 seconds. I think that's short enough to not be an issue.

Should we log the collection duration at Verbose level? Users could check pg_stat_statements instead, but it might be convenient to have this here for debugging.

seanlinsley · 2024-11-13T23:11:16Z

input/postgres/buffer_cache.go

+FROM pg_buffercache
+GROUP BY 1, 2`
+
+func GetBufferCache(ctx context.Context, server *state.Server, globalCollectionOpts state.CollectionOpts, logger *util.Logger, postgresVersion state.PostgresVersion, channel chan state.BufferCache) {


Currently this doesn't perform a Postgres version check or include a way to disable the functionality. Thoughts?

You could technically disable by setting MaxBufferCacheMonitoringGB to zero. Re: version check, what's in your mind?

I believe Lukas had mentioned that we won't want to run this for some older Postgres versions, though I don't know the details.

Checking the release notes for all the X.0 major versions we support (I don't think a performance improvement like this would be released in a minor version), I don't see anything about pg_buffercache except in 16, with the summary function. Going through the Postgres commit history, I see

commit 6e654546fb61f62cc982d0c8f62241b3b30e7ef8 Author: Heikki Linnakangas <[email protected]> Date: Thu Sep 29 13:16:30 2016 +0300 Don't bother to lock bufmgr partitions in pg_buffercache. That makes the view a lot less disruptive to use on a production system. Without the locks, you don't get a consistent snapshot across all buffers, but that's OK. It wasn't a very useful guarantee in practice. Ivan Kartyshov, reviewed by Tomas Vondra and Robert Haas. Discusssion: <[email protected]>

which I think might be what Lukas was talking about (I recall locking being mentioned). Running git tag --contains 6e654546fb61f62cc982d0c8f62241b3b30e7ef8 confirms this, showing all versions back to 10.0 including this. Going back one more version in the release notes and checking the release notes for 10 backs that up, noting the reduced locking requirement. So I don't think we need a version check, since we no longer support any versions that would have performance problems.

I agree with Keiko that setting the max to zero is probably sufficient to disable the feature.

output/transform/merge_partition_sizes.go

seanlinsley · 2024-11-13T23:15:52Z

output/transform/postgres_buffer_cache.go

+				untrackedBytes += bytes
+				continue
+			}
+			s.DatabaseStatictics[databaseIdx].UntrackedCacheBytes = untrackedBytes


This allows us to track buffer cache usage from untracked databases and tables.

Note: input/postgres/relations.go calls bufferCache[dataFilenode] = 0 to zero out buffer cache entries, so that the code here only tracks buffer usage not associated with a known table/index.

seanlinsley · 2024-11-14T20:43:46Z

config/config.go

@@ -212,6 +212,10 @@ type ServerConfig struct {
 	// once the server is promoted
 	SkipIfReplica bool `ini:"skip_if_replica"`

+	// The maximum shared_buffers size in gigabytes that the collector will monitor
+	// pg_buffercache. Defaults to 200 GB.
+	MaxBufferCacheMonitoringGB int `ini:"max_buffer_cache_monitoring_gb"`


Thoughts on the naming of this setting? It's difficult to find a name that's descriptive without being too long.

It reads okay to me

Yeah, seems okay.

keiko713 · 2024-11-15T01:09:20Z

config/config.go

@@ -212,6 +212,10 @@ type ServerConfig struct {
 	// once the server is promoted
 	SkipIfReplica bool `ini:"skip_if_replica"`

+	// The maximum shared_buffers size in gigabytes that the collector will monitor
+	// pg_buffercache. Defaults to 200 GB.
+	MaxBufferCacheMonitoringGB int `ini:"max_buffer_cache_monitoring_gb"`


It reads okay to me

keiko713 · 2024-11-15T01:12:54Z

input/postgres/buffer_cache.go

+FROM pg_buffercache
+GROUP BY 1, 2`
+
+func GetBufferCache(ctx context.Context, server *state.Server, globalCollectionOpts state.CollectionOpts, logger *util.Logger, postgresVersion state.PostgresVersion, channel chan state.BufferCache) {


You could technically disable by setting MaxBufferCacheMonitoringGB to zero. Re: version check, what's in your mind?

keiko713 · 2024-11-15T01:15:19Z

input/full.go

+	select {
+	case <-ctx.Done():
+	case ts.BufferCache = <-bufferCacheReady:
+	}


Does this mean that there can be num_of_servers extra connections happening concurrently? Could this be a problem with the pganalyze user connection limit?

input/postgres/relation_stats.go

keiko713 · 2024-11-15T04:51:11Z

input/postgres/relations.go

+			row.CachedDataBytes = bufferCache[dataFilenode]
+			row.CachedToastBytes = bufferCache[toastFilenode]
+			bufferCache[dataFilenode] = 0
+			bufferCache[toastFilenode] = 0


I'm not understanding the logic here well for why you're setting zero here (and below). Can you please explain?

Ah nvm, you explained it in https://github.com/pganalyze/collector/pull/633/files#r1841309323

Maybe that comment should be here instead? And a code comment rather than just a PR comment? I like the approach, but I, too, was confused on first reading this.

output/transform/postgres_relations.go

…nitoring for very large servers

msakrejda · 2024-11-18T17:22:44Z

config/config.go

@@ -212,6 +212,10 @@ type ServerConfig struct {
 	// once the server is promoted
 	SkipIfReplica bool `ini:"skip_if_replica"`

+	// The maximum shared_buffers size in gigabytes that the collector will monitor
+	// pg_buffercache. Defaults to 200 GB.
+	MaxBufferCacheMonitoringGB int `ini:"max_buffer_cache_monitoring_gb"`


Yeah, seems okay.

msakrejda · 2024-11-18T17:28:53Z

input/full.go

+	select {
+	case <-ctx.Done():
+	case ts.BufferCache = <-bufferCacheReady:
+	}


Should we log the collection duration at Verbose level? Users could check pg_stat_statements instead, but it might be convenient to have this here for debugging.

msakrejda · 2024-11-18T17:43:07Z

input/postgres/buffer_cache.go

+	sizeGB := 0
+	db.QueryRowContext(ctx, QueryMarkerSQL+bufferCacheSizeSQL).Scan(&sizeGB)
+	if sizeGB > server.Config.MaxBufferCacheMonitoringGB {
+		logger.PrintWarning("GetBufferCache: skipping collection. To enable, set max_buffer_cache_monitoring_gb to a value over %d", sizeGB)


Do we want this printed for every full snapshot? I think there are legitimate cases where you want the extension to be present (for manual checks), but don't want the performance hit. Maybe we should only log this for a test run?

msakrejda · 2024-11-18T17:52:06Z

input/postgres/buffer_cache.go

+FROM pg_buffercache
+GROUP BY 1, 2`
+
+func GetBufferCache(ctx context.Context, server *state.Server, globalCollectionOpts state.CollectionOpts, logger *util.Logger, postgresVersion state.PostgresVersion, channel chan state.BufferCache) {


Checking the release notes for all the X.0 major versions we support (I don't think a performance improvement like this would be released in a minor version), I don't see anything about pg_buffercache except in 16, with the summary function. Going through the Postgres commit history, I see

commit 6e654546fb61f62cc982d0c8f62241b3b30e7ef8 Author: Heikki Linnakangas <[email protected]> Date: Thu Sep 29 13:16:30 2016 +0300 Don't bother to lock bufmgr partitions in pg_buffercache. That makes the view a lot less disruptive to use on a production system. Without the locks, you don't get a consistent snapshot across all buffers, but that's OK. It wasn't a very useful guarantee in practice. Ivan Kartyshov, reviewed by Tomas Vondra and Robert Haas. Discusssion: <[email protected]>

which I think might be what Lukas was talking about (I recall locking being mentioned). Running git tag --contains 6e654546fb61f62cc982d0c8f62241b3b30e7ef8 confirms this, showing all versions back to 10.0 including this. Going back one more version in the release notes and checking the release notes for 10 backs that up, noting the reduced locking requirement. So I don't think we need a version check, since we no longer support any versions that would have performance problems.

I agree with Keiko that setting the max to zero is probably sufficient to disable the feature.

msakrejda · 2024-11-18T17:55:31Z

input/postgres/buffer_cache.go

+// See also https://www.postgresql.org/docs/current/pgbuffercache.html
+const bufferCacheSQL string = `
+SELECT reldatabase, relfilenode, count(*) * current_setting('block_size')::int
+FROM pg_buffercache


This could be installed in a schema other than public, or a schema that's not in the search path.

Separately, here and in other SQL in this PR, we should schema-qualify references to system tables and functions with pg_catalog, like we do elsewhere.

msakrejda · 2024-11-18T18:04:01Z

input/postgres/relations.go

+				COALESCE(toast.relname, '') AS toast_table,
+				coalesce(pg_relation_filenode(c.oid), 0) AS data_filenode,
+				coalesce(pg_relation_filenode(c.reltoastrelid), 0) AS toast_filenode


Suggested change

COALESCE(toast.relname, '') AS toast_table,

coalesce(pg_relation_filenode(c.oid), 0) AS data_filenode,

coalesce(pg_relation_filenode(c.reltoastrelid), 0) AS toast_filenode

COALESCE(toast.relname, '') AS toast_table,

COALESCE(pg_relation_filenode(c.oid), 0) AS data_filenode,

COALESCE(pg_relation_filenode(c.reltoastrelid), 0) AS toast_filenode

Or the other way around (assuming you change other occurrences here). I don't especially care, but the inconsistency hurts readability.

msakrejda · 2024-11-18T18:07:13Z

input/postgres/relations.go

+			row.CachedDataBytes = bufferCache[dataFilenode]
+			row.CachedToastBytes = bufferCache[toastFilenode]
+			bufferCache[dataFilenode] = 0
+			bufferCache[toastFilenode] = 0


Maybe that comment should be here instead? And a code comment rather than just a PR comment? I like the approach, but I, too, was confused on first reading this.

msakrejda · 2024-11-18T22:17:35Z

input/postgres/buffer_cache.go

-GROUP BY 1, 2`
+const bufferCacheExtensionSQL string = `
+SELECT COALESCE((
+	SELECT nspname


We should probably be calling quote_literal(nspname) here, to make sure we support extensions that are installed in schemas that require quoting (e.g., "this schema has spaces"), but I noticed that our handling of the pg_stat_statements schema doesn't do this either, and I don't think we've heard complaints, so we can leave this and fix both in a follow-up patch instead.

msakrejda

Great, looks good to me.

keiko713

Looks like Maciek dropped a few more feedback but otherwise, looks good to me too!
(also nice reviews, Maciek. +1 on all points you made)

seanlinsley force-pushed the 4388-buffer-cache branch from 00eef44 to 47d6489 Compare November 13, 2024 23:23

seanlinsley requested a review from a team November 13, 2024 23:36

seanlinsley commented Nov 13, 2024

View reviewed changes

Track Postgres buffer cache usage

b6aca62

seanlinsley force-pushed the 4388-buffer-cache branch from 47d6489 to 0ff0356 Compare November 14, 2024 18:21

seanlinsley commented Nov 14, 2024

View reviewed changes

keiko713 reviewed Nov 15, 2024

View reviewed changes

Add max_buffer_cache_monitoring_gb setting to disable buffer cache mo…

0ffac89

…nitoring for very large servers

seanlinsley force-pushed the 4388-buffer-cache branch from 0ff0356 to 0ffac89 Compare November 18, 2024 04:30

seanlinsley mentioned this pull request Nov 18, 2024

Aggregate stats for partition tables #635

Merged

msakrejda reviewed Nov 18, 2024

View reviewed changes

Address review feedback

8e9df3f

seanlinsley requested review from keiko713 and msakrejda November 18, 2024 19:01

msakrejda reviewed Nov 18, 2024

View reviewed changes

msakrejda approved these changes Nov 18, 2024

View reviewed changes

keiko713 approved these changes Nov 19, 2024

View reviewed changes

seanlinsley merged commit 867e346 into main Nov 19, 2024
3 checks passed

seanlinsley deleted the 4388-buffer-cache branch November 19, 2024 18:56

This was referenced Nov 19, 2024

Exclude unused pages from buffer cache monitoring #636

Merged

Release 0.63.0 #638

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Track Postgres buffer cache usage #633

Track Postgres buffer cache usage #633

seanlinsley commented Nov 13, 2024 •

edited

Loading

seanlinsley Nov 13, 2024

keiko713 Nov 15, 2024

seanlinsley Nov 15, 2024

keiko713 Nov 15, 2024

seanlinsley Nov 15, 2024

msakrejda Nov 18, 2024

seanlinsley Nov 13, 2024

keiko713 Nov 15, 2024

seanlinsley Nov 15, 2024

msakrejda Nov 18, 2024

seanlinsley Nov 13, 2024 •

edited

Loading

seanlinsley Nov 14, 2024

keiko713 Nov 15, 2024

msakrejda Nov 18, 2024

keiko713 Nov 15, 2024

keiko713 Nov 15, 2024

keiko713 Nov 15, 2024

keiko713 Nov 15, 2024

keiko713 Nov 15, 2024

msakrejda Nov 18, 2024

msakrejda Nov 18, 2024

msakrejda Nov 18, 2024

msakrejda Nov 18, 2024

msakrejda Nov 18, 2024

msakrejda Nov 18, 2024

msakrejda Nov 18, 2024

msakrejda Nov 18, 2024

msakrejda Nov 18, 2024

msakrejda Nov 18, 2024

msakrejda left a comment

keiko713 left a comment

Track Postgres buffer cache usage #633

Track Postgres buffer cache usage #633

Conversation

seanlinsley commented Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlinsley Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msakrejda left a comment

Choose a reason for hiding this comment

keiko713 left a comment

Choose a reason for hiding this comment

seanlinsley commented Nov 13, 2024 •

edited

Loading

seanlinsley Nov 13, 2024 •

edited

Loading