Thread: Summary function for pg_buffercache

Hi Melih,

> I would appreciate any feedback/comment on this change.

Another benefit of pg_buffercache_summary() you didn't mention is that
it allocates much less memory than pg_buffercache_pages() does.

Here is v3 where I added this to the documentation. The patch didn't
apply to the current master branch with the following error:

```
pg_buffercache_pages.c:286:19: error: no member named 'rlocator' in
'struct buftag'
                if (bufHdr->tag.rlocator.relNumber != InvalidOid)
                    ~~~~~~~~~~~ ^
1 error generated.
```

I fixed this too. Additionally, the patch was pgindent'ed and some
typos were fixed.

However I'm afraid you can't examine BufferDesc's without taking
locks. This is explicitly stated in buf_internals.h:

"""
Buffer header lock (BM_LOCKED flag) must be held to EXAMINE or change
TAG, state or wait_backend_pgprocno fields.
"""

Let's consider this code again (this is after my fix):

```
if (RelFileNumberIsValid(BufTagGetRelNumber(bufHdr))) {
 /* ... */
}
```

When somebody modifies relNumber concurrently (e.g. calls
ClearBufferTag()) this will cause an undefined behaviour.

I suggest we focus on saving the memory first and then think about the
performance, if necessary.

-- 
Best regards,
Aleksander Alekseev

On Fri, Sep 09, 2022 at 05:36:45PM +0300, Aleksander Alekseev wrote:
> However I'm afraid you can't examine BufferDesc's without taking
> locks. This is explicitly stated in buf_internals.h:

Yeah, when I glanced at this patch earlier, I wondered about this.

> I suggest we focus on saving the memory first and then think about the
> performance, if necessary.

+1

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Hi hackers,

> > I suggest we focus on saving the memory first and then think about the
> > performance, if necessary.
>
> +1

I made a mistake in v3 cfbot complained about. It should have been:

```
if (RelFileNumberIsValid(BufTagGetRelNumber(&bufHdr->tag)))
```

Here is the corrected patch.

-- 
Best regards,
Aleksander Alekseev

Hi,

On 2022-09-09 17:36:45 +0300, Aleksander Alekseev wrote:
> I suggest we focus on saving the memory first and then think about the
> performance, if necessary.

Personally I think the locks part is at least as important - it's what makes
the production impact higher.

Greetings,

Andres Freund

Hi Melih,

> I changed these names and updated the patch.

Thanks for the updated patch!

> Aleksander, do you still think the average usagecount is a bit useless? Or does it make sense to you to keep it like
this?

I don't mind keeping the average.

> I'm not sure how to avoid any undefined behaviour without locks though.
> Even with locks, performance is much better. But is it good enough for production?

Potentially you could avoid taking locks by utilizing atomic
operations and lock-free algorithms. But these algorithms are
typically error-prone and not always produce a faster code than the
lock-based ones. I'm pretty confident this is out of scope of this
particular patch.

The patch v6 had several defacts:

* Trailing whitespaces (can be checked by applying the patch with `git am`)
* Wrong code formatting (can be fixed with pgindent)
* Several empty lines were removed which is not related to the
proposed change (can be seen with `git diff`)
* An unlikely division by zero if buffers_used = 0
* Missing part of the commit message added in v4

Here is a corrected patch v7. To me it seems to be in pretty good
shape, unless cfbot and/or other hackers will report any issues.

-- 
Best regards,
Aleksander Alekseev

Hi hackers,

> Here is a corrected patch v7. To me it seems to be in pretty good
> shape, unless cfbot and/or other hackers will report any issues.

There was a missing empty line in pg_buffercache.out which made the
tests fail. Here is a corrected v8 patch.

-- 
Best regards,
Aleksander Alekseev

Hi Zhang,

> The doc says we don’t take lock during pg_buffercache_summary, but I see locks in the v8 patch, Isn’t it?
>
> ```
> Similar to <function>pg_buffercache_pages</function> function
>  <function>pg_buffercache_summary</function> doesn't take buffer manager
>  locks [...]
> ```

Correct, the procedure doesn't take the locks of the buffer manager.
It does take the locks of every individual buffer.

I agree that the text is somewhat confusing, but it is consistent with
the current description of pg_buffercache [1]. I think this is a
problem worth addressing but it also seems to be out of scope of the
proposed patch.

[1]: https://www.postgresql.org/docs/current/pgbuffercache.html

--
Best regards,
Aleksander Alekseev

Hi,

On 2022-09-20 12:45:24 +0300, Aleksander Alekseev wrote:
> > I'm not sure how to avoid any undefined behaviour without locks though.
> > Even with locks, performance is much better. But is it good enough for production?
>
> Potentially you could avoid taking locks by utilizing atomic
> operations and lock-free algorithms. But these algorithms are
> typically error-prone and not always produce a faster code than the
> lock-based ones. I'm pretty confident this is out of scope of this
> particular patch.

Why would you need lockfree operations?  All you need to do is to read
BufferDesc->state into a local variable and then make decisions based on that?


> +    for (int i = 0; i < NBuffers; i++)
> +    {
> +        BufferDesc *bufHdr;
> +        uint32        buf_state;
> +
> +        bufHdr = GetBufferDescriptor(i);
> +
> +        /* Lock each buffer header before inspecting. */
> +        buf_state = LockBufHdr(bufHdr);
> +
> +        /* Invalid RelFileNumber means the buffer is unused */
> +        if (RelFileNumberIsValid(BufTagGetRelNumber(&bufHdr->tag)))
> +        {
> +            buffers_used++;
> +            usagecount_avg += BUF_STATE_GET_USAGECOUNT(buf_state);
> +
> +            if (buf_state & BM_DIRTY)
> +                buffers_dirty++;
> +        }
> +        else
> +            buffers_unused++;
> +
> +        if (BUF_STATE_GET_REFCOUNT(buf_state) > 0)
> +            buffers_pinned++;
> +
> +        UnlockBufHdr(bufHdr, buf_state);
> +    }

I.e. instead of locking the buffer header as done above, this could just do
something along these lines:

        BufferDesc *bufHdr;
        uint32      buf_state;

        bufHdr = GetBufferDescriptor(i);

        buf_state = pg_atomic_read_u32(&bufHdr->state);

        if (buf_state & BM_VALID)
        {
            buffers_used++;
            usagecount_avg += BUF_STATE_GET_USAGECOUNT(buf_state);

            if (buf_state & BM_DIRTY)
                buffers_dirty++;
        }
        else
            buffers_unused++;

        if (BUF_STATE_GET_REFCOUNT(buf_state) > 0)
            buffers_pinned++;


Without a memory barrier you can get very slightly "out-of-date" values of the
state, but that's fine in this case.

Greetings,

Andres Freund

Hi Andres,

> All you need to do is to read BufferDesc->state into a local variable and then make decisions based on that

You are right, thanks.

Here is the corrected patch.

-- 
Best regards,
Aleksander Alekseev

Hi,

On 2022-09-22 18:22:44 +0300, Melih Mutlu wrote:
> Since header locks are removed again, I put some doc changes and comments
> back.

Due to the merge of the meson build system, this needs to adjust meson.build
as well.


> --- a/contrib/pg_buffercache/expected/pg_buffercache.out
> +++ b/contrib/pg_buffercache/expected/pg_buffercache.out
> @@ -8,3 +8,12 @@ from pg_buffercache;
>   t
>  (1 row)
>
> +select buffers_used + buffers_unused > 0,
> +        buffers_dirty < buffers_used,
> +        buffers_pinned < buffers_used

Doesn't these have to be "<=" instead of "<"?


> +    for (int i = 0; i < NBuffers; i++)
> +    {
> +        BufferDesc *bufHdr;
> +        uint32        buf_state;
> +
> +        /*
> +         * No need to get locks on buffer headers as we don't rely on the
> +         * results in detail. Therefore, we don't get a consistent snapshot
> +         * across all buffers and it is not guaranteed that the information of
> +         * each buffer is self-consistent as opposed to pg_buffercache_pages.
> +         */

I think the "consistent snapshot" bit is misleading - even taking buffer
header locks wouldn't give you that.


> +    if (buffers_used != 0)
> +        usagecount_avg = usagecount_avg / buffers_used;

Perhaps the average should be NULL in the buffers_used == 0 case?


> + <para>
> +  <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>
> +
> + <para>
> +  <function>pg_buffercache_summary</function> function returns a table with a single row
> +  that contains summarized and aggregated information about shared buffer caches.
>   </para>

I think these sentences are missing a "The " at the start?

"shared buffer caches" isn't right - I think I'd just drop the "caches".


> +  <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>
> +   <function>pg_buffercache_summary</function> doesn't take buffer manager
> +   locks. Unlike <function>pg_buffercache_pages</function> function
> +   <function>pg_buffercache_summary</function> doesn't take buffer headers locks
> +   either, thus the result is not consistent. 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>
> + </sect2>

I don't think this mentioning of buffer header locks is useful for users - nor
is it I think correct. Acquiring the buffer header locks wouldn't add *any*
additional consistency.

Greetings,

Andres Freund

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

Hi,

On 2022-10-12 12:27:54 -0700, Andres Freund wrote:
> I intentionally put my changes into a fixup commit, in case you want to look
> at the differences.

I pushed the (combined) patch now. Thanks for your contribution!

Greetings,

Andres Freund