public inbox for [email protected]  

help / color / mirror / Atom feed

From: Nazir Bilal Yavuz <[email protected]>
To: Aidar Imamov <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: [email protected]
Subject: Re: Add pg_buffercache_evict_all() and pg_buffercache_mark_dirty[_all]() functions
Date: Mon, 24 Mar 2025 15:39:08 +0300
Message-ID: <CAN55FZ08vpp0WFEaqarg08DKaU0yCjgPTk_HyJJYoempKBu72w@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CAN55FZ0h_YoSqqutxV6DES1RW8ig6wcA8CR9rJk358YRMxZFmw@mail.gmail.com>
	<bbxepmv3w2iapbxehkypzsut545valywzf62qlizsrzkbrdnfy@dzw5ju3itwgs>
	<CAN55FZ1Pq_80vcuHZK6dSuTfG-5jBoK0Bd2k1YUSpKT+7hOj6A@mail.gmail.com>
	<CAN55FZ2mNTVK81jqsy2NXg9k_ex_LrdU6JmL1wvsW9-GPy9XuA@mail.gmail.com>
	<[email protected]>

Hi,

On Wed, 19 Mar 2025 at 01:02, Aidar Imamov <[email protected]> wrote:
>
> Hi!
>
> This is my first time trying out as a patch reviewer, and I don't claim
> to be
> an expert. I am very interested in the topic of buffer cache and would
> like to
> learn more about it.

Thank you so much for the review!

> pg_buffercache_evict_all():
> > for (int buf = 1; buf < NBuffers; buf++)
> Mb it would be more correct to use <= NBuffers?

Yes, done.

> I also did not fully understand what A. Freund was referring to.
> However, we
> cannot avoid blocking the buffer header, as we need to ensure that the
> buffer
> is not being pinned by anyone else. Perhaps it would be beneficial to
> call
> LockBufHdr() outside and check if BUT_STATE_GET_REFCOUNT == 0 before
> calling
> EvictUnpinnedBuffer(). This would help to prevent unnecessary calls to
> EvictUnpinnedBuffer() itself, ResourceOwnerEnlarge() and
> ReservePrivateRefCountEntry().

I had an off-list talk with Andres and learned that
ResourceOwnerEnlarge() and ReservePrivateRefCountEntry() need to be
called before acquiring the buffer header lock. I introduced the
EvictUnpinnedBuffersFromSharedRelation() function for that [1].

> pg_buffercache_mark_dirty_all():
> > for (int buf = 1; buf < NBuffers; buf++)
> Mb it would be more correct to use <= NBuffers?

Done.

> pg_buffercache_evict_relation():
> > errmsg("must be superuser to use pg_buffercache_evict function")
> '_relation' postfix got lost here

Done.

> >   /* Open relation. */
> >  rel = relation_open(relOid, AccessShareLock);
> If I understand correctly, this function is meant to replicate the
> behavior of
> VACUUM FULL, but only for dirty relation pages. In this case, wouldn't
> it be
> necessary to obtain an exclusive access lock?

I think VACUUM FULL does more things but I agree with you, obtaining
an exclusive access lock is the correct way.

> > for (int buf = 1; buf < NBuffers; buf++)
> Mb it would be more correct to use <= NBuffers?

Done.

>
> >    /*
> >     * No need to call UnlockBufHdr() if BufTagMatchesRelFileLocator()
> >     * returns true, EvictUnpinnedBuffer() will take care of it.
> >     */
> >    buf_state = LockBufHdr(bufHdr);
> >    if (BufTagMatchesRelFileLocator(&bufHdr->tag, &rel->rd_locator))
> >    {
> >      if (EvictUnpinnedBuffer(buf, buf_state, &flushed))
> >        buffers_evicted++;
> >      if (flushed)
> >        buffers_flushed++;
> >    }
>
> If you have previously acquired the exclusive access lock to the
> relation,
> you may want to consider replacing the order of the
> BufTagMatchesRelFileLocator()
> and LockBufHdr() calls for improved efficiency (as mentioned in the
> comment on
> the BufTagMatchesRelFileLocator() call in the DropRelationBuffers()
> function in
> bufmgr.c).

I think you are right, we are basically copying FlushRelationBuffers()
to the contrib/pg_buffer_cache. Done.

> And it maybe useful also to add BUT_STATE_GET_REFCOUNT == 0 precheck
> before calling
> EvictUnpinnedBuffer()?

I think we do not need to do this. I see EvictUnpinnedBuffer() as a
central place that checks all conditions. If we add this pre-check
then we should not do the same check in the EvictUnpinnedBuffer(),
creating this logic would add unnecessary complexity to
EvictUnpinnedBuffer().

I addressed these reviews in v3. I will share the patches in my next reply.

--
Regards,
Nazir Bilal Yavuz
Microsoft

view thread (29+ 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]
  Subject: Re: Add pg_buffercache_evict_all() and pg_buffercache_mark_dirty[_all]() functions
  In-Reply-To: <CAN55FZ08vpp0WFEaqarg08DKaU0yCjgPTk_HyJJYoempKBu72w@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