Horton

Thread

Commits

  1. pg_buffercache: Add pg_buffercache_summary()

  2. Revert 56-bit relfilenode change and follow-up commits.

  1. Summary function for pg_buffercache

    Melih Mutlu <m.melihmutlu@gmail.com> — 2022-08-18T13:57:16Z

    Hi hackers,

Added a pg_buffercache_summary() function to retrieve an aggregated summary
information with less cost.

It's often useful to know only how many buffers are used, how many of them
are dirty etc. for monitoring purposes.
This info can already be retrieved by pg_buffercache. The extension
currently creates a row with many details for each buffer, then summary
info can be aggregated from that returned table.
But it is quite expensive to run regularly for monitoring.

The attached patch adds a pg_buffercache_summary() function to get this
summary info faster.
New function only collects following info and returns them in a single row:
- used_buffers = number of buffers with a valid relfilenode (both dirty and
not)
- unused_buffers = number of buffers with invalid relfilenode
- dirty_buffers = number of dirty buffers.
- pinned_buffers = number of buffers that have at least one pinning backend
(i.e. refcount > 0)
- average usagecount of used buffers

One other difference between pg_buffercache_summary and
pg_buffercache_pages is that pg_buffercache_summary does not get locks on
buffer headers as opposed to pg_buffercache_pages.
Since the purpose of pg_buffercache_summary is just to give us an overall
idea about shared buffers and to be a cheaper function, locks are not
strictly needed.

To compare pg_buffercache_summary() and  pg_buffercache_pages(), I used a
simple query to aggregate the summary information above by calling
 pg_buffercache_pages().
Here is the result:

postgres=# show shared_buffers;
 shared_buffers
----------------
 16GB
(1 row)

Time: 0.756 ms
postgres=# SELECT relfilenode <> 0 AS is_valid, isdirty, count(*) FROM
pg_buffercache GROUP BY relfilenode <> 0, isdirty;
 is_valid | isdirty |  count
----------+---------+---------
 t        | f       |     209
          |         | 2096904
 t        | t       |      39
(3 rows)

Time: 1434.870 ms (00:01.435)
postgres=# select * from pg_buffercache_summary();
 used_buffers | unused_buffers | dirty_buffers | pinned_buffers |
avg_usagecount
--------------+----------------+---------------+----------------+----------------
          248 |        2096904 |            39 |              0 |
 3.141129
(1 row)

Time: 9.712 ms

There is a significant difference between timings of those two functions,
even though they return similar results.

I would appreciate any feedback/comment on this change.

Thanks,
Melih

  2. Re: Summary function for pg_buffercache

    Melih Mutlu <m.melihmutlu@gmail.com> — 2022-09-09T13:41:07Z

    Hi hackers,

I also added documentation changes into the patch.
You can find it attached.

 I would appreciate any feedback about this pg_buffercache_summary function.

Best,
Melih

  3. Re: Summary function for pg_buffercache

    Aleksander Alekseev <aleksander@timescale.com> — 2022-09-09T14:36:45Z

    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

  4. Re: Summary function for pg_buffercache

    Nathan Bossart <nathandbossart@gmail.com> — 2022-09-09T17:23:11Z

    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

  5. Re: Summary function for pg_buffercache

    Aleksander Alekseev <aleksander@timescale.com> — 2022-09-09T21:14:33Z

    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

  6. Re: Summary function for pg_buffercache

    Melih Mutlu <m.melihmutlu@gmail.com> — 2022-09-09T23:59:50Z

    Hi Aleksander and Nathan,

Thanks for your comments.

Aleksander Alekseev <aleksander@timescale.com>, 9 Eyl 2022 Cum, 17:36
tarihinde şunu yazdı:

> 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.
> """
>

I wasn't aware of this explanation. Thanks for pointing it out.

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

I thought that it wouldn't really be a problem even if relNumber is
modified concurrently, since the function does not actually rely on the
actual values.
I'm not sure about what undefined behaviour could harm this badly. It
seemed to me that it would read an invalid relNumber in the worst case
scenario.
But I'm not actually familiar with buffer related parts of the code, so I
might be wrong.
And I'm okay with taking header locks if necessary.

In the attached patch, I added buffer header locks just before examining
tag as follows:

+ buf_state = LockBufHdr(bufHdr);
> +
> + /* Invalid RelFileNumber means the buffer is unused */
> + if (RelFileNumberIsValid(BufTagGetRelNumber(&bufHdr->tag)))
> + {
> ...
> + }
> ...
> + UnlockBufHdr(bufHdr, buf_state);
>


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

I again did the same quick benchmarking, here are the numbers with locks.

postgres=# show shared_buffers;
 shared_buffers
----------------
 16GB
(1 row)

postgres=#  SELECT relfilenode <> 0 AS is_valid, isdirty, count(*) FROM
pg_buffercache GROUP BY relfilenode <> 0, isdirty;
 is_valid | isdirty |  count
----------+---------+---------
 t        | f       |     256
          |         | 2096876
 t        | t       |      20
(3 rows)

Time: 1024.456 ms (00:01.024)

postgres=#  select * from pg_buffercache_summary();
 used_buffers | unused_buffers | dirty_buffers | pinned_buffers |
avg_usagecount
--------------+----------------+---------------+----------------+----------------
          282 |        2096870 |            20 |              0 |
 3.4574468
(1 row)

Time: 33.074 ms

Yes, locks slowed pg_buffercache_summary down. But there is still quite a
bit of performance improvement, plus memory saving as you mentioned.


> Here is the corrected patch.
>

Also thanks for corrections.

Best,
Melih

  7. Re: Summary function for pg_buffercache

    Aleksander Alekseev <aleksander@timescale.com> — 2022-09-10T09:28:30Z

    Hi Melih,

> I'm not sure about what undefined behaviour could harm this badly.

You are right that in practice nothing wrong will (probably) happen on
x86/x64 architecture with (most?) modern C compilers. This is not true in
the general case though. It's up to the compiler to decide how reading the
bufHdr->tag is going to be actually implemented. This can be one assembly
instruction or several instructions. This reading can be optimized-out if
the compiler believes the required value is already in the register, etc.
Since the result will be different depending on the assembly code used this
is an undefined behaviour and we can't use code like this.

> In the attached patch, I added buffer header locks just before examining
tag as follows

Many thanks for the updated patch! It looks better now.

However I have somewhat mixed feelings about avg_usagecount. Generally
AVG() is a relatively useless methric for monitoring. What if the user
wants MIN(), MAX() or let's say a 99th percentile? I suggest splitting it
into usagecount_min, usagecount_max and usagecount_sum. AVG() can be
derived as usercount_sum / used_buffers.

Also I suggest changing the names of the columns in order to make them
consistent with the rest of the system. If you consider pg_stat_activity
and family [1] you will notice that the columns are named
(entity)_(property), e.g. backend_xid, backend_type, client_addr, etc. So
instead of used_buffers and unused_buffers the naming should be
buffers_used and buffers_unused.

[1]: https://www.postgresql.org/docs/current/monitoring-stats.html

-- 
Best regards,
Aleksander Alekseev

  8. Re: Summary function for pg_buffercache

    Melih Mutlu <m.melihmutlu@gmail.com> — 2022-09-10T12:55:30Z

    Hello Aleksander,

> I'm not sure about what undefined behaviour could harm this badly.
>
> You are right that in practice nothing wrong will (probably) happen on
> x86/x64 architecture with (most?) modern C compilers. This is not true in
> the general case though. It's up to the compiler to decide how reading the
> bufHdr->tag is going to be actually implemented. This can be one assembly
> instruction or several instructions. This reading can be optimized-out if
> the compiler believes the required value is already in the register, etc.
> Since the result will be different depending on the assembly code used this
> is an undefined behaviour and we can't use code like this.
>

Got it. Thanks for explaining.


> However I have somewhat mixed feelings about avg_usagecount. Generally
> AVG() is a relatively useless methric for monitoring. What if the user
> wants MIN(), MAX() or let's say a 99th percentile? I suggest splitting it
> into usagecount_min, usagecount_max and usagecount_sum. AVG() can be
> derived as usercount_sum / used_buffers.
>

Won't be usagecount_max almost always 5 as "BM_MAX_USAGE_COUNT" set to 5 in
buf_internals.h? I'm not sure about how much usagecount_min would add
either.
A usagecount is always an integer between 0 and 5, it's not
something unbounded. I think the 99th percentile would be much better than
average if strong outlier values could occur. But in this case, I feel like
an average value would be sufficiently useful as well.
usagecount_sum would actually be useful since average can be derived from
it. If you think that the sum of usagecounts has a meaning just by itself,
it makes sense to include it. Otherwise, wouldn't showing directly averaged
value be more useful?



> Also I suggest changing the names of the columns in order to make them
> consistent with the rest of the system. If you consider pg_stat_activity
> and family [1] you will notice that the columns are named
> (entity)_(property), e.g. backend_xid, backend_type, client_addr, etc. So
> instead of used_buffers and unused_buffers the naming should be
> buffers_used and buffers_unused.
>
> [1]: https://www.postgresql.org/docs/current/monitoring-stats.html
>

You're right. I will change the names accordingly. Thanks.


Regards,
Melih

  9. Re: Summary function for pg_buffercache

    Andres Freund <andres@anarazel.de> — 2022-09-15T20:25:14Z

    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

  10. Re: Summary function for pg_buffercache

    Melih Mutlu <m.melihmutlu@gmail.com> — 2022-09-20T08:47:40Z

    Hi,

Also I suggest changing the names of the columns in order to make them
> consistent with the rest of the system. If you consider pg_stat_activity
> and family [1] you will notice that the columns are named
> (entity)_(property), e.g. backend_xid, backend_type, client_addr, etc. So
> instead of used_buffers and unused_buffers the naming should be
> buffers_used and buffers_unused.
>
> [1]: https://www.postgresql.org/docs/current/monitoring-stats.html


I changed these names and updated the patch.

However I have somewhat mixed feelings about avg_usagecount. Generally
>> AVG() is a relatively useless methric for monitoring. What if the user
>> wants MIN(), MAX() or let's say a 99th percentile? I suggest splitting it
>> into usagecount_min, usagecount_max and usagecount_sum. AVG() can be
>> derived as usercount_sum / used_buffers.
>>
>
> Won't be usagecount_max almost always 5 as "BM_MAX_USAGE_COUNT" set to 5
> in buf_internals.h? I'm not sure about how much usagecount_min would add
> either.
> A usagecount is always an integer between 0 and 5, it's not
> something unbounded. I think the 99th percentile would be much better than
> average if strong outlier values could occur. But in this case, I feel like
> an average value would be sufficiently useful as well.
> usagecount_sum would actually be useful since average can be derived from
> it. If you think that the sum of usagecounts has a meaning just by itself,
> it makes sense to include it. Otherwise, wouldn't showing directly averaged
> value be more useful?
>

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 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.
>

I agree that it's important due to its high impact. 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?


Thanks,
Melih

  11. Re: Summary function for pg_buffercache

    Aleksander Alekseev <aleksander@timescale.com> — 2022-09-20T09:45:24Z

    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

  12. Re: Summary function for pg_buffercache

    Aleksander Alekseev <aleksander@timescale.com> — 2022-09-20T10:57:32Z

    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

  13. Re: Summary function for pg_buffercache

    Melih Mutlu <m.melihmutlu@gmail.com> — 2022-09-20T11:00:19Z

    Aleksander Alekseev <aleksander@timescale.com>, 20 Eyl 2022 Sal, 13:57
tarihinde şunu yazdı:

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

I was just sending a corrected patch without the missing line.

Thanks a lot for all these reviews and the corrected patch.

Best,
Melih

  14. Re: Summary function for pg_buffercache

    Melih Mutlu <m.melihmutlu@gmail.com> — 2022-09-20T12:10:16Z

    Hi,

Seems like cfbot tests are passing now:
https://cirrus-ci.com/build/4727923671302144

Best,
Melih

Melih Mutlu <m.melihmutlu@gmail.com>, 20 Eyl 2022 Sal, 14:00 tarihinde şunu
yazdı:

> Aleksander Alekseev <aleksander@timescale.com>, 20 Eyl 2022 Sal, 13:57
> tarihinde şunu yazdı:
>
>> There was a missing empty line in pg_buffercache.out which made the
>> tests fail. Here is a corrected v8 patch.
>>
>
> I was just sending a corrected patch without the missing line.
>
> Thanks a lot for all these reviews and the corrected patch.
>
> Best,
> Melih
>

  15. Re: Summary function for pg_buffercache

    Zhang Mingli <zmlpostgres@gmail.com> — 2022-09-20T12:29:30Z

    Hi,

Correct me if I’m wrong.

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, thus the result is not 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.

```




Regards,
Zhang Mingli
On Sep 20, 2022, 20:10 +0800, Melih Mutlu <m.melihmutlu@gmail.com>, wrote:
> Hi,
>
> Seems like cfbot tests are passing now:
> https://cirrus-ci.com/build/4727923671302144
>
> Best,
> Melih
>
> > Melih Mutlu <m.melihmutlu@gmail.com>, 20 Eyl 2022 Sal, 14:00 tarihinde şunu yazdı:
> > > Aleksander Alekseev <aleksander@timescale.com>, 20 Eyl 2022 Sal, 13:57 tarihinde şunu yazdı:
> > > > > There was a missing empty line in pg_buffercache.out which made the
> > > > > tests fail. Here is a corrected v8 patch.
> > > >
> > > > I was just sending a corrected patch without the missing line.
> > > >
> > > > Thanks a lot for all these reviews and the corrected patch.
> > > >
> > > > Best,
> > > > Melih

  16. Re: Summary function for pg_buffercache

    Aleksander Alekseev <aleksander@timescale.com> — 2022-09-20T12:43:25Z

    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

  17. Re: Summary function for pg_buffercache

    Zhang Mingli <zmlpostgres@gmail.com> — 2022-09-20T12:48:59Z

    Hi,

Regards,
Zhang Mingli
On Sep 20, 2022, 20:43 +0800, Aleksander Alekseev <aleksander@timescale.com>, wrote:
>
> Correct, the procedure doesn't take the locks of the buffer manager.
> It does take the locks of every individual buffer.
Ah, now I get it, thanks.

  18. Re: Summary function for pg_buffercache

    Melih Mutlu <m.melihmutlu@gmail.com> — 2022-09-20T12:49:39Z

    Hi Zhang,

Those are two different locks.
The locks that are taken in the patch are for buffer headers. This locks
only the current buffer and makes that particular buffer's info consistent
within itself.

However, the lock mentioned in the doc is for buffer manager which would
prevent changes on any buffer if it's held.
pg_buffercache_summary (and pg_buffercache_pages) does not hold buffer
manager lock. Therefore, consistency across all buffers is not guaranteed.

For pg_buffercache_pages, self-consistent buffer information is useful
since it shows each buffer separately.

For pg_buffercache_summary, even self-consistency may not matter much since
results are aggregated and we can't see individual buffer information.
Consistency across all buffers is also not a concern since its purpose is
to give an overall idea about the state of buffers.

I see that these two different locks in the same context can be confusing.
I hope it is a bit more clear now.

Best,
Melih

>

  19. Re: Summary function for pg_buffercache

    Zhang Mingli <zmlpostgres@gmail.com> — 2022-09-20T12:52:13Z

    Hi,
On Sep 20, 2022, 20:49 +0800, Melih Mutlu <m.melihmutlu@gmail.com>, wrote:
> Hi Zhang,
>
> Those are two different locks.
> The locks that are taken in the patch are for buffer headers. This locks only the current buffer and makes that particular buffer's info consistent within itself.
>
> However, the lock mentioned in the doc is for buffer manager which would prevent changes on any buffer if it's held.
> pg_buffercache_summary (and pg_buffercache_pages) does not hold buffer manager lock. Therefore, consistency across all buffers is not guaranteed.
>
> For pg_buffercache_pages, self-consistent buffer information is useful since it shows each buffer separately.
>
> For pg_buffercache_summary, even self-consistency may not matter much since results are aggregated and we can't see individual buffer information.
> Consistency across all buffers is also not a concern since its purpose is to give an overall idea about the state of buffers.
>
> I see that these two different locks in the same context can be confusing. I hope it is a bit more clear now.
>
> Best,
> Melih
Thanks for your explanation, LGTM.

  20. Re: Summary function for pg_buffercache

    Andres Freund <andres@anarazel.de> — 2022-09-21T00:58:08Z

    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

  21. Re: Summary function for pg_buffercache

    Aleksander Alekseev <aleksander@timescale.com> — 2022-09-21T13:08:51Z

    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

  22. Re: Summary function for pg_buffercache

    Melih Mutlu <m.melihmutlu@gmail.com> — 2022-09-22T15:22:44Z

    Hi,

Since header locks are removed again, I put some doc changes and comments
back.

Thanks,
Melih

  23. Re: Summary function for pg_buffercache

    Andres Freund <andres@anarazel.de> — 2022-09-22T16:10:14Z

    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

  24. Re: Summary function for pg_buffercache

    Melih Mutlu <m.melihmutlu@gmail.com> — 2022-09-23T20:14:09Z

    Hi Andres,

Adjusted the patch so that it will work with meson now.

Also addressed your other reviews as well.
I hope explanations in comments/docs are better now.

Best,
Melih

  25. Re: Summary function for pg_buffercache

    Melih Mutlu <m.melihmutlu@gmail.com> — 2022-09-28T13:49:49Z

    Hi all,

The patch needed a rebase due to recent changes on pg_buffercache.
You can find the updated version attached.

Best,
Melih

  26. Re: Summary function for pg_buffercache

    Zhang Mingli <zmlpostgres@gmail.com> — 2022-09-28T14:31:34Z

    Regards,
Zhang Mingli
On Sep 28, 2022, 21:50 +0800, Melih Mutlu <m.melihmutlu@gmail.com>, wrote:
> Hi all,
>
> The patch needed a rebase due to recent changes on pg_buffercache.
> You can find the updated version attached.
>
> Best,
> Melih
>
>
```
+
+	if (buffers_used != 0)
+ usagecount_avg = usagecount_avg / buffers_used;
+
+	memset(nulls, 0, sizeof(nulls));
+	values[0] = Int32GetDatum(buffers_used);
+	values[1] = Int32GetDatum(buffers_unused);
+	values[2] = Int32GetDatum(buffers_dirty);
+	values[3] = Int32GetDatum(buffers_pinned);
+
+	if (buffers_used != 0)
+	{
+ usagecount_avg = usagecount_avg / buffers_used;
+ values[4] = Float4GetDatum(usagecount_avg);
+	}
+	else
+	{
+ nulls[4] = true;
+	}
```

Why compute usagecount_avg twice?

  27. Re: Summary function for pg_buffercache

    Melih Mutlu <m.melihmutlu@gmail.com> — 2022-09-28T14:41:45Z

    Zhang Mingli <zmlpostgres@gmail.com>, 28 Eyl 2022 Çar, 17:31 tarihinde şunu
yazdı:

> Why compute usagecount_avg twice?
>

I should have removed the first one, but I think I missed it.
Nice catch.

Attached an updated version.

Thanks,
Melih

  28. Re: Summary function for pg_buffercache

    Zhang Mingli <zmlpostgres@gmail.com> — 2022-09-28T15:07:49Z

    Hi,

On Sep 28, 2022, 22:41 +0800, Melih Mutlu <m.melihmutlu@gmail.com>, wrote:
>
>
> Zhang Mingli <zmlpostgres@gmail.com>, 28 Eyl 2022 Çar, 17:31 tarihinde şunu yazdı:
> > Why compute usagecount_avg twice?
>
> I should have removed the first one, but I think I missed it.
> Nice catch.
>
> Attached an updated version.
>
> Thanks,
> Melih
>
Hmm, I just apply v13 patch but failed.

Part of errors:
```
error: patch failed: contrib/pg_buffercache/pg_buffercache.control:1 error: contrib/pg_buffercache/pg_buffercache.control: patch does not apply
Checking patch contrib/pg_buffercache/pg_buffercache_pages.c...
error: while searching for:
 */
PG_FUNCTION_INFO_V1(pg_buffercache_pages);
PG_FUNCTION_INFO_V1(pg_buffercache_pages_v1_4);
```

Rebase on master and then apply our changes again?

Regards,
Zhang Mingli
>

  29. Re: Summary function for pg_buffercache

    Melih Mutlu <m.melihmutlu@gmail.com> — 2022-09-28T15:19:57Z

    Hi,

Seems like the commit a448e49bcbe40fb72e1ed85af910dd216d45bad8 reverts the
changes on pg_buffercache.

Why compute usagecount_avg twice?
>
Then, I'm going back to v11 + the fix for this.

Thanks,
Melih

  30. Re: Summary function for pg_buffercache

    Zhang Mingli <zmlpostgres@gmail.com> — 2022-09-29T06:11:30Z

    Hi,

On Sep 28, 2022, 23:20 +0800, Melih Mutlu <m.melihmutlu@gmail.com>, wrote:
> Hi,
>
> Seems like the commit a448e49bcbe40fb72e1ed85af910dd216d45bad8 reverts the changes on pg_buffercache.
>
> > Why compute usagecount_avg twice?
> Then, I'm going back to v11 + the fix for this.
>
> Thanks,
> Melih
Looks good to me.

Regards,
Zhang Mingli

  31. Re: Summary function for pg_buffercache

    Andres Freund <andres@anarazel.de> — 2022-10-12T19:27:54Z

    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

  32. Re: Summary function for pg_buffercache

    Andres Freund <andres@anarazel.de> — 2022-10-13T17:04:56Z

    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