Re: SLRU optimization - configurable buffer pool and partitioning the SLRU lock - Mailing list pgsql-hackers

From Amul Sul
Subject Re: SLRU optimization - configurable buffer pool and partitioning the SLRU lock
Date
Msg-id CAAJ_b94ONmqeKgrSxaH1=bUZ0ctuqQKLRF1LA591z34x1sqv8w@mail.gmail.com
Whole thread Raw
In response to Re: SLRU optimization - configurable buffer pool and partitioning the SLRU lock  (Dilip Kumar <dilipbalaut@gmail.com>)
Responses Re: SLRU optimization - configurable buffer pool and partitioning the SLRU lock
Re: SLRU optimization - configurable buffer pool and partitioning the SLRU lock
List pgsql-hackers
On Mon, Dec 11, 2023 at 10:42 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
On Thu, Nov 30, 2023 at 3:30 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Wed, Nov 29, 2023 at 4:58 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

Here is the updated patch based on some comments by tender wang (those
comments were sent only to me)

few nitpicks:

+
+   /*  
+    * Mask for slotno banks, considering 1GB SLRU buffer pool size and the
+    * SLRU_BANK_SIZE bits16 should be sufficient for the bank mask.
+    */  
+   bits16      bank_mask;
 } SlruCtlData;

...
...

+ int bankno = pageno & ctl->bank_mask;

I am a bit uncomfortable seeing it as a mask, why can't it be simply a number
of banks (num_banks) and get the bank number through modulus op (pageno %
num_banks) instead of bitwise & operation (pageno & ctl->bank_mask) which is a
bit difficult to read compared to modulus op which is quite simple,
straightforward and much common practice in hashing.

Are there any advantages of using &  over % ?

Also, a few places in 0002 and 0003 patch, need the bank number, it is better
to have a macro for that.
---

 extern bool SlruScanDirCbDeleteAll(SlruCtl ctl, char *filename, int64 segpage,
                                   void *data);
-
+extern bool check_slru_buffers(const char *name, int *newval);
 #endif                         /* SLRU_H */


Add an empty line after the declaration, in 0002 patch.
---

-TransactionIdSetStatusBit(TransactionId xid, XidStatus status, XLogRecPtr lsn, int slotno)
+TransactionIdSetStatusBit(TransactionId xid, XidStatus status, XLogRecPtr lsn,
+                         int slotno)

Unrelated change for 0003 patch. 
---

Regards,
Amul 

pgsql-hackers by date:

Previous
From: NINGWEI CHEN
Date:
Subject: Re: Remove MSVC scripts from the tree
Next
From: Amit Kapila
Date:
Subject: Re: "pgoutput" options missing on documentation