MIME-Version: 1.0
References: 
 <CAN55FZ0h_YoSqqutxV6DES1RW8ig6wcA8CR9rJk358YRMxZFmw@mail.gmail.com>
 <bbxepmv3w2iapbxehkypzsut545valywzf62qlizsrzkbrdnfy@dzw5ju3itwgs>
 <CAN55FZ1Pq_80vcuHZK6dSuTfG-5jBoK0Bd2k1YUSpKT+7hOj6A@mail.gmail.com>
 <CAN55FZ2mNTVK81jqsy2NXg9k_ex_LrdU6JmL1wvsW9-GPy9XuA@mail.gmail.com>
 <568288111d505a81ebb2a89cea9eacb2@postgrespro.ru>
In-Reply-To: <568288111d505a81ebb2a89cea9eacb2@postgrespro.ru>
From: Nazir Bilal Yavuz <byavuz81@gmail.com>
Date: Mon, 24 Mar 2025 15:39:08 +0300
Message-ID: 
 <CAN55FZ08vpp0WFEaqarg08DKaU0yCjgPTk_HyJJYoempKBu72w@mail.gmail.com>
Subject: Re: Add pg_buffercache_evict_all() and
 pg_buffercache_mark_dirty[_all]()
 functions
To: Aidar Imamov <a.imamov@postgrespro.ru>
Cc: Andres Freund <andres@anarazel.de>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, koshy44@gmail.com
Content-Type: text/plain; charset="UTF-8"
Archived-At: 
 <https://www.postgresql.org/message-id/CAN55FZ08vpp0WFEaqarg08DKaU0yCjgPTk_HyJJYoempKBu72w%40mail.gmail.com>
Precedence: bulk

Hi,

On Wed, 19 Mar 2025 at 01:02, Aidar Imamov <a.imamov@postgrespro.ru> 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