public inbox for [email protected]  

help / color / mirror / Atom feed

From: Aidar Imamov <[email protected]>
To: Nazir Bilal Yavuz <[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: Wed, 19 Mar 2025 01:02:52 +0300
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAN55FZ2mNTVK81jqsy2NXg9k_ex_LrdU6JmL1wvsW9-GPy9XuA@mail.gmail.com>
References: <CAN55FZ0h_YoSqqutxV6DES1RW8ig6wcA8CR9rJk358YRMxZFmw@mail.gmail.com>
	<bbxepmv3w2iapbxehkypzsut545valywzf62qlizsrzkbrdnfy@dzw5ju3itwgs>
	<CAN55FZ1Pq_80vcuHZK6dSuTfG-5jBoK0Bd2k1YUSpKT+7hOj6A@mail.gmail.com>
	<CAN55FZ2mNTVK81jqsy2NXg9k_ex_LrdU6JmL1wvsW9-GPy9XuA@mail.gmail.com>

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.

I have reviewed the patches after they were
edited and I would like to share my thoughts on some points.

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

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().


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


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

>   /* 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?

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

>    /*
>     * 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).
And it maybe useful also to add BUT_STATE_GET_REFCOUNT == 0 precheck 
before calling
EvictUnpinnedBuffer()?


regards,
Aidar Imamov

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: <[email protected]>

* 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