Message-ID: <8b8c709a-c7cc-4965-8296-64f549c27501@abcsql.com>
Date: Wed, 3 Jan 2024 17:25:03 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
From: =?UTF-8?Q?C=C3=A9dric_Villemain?= <cedric.villemain+pgsql@abcsql.com>
Subject: Re: Extension Enhancement: Buffer Invalidation in pg_buffercache
To: Palak Chaturvedi <chaturvedipalak1911@gmail.com>,
 Nitin Jadhav <nitinjadhavpostgres@gmail.com>, pgsql-hackers@postgresql.org,
 Thomas Munro <thomas.munro@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>
Content-Language: en-US
Organization: Data Bene
In-Reply-To: <CALfch1_s-X3wxeQbYxhXVuWZKBeY5YvU1D+nJxe21ri5iv0qrw@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: <https://www.postgresql.org/message-id/8b8c709a-c7cc-4965-8296-64f549c27501%40abcsql.com>
Precedence: bulk

Hi Palak,

I did a quick review of the patch:

+CREATE FUNCTION pg_buffercache_invalidate(IN int, IN bool default true)
+RETURNS bool
+AS 'MODULE_PATHNAME', 'pg_buffercache_invalidate'
+LANGUAGE C PARALLEL SAFE;

--> Not enforced anywhere, but you can also add a comment to the 
function, for end users...

+PG_FUNCTION_INFO_V1(pg_buffercache_invalidate);
+Datum
+pg_buffercache_invalidate(PG_FUNCTION_ARGS)
+{
+    Buffer        bufnum;

"Buffer blocknum" is not correct in this context I believe. Buffer is 
when you have to manage Local buffer too (negative number).
Here uint32 is probably the good choice at the end, as used in 
pg_buffercache in other places.

Also in this extension bufferid is used, not buffernum.

+    bufnum = PG_GETARG_INT32(0);

+    if (bufnum <= 0 || bufnum > NBuffers)

maybe have a look at pageinspect and its PG_GETARG_UINT32.


+    {
+        ereport(ERROR,
+                (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                 errmsg("buffernum is not valid")));

https://www.postgresql.org/docs/16/error-style-guide.html let me think 
that message like 'buffernum is not valid' can be enhanced: out of 
range, cannot be negative or exceed number of shared buffers.... ? Maybe 
add the value to the message.

+
+    }
+
+    /*
+     * Check whether to force invalidate the dirty buffer. The default 
value of force is true.
+     */
+
+    force = PG_GETARG_BOOL(1);

I think you also need to test PG_ARGISNULL with force parameter.

+/*
+ * Try Invalidating a buffer using bufnum.
+ * If the buffer is invalid, the function returns false.
+ * The function checks for dirty buffer and flushes the dirty buffer 
before invalidating.
+ * If the buffer is still dirty it returns false.
+ */
+bool
+TryInvalidateBuffer(Buffer bufnum, bool force)
+{
+    BufferDesc *bufHdr = GetBufferDescriptor(bufnum - 1);

this is not safe, GetBufferDescriptor() accepts uint, but can receive 
negative here. Use uint32 and bufferid.

+    uint32        buf_state;
+    ReservePrivateRefCountEntry();
+
+    buf_state = LockBufHdr(bufHdr);
+    if ((buf_state & BM_VALID) == BM_VALID)
+    {
+        /*
+         * The buffer is pinned therefore cannot invalidate.
+         */
+        if (BUF_STATE_GET_REFCOUNT(buf_state) > 0)
+        {
+            UnlockBufHdr(bufHdr, buf_state);
+            return false;
+        }
+        if ((buf_state & BM_DIRTY) == BM_DIRTY)
+        {
+            /*
+             * If the buffer is dirty and the user has not asked to 
clear the dirty buffer return false.
+             * Otherwise clear the dirty buffer.
+             */
+            if(!force){
+                return false;

probably need to unlockbuffer here too.

+            }
+            /*
+             * Try once to flush the dirty buffer.
+             */
+            ResourceOwnerEnlargeBuffers(CurrentResourceOwner);
+            PinBuffer_Locked(bufHdr);
+            LWLockAcquire(BufferDescriptorGetContentLock(bufHdr), 
LW_SHARED);
+            FlushBuffer(bufHdr, NULL, IOOBJECT_RELATION, IOCONTEXT_NORMAL);
+ LWLockRelease(BufferDescriptorGetContentLock(bufHdr));
+            UnpinBuffer(bufHdr);

I am unsure of this area (the code is correct, but I wonder why there is 
no static code for this part -from pin to unpin- in PostgreSQL), and 
maybe better to go with FlushOneBuffer() ?
Also it is probably required to account for the shared buffer eviction 
in some pg_stat* view or table.
Not sure how disk syncing is handled after this sequence nor if it's 
important ?


+            buf_state = LockBufHdr(bufHdr);
+            if (BUF_STATE_GET_REFCOUNT(buf_state) > 0)
+            {
+                UnlockBufHdr(bufHdr, buf_state);
+                return false;
+            }
+
+            /*
+             * If its dirty again or not valid anymore give up.
+             */
+
+            if ((buf_state & (BM_DIRTY | BM_VALID)) != (BM_VALID))
+            {
+                UnlockBufHdr(bufHdr, buf_state);
+                return false;
+            }
+
+        }
+
+        InvalidateBuffer(bufHdr);
+        return true;
+    }
+    else
+    {
+        UnlockBufHdr(bufHdr, buf_state);
+        return false;
+    }


Maybe safe to remove the else {} ...
Maybe more tempting to start the big if with the following instead less 
nested...):
+    if ((buf_state & BM_VALID) != BM_VALID)
+    {
+        UnlockBufHdr(bufHdr, buf_state);
+        return false;
+    }

Doc and test are absent.

---
Cédric Villemain +33 (0)6 20 30 22 52
https://Data-Bene.io
PostgreSQL Expertise, Support, Training, R&D