Re: SLRU optimization - configurable buffer pool and partitioning the SLRU lock

On Thu, Dec 14, 2023 at 8:43 AM Amul Sul <sulamul@gmail.com> wrote:
>
> 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 % ?

I am not sure either but since this change in 0002 is by Andrey, I
will let him comment on this before we change or take any decision.

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

 Fixed

Thanks for your review, PFA updated version.

I have added @Amit Kapila to the list to view his opinion about
whether anything can break in the clog group update with our changes
of bank-wise SLRU lock.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com