Re: Summary function for pg_buffercache

Hi,

On 2022-09-28 18:19:57 +0300, Melih Mutlu wrote:
> diff --git a/contrib/pg_buffercache/pg_buffercache--1.3--1.4.sql b/contrib/pg_buffercache/pg_buffercache--1.3--1.4.sql
> new file mode 100644
> index 0000000000..77e250b430
> --- /dev/null
> +++ b/contrib/pg_buffercache/pg_buffercache--1.3--1.4.sql
> @@ -0,0 +1,13 @@
> +/* contrib/pg_buffercache/pg_buffercache--1.3--1.4.sql */
> +
> +-- complain if script is sourced in psql, rather than via ALTER EXTENSION
> +\echo Use "ALTER EXTENSION pg_buffercache UPDATE TO '1.4'" to load this file. \quit
> +
> +CREATE FUNCTION pg_buffercache_summary()
> +RETURNS TABLE (buffers_used int4, buffers_unused int4, buffers_dirty int4,
> + buffers_pinned int4, usagecount_avg real)
> +AS 'MODULE_PATHNAME', 'pg_buffercache_summary'
> +LANGUAGE C PARALLEL SAFE;

I think using RETURNS TABLE isn't quite right here, as it implies 'SETOF'. But
the function doesn't return a set of rows. I changed this to use OUT
parameters.

> +-- Don't want these to be available to public.
> +REVOKE ALL ON FUNCTION pg_buffercache_summary() FROM PUBLIC;

I think this needs to grant to pg_monitor too. See
pg_buffercache--1.2--1.3.sql

I added a test verifying the permissions are right, with the hope that it'll
make future contributors try to add a parallel test and notice the permissions
aren't right.

> + /* Construct a tuple descriptor for the result rows. */
> + tupledesc = CreateTemplateTupleDesc(NUM_BUFFERCACHE_SUMMARY_ELEM);

Given that we define the return type on the SQL level, it imo is nicer to use
get_call_result_type() here.

> + TupleDescInitEntry(tupledesc, (AttrNumber) 5, "usagecount_avg",
> + FLOAT4OID, -1, 0);

I changed this to FLOAT8. Not that the precision will commonly be useful, but
it doesn't seem worth having to even think about whether there are cases where
it'd matter.

I also changed it so that the accumulation happens in an int64 variable named
usagecount_total, which gets converted to a double only when actually
computing the result.

> <para>
> The <filename>pg_buffercache</filename> module provides a means for
> - examining what's happening in the shared buffer cache in real time.
> + examining what's happening in the shared buffer in real time.
> </para>

This seems to be an unnecessary / unrelated change. I suspect you made it in
response to
https://postgr.es/m/20220922161014.copbzwdl3ja4nt6z%40awork3.anarazel.de
but that was about a different sentence, where you said 'shared buffer caches'
(even though there is only a single shared buffer cache).

> <indexterm>
> @@ -17,10 +17,19 @@
> </indexterm>
>
> <para>
> - The module provides a C function <function>pg_buffercache_pages</function>
> - that returns a set of records, plus a view
> - <structname>pg_buffercache</structname> that wraps the function for
> - convenient use.
> + The module provides C functions <function>pg_buffercache_pages</function>
> + and <function>pg_buffercache_summary</function>.
> + </para>
> +
> + <para>
> + The <function>pg_buffercache_pages</function> function
> + returns a set of records, plus a view <structname>pg_buffercache</structname> that wraps the function for
> + convenient use is provided.
> + </para>

I rephrased this, because it sounds like the function returns a set of records
and a view.

> + <para>
> + The <function>pg_buffercache_summary</function> function returns a table with a single row
> + that contains summarized and aggregated information about shared buffer.
> </para>

"summarized and aggregated" is quite redundant.

> + <table id="pgbuffercachesummary-columns">
> + <title><structname>pg_buffercachesummary</structname> Columns</title>

Missing underscore.

> + <row>
> + <entry role="catalog_table_entry"><para role="column_definition">
> + <structfield>buffers_unused</structfield> <type>int4</type>
> + </para>
> + <para>
> + Number of shared buffers that not currently being used
> + </para></entry>
> + </row>

There's a missing 'are' in here, I think. I rephrased all of these to
"Number of (used|unused|dirty|pinned) shared buffers"

> + <row>
> + <entry role="catalog_table_entry"><para role="column_definition">
> + <structfield>buffers_dirty</structfield> <type>int4</type>
> + </para>
> + <para>
> + Number of dirty shared buffers
> + </para></entry>
> + </row>
> +
> + <row>
> + <entry role="catalog_table_entry"><para role="column_definition">
> + <structfield>buffers_pinned</structfield> <type>int4</type>
> + </para>
> + <para>
> + Number of shared buffers that has a pinned backend
> + </para></entry>
> + </row>

Backends pin buffers, not the other way round...

> + <para>
> + There is a single row to show summarized information of all shared buffers.
> + <function>pg_buffercache_summary</function> is not interested
> + in the state of each shared buffer, only shows aggregated information.
> + </para>
> +
> + <para>
> + The <function>pg_buffercache_summary</function> doesn't provide a result
> + that is consistent across all buffers. This is intentional. The purpose
> + of this function is to provide a general idea about the state of shared
> + buffers as fast as possible. Additionally, <function>pg_buffercache_summary</function>
> + allocates much less memory.
> + </para>

I still didn't like this comment. Please see the attached.

I intentionally put my changes into a fixup commit, in case you want to look
at the differences.

Greetings,

Andres Freund