public inbox for [email protected]  

help / color / mirror / Atom feed

From: Thomas Munro <[email protected]>
To: Melanie Plageman <[email protected]>
Cc: Maxim Orlov <[email protected]>
Cc: vignesh C <[email protected]>
Cc: Palak Chaturvedi <[email protected]>
Cc: Jim Nasby <[email protected]>
Cc: [email protected]
Cc: Nitin Jadhav <[email protected]>
Subject: Re: Extension Enhancement: Buffer Invalidation in pg_buffercache
Date: Mon, 8 Apr 2024 17:02:58 +1200
Message-ID: <CA+hUKGL8fHyfAqZ1F_GJdMj5HyOX_7Xo4EABsLSuGPG8Ldz9cw@mail.gmail.com> (raw)
In-Reply-To: <CAAKRu_ZMgERqsOEEfbv+Y4+=gDiWghH8J6zXVDeO1VW8eYEJHA@mail.gmail.com>
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>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<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>

On Mon, Apr 8, 2024 at 11:53 AM Melanie Plageman
<[email protected]> 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 out.
> + * Returns true if the buffer was valid and has now been made invalid. Returns
> + * 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.

view thread (2+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Extension Enhancement: Buffer Invalidation in pg_buffercache
  In-Reply-To: <CA+hUKGL8fHyfAqZ1F_GJdMj5HyOX_7Xo4EABsLSuGPG8Ldz9cw@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox