Hi Masahiko-san, Paul and Khoa,
Thanks for the review!
On Tue, Mar 24, 2026 at 12:09 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> ---
> - pg_buffercache--1.5--1.6.sql pg_buffercache--1.6--1.7.sql
> + pg_buffercache--1.5--1.6.sql pg_buffercache--1.6--1.7.sql \
> + pg_buffercache--1.7--1.8.sql
>
> Since commit 4b203d499c6 bumped the version from 1.6 to 1.7 last
> November, we think we don't need to bump the version again for this new
> feature.
Makes sense, adjusted.
> Can we move these typedefs above function prototypes as other typedefs
> are defined there?
Makes sense, done.
> + relstats_hash = hash_create("pg_buffercache relation stats",
> + 128,
> + &hash_ctl,
> + HASH_ELEM | HASH_BLOBS | HASH_CONTEXT);
>
> It might be worth considering simplehash.h for even better performance.
Good point, adjusted.
> + while ((entry = (BufferRelStatsEntry *) hash_seq_search(&hash_seq)) != NULL)
> + {
> + if (entry->buffers == 0)
> + continue;
> +
>
> We might want to put CHECK_FOR_INTERRUPTS() here too as the number of
> entries can be as many as NBuffers in principle.
Sure, that makes sense.
> We've discussed there might be room for improvement in the function
> name. For example, pg_buffercache_relations instead of
> pg_buffercache_relation_stats might be a good name, since everything
> in this module
> is stats. if we drop "_stats" then "relation" should be plural, to
> match other functions in the module ("pages", "os_pages",
> "numa_pages", "usage_counts").
I've renamed this to "pg_buffercache_relations", though per Bertrand's
earlier email, I could also see that it makes sense to incorporate the
fact more clearly that we're returning physical relfilenodes, not
logical relations.
See attached v2 that incorporates the review feedback.
Thank you all for reviewing!
Thanks,
Lukas
--
Lukas Fittl
