MIME-Version: 1.0
References: <CALfch19pW48ZwWzUoRSpsaV9hqt0UPyaBPC4bOZ4W+c7FF566A@mail.gmail.com>
 <CAMm1aWbk-0C460G9ooy2yNSy1riecqvw4F7jPnqvToFZ_s4oGw@mail.gmail.com>
 <CALfch1890bL5SddyOpZDC8d-jZFOUGCGbOr+_11jLTwwJ_WEOA@mail.gmail.com>
 <CALfch1_s-X3wxeQbYxhXVuWZKBeY5YvU1D+nJxe21ri5iv0qrw@mail.gmail.com>
 <8b8c709a-c7cc-4965-8296-64f549c27501@abcsql.com> <3510716d-d3f3-40c8-9f7f-d644171acfb6@gmail.com>
 <a2c3312f-0866-443f-a799-dd76ebf9c7f5@abcsql.com> <CALDaNm20+pri5yvEF-4HZqCwRjHB+qjWHLcfanyEjnwgF96z6Q@mail.gmail.com>
 <CA+hUKGL9Y3RgK=SjzSwvUnS4-5RcFEXJ3fwqbLCa9bo8g4uOOg@mail.gmail.com>
 <CACG=ezbBCXaP_KH3N15_tHSwAcCmHQjTyOVsd2o5qG6B_dPzPg@mail.gmail.com>
 <CA+hUKGKGyVjqoaSC7AZrgYKHAk9Nid9G4Ao3ZtL_OtLZDb+_cA@mail.gmail.com>
 <CA+hUKG+CjngU9+7mfPsV6tA8Rze9keoV887mHxUL_9qiFUrjJA@mail.gmail.com> <CAAKRu_ZMgERqsOEEfbv+Y4+=gDiWghH8J6zXVDeO1VW8eYEJHA@mail.gmail.com>
In-Reply-To: <CAAKRu_ZMgERqsOEEfbv+Y4+=gDiWghH8J6zXVDeO1VW8eYEJHA@mail.gmail.com>
From: Thomas Munro <thomas.munro@gmail.com>
Date: Mon, 8 Apr 2024 17:02:58 +1200
Message-ID: <CA+hUKGL8fHyfAqZ1F_GJdMj5HyOX_7Xo4EABsLSuGPG8Ldz9cw@mail.gmail.com>
Subject: Re: Extension Enhancement: Buffer Invalidation in pg_buffercache
To: Melanie Plageman <melanieplageman@gmail.com>
Cc: Maxim Orlov <orlovmg@gmail.com>, vignesh C <vignesh21@gmail.com>, 
	Palak Chaturvedi <chaturvedipalak1911@gmail.com>, Jim Nasby <jim.nasby@gmail.com>, 
	pgsql-hackers@postgresql.org, Nitin Jadhav <nitinjadhavpostgres@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CA%2BhUKGL8fHyfAqZ1F_GJdMj5HyOX_7Xo4EABsLSuGPG8Ldz9cw%40mail.gmail.com>
Precedence: bulk

On Mon, Apr 8, 2024 at 11:53=E2=80=AFAM Melanie Plageman
<melanieplageman@gmail.com> wrote:
> I've reviewed v6. I think you should mention in the docs that it only
> works for shared buffers -- so specifically not buffers containing
> blocks of temp tables.

Thanks for looking!  The whole pg_buffercache extension is for working
with shared buffers only, as mentioned at the top.  I have tried to
improve that paragraph though, as it only mentioned examining them.

> In the function pg_buffercache_invalidate(), why not use the
> BufferIsValid() function?
>
> -   if (buf < 1 || buf > NBuffers)
> +   if (!BufferIsValid(buf) || buf > NBuffers)

It doesn't check the range (it has assertions, not errors).

> I thought the below would be more clear for the comment above
> InvalidateUnpinnedBuffer().
>
> - * Returns true if the buffer was valid and it has now been made invalid=
.
> - * Returns false if the wasn't valid, or it couldn't be evicted due to a=
 pin,
> - * or if the buffer becomes dirty again while we're trying to write it o=
ut.
> + * Returns true if the buffer was valid and has now been made invalid. R=
eturns
> + * false if it wasn't valid, if it couldn't be evicted due to a pin, or =
if the
> + * buffer becomes dirty again while we're trying to write it out.

Fixed.

> Some of that probably applies for the docs too (i.e. you have some
> similar wording in the docs). There is actually one typo in your
> version, so even if you don't adopt my suggestion, you should fix that
> typo.

Yeah, thanks, improved similarly there.

> I didn't notice anything else out of place. I tried it and it worked
> as expected. I'm excited to have this feature!

Thanks!

> I didn't read through this whole thread, but was there any talk of
> adding other functions to let me invalidate a bunch of buffers at once
> or even some options -- like invalidate every 3rd buffer or whatever?
> (Not the concern of this patch, but just wondering because that would
> be a useful future enhancement IMO).

TBH I tried to resist people steering in that direction because you
can also just define a SQL function to do that built on this, and if
you had specialised functions they'd never be quite right.  IMHO we
succeeded in minimising the engineering and maximising flexibility,
'cause it's for hackers.  Crude, but already able to express a wide
range of stuff by punting the problem to SQL.

Thanks to Palak for the patch.  Pushed.