Re: Add shared buffer hits to pg_stat_io - Mailing list pgsql-hackers

Thanks for the review!

On Tue, Feb 28, 2023 at 7:36 AM Drouvot, Bertrand
<bertranddrouvot.pg@gmail.com> wrote:
>   BufferDesc *
>   LocalBufferAlloc(SMgrRelation smgr, ForkNumber forkNum, BlockNumber blockNum,
> -                                bool *foundPtr, IOContext *io_context)
> +                                bool *foundPtr, IOContext io_context)
>   {
>          BufferTag       newTag;                 /* identity of requested block */
>          LocalBufferLookupEnt *hresult;
> @@ -128,14 +128,6 @@ LocalBufferAlloc(SMgrRelation smgr, ForkNumber forkNum, BlockNumber blockNum,
>          hresult = (LocalBufferLookupEnt *)
>                  hash_search(LocalBufHash, &newTag, HASH_FIND, NULL);
>
> -       /*
> -        * IO Operations on local buffers are only done in IOCONTEXT_NORMAL. Set
> -        * io_context here (instead of after a buffer hit would have returned) for
> -        * convenience since we don't have to worry about the overhead of calling
> -        * IOContextForStrategy().
> -        */
> -       *io_context = IOCONTEXT_NORMAL;
>
>
> It looks like that io_context is not used in LocalBufferAlloc() anymore and then can be removed as an argument.

Good catch. Updated patchset attached.

> > While adding this, I noticed that I had made all of the IOOP columns
> > int8 in the view, and I was wondering if this is sufficient for hits (I
> > imagine you could end up with quite a lot of those).
> >
>
> I think that's ok and bigint is what is already used for pg_statio_user_tables.heap_blks_hit for example.

Ah, I was silly and didn't understand that the SQL type int8 is eight
bytes and not 1. That makes a lot of things make more sense :)

https://www.postgresql.org/docs/current/xfunc-c.html#XFUNC-C-TYPE-TABLE

- Melanie