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

On Thu, Dec 14, 2023 at 1:53 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> 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.

Updated the comments about group commit safety based on the off-list
suggestion by Alvaro.


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